[DSM] PEPPER-1408 Fix for Tracking Scan Duplicate Error

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/kit/KitDao.java

@@ -631,10 +631,12 @@ private Optional<ScanResult> 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<ScanResult> 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);

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";

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

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/db/dao/KitDaoTest.java → 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<ILoggingEvent> 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<ScanResult> 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<ScanResult> 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,39 +193,63 @@ public void testKitScanInfoUpdateWithAndWithoutExistingScanInfo() {
 
         //now a final scan
         Optional<ScanResult> 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());
 
         //repeating initial and final scan now should throw an error
         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<ScanResult> 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<ScanResult> 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<ScanResult> firstScanResult = kitDao.insertKitTrackingIfNotExists(kitLabel, trackingId, userId);
+
+        assertTrue(firstScanResult.isEmpty());
+
+        Optional<ScanResult> 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);
+    }
+
+
 }