Feature/core 421to423 final #202
Conversation
There was a problem hiding this comment.
Overall the implementation looks good to me and can be understood easily.
Although I think, some changes need to be done to make the code consistent to the already existing code.
- Some classes and tables seem to be misspelled with "synch" instead of "sync"
- I don't get, why mapstruct is introduced and used in this specific case. It seems to, as there are many manual mappings done. So we can just map it manual completely. In both ways, we need complete tests for this mappings, either with or without mapstruct
- Please consider using a @ConfigurationProperties class instead using "@value" configurations manually. Have a look at "DepartmentProperties"
- We have a version mismatch in the pom.xml. Please use the dependencies, that match the specific used spring-version.
- Please have a closer look at the commented classes, in case I missed any points in this summary
Thanks for your work!
backend/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-webflux</artifactId> | ||
| <version>2.3.2.RELEASE</version> |
There was a problem hiding this comment.
Why has this version to be set? This should be injected by parent.
Additionally we're on spring 2.4.X
backend/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> | ||
| <version>2.11.2</version> |
There was a problem hiding this comment.
Why has this version to be set? This should be injected by parent.
| @Setter(AccessLevel.NONE) | ||
| @RequiredArgsConstructor | ||
| @Component | ||
| public class SynchContacts { |
There was a problem hiding this comment.
Typo error: please remove "h"
| private final @NonNull ContactsSyncReportRepository reports; | ||
| private final @NonNull ContactsSyncBacklogRepository backlog; | ||
|
|
||
| @org.springframework.beans.factory.annotation.Value("${quarano.sormas-integration.sormasurl:}") |
There was a problem hiding this comment.
Please consider to use a configuration-properties class and component as it is done with "DepartmentProperties"
| public interface ContactsSyncBacklogRepository extends JpaRepository<ContactsSynchBacklog, Long> { | ||
| // Returns all modified or inserted records after specified date | ||
| @Query("select distinct b.id from ContactsSynchBacklog b where b.syncDate <= :syncDate") | ||
| ArrayList<UUID> findBySyncDate(Date syncDate); |
There was a problem hiding this comment.
Please consider to use more general interface "List<>" or "Streamable<>"
| @Slf4j | ||
| @NoArgsConstructor(force = true, access = AccessLevel.PUBLIC) | ||
| @AllArgsConstructor(access = AccessLevel.PUBLIC) | ||
| public class ContactsSynchBacklog { |
There was a problem hiding this comment.
Typo in Word "Synch". please remove "h"
| public interface IndexSyncBacklogRepository extends JpaRepository<IndexSynchBacklog, Long> { | ||
| // Returns all modified or inserted records after specified date | ||
| @Query("select distinct b.id from IndexSynchBacklog b where b.syncDate <= :syncDate") | ||
| ArrayList<UUID> findBySyncDate(Date syncDate); |
There was a problem hiding this comment.
Please consider to use more general interface "List<>" or "Streamable<>"
| @NoArgsConstructor(access = AccessLevel.PUBLIC) | ||
| public class SormasCase { | ||
| private String uuid; | ||
| private Date reportDate; |
There was a problem hiding this comment.
Please use new java.time ZonedDateTime or LocalDateTime
|
Looks good now. Please squash the commits and follow our merge instructions in slack to merge it |
No description provided.