From 3ee0b656ea211db19baa2f75321b0dd5b90569d4 Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Tue, 21 Apr 2020 16:03:50 -0500 Subject: [PATCH 01/12] updated controller to gracefully manage things if sample already exists --- .../RESTProjectSamplesController.java | 76 +++++++++++-------- 1 file changed, 46 insertions(+), 30 deletions(-) diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java index 63a34419a4f..da0e6af46df 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java @@ -1,5 +1,27 @@ package ca.corefacility.bioinformatics.irida.web.controller.api.projects; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import javax.servlet.http.HttpServletResponse; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.hateoas.Link; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; +import org.springframework.stereotype.Controller; +import org.springframework.ui.ModelMap; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.servlet.ModelAndView; +import org.springframework.web.servlet.view.RedirectView; + +import ca.corefacility.bioinformatics.irida.exceptions.EntityExistsException; import ca.corefacility.bioinformatics.irida.model.joins.Join; import ca.corefacility.bioinformatics.irida.model.project.Project; import ca.corefacility.bioinformatics.irida.model.sample.Sample; @@ -14,24 +36,8 @@ import ca.corefacility.bioinformatics.irida.web.controller.api.samples.RESTSampleAssemblyController; import ca.corefacility.bioinformatics.irida.web.controller.api.samples.RESTSampleMetadataController; import ca.corefacility.bioinformatics.irida.web.controller.api.samples.RESTSampleSequenceFilesController; -import com.google.common.net.HttpHeaders; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.hateoas.Link; -import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; -import org.springframework.stereotype.Controller; -import org.springframework.ui.ModelMap; -import org.springframework.web.bind.annotation.PathVariable; -import org.springframework.web.bind.annotation.RequestBody; -import org.springframework.web.bind.annotation.RequestMapping; -import org.springframework.web.bind.annotation.RequestMethod; -import org.springframework.web.servlet.ModelAndView; -import org.springframework.web.servlet.view.RedirectView; -import javax.servlet.http.HttpServletResponse; -import java.util.List; -import java.util.Map; -import java.util.Optional; +import com.google.common.net.HttpHeaders; import static org.springframework.hateoas.mvc.ControllerLinkBuilder.linkTo; import static org.springframework.hateoas.mvc.ControllerLinkBuilder.methodOn; @@ -43,6 +49,8 @@ */ @Controller public class RESTProjectSamplesController { + + private static final Logger logger = LoggerFactory.getLogger(RESTProjectSamplesController.class); /** * Rel to get to the project that this sample belongs to. @@ -97,28 +105,36 @@ public ModelMap copySampleToProject(final @PathVariable Long projectId, final @R sampleIds.size()); for (final long sampleId : sampleIds) { Sample sample = sampleService.read(sampleId); - Join r = projectService.addSampleToProject(p, sample, false); + Join join; + try { + join = projectService.addSampleToProject(p, sample, false); + } catch (EntityExistsException e) { + logger.warn("User tried to add a sample to a project where it already existed. project: " + projectId + + " sample: " + sampleId); + + join = sampleService.getSampleForProject(p, sampleId); + } LabelledRelationshipResource resource = new LabelledRelationshipResource( - r.getLabel(), r); + join.getLabel(), join); // add a labeled relationship resource to the resource collection // that will fill the body of the response. - resource.add(linkTo( - methodOn(RESTProjectSamplesController.class).getSample(sample.getId())) - .withSelfRel()); - resource.add(linkTo(methodOn(RESTProjectSamplesController.class).getProjectSample(projectId, sample.getId())).withRel(REL_PROJECT_SAMPLE)); - resource.add(linkTo( - methodOn(RESTSampleSequenceFilesController.class).getSampleSequenceFiles(sample.getId())) - .withRel(RESTSampleSequenceFilesController.REL_SAMPLE_SEQUENCE_FILES)); - resource.add(linkTo(RESTProjectsController.class).slash(projectId).withRel(REL_PROJECT)); + resource.add(linkTo(methodOn(RESTProjectSamplesController.class).getSample(sample.getId())).withSelfRel()); + resource.add(linkTo(methodOn(RESTProjectSamplesController.class).getProjectSample(projectId, + sample.getId())).withRel(REL_PROJECT_SAMPLE)); + resource.add(linkTo(methodOn(RESTSampleSequenceFilesController.class).getSampleSequenceFiles( + sample.getId())).withRel(RESTSampleSequenceFilesController.REL_SAMPLE_SEQUENCE_FILES)); + resource.add(linkTo(RESTProjectsController.class).slash(projectId) + .withRel(REL_PROJECT)); labeledProjectSampleResources.add(resource); final String location = linkTo( - methodOn(RESTProjectSamplesController.class).getProjectSample(projectId,sampleId)).withSelfRel() + methodOn(RESTProjectSamplesController.class).getProjectSample(projectId, sampleId)).withSelfRel() .getHref(); response.addHeader(HttpHeaders.LOCATION, location); + } // add a link to the project that was copied to. - labeledProjectSampleResources.add(linkTo( - methodOn(RESTProjectSamplesController.class).getProjectSamples(projectId)).withSelfRel()); + labeledProjectSampleResources.add( + linkTo(methodOn(RESTProjectSamplesController.class).getProjectSamples(projectId)).withSelfRel()); modelMap.addAttribute(RESTGenericController.RESOURCE_NAME, labeledProjectSampleResources); response.setStatus(HttpStatus.CREATED.value()); From 52f5801a1fc241e731d44c557ccc5dd1d85aada6 Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Tue, 21 Apr 2020 16:04:59 -0500 Subject: [PATCH 02/12] updated test for existing samples to new response --- .../RESTProjectSamplesControllerTest.java | 48 +++++++++++++++++-- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java index a2460d2ec00..6b49fc0c43f 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java @@ -275,17 +275,55 @@ public void testCopySampleToProject() { assertTrue("Rels should be empty after removing expected links", rels.isEmpty()); } - @Test(expected = EntityExistsException.class) - public void testAlreadyCopiedSampleToProject() { + @Test + public void testSampleAlreadyCopiedToProject() { final Project p = TestDataFactory.constructProject(); final Sample s = TestDataFactory.constructSample(); - + final ProjectSampleJoin r = new ProjectSampleJoin(p,s, true); + MockHttpServletResponse response = new MockHttpServletResponse(); when(projectService.read(p.getId())).thenReturn(p); when(sampleService.read(s.getId())).thenReturn(s); - when(projectService.addSampleToProject(p, s, false)).thenThrow(new EntityExistsException("sample already exists!")); + when(sampleService.getSampleForProject(p, s.getId())).thenReturn(r); - controller.copySampleToProject(p.getId(), Lists.newArrayList(s.getId()),new MockHttpServletResponse()); + ModelMap modelMap = controller + .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), response); + + verify(projectService).addSampleToProject(p, s, false); + verify(sampleService).getSampleForProject(p,s.getId()); + + assertEquals("response should have CREATED status", HttpStatus.CREATED.value(), response.getStatus()); + final String location = response.getHeader(HttpHeaders.LOCATION); + assertEquals("location should include sample and project IDs", "http://localhost/api/projects/" + p.getId() + + "/samples/" + s.getId(), location); + //test that the modelMap contains a correct resource collection. + Object o = modelMap.get(RESTGenericController.RESOURCE_NAME); + assertTrue("Object should be an instance of ResourceCollection",o instanceof ResourceCollection); + @SuppressWarnings("unchecked") + ResourceCollection> labeledRRs = + (ResourceCollection>) o; + assertEquals("There should be one item in the resource collection",1, labeledRRs.size()); + List resourceLinks = labeledRRs.getLinks(); + assertEquals("There should be one link",1, resourceLinks.size()); + Link self = resourceLinks.iterator().next(); + assertEquals("Self link should be correct","self", self.getRel()); + assertEquals("http://localhost/api/projects/" + p.getId() + "/samples", self.getHref()); + LabelledRelationshipResource resource = labeledRRs.iterator().next(); + Object o2 = resource.getResource(); + assertTrue("Object should be an instance of ProjectSampleJoin",o2 instanceof ProjectSampleJoin); + ProjectSampleJoin join = (ProjectSampleJoin)o2; + Object o3 = join.getObject(); + assertTrue("Object should be an instance of Sample",o3 instanceof Sample); + Sample sample = (Sample)o3; + assertEquals("Sample name should be correct",s.getSampleName(), sample.getSampleName()); + List links = resource.getLinks(); + Set rels = Sets.newHashSet(Link.REL_SELF, RESTSampleSequenceFilesController.REL_SAMPLE_SEQUENCE_FILES, + RESTProjectSamplesController.REL_PROJECT, RESTProjectSamplesController.REL_PROJECT_SAMPLE); + for (Link link : links) { + assertTrue("Rels should contain link [" + link + "]", rels.contains(link.getRel())); + assertNotNull("Rels should remove link [" + link + "]", rels.remove(link.getRel())); + } + assertTrue("Rels should be empty after removing expected links", rels.isEmpty()); } private Map linksToMap(List links) { From 544340ff53cc9cfd9d8193f686f36f287f86397f Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Tue, 21 Apr 2020 16:20:38 -0500 Subject: [PATCH 03/12] added rest sample update to CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86303479552..f951c9ded58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ Changes * [UI/Developer]: Added excel file viewer to analysis outputs view. * [UI]: Updated Project > Members page to remove `DataDables` and use Ant Design. * [UI/Developer]: Added image file viewer to analysis outputs view. +* [REST]: Updated REST API to handle errors when copying existing samples more gracefully. 19.09 to 20.01 -------------- From 9ce549cfc4219b565f89d777f9eb46ae48a87c9f Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Wed, 22 Apr 2020 07:35:24 -0500 Subject: [PATCH 04/12] checking that the sample POST works on a conflict --- .../test/integration/project/ProjectSamplesIT.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/integration/project/ProjectSamplesIT.java b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/integration/project/ProjectSamplesIT.java index e0aef5d6cf4..520a7a6b2ea 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/integration/project/ProjectSamplesIT.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/integration/project/ProjectSamplesIT.java @@ -82,8 +82,15 @@ public void testShareSampleToProjectWithSameId() { final String projectJson = asUser().get(projectUri).asString(); final String samplesUri = from(projectJson).get("resource.links.find{it.rel == 'project/samples'}.href"); - asUser().contentType(ContentType.JSON).body(samples).header("Content-Type", "application/idcollection+json") - .expect().response().statusCode(HttpStatus.CONFLICT.value()).when().post(samplesUri); + //this used to return CONFLICT, but ended up in errors where samples were partially added. Now it accepts the POST but doesn't change anything since the sample is already there + asUser().contentType(ContentType.JSON) + .body(samples) + .header("Content-Type", "application/idcollection+json") + .expect() + .response() + .statusCode(HttpStatus.CREATED.value()) + .when() + .post(samplesUri); } @Test From e971f6fd94916b8455960291c0b2c2c445079821 Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Wed, 22 Apr 2020 16:33:00 -0500 Subject: [PATCH 05/12] changed sample numbers because there's a duplicate in the test code --- .../controller/test/integration/project/ProjectSamplesIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/integration/project/ProjectSamplesIT.java b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/integration/project/ProjectSamplesIT.java index 520a7a6b2ea..170a42e8acb 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/integration/project/ProjectSamplesIT.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/integration/project/ProjectSamplesIT.java @@ -76,9 +76,9 @@ public void testShareSampleToProject() { @Test public void testShareSampleToProjectWithSameId() { - final List samples = Lists.newArrayList("3"); + final List samples = Lists.newArrayList("1"); - final String projectUri = "/api/projects/4"; + final String projectUri = "/api/projects/1"; final String projectJson = asUser().get(projectUri).asString(); final String samplesUri = from(projectJson).get("resource.links.find{it.rel == 'project/samples'}.href"); From 465a80c75e947b3fb6fa6ab38fbe136f19508bbf Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Fri, 24 Apr 2020 09:44:27 -0500 Subject: [PATCH 06/12] changed some logic so checking for sample in project before checking the sample id. this way we can differentiate the errors produced --- .../ExistingSampleNameException.java | 27 ++++++++ .../service/impl/ProjectServiceImpl.java | 10 ++- .../RESTProjectSamplesController.java | 63 ++++++++++++------- src/main/resources/i18n/messages.properties | 8 +++ .../RESTProjectSamplesControllerTest.java | 14 ++--- 5 files changed, 91 insertions(+), 31 deletions(-) create mode 100644 src/main/java/ca/corefacility/bioinformatics/irida/exceptions/ExistingSampleNameException.java diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/exceptions/ExistingSampleNameException.java b/src/main/java/ca/corefacility/bioinformatics/irida/exceptions/ExistingSampleNameException.java new file mode 100644 index 00000000000..7f1cad0d45a --- /dev/null +++ b/src/main/java/ca/corefacility/bioinformatics/irida/exceptions/ExistingSampleNameException.java @@ -0,0 +1,27 @@ +package ca.corefacility.bioinformatics.irida.exceptions; + +import ca.corefacility.bioinformatics.irida.model.sample.Sample; + +/** + * Exception thrown when trying to add a {@link ca.corefacility.bioinformatics.irida.model.sample.Sample} to a {@link ca.corefacility.bioinformatics.irida.model.project.Project} with a duplicate sample name + */ +public class ExistingSampleNameException extends EntityExistsException { + Sample sample; + + public ExistingSampleNameException(String message) { + super(message); + } + + public ExistingSampleNameException(String message, Throwable cause) { + super(message, cause); + } + + public ExistingSampleNameException(String message, Sample sample) { + super(message); + this.sample = sample; + } + + public Sample getSample() { + return sample; + } +} diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java b/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java index 3b8cb1d3356..5195e57e474 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java @@ -343,11 +343,17 @@ private boolean allowRoleChange(final Project project, final ProjectRole project public ProjectSampleJoin addSampleToProject(Project project, Sample sample, boolean owner) { logger.trace("Adding sample to project."); + if (psjRepository.readSampleForProject(project, sample) != null) { + throw new EntityExistsException( + "Sample [" + sample.getId() + "] has already been added to project [" + project.getId() + "]"); + } + // Check to ensure a sample with this sequencer id doesn't exist in this // project already if (sampleRepository.getSampleBySampleName(project, sample.getSampleName()) != null) { - throw new EntityExistsException( - "Sample with sequencer id '" + sample.getSampleName() + "' already exists in project " + project.getId()); + throw new ExistingSampleNameException( + "Sample with sequencer id '" + sample.getSampleName() + "' already exists in project " + + project.getId(), sample); } // the sample hasn't been persisted before, persist it before calling diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java index da0e6af46df..2bc9c0502d5 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java @@ -1,14 +1,13 @@ package ca.corefacility.bioinformatics.irida.web.controller.api.projects; -import java.util.List; -import java.util.Map; -import java.util.Optional; +import java.util.*; import javax.servlet.http.HttpServletResponse; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.MessageSource; import org.springframework.hateoas.Link; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; @@ -22,6 +21,7 @@ import org.springframework.web.servlet.view.RedirectView; import ca.corefacility.bioinformatics.irida.exceptions.EntityExistsException; +import ca.corefacility.bioinformatics.irida.exceptions.ExistingSampleNameException; import ca.corefacility.bioinformatics.irida.model.joins.Join; import ca.corefacility.bioinformatics.irida.model.project.Project; import ca.corefacility.bioinformatics.irida.model.sample.Sample; @@ -72,13 +72,17 @@ public class RESTProjectSamplesController { */ private SampleService sampleService; + private MessageSource messageSource; + protected RESTProjectSamplesController() { } @Autowired - public RESTProjectSamplesController(ProjectService projectService, SampleService sampleService) { + public RESTProjectSamplesController(ProjectService projectService, SampleService sampleService, + MessageSource messageSource) { this.projectService = projectService; this.sampleService = sampleService; + this.messageSource = messageSource; } /** @@ -95,47 +99,62 @@ public RESTProjectSamplesController(ProjectService projectService, SampleService */ @RequestMapping(value = "/api/projects/{projectId}/samples", method = RequestMethod.POST, consumes = "application/idcollection+json") public ModelMap copySampleToProject(final @PathVariable Long projectId, final @RequestBody List sampleIds, - HttpServletResponse response) { + HttpServletResponse response, Locale locale) { ModelMap modelMap = new ModelMap(); Project p = projectService.read(projectId); + List errors = new ArrayList<>(); + ResourceCollection> labeledProjectSampleResources = new ResourceCollection<>( sampleIds.size()); for (final long sampleId : sampleIds) { Sample sample = sampleService.read(sampleId); - Join join; + Join join = null; try { join = projectService.addSampleToProject(p, sample, false); + } catch (ExistingSampleNameException e) { + logger.error( + "Could not add sample to project because another sample exists with this name :" + e.getSample() + .getSampleName()); + errors.add(messageSource.getMessage("rest.api.project.samples.warning.duplicate", + new Object[] { sample.getId(), sample.getSampleName() }, locale)); } catch (EntityExistsException e) { logger.warn("User tried to add a sample to a project where it already existed. project: " + projectId + " sample: " + sampleId); + errors.add(messageSource.getMessage("rest.api.project.samples.warning.exists", + new Object[] { sample.getId(), sample.getSampleName() }, locale)); join = sampleService.getSampleForProject(p, sampleId); } - LabelledRelationshipResource resource = new LabelledRelationshipResource( - join.getLabel(), join); - // add a labeled relationship resource to the resource collection - // that will fill the body of the response. - resource.add(linkTo(methodOn(RESTProjectSamplesController.class).getSample(sample.getId())).withSelfRel()); - resource.add(linkTo(methodOn(RESTProjectSamplesController.class).getProjectSample(projectId, - sample.getId())).withRel(REL_PROJECT_SAMPLE)); - resource.add(linkTo(methodOn(RESTSampleSequenceFilesController.class).getSampleSequenceFiles( - sample.getId())).withRel(RESTSampleSequenceFilesController.REL_SAMPLE_SEQUENCE_FILES)); - resource.add(linkTo(RESTProjectsController.class).slash(projectId) - .withRel(REL_PROJECT)); - labeledProjectSampleResources.add(resource); - final String location = linkTo( - methodOn(RESTProjectSamplesController.class).getProjectSample(projectId, sampleId)).withSelfRel() - .getHref(); - response.addHeader(HttpHeaders.LOCATION, location); + + if (join != null) { + LabelledRelationshipResource resource = new LabelledRelationshipResource( + join.getLabel(), join); + // add a labeled relationship resource to the resource collection + // that will fill the body of the response. + resource.add( + linkTo(methodOn(RESTProjectSamplesController.class).getSample(sample.getId())).withSelfRel()); + resource.add(linkTo(methodOn(RESTProjectSamplesController.class).getProjectSample(projectId, + sample.getId())).withRel(REL_PROJECT_SAMPLE)); + resource.add(linkTo(methodOn(RESTSampleSequenceFilesController.class).getSampleSequenceFiles( + sample.getId())).withRel(RESTSampleSequenceFilesController.REL_SAMPLE_SEQUENCE_FILES)); + resource.add(linkTo(RESTProjectsController.class).slash(projectId) + .withRel(REL_PROJECT)); + labeledProjectSampleResources.add(resource); + final String location = linkTo(methodOn(RESTProjectSamplesController.class).getProjectSample(projectId, + sampleId)).withSelfRel() + .getHref(); + response.addHeader(HttpHeaders.LOCATION, location); + } } // add a link to the project that was copied to. labeledProjectSampleResources.add( linkTo(methodOn(RESTProjectSamplesController.class).getProjectSamples(projectId)).withSelfRel()); modelMap.addAttribute(RESTGenericController.RESOURCE_NAME, labeledProjectSampleResources); + modelMap.addAttribute("warnings", errors); response.setStatus(HttpStatus.CREATED.value()); return modelMap; diff --git a/src/main/resources/i18n/messages.properties b/src/main/resources/i18n/messages.properties index f0a7e5a7a67..c7b1ce085d2 100644 --- a/src/main/resources/i18n/messages.properties +++ b/src/main/resources/i18n/messages.properties @@ -1317,6 +1317,10 @@ grammar.of=of global.submit=Submit global.reset=Reset +# ========================================================================================== # +# REST API Messages # +# ========================================================================================== # + ##Login page login.api.title=Login to remote instance of irida IRIDA login.api.username=User name: @@ -1334,6 +1338,10 @@ auth.projects=Projects auth.samples=Samples auth.sequenceFiles=Sequence Files +##Project samples +rest.api.project.samples.warning.duplicate=Cannot add sample id {0} ({1}) as a sample with this name already exists in the project. +rest.api.project.samples.warning.exists=Cannot add sample id {0} ({1}) as this sample already exists in the project. + # ========================================================================================== # # Galaxy Exporter # # ========================================================================================== # diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java index 6b49fc0c43f..bef00b140f9 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java @@ -9,14 +9,12 @@ import static org.mockito.Mockito.when; import java.io.IOException; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; import ca.corefacility.bioinformatics.irida.web.controller.api.samples.RESTSampleAssemblyController; import org.junit.Before; import org.junit.Test; +import org.springframework.context.MessageSource; import org.springframework.hateoas.Link; import org.springframework.http.HttpStatus; import org.springframework.mock.web.MockHttpServletResponse; @@ -51,12 +49,14 @@ public class RESTProjectSamplesControllerTest { private RESTProjectSamplesController controller; private ProjectService projectService; private SampleService sampleService; + private MessageSource messageSource; @Before public void setUp() { projectService = mock(ProjectService.class); sampleService = mock(SampleService.class); - controller = new RESTProjectSamplesController(projectService, sampleService); + messageSource= mock(MessageSource.class); + controller = new RESTProjectSamplesController(projectService, sampleService, messageSource); } @Test @@ -238,7 +238,7 @@ public void testCopySampleToProject() { when(sampleService.read(s.getId())).thenReturn(s); when(projectService.addSampleToProject(p, s, false)).thenReturn(r); ModelMap modelMap = controller - .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), response); + .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), response, Locale.ENGLISH); verify(projectService).addSampleToProject(p, s, false); assertEquals("response should have CREATED status", HttpStatus.CREATED.value(), response.getStatus()); @@ -287,7 +287,7 @@ public void testSampleAlreadyCopiedToProject() { when(sampleService.getSampleForProject(p, s.getId())).thenReturn(r); ModelMap modelMap = controller - .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), response); + .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), response, Locale.ENGLISH); verify(projectService).addSampleToProject(p, s, false); verify(sampleService).getSampleForProject(p,s.getId()); From 1f6b1c9aa494f2a6057e058e027f81ed83565ab7 Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Tue, 28 Apr 2020 09:52:25 -0500 Subject: [PATCH 07/12] fixed missing param docs error --- .../api/projects/RESTProjectSamplesController.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java index 2bc9c0502d5..5a1e550b879 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java @@ -87,15 +87,13 @@ public RESTProjectSamplesController(ProjectService projectService, SampleService /** * Copy an existing sample to a project. - * - * @param projectId - * the project to copy the sample to. - * @param sampleIds - * the collection of sample IDs to copy. - * @param response - * a reference to the servlet response. + * + * @param projectId the project to copy the sample to. + * @param sampleIds the collection of sample IDs to copy. + * @param response a reference to the servlet response. + * @param locale The requesting user's locale for any warning messages * @return the response indicating that the sample was joined to the - * project. + * project. */ @RequestMapping(value = "/api/projects/{projectId}/samples", method = RequestMethod.POST, consumes = "application/idcollection+json") public ModelMap copySampleToProject(final @PathVariable Long projectId, final @RequestBody List sampleIds, From 2d763fed36c37dfa5c2ea2b4a6340c39bec171f0 Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Tue, 28 Apr 2020 11:00:55 -0500 Subject: [PATCH 08/12] fixed to check for if sample is persisted before checking if it's in the project. also not sending warnings if they're not needed in the REST request --- .../irida/service/impl/ProjectServiceImpl.java | 9 ++++++--- .../api/projects/RESTProjectSamplesController.java | 6 +++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java b/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java index 5195e57e474..5b147d07240 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java @@ -343,9 +343,12 @@ private boolean allowRoleChange(final Project project, final ProjectRole project public ProjectSampleJoin addSampleToProject(Project project, Sample sample, boolean owner) { logger.trace("Adding sample to project."); - if (psjRepository.readSampleForProject(project, sample) != null) { - throw new EntityExistsException( - "Sample [" + sample.getId() + "] has already been added to project [" + project.getId() + "]"); + //if the sample exists, ensure it isn't already on the project + if (sample.getId() != null) { + if (psjRepository.readSampleForProject(project, sample) != null) { + throw new EntityExistsException( + "Sample [" + sample.getId() + "] has already been added to project [" + project.getId() + "]"); + } } // Check to ensure a sample with this sequencer id doesn't exist in this diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java index 5a1e550b879..a3f86be3742 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java @@ -152,7 +152,11 @@ public ModelMap copySampleToProject(final @PathVariable Long projectId, final @R labeledProjectSampleResources.add( linkTo(methodOn(RESTProjectSamplesController.class).getProjectSamples(projectId)).withSelfRel()); modelMap.addAttribute(RESTGenericController.RESOURCE_NAME, labeledProjectSampleResources); - modelMap.addAttribute("warnings", errors); + + if (!errors.isEmpty()) { + modelMap.addAttribute("warnings", errors); + } + response.setStatus(HttpStatus.CREATED.value()); return modelMap; From 3dac8db64b994d88c9639c72f303965d0fa1fb01 Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Mon, 31 Aug 2020 09:13:33 -0500 Subject: [PATCH 09/12] missed doc for locale param --- .../controller/api/projects/RESTProjectSamplesController.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java index ca375fa3eb2..0d3c811efc2 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java @@ -91,6 +91,7 @@ public RESTProjectSamplesController(ProjectService projectService, SampleService * @param projectId the project to copy the sample to. * @param sampleIds the collection of sample IDs to copy. * @param response a reference to the servlet response. + * @param locale The user's in case a warning message is needed * @return the response indicating that the sample was joined to the * project. */ From 482c980bb846ccad6c2637f39474477294b57bb4 Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Fri, 11 Sep 2020 13:39:20 -0500 Subject: [PATCH 10/12] found a case where this method could unintentially be used to copy samples. adding a validation to ensure the sample is valid to stop this --- .../controller/api/projects/RESTProjectSamplesController.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java index 0d3c811efc2..c60fe2677eb 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java @@ -3,6 +3,7 @@ import java.util.*; import javax.servlet.http.HttpServletResponse; +import javax.validation.Valid; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -174,7 +175,7 @@ public ModelMap copySampleToProject(final @PathVariable Long projectId, final @R * location information. */ @RequestMapping(value = "/api/projects/{projectId}/samples", method = RequestMethod.POST, consumes = "!application/idcollection+json") - public ModelMap addSampleToProject(@PathVariable Long projectId, @RequestBody Sample sample, + public ModelMap addSampleToProject(@PathVariable Long projectId, @RequestBody @Valid Sample sample, HttpServletResponse response) { ModelMap model = new ModelMap(); From a50d152d43cf2165cabaea4417749207d54a30b2 Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Fri, 11 Sep 2020 14:48:58 -0500 Subject: [PATCH 11/12] changed the overloading of addSampleToProject to only accept non-persisted samples. Added another method for adding existing samples to a project --- .../web/services/UIProjectSampleService.java | 2 +- .../irida/service/ProjectService.java | 9 ++-- .../service/impl/ProjectServiceImpl.java | 54 ++++++++++++------- .../remote/ProjectSynchronizationService.java | 2 +- .../RESTProjectSamplesController.java | 12 ++--- .../ProjectSamplesControllerTest.java | 3 +- .../ProjectSynchronizationServiceTest.java | 6 +-- .../ProjectEventServiceImplIT.java | 4 +- .../integration/ProjectServiceImplIT.java | 13 +++-- .../impl/unit/ProjectServiceImplTest.java | 13 +++-- .../RESTProjectSamplesControllerTest.java | 16 +++--- 11 files changed, 74 insertions(+), 60 deletions(-) diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/ria/web/services/UIProjectSampleService.java b/src/main/java/ca/corefacility/bioinformatics/irida/ria/web/services/UIProjectSampleService.java index 319cb28a57a..2ce82b6d138 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/ria/web/services/UIProjectSampleService.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/ria/web/services/UIProjectSampleService.java @@ -96,7 +96,7 @@ public ResponseEntity createSample(CreateSampleRequest request, lo if (!Strings.isNullOrEmpty(request.getOrganism())) { sample.setOrganism(request.getOrganism()); } - Join join = projectService.addSampleToProject(project, sample, true); + Join join = projectService.createNewSampleInProject(project, sample); return ResponseEntity.ok(new AjaxCreateItemSuccessResponse(join.getObject() .getId())); } catch (EntityNotFoundException e) { diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/service/ProjectService.java b/src/main/java/ca/corefacility/bioinformatics/irida/service/ProjectService.java index cb37a66b20f..7fdc52543ce 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/service/ProjectService.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/service/ProjectService.java @@ -5,6 +5,8 @@ import java.util.Map; import java.util.Set; +import javax.validation.Valid; + import org.springframework.data.domain.Page; import org.springframework.data.domain.Sort; import org.springframework.data.domain.Sort.Direction; @@ -126,15 +128,16 @@ public Join updateUserGroupProjectRole(Project project, User ProjectRole projectRole) throws ProjectWithoutOwnerException; /** - * Add the specified {@link Sample} to the {@link Project}. + * Create a new {@link Sample} to the given {@link Project}. * * @param project the {@link Project} to add the {@link Sample} to. * @param sample the {@link Sample} to add to the {@link Project}. If the {@link Sample} has not previously been persisted, the * service will persist the {@link Sample}. - * @param owner Whether the project will have modification access for this sample * @return a reference to the relationship resource created between the two entities. */ - public Join addSampleToProject(Project project, Sample sample, boolean owner); + public Join createNewSampleInProject(Project project, Sample sample); + + public ProjectSampleJoin addExistingSampleToProject(Project project, @Valid Sample sample, boolean owner); /** * Move a {@link Sample} from one {@link Project} to another diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java b/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java index 5b147d07240..c3819bfca04 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java @@ -6,6 +6,7 @@ import javax.persistence.criteria.*; import javax.validation.ConstraintViolation; import javax.validation.ConstraintViolationException; +import javax.validation.Valid; import javax.validation.Validator; import org.slf4j.Logger; @@ -340,15 +341,12 @@ private boolean allowRoleChange(final Project project, final ProjectRole project @Transactional @LaunchesProjectEvent(SampleAddedProjectEvent.class) @PreAuthorize("hasAnyRole('ROLE_ADMIN', 'ROLE_SEQUENCER') or (hasPermission(#project, 'isProjectOwner'))") - public ProjectSampleJoin addSampleToProject(Project project, Sample sample, boolean owner) { + public ProjectSampleJoin createNewSampleInProject(Project project, Sample sample) { logger.trace("Adding sample to project."); //if the sample exists, ensure it isn't already on the project if (sample.getId() != null) { - if (psjRepository.readSampleForProject(project, sample) != null) { - throw new EntityExistsException( - "Sample [" + sample.getId() + "] has already been added to project [" + project.getId() + "]"); - } + throw new IllegalArgumentException("Samples provided to this method must not have been persisted to the database"); } // Check to ensure a sample with this sequencer id doesn't exist in this @@ -359,19 +357,37 @@ public ProjectSampleJoin addSampleToProject(Project project, Sample sample, bool + project.getId(), sample); } - // the sample hasn't been persisted before, persist it before calling - // the relationshipRepository. - if (sample.getId() == null) { - logger.trace("Going to validate and persist sample prior to creating relationship."); - // validate the sample, then persist it: - Set> constraintViolations = validator.validate(sample); - if (constraintViolations.isEmpty()) { - sample = sampleRepository.save(sample); - } else { - throw new ConstraintViolationException(constraintViolations); - } + logger.trace("Going to validate and persist sample prior to creating relationship."); + // validate the sample, then persist it: + Set> constraintViolations = validator.validate(sample); + if (constraintViolations.isEmpty()) { + sample = sampleRepository.save(sample); + } else { + throw new ConstraintViolationException(constraintViolations); } + ProjectSampleJoin join = new ProjectSampleJoin(project, sample, true); + + try { + return psjRepository.save(join); + } catch (DataIntegrityViolationException e) { + throw new EntityExistsException( + "Sample [" + sample.getId() + "] has already been added to project [" + project.getId() + "]"); + } + } + + @PreAuthorize("hasPermission(#project, 'isProjectOwner') and hasPermission(#sample, 'canReadSample')" + + " and ((not #giveOwner) or hasPermission(#sample, 'canUpdateSample'))") + public ProjectSampleJoin addExistingSampleToProject(Project project, @Valid Sample sample, boolean owner) { + logger.trace("Adding sample to project."); + + // Check to ensure a sample with this sequencer id doesn't exist in this + // project already + if (sampleRepository.getSampleBySampleName(project, sample.getSampleName()) != null) { + throw new ExistingSampleNameException( + "Sample with sequencer id '" + sample.getSampleName() + "' already exists in project " + + project.getId(), sample); + } ProjectSampleJoin join = new ProjectSampleJoin(project, sample, owner); try { @@ -394,7 +410,7 @@ public ProjectSampleJoin moveSampleBetweenProjects(Project source, Project desti ProjectSampleJoin projectSampleJoin = psjRepository.readSampleForProject(source, sample); //set the ownership on the sample given the existing permissions - ProjectSampleJoin join = addSampleToProject(destination, sample, projectSampleJoin.isOwner()); + ProjectSampleJoin join = addExistingSampleToProject(destination, sample, projectSampleJoin.isOwner()); //remove the old join removeSampleFromProject(source, sample); @@ -417,7 +433,7 @@ public List shareSamples(Project source, Project destination, List newJoins = new ArrayList<>(); for (Sample sample : samples) { - ProjectSampleJoin newJoin = addSampleToProject(destination, sample, giveOwner); + ProjectSampleJoin newJoin = addExistingSampleToProject(destination, sample, giveOwner); logger.trace("Shared sample " + sample.getId() + " to project " + destination.getId()); @@ -742,7 +758,7 @@ public Project createProjectWithSamples(Project project, List sampleIds, b sampleIds.forEach(sid -> { Sample s = sampleRepository.findById(sid).orElse(null); - addSampleToProject(project, s, owner); + addExistingSampleToProject(project, s, owner); }); return created; diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/service/remote/ProjectSynchronizationService.java b/src/main/java/ca/corefacility/bioinformatics/irida/service/remote/ProjectSynchronizationService.java index 5c725f08dee..d61a2795e63 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/service/remote/ProjectSynchronizationService.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/service/remote/ProjectSynchronizationService.java @@ -302,7 +302,7 @@ public List syncSample(Sample sample, Project p // if the sample doesn't already exist create it sample.getRemoteStatus().setSyncStatus(SyncStatus.UPDATING); localSample = sampleService.create(sample); - projectService.addSampleToProject(project, sample, true); + projectService.createNewSampleInProject(project, sample); } //get a collection of the files already sync'd. we don't want to grab them a 2nd time. diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java index c60fe2677eb..dbd0292b37c 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java @@ -14,10 +14,7 @@ import org.springframework.http.MediaType; import org.springframework.stereotype.Controller; import org.springframework.ui.ModelMap; -import org.springframework.web.bind.annotation.PathVariable; -import org.springframework.web.bind.annotation.RequestBody; -import org.springframework.web.bind.annotation.RequestMapping; -import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.*; import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.view.RedirectView; @@ -98,7 +95,8 @@ public RESTProjectSamplesController(ProjectService projectService, SampleService */ @RequestMapping(value = "/api/projects/{projectId}/samples", method = RequestMethod.POST, consumes = "application/idcollection+json") public ModelMap copySampleToProject(final @PathVariable Long projectId, final @RequestBody List sampleIds, - HttpServletResponse response, Locale locale) { + @RequestParam(required = false, defaultValue = "false") boolean owner, HttpServletResponse response, + Locale locale) { ModelMap modelMap = new ModelMap(); @@ -112,7 +110,7 @@ public ModelMap copySampleToProject(final @PathVariable Long projectId, final @R Sample sample = sampleService.read(sampleId); Join join = null; try { - join = projectService.addSampleToProject(p, sample, false); + join = projectService.addExistingSampleToProject(p, sample, owner); } catch (ExistingSampleNameException e) { logger.error( "Could not add sample to project because another sample exists with this name :" + e.getSample() @@ -183,7 +181,7 @@ public ModelMap addSampleToProject(@PathVariable Long projectId, @RequestBody @V Project p = projectService.read(projectId); // add the sample to the project - Join r = projectService.addSampleToProject(p, sample, true); + Join r = projectService.createNewSampleInProject(p, sample); // construct a link to the sample itself on the samples controller Long sampleId = r.getObject() diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/ria/unit/web/projects/ProjectSamplesControllerTest.java b/src/test/java/ca/corefacility/bioinformatics/irida/ria/unit/web/projects/ProjectSamplesControllerTest.java index 827ade49845..fd625c1e9c1 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/ria/unit/web/projects/ProjectSamplesControllerTest.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/ria/unit/web/projects/ProjectSamplesControllerTest.java @@ -47,7 +47,6 @@ import ca.corefacility.bioinformatics.irida.service.ProjectService; import ca.corefacility.bioinformatics.irida.service.SequencingObjectService; import ca.corefacility.bioinformatics.irida.service.sample.SampleService; -import ca.corefacility.bioinformatics.irida.service.user.UserService; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; @@ -204,7 +203,7 @@ public void testDeleteProjectSamples() { Project project1 = getProject(); Sample sample = new Sample("test"); sample.setId(1L); - projectService.addSampleToProject(project1, sample, true); + projectService.createNewSampleInProject(project1, sample); List idList = new ArrayList<>(); idList.add(1L); when(projectService.read(PROJECT_ID)).thenReturn(project1); diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/service/ProjectSynchronizationServiceTest.java b/src/test/java/ca/corefacility/bioinformatics/irida/service/ProjectSynchronizationServiceTest.java index 6ab0706b220..dee563564f6 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/service/ProjectSynchronizationServiceTest.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/service/ProjectSynchronizationServiceTest.java @@ -4,7 +4,7 @@ import ca.corefacility.bioinformatics.irida.exceptions.IridaOAuthException; import ca.corefacility.bioinformatics.irida.model.assembly.UploadedAssembly; -import ca.corefacility.bioinformatics.irida.service.impl.TestEmailController; + import org.junit.Before; import org.junit.Test; import org.mockito.Mock; @@ -167,7 +167,7 @@ public void testSyncNewSample() { syncService.syncSample(sample, expired, Maps.newHashMap()); - verify(projectService).addSampleToProject(expired, sample, true); + verify(projectService).createNewSampleInProject(expired, sample); assertEquals(SyncStatus.SYNCHRONIZED,sample.getRemoteStatus().getSyncStatus()); } @@ -185,7 +185,7 @@ public void testExistingSample(){ syncService.syncSample(sample, expired, ImmutableMap.of("http://sample",existingSample)); - verify(projectService,times(0)).addSampleToProject(expired, sample, true); + verify(projectService,times(0)).createNewSampleInProject(expired, sample); verify(sampleService,times(2)).update(any(Sample.class)); } diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectEventServiceImplIT.java b/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectEventServiceImplIT.java index f071c198af4..9dfa0311fcd 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectEventServiceImplIT.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectEventServiceImplIT.java @@ -129,7 +129,7 @@ public void testAddProjectSample() { Sample sample = sampleService.read(2L); - projectService.addSampleToProject(project, sample, true); + projectService.createNewSampleInProject(project, sample); Page eventsForProject = projectEventService.getEventsForProject(project, PageRequest.of(0, 10)); @@ -150,7 +150,7 @@ public void testErrorThrownNoEvent() { Sample sample = sampleService.read(1L); try { - projectService.addSampleToProject(project, sample, true); + projectService.createNewSampleInProject(project, sample); fail("EntityExistsException should have been thrown"); } catch (EntityExistsException ex) { // it's all good diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectServiceImplIT.java b/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectServiceImplIT.java index 43435327646..b4bfa0399fd 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectServiceImplIT.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectServiceImplIT.java @@ -11,7 +11,6 @@ import java.util.Set; import java.util.stream.Collectors; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; @@ -303,7 +302,7 @@ public void testAddSampleToProject() { Sample s = sampleService.read(1L); Project p = projectService.read(1L); - Join join = projectService.addSampleToProject(p, s, true); + Join join = projectService.createNewSampleInProject(p, s); assertEquals("Project should equal original project.", p, join.getSubject()); assertEquals("Sample should equal orginal sample.", s, join.getObject()); @@ -317,8 +316,8 @@ public void testAddSampleToProjectTwice() { Sample s = sampleService.read(1L); Project p = projectService.read(1L); - projectService.addSampleToProject(p, s, true); - projectService.addSampleToProject(p, s, true); + projectService.createNewSampleInProject(p, s); + projectService.createNewSampleInProject(p, s); } @Test(expected = EntityExistsException.class) @@ -327,11 +326,11 @@ public void testAddSampleToProjectWithSameSequencerId() { Sample s = sampleService.read(1L); Project p = projectService.read(1L); - projectService.addSampleToProject(p, s, true); + projectService.createNewSampleInProject(p, s); Sample otherSample = new Sample(s.getSampleName()); - projectService.addSampleToProject(p, otherSample, true); + projectService.createNewSampleInProject(p, otherSample); // if 2 exist with the same id, this call will fail Sample sampleBySequencerSampleId = sampleService.getSampleBySampleName(p, otherSample.getSampleName()); @@ -385,7 +384,7 @@ public void testAddSampleToProjectAsSequencer() { Project p = projectService.read(1L); Sample s = s(); - Join join = projectService.addSampleToProject(p, s, true); + Join join = projectService.createNewSampleInProject(p, s); assertNotNull("Join should not be empty.", join); assertEquals("Wrong project in join.", p, join.getSubject()); assertEquals("Wrong sample in join.", s, join.getObject()); diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/unit/ProjectServiceImplTest.java b/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/unit/ProjectServiceImplTest.java index 7602cb431fd..8cd40cc1c68 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/unit/ProjectServiceImplTest.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/unit/ProjectServiceImplTest.java @@ -28,7 +28,6 @@ import org.springframework.dao.DataIntegrityViolationException; import org.springframework.data.domain.Page; import org.springframework.data.domain.PageImpl; -import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.domain.Specification; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; @@ -131,14 +130,14 @@ public void testCreateProject() { public void testAddSampleToProject() { Sample s = new Sample(); s.setSampleName("sample"); - s.setId(2222L); Project p = project(); ProjectSampleJoin join = new ProjectSampleJoin(p, s, true); when(psjRepository.save(join)).thenReturn(join); + when(sampleRepository.save(any(Sample.class))).thenReturn(s); - Join rel = projectService.addSampleToProject(p, s, true); + Join rel = projectService.createNewSampleInProject(p, s); verify(psjRepository).save(join); verify(sampleRepository).getSampleBySampleName(p, s.getSampleName()); @@ -187,7 +186,7 @@ public void testAddSampleToProjectNoSamplePersisted() { when(validator.validate(s)).thenReturn(noViolations); when(sampleRepository.save(s)).thenReturn(s); - projectService.addSampleToProject(p, s, true); + projectService.createNewSampleInProject(p, s); verify(sampleRepository).save(s); verify(psjRepository).save(new ProjectSampleJoin(p, s, true)); @@ -204,7 +203,7 @@ public void testAddSampleToProjectNoSamplePersistedInvalidSample() { when(validator.validate(s)).thenReturn(violations); - projectService.addSampleToProject(p, s, true); + projectService.createNewSampleInProject(p, s); verifyZeroInteractions(sampleRepository, psjRepository); } @@ -221,7 +220,7 @@ public void testAddSampleToProjectAlreadyAdded() { when(psjRepository.save(any(ProjectSampleJoin.class))).thenThrow( new DataIntegrityViolationException("duplicate")); - projectService.addSampleToProject(p, s, true); + projectService.createNewSampleInProject(p, s); verify(sampleRepository).save(s); } @@ -235,7 +234,7 @@ public void testAddSampleWithSameSequencerId() { when(sampleRepository.getSampleBySampleName(p, s.getSampleName())).thenReturn(otherSample); - projectService.addSampleToProject(p, s, true); + projectService.createNewSampleInProject(p, s); } @SuppressWarnings("unchecked") diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java index e884437ddbc..46163b3d373 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/web/controller/test/unit/project/RESTProjectSamplesControllerTest.java @@ -62,7 +62,7 @@ public void testAddSampleToProject() { Join r = new ProjectSampleJoin(p, s, true); when(projectService.read(p.getId())).thenReturn(p); - when(projectService.addSampleToProject(p, s, true)).thenReturn(r); + when(projectService.createNewSampleInProject(p, s)).thenReturn(r); ModelMap modelMap = controller.addSampleToProject(p.getId(), s, response); @@ -71,7 +71,7 @@ public void testAddSampleToProject() { assertTrue("ModelMap should contan a SampleResource",o instanceof Sample); verify(projectService, times(1)).read(p.getId()); - verify(projectService, times(1)).addSampleToProject(p, s, true); + verify(projectService, times(1)).createNewSampleInProject(p, s); Link selfLink = s.getLink(Link.REL_SELF); Link sequenceFilesLink = s.getLink(RESTSampleSequenceFilesController.REL_SAMPLE_SEQUENCE_FILES); @@ -232,11 +232,11 @@ public void testCopySampleToProject() { MockHttpServletResponse response = new MockHttpServletResponse(); when(projectService.read(p.getId())).thenReturn(p); when(sampleService.read(s.getId())).thenReturn(s); - when(projectService.addSampleToProject(p, s, false)).thenReturn(r); + when(projectService.addExistingSampleToProject(p, s, false)).thenReturn(r); ModelMap modelMap = controller - .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), response, Locale.ENGLISH); + .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), false, response, Locale.ENGLISH); - verify(projectService).addSampleToProject(p, s, false); + verify(projectService).addExistingSampleToProject(p, s, false); assertEquals("response should have CREATED status", HttpStatus.CREATED.value(), response.getStatus()); final String location = response.getHeader(HttpHeaders.LOCATION); assertEquals("location should include sample and project IDs", "http://localhost/api/projects/" + p.getId() @@ -279,13 +279,13 @@ public void testSampleAlreadyCopiedToProject() { MockHttpServletResponse response = new MockHttpServletResponse(); when(projectService.read(p.getId())).thenReturn(p); when(sampleService.read(s.getId())).thenReturn(s); - when(projectService.addSampleToProject(p, s, false)).thenThrow(new EntityExistsException("sample already exists!")); + when(projectService.addExistingSampleToProject(p, s, false)).thenThrow(new EntityExistsException("sample already exists!")); when(sampleService.getSampleForProject(p, s.getId())).thenReturn(r); ModelMap modelMap = controller - .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), response, Locale.ENGLISH); + .copySampleToProject(p.getId(), Lists.newArrayList(s.getId()), false, response, Locale.ENGLISH); - verify(projectService).addSampleToProject(p, s, false); + verify(projectService).addExistingSampleToProject(p, s, false); verify(sampleService).getSampleForProject(p,s.getId()); assertEquals("response should have CREATED status", HttpStatus.CREATED.value(), response.getStatus()); From cd245c770610afd53a8b8430c046a888d2155eba Mon Sep 17 00:00:00 2001 From: Tom Matthews Date: Fri, 11 Sep 2020 15:12:14 -0500 Subject: [PATCH 12/12] fixed docs and IT tests --- .../bioinformatics/irida/service/ProjectService.java | 8 ++++++++ .../irida/service/impl/ProjectServiceImpl.java | 6 ++++++ .../api/projects/RESTProjectSamplesController.java | 3 ++- .../service/impl/integration/ProjectServiceImplIT.java | 10 +++++----- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/service/ProjectService.java b/src/main/java/ca/corefacility/bioinformatics/irida/service/ProjectService.java index 7fdc52543ce..286bf7c7b57 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/service/ProjectService.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/service/ProjectService.java @@ -137,6 +137,14 @@ public Join updateUserGroupProjectRole(Project project, User */ public Join createNewSampleInProject(Project project, Sample sample); + /** + * Share an existing sample to a given project + * + * @param project the project to share the sample to + * @param sample the sample to share + * @param owner whether to grant modification access to the sample in the new project + * @return the newly created join + */ public ProjectSampleJoin addExistingSampleToProject(Project project, @Valid Sample sample, boolean owner); /** diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java b/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java index c3819bfca04..43c0bb164e5 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/service/impl/ProjectServiceImpl.java @@ -376,6 +376,12 @@ public ProjectSampleJoin createNewSampleInProject(Project project, Sample sample } } + /** + * {@inheritDoc} + */ + @Override + @Transactional + @LaunchesProjectEvent(SampleAddedProjectEvent.class) @PreAuthorize("hasPermission(#project, 'isProjectOwner') and hasPermission(#sample, 'canReadSample')" + " and ((not #giveOwner) or hasPermission(#sample, 'canUpdateSample'))") public ProjectSampleJoin addExistingSampleToProject(Project project, @Valid Sample sample, boolean owner) { diff --git a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java index dbd0292b37c..dab8f1ef11a 100644 --- a/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java +++ b/src/main/java/ca/corefacility/bioinformatics/irida/web/controller/api/projects/RESTProjectSamplesController.java @@ -88,8 +88,9 @@ public RESTProjectSamplesController(ProjectService projectService, SampleService * * @param projectId the project to copy the sample to. * @param sampleIds the collection of sample IDs to copy. + * @param owner Whether the new project should have write access to this sample * @param response a reference to the servlet response. - * @param locale The user's in case a warning message is needed + * @param locale The user's in case a warning message is needed * @return the response indicating that the sample was joined to the * project. */ diff --git a/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectServiceImplIT.java b/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectServiceImplIT.java index b4bfa0399fd..9def5fc9153 100644 --- a/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectServiceImplIT.java +++ b/src/test/java/ca/corefacility/bioinformatics/irida/service/impl/integration/ProjectServiceImplIT.java @@ -298,11 +298,11 @@ public void testGetProjectsForUser() { @Test @WithMockUser(username = "admin", roles = "ADMIN") - public void testAddSampleToProject() { + public void testAddExistingSampleToProject() { Sample s = sampleService.read(1L); Project p = projectService.read(1L); - Join join = projectService.createNewSampleInProject(p, s); + Join join = projectService.addExistingSampleToProject(p, s, false); assertEquals("Project should equal original project.", p, join.getSubject()); assertEquals("Sample should equal orginal sample.", s, join.getObject()); @@ -316,8 +316,8 @@ public void testAddSampleToProjectTwice() { Sample s = sampleService.read(1L); Project p = projectService.read(1L); - projectService.createNewSampleInProject(p, s); - projectService.createNewSampleInProject(p, s); + projectService.addExistingSampleToProject(p, s, false); + projectService.addExistingSampleToProject(p, s, false); } @Test(expected = EntityExistsException.class) @@ -326,7 +326,7 @@ public void testAddSampleToProjectWithSameSequencerId() { Sample s = sampleService.read(1L); Project p = projectService.read(1L); - projectService.createNewSampleInProject(p, s); + projectService.addExistingSampleToProject(p, s, false); Sample otherSample = new Sample(s.getSampleName());