diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/kit/KitDao.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/kit/KitDao.java index 98ca6dfd7b..0f08053b5d 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/kit/KitDao.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/kit/KitDao.java @@ -631,10 +631,12 @@ private Optional insertKitTrackingIfNotExists(KitRequestShipping kit String errorMessage = String.format("Unable to insert tracking %s for %s.", kitRequestShipping.getTrackingId(), kitRequestShipping.getKitLabel()); SimpleResult results = inTransaction((conn) -> { + // Preferred approach is to check for existing values up front instead of relying on SQLException handling. SimpleResult dbVals = new SimpleResult(); try (PreparedStatement stmt = conn.prepareStatement(SELECT_KIT_TRACKING_BY_KIT_LABEL_OR_TRACKING_ID)) { // If a kit exists with the given kit label or tracking id, the unique key will be violated - // on insert. Return an error to the user with some information about the existing row. + // on insert. First check here for previous existence and return an error to the user with + // some information about the existing values. stmt.setString(1, kitRequestShipping.getKitLabel()); stmt.setString(2, kitRequestShipping.getTrackingId()); try (ResultSet rs = stmt.executeQuery()) { @@ -654,7 +656,10 @@ private Optional insertKitTrackingIfNotExists(KitRequestShipping kit logger.error(errorMessage, ex); return dbVals; } - + if (dbVals.resultValue != null) { + // an existing value was found, return it and not try to insert + return dbVals; + } try (PreparedStatement stmt = conn.prepareStatement(INSERT_KIT_TRACKING)) { stmt.setLong(1, System.currentTimeMillis()); stmt.setInt(2, userId); diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/statics/ApplicationConfigConstants.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/statics/ApplicationConfigConstants.java index 4fba2db28f..aa5aae348e 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/statics/ApplicationConfigConstants.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/statics/ApplicationConfigConstants.java @@ -88,7 +88,6 @@ public class ApplicationConfigConstants { public static final String GET_ALLOWED_REALMS_FOR_USER_ROLE_STARTS_LIKE = "portal.selectAllowedRealmsStartsLike"; public static final String GET_ROLES_LIKE = "portal.getRoles"; public static final String UPDATE_KIT_REQUEST = "portal.updateKitRequest"; - public static final String INSERT_KIT_TRACKING = "portal.insertKitTrackingRequest"; public static final String UPDATE_KIT_ERROR = "portal.updateKitRequestError"; public static final String GET_DDP_PARTICIPANT_ID = "portal.getDDPParticipantId"; public static final String GET_DASHBOARD_INFORMATION_OF_KIT_REQUESTS = "portal.dashboardKitRequests"; diff --git a/pepper-apis/dsm-core/src/main/resources/application.conf b/pepper-apis/dsm-core/src/main/resources/application.conf index 786b627697..6e099dfc15 100644 --- a/pepper-apis/dsm-core/src/main/resources/application.conf +++ b/pepper-apis/dsm-core/src/main/resources/application.conf @@ -168,16 +168,6 @@ portal { and deactivated_date is null """ - insertKitTrackingRequest:""" - insert into - ddp_kit_tracking - set - scan_date = ?, - scan_by = ?, - tracking_id = ?, - kit_label = ? - """ - checkKitTypeNeedsTrackingQuery:""" select kt.requires_insert_in_kit_tracking as found from ddp_kit_request request diff --git a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/db/dao/KitDaoTest.java b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/db/dao/kit/KitDaoTest.java similarity index 74% rename from pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/db/dao/KitDaoTest.java rename to pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/db/dao/kit/KitDaoTest.java index cebc4776f4..f146a61f77 100644 --- a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/db/dao/KitDaoTest.java +++ b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/db/dao/kit/KitDaoTest.java @@ -1,19 +1,25 @@ -package org.broadinstitute.dsm.db.dao; +package org.broadinstitute.dsm.db.dao.kit; import static org.broadinstitute.dsm.service.admin.UserAdminService.USER_ADMIN_ROLE; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import java.util.ArrayList; import java.util.List; import java.util.Optional; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.Appender; import lombok.extern.slf4j.Slf4j; import org.broadinstitute.dsm.DbTxnBaseTest; import org.broadinstitute.dsm.db.KitRequestShipping; import org.broadinstitute.dsm.db.dao.ddp.instance.DDPInstanceDao; import org.broadinstitute.dsm.db.dao.ddp.participant.ParticipantDao; -import org.broadinstitute.dsm.db.dao.kit.KitDao; -import org.broadinstitute.dsm.db.dao.kit.KitTypeDao; -import org.broadinstitute.dsm.db.dao.kit.KitTypeImpl; import org.broadinstitute.dsm.db.dto.ddp.instance.DDPInstanceDto; import org.broadinstitute.dsm.db.dto.ddp.participant.ParticipantDto; import org.broadinstitute.dsm.db.dto.kit.KitTypeDto; @@ -26,8 +32,12 @@ import org.junit.After; import org.junit.AfterClass; import org.junit.Assert; +import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.slf4j.LoggerFactory; @Slf4j public class KitDaoTest extends DbTxnBaseTest { @@ -61,6 +71,16 @@ public class KitDaoTest extends DbTxnBaseTest { private static KitRequestShipping kitReq; + @Mock + private Appender mockAppender; + + private Logger logger; + + @Before + public void setUp() throws Exception { + MockitoAnnotations.openMocks(this); + } + @BeforeClass public static void insertTestKit() { userAdminTestUtil.createRealmAndStudyGroup(KIT_NAME, KIT_NAME, KIT_NAME, KIT_NAME, KIT_NAME); @@ -115,13 +135,13 @@ public static void deleteTestKit() { } private void insertKitTrackingAndRunAssertions(boolean shouldTrackingRowExistAndScanErrorShouldExist) { - Assert.assertEquals(shouldTrackingRowExistAndScanErrorShouldExist, kitDao.hasTrackingScan(KIT_NAME)); + assertEquals(shouldTrackingRowExistAndScanErrorShouldExist, kitDao.hasTrackingScan(KIT_NAME)); Optional scanError = kitDao.insertKitTrackingIfNotExists(KIT_NAME, TRACKING_RETURN_ID, userId); - Assert.assertEquals(shouldTrackingRowExistAndScanErrorShouldExist, !scanError.isEmpty()); + assertEquals(shouldTrackingRowExistAndScanErrorShouldExist, !scanError.isEmpty()); if (shouldTrackingRowExistAndScanErrorShouldExist) { - Assert.assertTrue(scanError.get().hasError()); + assertTrue(scanError.get().hasError()); } - Assert.assertTrue(kitDao.hasTrackingScan(KIT_NAME)); + assertTrue(kitDao.hasTrackingScan(KIT_NAME)); } @After @@ -148,7 +168,7 @@ public void testKitScanInfoUpdateWithAndWithoutExistingScanInfo() { // create a kit Optional kitInsertScanResult = kitDao.insertKitTrackingIfNotExists(KIT_NAME, TRACKING_RETURN_ID, userId); - Assert.assertTrue(kitInsertScanResult.isEmpty()); + assertTrue(kitInsertScanResult.isEmpty()); // now try to update scan information that is already present KitRequestShipping scanUpdate = new KitRequestShipping(); @@ -173,7 +193,7 @@ public void testKitScanInfoUpdateWithAndWithoutExistingScanInfo() { //now a final scan Optional shouldBeEmptyScanError = kitDao.updateKitScanInfo(scanUpdate, "tester"); - Assert.assertTrue("The kit was not sent yet and only has tracking and initial information, so scan info should get updated without " + assertTrue("The kit was not sent yet and only has tracking and initial information, so scan info should get updated without " + "errors", shouldBeEmptyScanError.isEmpty()); @@ -181,31 +201,55 @@ public void testKitScanInfoUpdateWithAndWithoutExistingScanInfo() { kitInitialScanUseCase = new KitInitialScanUseCase(kitPayload, kitDao); initialScanError = kitInitialScanUseCase.get(); initialScanError.forEach(error -> Assert.assertNotNull("Initial scan should not cause any errors", error)); - initialScanError.forEach(error -> Assert.assertTrue("Error expected for repeating initial scan after the kit is sent out", + initialScanError.forEach(error -> assertTrue("Error expected for repeating initial scan after the kit is sent out", error.hasError())); Optional scanError = kitDao.updateKitScanInfo(scanUpdate, "tester"); - Assert.assertTrue("Updating an existing kit that already has scan information should result in an error.", + assertTrue("Updating an existing kit that already has scan information should result in an error.", scanError.get().hasError()); // reset some ddp_kit fields and confirm that you can update the scan fields kitDao.updateKitComplete(kitId, false); scanUpdate.setKitLabel(KIT_NAME + ".ignore"); - Assert.assertTrue(kitDao.updateKitLabel(scanUpdate).isEmpty()); + assertTrue(kitDao.updateKitLabel(scanUpdate).isEmpty()); scanUpdate.setKitLabel(KIT_NAME); Optional hasScanError = kitDao.updateKitScanInfo(scanUpdate, "tester"); - Assert.assertTrue("Updating an existing kit with no scan information should not result in a scan error.", + assertTrue("Updating an existing kit with no scan information should not result in a scan error.", hasScanError.isEmpty()); kitDao.updateKitLabel(scanUpdate); // change the kit label and confirm that there is an error because there's no ddp_kit with the given label scanUpdate.setDdpLabel(KIT_NAME + System.currentTimeMillis()); scanError = kitDao.updateKitScanInfo(scanUpdate, "tester"); - Assert.assertTrue("Updating an existing kit request that has no ddp_kit row should result in a scan error.", + assertTrue("Updating an existing kit request that has no ddp_kit row should result in a scan error.", scanError.get().hasError()); scanUpdate.setDdpLabel(KIT_NAME); } + @Test + public void testInsertKitTrackingIfExistsDoesNotLogError() throws Exception { + // Set up the logger and attach the mock appender to be able to verify that no error log was generated + logger = (Logger) LoggerFactory.getLogger(KitDao.class); + logger.setLevel(Level.ERROR); + logger.addAppender(mockAppender); + String kitLabel = "kitLabel123"; + String trackingId = "trackingId123"; + int userId = 1; + + Optional firstScanResult = kitDao.insertKitTrackingIfNotExists(kitLabel, trackingId, userId); + + assertTrue(firstScanResult.isEmpty()); + + Optional secondScanResult = kitDao.insertKitTrackingIfNotExists(kitLabel, trackingId, userId); + assertTrue(secondScanResult.isPresent()); + assertTrue(secondScanResult.get().hasError()); + + // Verify that no error log was generated + verify(mockAppender, never()).doAppend(any()); + kitDao.deleteKitTrackingByKitLabel(kitLabel); + } + + }