From 9e041e7f414b8b979f2a8fbc2b1aa244cf1fb3b5 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 19 Dec 2023 12:49:33 +0100 Subject: [PATCH 1/5] [kbss-cvut/record-manager-ui#38] Add REST endpoint for importing records. --- .../kbss/study/dto/RecordImportResult.java | 72 +++++++++++++++++++ .../study/rest/PatientRecordController.java | 8 +++ .../study/service/PatientRecordService.java | 13 ++++ .../RepositoryPatientRecordService.java | 8 +++ .../rest/PatientRecordControllerTest.java | 25 +++++++ 5 files changed, 126 insertions(+) create mode 100644 src/main/java/cz/cvut/kbss/study/dto/RecordImportResult.java diff --git a/src/main/java/cz/cvut/kbss/study/dto/RecordImportResult.java b/src/main/java/cz/cvut/kbss/study/dto/RecordImportResult.java new file mode 100644 index 00000000..7c0a353e --- /dev/null +++ b/src/main/java/cz/cvut/kbss/study/dto/RecordImportResult.java @@ -0,0 +1,72 @@ +package cz.cvut.kbss.study.dto; + +import java.util.ArrayList; +import java.util.List; + +/** + * Represents the result of importing records to this instance. + */ +public class RecordImportResult { + + /** + * Total number of processed records. + */ + private int totalCount; + + /** + * Number of successfully imported records. + */ + private int importedCount; + + /** + * Errors that occurred during import. + */ + private List errors; + + public RecordImportResult() { + } + + public RecordImportResult(int totalCount) { + this.totalCount = totalCount; + } + + public int getTotalCount() { + return totalCount; + } + + public void setTotalCount(int totalCount) { + this.totalCount = totalCount; + } + + public int getImportedCount() { + return importedCount; + } + + public void setImportedCount(int importedCount) { + this.importedCount = importedCount; + } + + public List getErrors() { + return errors; + } + + public void setErrors(List errors) { + this.errors = errors; + } + + public void addError(String error) { + if (this.errors == null) { + this.errors = new ArrayList<>(); + } + errors.add(error); + } + + @Override + public String toString() { + return "RecordImportResult{" + + "totalCount=" + totalCount + + ", importedCount=" + importedCount + + (errors != null ? ", errors=" + errors : "") + + '}'; + } +} diff --git a/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java b/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java index ad298b38..94ec33c4 100644 --- a/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java +++ b/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java @@ -1,6 +1,7 @@ package cz.cvut.kbss.study.rest; import cz.cvut.kbss.study.dto.PatientRecordDto; +import cz.cvut.kbss.study.dto.RecordImportResult; import cz.cvut.kbss.study.exception.NotFoundException; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; @@ -94,6 +95,13 @@ public ResponseEntity createRecord(@RequestBody PatientRecord record) { return new ResponseEntity<>(headers, HttpStatus.CREATED); } + @PostMapping(value = "/import", consumes = MediaType.APPLICATION_JSON_VALUE) + public RecordImportResult importRecords(@RequestBody List records) { + final RecordImportResult result = recordService.importRecords(records); + LOG.trace("Records imported with result: {}.", result); + return result; + } + @PutMapping(value = "/{key}", consumes = MediaType.APPLICATION_JSON_VALUE) @ResponseStatus(HttpStatus.NO_CONTENT) public void updateRecord(@PathVariable("key") String key, @RequestBody PatientRecord record) { diff --git a/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java index 59368a84..6c320727 100644 --- a/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java @@ -1,6 +1,7 @@ package cz.cvut.kbss.study.service; import cz.cvut.kbss.study.dto.PatientRecordDto; +import cz.cvut.kbss.study.dto.RecordImportResult; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; import cz.cvut.kbss.study.model.User; @@ -51,4 +52,16 @@ public interface PatientRecordService extends BaseService { * @see #findAllRecords() */ List findAllFull(RecordFilterParams filterParams); + + /** + * Imports the specified records. + *

+ * The current user is set as the author of the records. Only records whose identifiers do not already exist in the + * repository are imported. Existing records are skipped and the returned object contains a note that the record + * already exists. + * + * @param records Records to import + * @return Instance representing the import result + */ + RecordImportResult importRecords(List records); } diff --git a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java index 8c90477f..7c7b4b68 100644 --- a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java @@ -1,6 +1,7 @@ package cz.cvut.kbss.study.service.repository; import cz.cvut.kbss.study.dto.PatientRecordDto; +import cz.cvut.kbss.study.dto.RecordImportResult; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; import cz.cvut.kbss.study.model.User; @@ -75,4 +76,11 @@ protected void preUpdate(PatientRecord instance) { instance.setLastModified(new Date()); recordDao.requireUniqueNonEmptyLocalName(instance); } + + @Transactional + @Override + public RecordImportResult importRecords(List records) { + // TODO + return null; + } } diff --git a/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java b/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java index 3b5cc2e7..f80c076a 100644 --- a/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java +++ b/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java @@ -2,6 +2,7 @@ import com.fasterxml.jackson.core.type.TypeReference; import cz.cvut.kbss.study.dto.PatientRecordDto; +import cz.cvut.kbss.study.dto.RecordImportResult; import cz.cvut.kbss.study.environment.generator.Generator; import cz.cvut.kbss.study.environment.util.Environment; import cz.cvut.kbss.study.model.Institution; @@ -14,6 +15,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -28,9 +30,13 @@ import static cz.cvut.kbss.study.environment.util.ContainsSameEntities.containsSameEntities; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.anyOf; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; @@ -290,4 +296,23 @@ void exportRecordsExportsRecordsForProvidedInstitutionForSpecifiedPeriod() throw verify(patientRecordServiceMock).findAllFull( new RecordFilterParams(user.getInstitution().getKey(), minDate, maxDate, Collections.emptySet())); } + + @Test + void importRecordsImportsSpecifiedRecordsAndReturnsImportResult() throws Exception { + final List records = + List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); + final RecordImportResult importResult = new RecordImportResult(records.size()); + importResult.setImportedCount(records.size()); + when(patientRecordServiceMock.importRecords(anyList())).thenReturn(importResult); + + final MvcResult mvcResult = mockMvc.perform( + post("/records/import").content(toJson(records)).contentType(MediaType.APPLICATION_JSON)).andReturn(); + final RecordImportResult result = readValue(mvcResult, RecordImportResult.class); + assertEquals(importResult.getTotalCount(), result.getTotalCount()); + assertEquals(importResult.getImportedCount(), result.getImportedCount()); + assertThat(importResult.getErrors(), anyOf(nullValue(), empty())); + final ArgumentCaptor> captor = ArgumentCaptor.forClass(List.class); + verify(patientRecordServiceMock).importRecords(captor.capture()); + assertEquals(records.size(), captor.getValue().size()); + } } From 58e584e3688a24018c74d970c663c6c266e959b7 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 19 Dec 2023 14:20:25 +0100 Subject: [PATCH 2/5] [kbss-cvut/record-manager-ui#38] Implement record import. --- .../kbss/study/dto/RecordImportResult.java | 4 + .../persistence/dao/PatientRecordDao.java | 8 +- .../RepositoryPatientRecordService.java | 28 ++++- .../java/cz/cvut/kbss/study/util/Utils.java | 16 ++- .../persistence/dao/PatientRecordDaoTest.java | 18 +++ .../RepositoryPatientRecordServiceTest.java | 103 ++++++++++++++++++ 6 files changed, 170 insertions(+), 7 deletions(-) create mode 100644 src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java diff --git a/src/main/java/cz/cvut/kbss/study/dto/RecordImportResult.java b/src/main/java/cz/cvut/kbss/study/dto/RecordImportResult.java index 7c0a353e..c6b0a839 100644 --- a/src/main/java/cz/cvut/kbss/study/dto/RecordImportResult.java +++ b/src/main/java/cz/cvut/kbss/study/dto/RecordImportResult.java @@ -46,6 +46,10 @@ public void setImportedCount(int importedCount) { this.importedCount = importedCount; } + public void incrementImportedCount() { + this.importedCount++; + } + public List getErrors() { return errors; } diff --git a/src/main/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDao.java b/src/main/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDao.java index 99a9bdc6..e185f217 100644 --- a/src/main/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDao.java +++ b/src/main/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDao.java @@ -63,8 +63,12 @@ public PatientRecord findByKey(String key) { @Override public void persist(PatientRecord entity) { Objects.requireNonNull(entity); - entity.setKey(IdentificationUtils.generateKey()); - entity.setUri(generateRecordUriFromKey(entity.getKey())); + if (entity.getKey() == null) { + entity.setKey(IdentificationUtils.generateKey()); + } + if (entity.getUri() == null) { + entity.setUri(generateRecordUriFromKey(entity.getKey())); + } try { final Descriptor descriptor = getDescriptor(entity.getUri()); em.persist(entity, descriptor); diff --git a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java index 7c7b4b68..4f529f23 100644 --- a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java @@ -10,17 +10,22 @@ import cz.cvut.kbss.study.persistence.dao.util.RecordFilterParams; import cz.cvut.kbss.study.service.PatientRecordService; import cz.cvut.kbss.study.service.security.SecurityUtils; -import cz.cvut.kbss.study.util.IdentificationUtils; +import cz.cvut.kbss.study.util.Utils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; import java.util.Date; import java.util.List; +import java.util.Objects; @Service public class RepositoryPatientRecordService extends KeySupportingRepositoryService implements PatientRecordService { + private static final Logger LOG = LoggerFactory.getLogger(RepositoryPatientRecordService.class); + private final PatientRecordDao recordDao; private final SecurityUtils securityUtils; @@ -66,7 +71,6 @@ protected void prePersist(PatientRecord instance) { instance.setAuthor(author); instance.setDateCreated(new Date()); instance.setInstitution(author.getInstitution()); - instance.setKey(IdentificationUtils.generateKey()); recordDao.requireUniqueNonEmptyLocalName(instance); } @@ -80,7 +84,23 @@ protected void preUpdate(PatientRecord instance) { @Transactional @Override public RecordImportResult importRecords(List records) { - // TODO - return null; + Objects.requireNonNull(records); + LOG.debug("Importing records."); + final User author = securityUtils.getCurrentUser(); + final Date created = new Date(); + final RecordImportResult result = new RecordImportResult(records.size()); + records.forEach(r -> { + r.setAuthor(author); + r.setInstitution(author.getInstitution()); + r.setDateCreated(created); + if (recordDao.exists(r.getUri())) { + LOG.warn("Record {} already exists. Skipping it.", Utils.uriToString(r.getUri())); + result.addError("Record " + Utils.uriToString(r.getUri()) + " already exists."); + } else { + recordDao.persist(r); + result.incrementImportedCount(); + } + }); + return result; } } diff --git a/src/main/java/cz/cvut/kbss/study/util/Utils.java b/src/main/java/cz/cvut/kbss/study/util/Utils.java index c549a274..15c57ebc 100644 --- a/src/main/java/cz/cvut/kbss/study/util/Utils.java +++ b/src/main/java/cz/cvut/kbss/study/util/Utils.java @@ -2,7 +2,11 @@ import cz.cvut.kbss.study.exception.RecordManagerException; -import java.io.*; +import java.io.BufferedReader; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; import java.net.URI; import java.nio.charset.StandardCharsets; import java.util.Map; @@ -43,4 +47,14 @@ public static URI prepareUri(String remoteUrl, Map queryParams) } return URI.create(sb.toString()); } + + /** + * Returns specified URI enclosed in < and >. + * + * @param uri URI to stringify + * @return URI in angle brackets + */ + public static String uriToString(URI uri) { + return "<" + uri + ">"; + } } diff --git a/src/test/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDaoTest.java b/src/test/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDaoTest.java index b7c8a5c7..93ff7cb7 100644 --- a/src/test/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDaoTest.java +++ b/src/test/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDaoTest.java @@ -19,6 +19,7 @@ import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import java.net.URI; import java.time.LocalDate; import java.time.ZoneOffset; import java.util.ArrayList; @@ -322,4 +323,21 @@ void findAllFullReturnsRecordsMatchingSpecifiedPhase() { assertFalse(result.isEmpty()); result.forEach(res -> assertEquals(phase, res.getPhase())); } + + @Test + void persistDoesNotGenerateIdentificationWhenRecordAlreadyHasIt() { + final User author = generateAuthorWithInstitution(); + final PatientRecord record = Generator.generatePatientRecord(author); + final String key = IdentificationUtils.generateKey(); + record.setKey(key); + final URI uri = Generator.generateUri(); + record.setUri(uri); + + transactional(() -> sut.persist(record)); + + final PatientRecord result = em.find(PatientRecord.class, uri); + assertNotNull(result); + assertEquals(uri, result.getUri()); + assertEquals(key, result.getKey()); + } } diff --git a/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java b/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java new file mode 100644 index 00000000..49688a13 --- /dev/null +++ b/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java @@ -0,0 +1,103 @@ +package cz.cvut.kbss.study.service.repository; + +import cz.cvut.kbss.study.dto.RecordImportResult; +import cz.cvut.kbss.study.environment.generator.Generator; +import cz.cvut.kbss.study.environment.util.Environment; +import cz.cvut.kbss.study.model.PatientRecord; +import cz.cvut.kbss.study.model.User; +import cz.cvut.kbss.study.persistence.dao.PatientRecordDao; +import cz.cvut.kbss.study.service.security.SecurityUtils; +import cz.cvut.kbss.study.util.IdentificationUtils; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.util.Date; +import java.util.List; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class RepositoryPatientRecordServiceTest { + + @Mock + private PatientRecordDao recordDao; + + @Mock + private SecurityUtils securityUtils; + + @InjectMocks + private RepositoryPatientRecordService sut; + + private User user; + + @BeforeEach + void setUp() { + this.user = Generator.generateUser(Generator.generateInstitution()); + Environment.setCurrentUser(user); + when(securityUtils.getCurrentUser()).thenReturn(user); + when(recordDao.exists(any())).thenReturn(false); + } + + @Test + void importRecordsSetsCurrentUserAsAuthorAndImportsSpecifiedRecords() { + final List toImport = + List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); + final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + toImport.forEach(r -> { + // Simulate that the records existed in another deployment from which they are imported + r.setKey(IdentificationUtils.generateKey()); + r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); + r.setAuthor(originalAuthor); + }); + + final RecordImportResult result = sut.importRecords(toImport); + assertEquals(toImport.size(), result.getTotalCount()); + assertEquals(toImport.size(), result.getImportedCount()); + assertThat(result.getErrors(), anyOf(nullValue(), empty())); + final ArgumentCaptor captor = ArgumentCaptor.forClass(PatientRecord.class); + verify(recordDao, times(toImport.size())).persist(captor.capture()); + final List imported = captor.getAllValues(); + assertEquals(toImport.size(), imported.size()); + for (int i = 0; i < toImport.size(); i++) { + assertEquals(toImport.get(i).getUri(), imported.get(i).getUri()); + assertEquals(toImport.get(i).getKey(), imported.get(i).getKey()); + assertEquals(user, imported.get(i).getAuthor()); + assertThat(imported.get(i).getDateCreated().getTime(), greaterThan(System.currentTimeMillis() - 1000L)); + } + } + + @Test + void importRecordsSkipsImportingRecordsThatAlreadyExist() { + final List toImport = + List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); + final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final PatientRecord existing = toImport.get(Generator.randomIndex(toImport)); + when(recordDao.exists(existing.getUri())).thenReturn(true); + toImport.forEach(r -> { + // Simulate that the records existed in another deployment from which they are imported + r.setKey(IdentificationUtils.generateKey()); + r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); + r.setAuthor(originalAuthor); + }); + + final RecordImportResult result = sut.importRecords(toImport); + assertEquals(toImport.size(), result.getTotalCount()); + assertEquals(toImport.size() - 1, result.getImportedCount()); + assertEquals(1, result.getErrors().size()); + toImport.forEach(r -> verify(recordDao).exists(r.getUri())); + } +} \ No newline at end of file From ee9872da5253ab9b95c39714a49d55e9c86f4ffa Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Tue, 19 Dec 2023 15:03:23 +0100 Subject: [PATCH 3/5] [kbss-cvut/record-manager-ui#38] Allow providing phase to set on all imported records. --- .../cz/cvut/kbss/study/model/RecordPhase.java | 16 ++++++++++++ .../study/rest/PatientRecordController.java | 16 +++++++++--- .../study/service/PatientRecordService.java | 16 ++++++++++++ .../RepositoryPatientRecordService.java | 10 +++++++ .../environment/generator/Generator.java | 1 + .../kbss/study/model/RecordPhaseTest.java | 26 +++++++++++++++++++ .../rest/PatientRecordControllerTest.java | 19 +++++++++++++- .../RepositoryPatientRecordServiceTest.java | 20 ++++++++++++++ 8 files changed, 119 insertions(+), 5 deletions(-) create mode 100644 src/test/java/cz/cvut/kbss/study/model/RecordPhaseTest.java diff --git a/src/main/java/cz/cvut/kbss/study/model/RecordPhase.java b/src/main/java/cz/cvut/kbss/study/model/RecordPhase.java index 398a39d9..20a09a20 100644 --- a/src/main/java/cz/cvut/kbss/study/model/RecordPhase.java +++ b/src/main/java/cz/cvut/kbss/study/model/RecordPhase.java @@ -23,4 +23,20 @@ public enum RecordPhase { public String getIri() { return iri; } + + /** + * Returns {@link RecordPhase} with the specified IRI. + * + * @param iri record phase identifier + * @return matching {@code RecordPhase} + * @throws IllegalArgumentException When no matching phase is found + */ + public static RecordPhase fromString(String iri) { + for (RecordPhase p : values()) { + if (p.getIri().equals(iri)) { + return p; + } + } + throw new IllegalArgumentException("Unknown record phase identifier '" + iri + "'."); + } } diff --git a/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java b/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java index 94ec33c4..fa0c4bbe 100644 --- a/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java +++ b/src/main/java/cz/cvut/kbss/study/rest/PatientRecordController.java @@ -5,6 +5,7 @@ import cz.cvut.kbss.study.exception.NotFoundException; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; +import cz.cvut.kbss.study.model.RecordPhase; import cz.cvut.kbss.study.rest.exception.BadRequestException; import cz.cvut.kbss.study.rest.util.RecordFilterMapper; import cz.cvut.kbss.study.rest.util.RestUtils; @@ -96,10 +97,17 @@ public ResponseEntity createRecord(@RequestBody PatientRecord record) { } @PostMapping(value = "/import", consumes = MediaType.APPLICATION_JSON_VALUE) - public RecordImportResult importRecords(@RequestBody List records) { - final RecordImportResult result = recordService.importRecords(records); - LOG.trace("Records imported with result: {}.", result); - return result; + public RecordImportResult importRecords(@RequestBody List records, + @RequestParam(name = "phase", required = false) String phaseIri) { + final RecordImportResult importResult; + if (phaseIri != null) { + final RecordPhase targetPhase = RecordPhase.fromString(phaseIri); + importResult = recordService.importRecords(records, targetPhase); + } else { + importResult = recordService.importRecords(records); + } + LOG.trace("Records imported with result: {}.", importResult); + return importResult; } @PutMapping(value = "/{key}", consumes = MediaType.APPLICATION_JSON_VALUE) diff --git a/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java index 6c320727..9887789c 100644 --- a/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java @@ -4,6 +4,7 @@ import cz.cvut.kbss.study.dto.RecordImportResult; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; +import cz.cvut.kbss.study.model.RecordPhase; import cz.cvut.kbss.study.model.User; import cz.cvut.kbss.study.persistence.dao.util.RecordFilterParams; @@ -59,9 +60,24 @@ public interface PatientRecordService extends BaseService { * The current user is set as the author of the records. Only records whose identifiers do not already exist in the * repository are imported. Existing records are skipped and the returned object contains a note that the record * already exists. + *

+ * This method, in contrast to {@link #importRecords(List, RecordPhase)}, preserves the phase of the imported + * records. * * @param records Records to import * @return Instance representing the import result */ RecordImportResult importRecords(List records); + + /** + * Imports the specified records and sets them all to the specified phase. + *

+ * The current user is set as the author of the records. Only records whose identifiers do not already exist in the + * repository are imported. Existing records are skipped and the returned object contains a note that the record + * already exists. + * + * @param records Records to import + * @return Instance representing the import result + */ + RecordImportResult importRecords(List records, RecordPhase targetPhase); } diff --git a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java index 4f529f23..1684cf37 100644 --- a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java @@ -4,6 +4,7 @@ import cz.cvut.kbss.study.dto.RecordImportResult; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; +import cz.cvut.kbss.study.model.RecordPhase; import cz.cvut.kbss.study.model.User; import cz.cvut.kbss.study.persistence.dao.OwlKeySupportingDao; import cz.cvut.kbss.study.persistence.dao.PatientRecordDao; @@ -103,4 +104,13 @@ public RecordImportResult importRecords(List records) { }); return result; } + + @Transactional + @Override + public RecordImportResult importRecords(List records, RecordPhase targetPhase) { + Objects.requireNonNull(records); + LOG.debug("Importing records to target phase '{}'.", targetPhase); + records.forEach(r -> r.setPhase(targetPhase)); + return importRecords(records); + } } diff --git a/src/test/java/cz/cvut/kbss/study/environment/generator/Generator.java b/src/test/java/cz/cvut/kbss/study/environment/generator/Generator.java index 77b67ac7..2a8b5693 100644 --- a/src/test/java/cz/cvut/kbss/study/environment/generator/Generator.java +++ b/src/test/java/cz/cvut/kbss/study/environment/generator/Generator.java @@ -170,6 +170,7 @@ public static PatientRecord generatePatientRecord(User author) { rec.setLocalName("RandomRecord" + randomInt()); rec.setUri(generateUri()); rec.setInstitution(author.getInstitution()); + rec.setPhase(RecordPhase.values()[Generator.randomInt(0, RecordPhase.values().length)]); return rec; } diff --git a/src/test/java/cz/cvut/kbss/study/model/RecordPhaseTest.java b/src/test/java/cz/cvut/kbss/study/model/RecordPhaseTest.java new file mode 100644 index 00000000..59ad29af --- /dev/null +++ b/src/test/java/cz/cvut/kbss/study/model/RecordPhaseTest.java @@ -0,0 +1,26 @@ +package cz.cvut.kbss.study.model; + +import cz.cvut.kbss.study.environment.generator.Generator; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +class RecordPhaseTest { + + @Test + void fromStringReturnsMatchingRecordPhase() { + for (RecordPhase p : RecordPhase.values()) { + assertEquals(p, RecordPhase.fromString(p.getIri())); + } + } + + @Test + void fromStringThrowsIllegalArgumentForUnknownPhaseIri() { + assertThrows(IllegalArgumentException.class, () -> RecordPhase.fromString(Generator.generateUri().toString())); + } + + @Test + void fromStringThrowsIllegalArgumentForNullArgument() { + assertThrows(IllegalArgumentException.class, () -> RecordPhase.fromString(null)); + } +} \ No newline at end of file diff --git a/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java b/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java index f80c076a..92099428 100644 --- a/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java +++ b/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java @@ -7,6 +7,7 @@ import cz.cvut.kbss.study.environment.util.Environment; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; +import cz.cvut.kbss.study.model.RecordPhase; import cz.cvut.kbss.study.model.User; import cz.cvut.kbss.study.persistence.dao.util.RecordFilterParams; import cz.cvut.kbss.study.service.InstitutionService; @@ -30,19 +31,21 @@ import static cz.cvut.kbss.study.environment.util.ContainsSameEntities.containsSameEntities; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.nullValue; -import static org.hamcrest.Matchers.anyOf; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyList; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @ExtendWith(MockitoExtension.class) public class PatientRecordControllerTest extends BaseControllerTestRunner { @@ -315,4 +318,18 @@ void importRecordsImportsSpecifiedRecordsAndReturnsImportResult() throws Excepti verify(patientRecordServiceMock).importRecords(captor.capture()); assertEquals(records.size(), captor.getValue().size()); } + + @Test + void importRecordsImportsSpecifiedRecordsWithSpecifiedPhaseAndReturnsImportResult() throws Exception { + final List records = + List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); + final RecordImportResult importResult = new RecordImportResult(records.size()); + importResult.setImportedCount(records.size()); + final RecordPhase targetPhase = RecordPhase.values()[Generator.randomInt(0, RecordPhase.values().length)]; + when(patientRecordServiceMock.importRecords(anyList(), any(RecordPhase.class))).thenReturn(importResult); + + mockMvc.perform(post("/records/import").content(toJson(records)).contentType(MediaType.APPLICATION_JSON) + .param("phase", targetPhase.getIri())).andExpect(status().isOk()); + verify(patientRecordServiceMock).importRecords(anyList(), eq(targetPhase)); + } } diff --git a/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java b/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java index 49688a13..eae9688d 100644 --- a/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java +++ b/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java @@ -4,6 +4,7 @@ import cz.cvut.kbss.study.environment.generator.Generator; import cz.cvut.kbss.study.environment.util.Environment; import cz.cvut.kbss.study.model.PatientRecord; +import cz.cvut.kbss.study.model.RecordPhase; import cz.cvut.kbss.study.model.User; import cz.cvut.kbss.study.persistence.dao.PatientRecordDao; import cz.cvut.kbss.study.service.security.SecurityUtils; @@ -100,4 +101,23 @@ void importRecordsSkipsImportingRecordsThatAlreadyExist() { assertEquals(1, result.getErrors().size()); toImport.forEach(r -> verify(recordDao).exists(r.getUri())); } + + @Test + void importRecordsWithPhaseSetsSpecifiedPhaseToAllRecords() { + final List toImport = + List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); + final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final RecordPhase targetPhase = RecordPhase.values()[Generator.randomInt(0, RecordPhase.values().length)]; + toImport.forEach(r -> { + // Simulate that the records existed in another deployment from which they are imported + r.setKey(IdentificationUtils.generateKey()); + r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); + r.setAuthor(originalAuthor); + }); + + sut.importRecords(toImport, targetPhase); + final ArgumentCaptor captor = ArgumentCaptor.forClass(PatientRecord.class); + verify(recordDao, times(toImport.size())).persist(captor.capture()); + captor.getAllValues().forEach(r -> assertEquals(targetPhase, r.getPhase())); + } } \ No newline at end of file From dce1d6d7ff6bf190a69c0fef15a157c0730dfe37 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Fri, 22 Dec 2023 14:50:39 +0100 Subject: [PATCH 4/5] [kbss-cvut/record-manager-ui#38] Set imported record provenance according to acceptance criteria. Importing as admin preserves imported record provenance, as regular user replaces original author/created with current user/datetime. --- .../RecordAuthorNotFoundException.java | 11 ++++ .../java/cz/cvut/kbss/study/model/User.java | 20 +++++- .../rest/handler/RestExceptionHandler.java | 22 ++++--- .../study/service/PatientRecordService.java | 22 +++++-- .../RepositoryPatientRecordService.java | 24 ++++++-- .../rest/PatientRecordControllerTest.java | 11 ++++ .../RepositoryPatientRecordServiceTest.java | 61 +++++++++++++++++-- 7 files changed, 146 insertions(+), 25 deletions(-) create mode 100644 src/main/java/cz/cvut/kbss/study/exception/RecordAuthorNotFoundException.java diff --git a/src/main/java/cz/cvut/kbss/study/exception/RecordAuthorNotFoundException.java b/src/main/java/cz/cvut/kbss/study/exception/RecordAuthorNotFoundException.java new file mode 100644 index 00000000..ab7b1574 --- /dev/null +++ b/src/main/java/cz/cvut/kbss/study/exception/RecordAuthorNotFoundException.java @@ -0,0 +1,11 @@ +package cz.cvut.kbss.study.exception; + +/** + * Indicates that the application is attempting to import a record with a nonexistent author. + */ +public class RecordAuthorNotFoundException extends RecordManagerException { + + public RecordAuthorNotFoundException(String message) { + super(message); + } +} diff --git a/src/main/java/cz/cvut/kbss/study/model/User.java b/src/main/java/cz/cvut/kbss/study/model/User.java index c486b324..836d365a 100644 --- a/src/main/java/cz/cvut/kbss/study/model/User.java +++ b/src/main/java/cz/cvut/kbss/study/model/User.java @@ -1,7 +1,13 @@ package cz.cvut.kbss.study.model; import com.fasterxml.jackson.annotation.JsonProperty; -import cz.cvut.kbss.jopa.model.annotations.*; +import cz.cvut.kbss.jopa.model.annotations.FetchType; +import cz.cvut.kbss.jopa.model.annotations.Id; +import cz.cvut.kbss.jopa.model.annotations.OWLClass; +import cz.cvut.kbss.jopa.model.annotations.OWLDataProperty; +import cz.cvut.kbss.jopa.model.annotations.OWLObjectProperty; +import cz.cvut.kbss.jopa.model.annotations.ParticipationConstraints; +import cz.cvut.kbss.jopa.model.annotations.Types; import cz.cvut.kbss.study.model.util.HasDerivableUri; import cz.cvut.kbss.study.util.Constants; import cz.cvut.kbss.study.util.IdentificationUtils; @@ -148,6 +154,18 @@ public void addType(String type) { getTypes().add(type); } + /** + * Returns true if this user is an admin. + *

+ * That is, it has an admin type. + * + * @return {@code true} if this is admin, {@code false} otherwise + */ + public boolean isAdmin() { + assert types != null; + return getTypes().contains(Vocabulary.s_c_administrator); + } + public String getToken() { return token; } diff --git a/src/main/java/cz/cvut/kbss/study/rest/handler/RestExceptionHandler.java b/src/main/java/cz/cvut/kbss/study/rest/handler/RestExceptionHandler.java index 894d58f2..1c375566 100644 --- a/src/main/java/cz/cvut/kbss/study/rest/handler/RestExceptionHandler.java +++ b/src/main/java/cz/cvut/kbss/study/rest/handler/RestExceptionHandler.java @@ -1,6 +1,11 @@ package cz.cvut.kbss.study.rest.handler; -import cz.cvut.kbss.study.exception.*; +import cz.cvut.kbss.study.exception.EntityExistsException; +import cz.cvut.kbss.study.exception.NotFoundException; +import cz.cvut.kbss.study.exception.PersistenceException; +import cz.cvut.kbss.study.exception.RecordAuthorNotFoundException; +import cz.cvut.kbss.study.exception.ValidationException; +import cz.cvut.kbss.study.exception.WebServiceIntegrationException; import jakarta.servlet.http.HttpServletRequest; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -83,13 +88,14 @@ public ResponseEntity persistenceException(HttpServletRequest request return new ResponseEntity<>(errorInfo(request, e.getCause()), HttpStatus.INTERNAL_SERVER_ERROR); } + @ExceptionHandler(RecordAuthorNotFoundException.class) + public ResponseEntity recordAuthorNotFoundException(HttpServletRequest request, + RecordAuthorNotFoundException e) { + logException(request, e); + return new ResponseEntity<>(errorInfo(request, e), HttpStatus.CONFLICT); + } + void logException(HttpServletRequest request, RuntimeException e) { - LOG.debug( - String.format( - "Request to '%s' failed due to error: %s", - request.getRequestURI(), - e.getMessage() - ) - ); + LOG.debug("Request to '{}' failed due to error: {}", request.getRequestURI(), e.getMessage()); } } diff --git a/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java index 9887789c..aa5dd528 100644 --- a/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java @@ -57,27 +57,37 @@ public interface PatientRecordService extends BaseService { /** * Imports the specified records. *

- * The current user is set as the author of the records. Only records whose identifiers do not already exist in the - * repository are imported. Existing records are skipped and the returned object contains a note that the record - * already exists. + * Only records whose identifiers do not already exist in the repository are imported. Existing records are skipped + * and the returned object contains a note that the record already exists. *

* This method, in contrast to {@link #importRecords(List, RecordPhase)}, preserves the phase of the imported * records. + *

+ * If the current user is an admin, the import procedure retains provenance data of the record. Otherwise, the + * current user is set as the record's author. * * @param records Records to import * @return Instance representing the import result + * @throws cz.cvut.kbss.study.exception.RecordAuthorNotFoundException Thrown when importing a record whose author + * does not exist in this application instance's + * repository */ RecordImportResult importRecords(List records); /** * Imports the specified records and sets them all to the specified phase. *

- * The current user is set as the author of the records. Only records whose identifiers do not already exist in the - * repository are imported. Existing records are skipped and the returned object contains a note that the record - * already exists. + * Only records whose identifiers do not already exist in the repository are imported. Existing records are skipped + * and the returned object contains a note that the record already exists. + *

+ * If the current user is an admin, the import procedure retains provenance data of the record. Otherwise, the + * current user is set as the record's author. * * @param records Records to import * @return Instance representing the import result + * @throws cz.cvut.kbss.study.exception.RecordAuthorNotFoundException Thrown when importing a record whose author + * does not exist in this application instance's + * repository */ RecordImportResult importRecords(List records, RecordPhase targetPhase); } diff --git a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java index 1684cf37..49e8477f 100644 --- a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java @@ -2,6 +2,7 @@ import cz.cvut.kbss.study.dto.PatientRecordDto; import cz.cvut.kbss.study.dto.RecordImportResult; +import cz.cvut.kbss.study.exception.RecordAuthorNotFoundException; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; import cz.cvut.kbss.study.model.RecordPhase; @@ -10,6 +11,7 @@ import cz.cvut.kbss.study.persistence.dao.PatientRecordDao; import cz.cvut.kbss.study.persistence.dao.util.RecordFilterParams; import cz.cvut.kbss.study.service.PatientRecordService; +import cz.cvut.kbss.study.service.UserService; import cz.cvut.kbss.study.service.security.SecurityUtils; import cz.cvut.kbss.study.util.Utils; import org.slf4j.Logger; @@ -31,10 +33,13 @@ public class RepositoryPatientRecordService extends KeySupportingRepositoryServi private final SecurityUtils securityUtils; - public RepositoryPatientRecordService(PatientRecordDao recordDao, - SecurityUtils securityUtils) { + private final UserService userService; + + public RepositoryPatientRecordService(PatientRecordDao recordDao, SecurityUtils securityUtils, + UserService userService) { this.recordDao = recordDao; this.securityUtils = securityUtils; + this.userService = userService; } @Override @@ -91,9 +96,7 @@ public RecordImportResult importRecords(List records) { final Date created = new Date(); final RecordImportResult result = new RecordImportResult(records.size()); records.forEach(r -> { - r.setAuthor(author); - r.setInstitution(author.getInstitution()); - r.setDateCreated(created); + setImportedRecordProvenance(author, created, r); if (recordDao.exists(r.getUri())) { LOG.warn("Record {} already exists. Skipping it.", Utils.uriToString(r.getUri())); result.addError("Record " + Utils.uriToString(r.getUri()) + " already exists."); @@ -105,6 +108,17 @@ public RecordImportResult importRecords(List records) { return result; } + private void setImportedRecordProvenance(User currentUser, Date now, PatientRecord record) { + if (!currentUser.isAdmin()) { + record.setAuthor(currentUser); + record.setInstitution(currentUser.getInstitution()); + record.setDateCreated(now); + } else if (!userService.exists(record.getAuthor().getUri())) { + throw new RecordAuthorNotFoundException("Author of record " + record + "not found during import."); + } + } + + @Transactional @Override public RecordImportResult importRecords(List records, RecordPhase targetPhase) { diff --git a/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java b/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java index 92099428..b3ec5d22 100644 --- a/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java +++ b/src/test/java/cz/cvut/kbss/study/rest/PatientRecordControllerTest.java @@ -5,6 +5,7 @@ import cz.cvut.kbss.study.dto.RecordImportResult; import cz.cvut.kbss.study.environment.generator.Generator; import cz.cvut.kbss.study.environment.util.Environment; +import cz.cvut.kbss.study.exception.RecordAuthorNotFoundException; import cz.cvut.kbss.study.model.Institution; import cz.cvut.kbss.study.model.PatientRecord; import cz.cvut.kbss.study.model.RecordPhase; @@ -332,4 +333,14 @@ void importRecordsImportsSpecifiedRecordsWithSpecifiedPhaseAndReturnsImportResul .param("phase", targetPhase.getIri())).andExpect(status().isOk()); verify(patientRecordServiceMock).importRecords(anyList(), eq(targetPhase)); } + + @Test + void importRecordsReturnsConflictWhenServiceThrowsRecordAuthorNotFound() throws Exception { + final List records = + List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); + when(patientRecordServiceMock.importRecords(anyList())).thenThrow(RecordAuthorNotFoundException.class); + + mockMvc.perform(post("/records/import").content(toJson(records)).contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isConflict()); + } } diff --git a/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java b/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java index eae9688d..d49c0a27 100644 --- a/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java +++ b/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java @@ -3,10 +3,13 @@ import cz.cvut.kbss.study.dto.RecordImportResult; import cz.cvut.kbss.study.environment.generator.Generator; import cz.cvut.kbss.study.environment.util.Environment; +import cz.cvut.kbss.study.exception.RecordAuthorNotFoundException; import cz.cvut.kbss.study.model.PatientRecord; import cz.cvut.kbss.study.model.RecordPhase; import cz.cvut.kbss.study.model.User; +import cz.cvut.kbss.study.model.Vocabulary; import cz.cvut.kbss.study.persistence.dao.PatientRecordDao; +import cz.cvut.kbss.study.service.UserService; import cz.cvut.kbss.study.service.security.SecurityUtils; import cz.cvut.kbss.study.util.IdentificationUtils; import org.junit.jupiter.api.BeforeEach; @@ -26,6 +29,7 @@ import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -40,6 +44,9 @@ class RepositoryPatientRecordServiceTest { @Mock private SecurityUtils securityUtils; + @Mock + private UserService userService; + @InjectMocks private RepositoryPatientRecordService sut; @@ -50,20 +57,19 @@ void setUp() { this.user = Generator.generateUser(Generator.generateInstitution()); Environment.setCurrentUser(user); when(securityUtils.getCurrentUser()).thenReturn(user); - when(recordDao.exists(any())).thenReturn(false); } @Test - void importRecordsSetsCurrentUserAsAuthorAndImportsSpecifiedRecords() { - final List toImport = - List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); + void importRecordsSetsCurrentUserAsAuthorWhenTheyAreRegularUserAndImportsSpecifiedRecords() { final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final List toImport = + List.of(Generator.generatePatientRecord(originalAuthor), Generator.generatePatientRecord(originalAuthor)); toImport.forEach(r -> { // Simulate that the records existed in another deployment from which they are imported r.setKey(IdentificationUtils.generateKey()); r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); - r.setAuthor(originalAuthor); }); + when(recordDao.exists(any())).thenReturn(false); final RecordImportResult result = sut.importRecords(toImport); assertEquals(toImport.size(), result.getTotalCount()); @@ -81,6 +87,50 @@ void importRecordsSetsCurrentUserAsAuthorAndImportsSpecifiedRecords() { } } + @Test + void importRecordsRetainsRecordProvenanceDataWhenCurrentUserIsAdmin() { + final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final List toImport = + List.of(Generator.generatePatientRecord(originalAuthor), Generator.generatePatientRecord(originalAuthor)); + toImport.forEach(r -> { + // Simulate that the records existed in another deployment from which they are imported + r.setKey(IdentificationUtils.generateKey()); + r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); + }); + user.addType(Vocabulary.s_c_administrator); + Environment.setCurrentUser(user); + when(userService.exists(originalAuthor.getUri())).thenReturn(true); + when(recordDao.exists(any())).thenReturn(false); + + sut.importRecords(toImport); + final ArgumentCaptor captor = ArgumentCaptor.forClass(PatientRecord.class); + verify(recordDao, times(toImport.size())).persist(captor.capture()); + final List imported = captor.getAllValues(); + assertEquals(toImport.size(), imported.size()); + for (int i = 0; i < toImport.size(); i++) { + assertEquals(toImport.get(i).getUri(), imported.get(i).getUri()); + assertEquals(toImport.get(i).getKey(), imported.get(i).getKey()); + assertEquals(originalAuthor, imported.get(i).getAuthor()); + assertEquals(toImport.get(i).getDateCreated(), imported.get(i).getDateCreated()); + } + } + + @Test + void importRecordsThrowsRecordAuthorNotFoundExceptionWhenAdminImportsRecordsAndRecordAuthorIsNotFound() { + final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final List toImport = + List.of(Generator.generatePatientRecord(originalAuthor), Generator.generatePatientRecord(originalAuthor)); + toImport.forEach(r -> { + // Simulate that the records existed in another deployment from which they are imported + r.setKey(IdentificationUtils.generateKey()); + r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); + }); + user.addType(Vocabulary.s_c_administrator); + Environment.setCurrentUser(user); + + assertThrows(RecordAuthorNotFoundException.class, () -> sut.importRecords(toImport)); + } + @Test void importRecordsSkipsImportingRecordsThatAlreadyExist() { final List toImport = @@ -114,6 +164,7 @@ void importRecordsWithPhaseSetsSpecifiedPhaseToAllRecords() { r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); r.setAuthor(originalAuthor); }); + when(recordDao.exists(any())).thenReturn(false); sut.importRecords(toImport, targetPhase); final ArgumentCaptor captor = ArgumentCaptor.forClass(PatientRecord.class); From 69da5c13c0f68c3aec6f555eab907cecd47fdcf2 Mon Sep 17 00:00:00 2001 From: Martin Ledvinka Date: Fri, 22 Dec 2023 16:18:32 +0100 Subject: [PATCH 5/5] [kbss-cvut/record-manager-ui#38] Set imported record phase to open unless specified otherwise. When current user is admin, imported record phase is preserved. --- .../study/service/PatientRecordService.java | 3 +- .../RepositoryPatientRecordService.java | 21 +++++-- .../RepositoryPatientRecordServiceTest.java | 60 +++++++++---------- 3 files changed, 44 insertions(+), 40 deletions(-) diff --git a/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java index aa5dd528..6e57840f 100644 --- a/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/PatientRecordService.java @@ -64,7 +64,8 @@ public interface PatientRecordService extends BaseService { * records. *

* If the current user is an admin, the import procedure retains provenance data of the record. Otherwise, the - * current user is set as the record's author. + * current user is set as the record's author. Also, if the current user is not an admin, the phase of all + * the imported records is set to {@link RecordPhase#open}, for admin, the phase of the records is retained. * * @param records Records to import * @return Instance representing the import result diff --git a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java index 49e8477f..cc20e7ba 100644 --- a/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java +++ b/src/main/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordService.java @@ -22,6 +22,7 @@ import java.util.Date; import java.util.List; import java.util.Objects; +import java.util.Optional; @Service public class RepositoryPatientRecordService extends KeySupportingRepositoryService @@ -92,11 +93,15 @@ protected void preUpdate(PatientRecord instance) { public RecordImportResult importRecords(List records) { Objects.requireNonNull(records); LOG.debug("Importing records."); + return importRecordsImpl(records, Optional.empty()); + } + + private RecordImportResult importRecordsImpl(List records, Optional targetPhase) { final User author = securityUtils.getCurrentUser(); final Date created = new Date(); final RecordImportResult result = new RecordImportResult(records.size()); records.forEach(r -> { - setImportedRecordProvenance(author, created, r); + setImportedRecordProvenance(author, created, targetPhase, r); if (recordDao.exists(r.getUri())) { LOG.warn("Record {} already exists. Skipping it.", Utils.uriToString(r.getUri())); result.addError("Record " + Utils.uriToString(r.getUri()) + " already exists."); @@ -108,13 +113,18 @@ public RecordImportResult importRecords(List records) { return result; } - private void setImportedRecordProvenance(User currentUser, Date now, PatientRecord record) { + private void setImportedRecordProvenance(User currentUser, Date now, Optional targetPhase, + PatientRecord record) { if (!currentUser.isAdmin()) { record.setAuthor(currentUser); record.setInstitution(currentUser.getInstitution()); record.setDateCreated(now); - } else if (!userService.exists(record.getAuthor().getUri())) { - throw new RecordAuthorNotFoundException("Author of record " + record + "not found during import."); + targetPhase.ifPresentOrElse(record::setPhase, () -> record.setPhase(RecordPhase.open)); + } else { + targetPhase.ifPresent(record::setPhase); + if (!userService.exists(record.getAuthor().getUri())) { + throw new RecordAuthorNotFoundException("Author of record " + record + "not found during import."); + } } } @@ -124,7 +134,6 @@ private void setImportedRecordProvenance(User currentUser, Date now, PatientReco public RecordImportResult importRecords(List records, RecordPhase targetPhase) { Objects.requireNonNull(records); LOG.debug("Importing records to target phase '{}'.", targetPhase); - records.forEach(r -> r.setPhase(targetPhase)); - return importRecords(records); + return importRecordsImpl(records, Optional.ofNullable(targetPhase)); } } diff --git a/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java b/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java index d49c0a27..a337ff52 100644 --- a/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java +++ b/src/test/java/cz/cvut/kbss/study/service/repository/RepositoryPatientRecordServiceTest.java @@ -20,6 +20,7 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import java.net.URI; import java.util.Date; import java.util.List; @@ -62,13 +63,7 @@ void setUp() { @Test void importRecordsSetsCurrentUserAsAuthorWhenTheyAreRegularUserAndImportsSpecifiedRecords() { final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); - final List toImport = - List.of(Generator.generatePatientRecord(originalAuthor), Generator.generatePatientRecord(originalAuthor)); - toImport.forEach(r -> { - // Simulate that the records existed in another deployment from which they are imported - r.setKey(IdentificationUtils.generateKey()); - r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); - }); + final List toImport = generateRecordsToImport(originalAuthor); when(recordDao.exists(any())).thenReturn(false); final RecordImportResult result = sut.importRecords(toImport); @@ -87,9 +82,7 @@ void importRecordsSetsCurrentUserAsAuthorWhenTheyAreRegularUserAndImportsSpecifi } } - @Test - void importRecordsRetainsRecordProvenanceDataWhenCurrentUserIsAdmin() { - final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + private List generateRecordsToImport(User originalAuthor) { final List toImport = List.of(Generator.generatePatientRecord(originalAuthor), Generator.generatePatientRecord(originalAuthor)); toImport.forEach(r -> { @@ -97,6 +90,13 @@ void importRecordsRetainsRecordProvenanceDataWhenCurrentUserIsAdmin() { r.setKey(IdentificationUtils.generateKey()); r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); }); + return toImport; + } + + @Test + void importRecordsRetainsRecordProvenanceDataWhenCurrentUserIsAdmin() { + final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final List toImport = generateRecordsToImport(originalAuthor); user.addType(Vocabulary.s_c_administrator); Environment.setCurrentUser(user); when(userService.exists(originalAuthor.getUri())).thenReturn(true); @@ -112,19 +112,14 @@ void importRecordsRetainsRecordProvenanceDataWhenCurrentUserIsAdmin() { assertEquals(toImport.get(i).getKey(), imported.get(i).getKey()); assertEquals(originalAuthor, imported.get(i).getAuthor()); assertEquals(toImport.get(i).getDateCreated(), imported.get(i).getDateCreated()); + assertEquals(toImport.get(i).getPhase(), imported.get(i).getPhase()); } } @Test void importRecordsThrowsRecordAuthorNotFoundExceptionWhenAdminImportsRecordsAndRecordAuthorIsNotFound() { final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); - final List toImport = - List.of(Generator.generatePatientRecord(originalAuthor), Generator.generatePatientRecord(originalAuthor)); - toImport.forEach(r -> { - // Simulate that the records existed in another deployment from which they are imported - r.setKey(IdentificationUtils.generateKey()); - r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); - }); + final List toImport = generateRecordsToImport(originalAuthor); user.addType(Vocabulary.s_c_administrator); Environment.setCurrentUser(user); @@ -133,17 +128,11 @@ void importRecordsThrowsRecordAuthorNotFoundExceptionWhenAdminImportsRecordsAndR @Test void importRecordsSkipsImportingRecordsThatAlreadyExist() { - final List toImport = - List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final List toImport = generateRecordsToImport(originalAuthor); final PatientRecord existing = toImport.get(Generator.randomIndex(toImport)); + when(recordDao.exists(any(URI.class))).thenReturn(false); when(recordDao.exists(existing.getUri())).thenReturn(true); - toImport.forEach(r -> { - // Simulate that the records existed in another deployment from which they are imported - r.setKey(IdentificationUtils.generateKey()); - r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); - r.setAuthor(originalAuthor); - }); final RecordImportResult result = sut.importRecords(toImport); assertEquals(toImport.size(), result.getTotalCount()); @@ -154,16 +143,9 @@ void importRecordsSkipsImportingRecordsThatAlreadyExist() { @Test void importRecordsWithPhaseSetsSpecifiedPhaseToAllRecords() { - final List toImport = - List.of(Generator.generatePatientRecord(user), Generator.generatePatientRecord(user)); final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final List toImport = generateRecordsToImport(originalAuthor); final RecordPhase targetPhase = RecordPhase.values()[Generator.randomInt(0, RecordPhase.values().length)]; - toImport.forEach(r -> { - // Simulate that the records existed in another deployment from which they are imported - r.setKey(IdentificationUtils.generateKey()); - r.setDateCreated(new Date(System.currentTimeMillis() - 10000L)); - r.setAuthor(originalAuthor); - }); when(recordDao.exists(any())).thenReturn(false); sut.importRecords(toImport, targetPhase); @@ -171,4 +153,16 @@ void importRecordsWithPhaseSetsSpecifiedPhaseToAllRecords() { verify(recordDao, times(toImport.size())).persist(captor.capture()); captor.getAllValues().forEach(r -> assertEquals(targetPhase, r.getPhase())); } + + @Test + void importRecordsSetsRecordPhaseToOpenOnAllImportedRecordsWhenCurrentUserIsRegularUser() { + final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); + final List toImport = generateRecordsToImport(originalAuthor); + when(recordDao.exists(any())).thenReturn(false); + + sut.importRecords(toImport); + final ArgumentCaptor captor = ArgumentCaptor.forClass(PatientRecord.class); + verify(recordDao, times(toImport.size())).persist(captor.capture()); + captor.getAllValues().forEach(r -> assertEquals(RecordPhase.open, r.getPhase())); + } } \ No newline at end of file