Enable several disabled Tests (#2026)

## 🎫 Ticket

https://jira.cms.gov/browse/DPC-3747

## 🛠 Changes

- dpc-aggregation: health checks for AggregationEngine and
BlueButtonClient active
- dpc-attribution: force server restart in test to make new
configuration take
- dpc-consent: update code for search for multiple mbis, enable cli
tests

## ✅ Acceptance Validation

Tests need to pass

## 🔒 Security Implications

- [ ] This PR adds a new software dependency or dependencies.
- [ ] This PR modifies or invalidates one or more of our security
controls.
- [ ] This PR stores or transmits data that was not stored or
transmitted before.
- [ ] This PR requires additional review of its security implications
for other reasons.

If any security implications apply, add Jason Ashbaugh (GitHub username:
StewGoin) as a reviewer and do not merge this PR without his approval.

dpc-aggregation/src/main/java/gov/cms/dpc/aggregation/AggregationAppModule.java

@@ -12,6 +12,7 @@
 import gov.cms.dpc.aggregation.engine.JobBatchProcessor;
 import gov.cms.dpc.aggregation.engine.JobBatchProcessorV2;
 import gov.cms.dpc.aggregation.engine.OperationsConfig;
+import gov.cms.dpc.aggregation.health.AggregationEngineHealthCheck;
 import gov.cms.dpc.aggregation.service.*;
 import gov.cms.dpc.common.annotations.ExportPath;
 import gov.cms.dpc.common.annotations.JobTimeout;
@@ -40,12 +41,13 @@ public void configure() {
         binder.bind(AggregationManager.class).asEagerSingleton();
         binder.bind(JobBatchProcessor.class);
         binder.bind(JobBatchProcessorV2.class);
+        binder.bind(AggregationEngineHealthCheck.class);
 
         // Healthchecks
         // Additional health-checks can be added here
-        // By default, Dropwizard adds a check for Hibernate and each additonal database (e.g. auth, queue, etc)
+        // By default, Dropwizard adds a check for Hibernate and each additional database (e.g. auth, queue, etc)
         // We also have JobQueueHealthy which ensures the queue is operation correctly
-        // We have the BlueButton Client healthcheck as well
+        // We have the BlueButton Client healthcheck as well, which adds itself based on configuration
     }
 
     @Provides

dpc-aggregation/src/main/java/gov/cms/dpc/aggregation/health/AggregationEngineHealthCheck.java

@@ -1,13 +1,13 @@
 package gov.cms.dpc.aggregation.health;
 
-import com.codahale.metrics.health.HealthCheck;
 import gov.cms.dpc.aggregation.engine.AggregationEngine;
+import ru.vyarus.dropwizard.guice.module.installer.feature.health.NamedHealthCheck;
 
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
 @Singleton
-public class AggregationEngineHealthCheck extends HealthCheck {
+public class AggregationEngineHealthCheck extends NamedHealthCheck {
 
     private AggregationEngine aggregationEngine;
 
@@ -24,4 +24,9 @@ public Result check() {
         }
         return result;
     }
+
+    @Override
+    public String getName() {
+        return "aggregation-engine";
+    }
 }

dpc-aggregation/src/test/java/gov/cms/dpc/aggregation/AggregationServiceTest.java

@@ -16,7 +16,6 @@
 import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-@Disabled
 @IntegrationTest
 @ExtendWith(BufferedLoggerHandler.class)
 public class AggregationServiceTest {
@@ -39,7 +38,8 @@ void testHealthChecks() {
         final SortedSet<String> names = checks.getNames();
 
         // Ensure that the various healthchecks are propagated from the modules
-        assertAll(() -> assertTrue(names.contains("BlueButtonHealthCheck"), "Should have BB health check"));
+        assertAll(() -> assertTrue(names.contains("blue-button-client"), "Should have BB health check"));
+        assertAll(() -> assertTrue(names.contains("aggregation-engine"), "Should have Aggregation Engine health check"));
 
         // Everything should be true
         checks.runHealthChecks().forEach((key, value) -> assertTrue(value.isHealthy(), String.format("Healthcheck: %s is not ok.", key)));

dpc-attribution/src/test/java/gov/cms/dpc/attribution/AbstractAttributionTest.java

@@ -26,7 +26,7 @@ public abstract class AbstractAttributionTest {
     private static final String KEY_PREFIX = "dpc.attribution";
     private static final ObjectMapper mapper = new ObjectMapper();
 
-    protected static final DropwizardTestSupport<DPCAttributionConfiguration> APPLICATION = new DropwizardTestSupport<>(DPCAttributionService.class, "ci.application.conf", ConfigOverride.config(KEY_PREFIX, "", ""),
+    protected static final DropwizardTestSupport<DPCAttributionConfiguration> APPLICATION = new DropwizardTestSupport<>(DPCAttributionService.class, "ci.application.conf",
             ConfigOverride.config(KEY_PREFIX, "logging.level", "ERROR"));
 
     protected static final String ORGANIZATION_ID = "0c527d2e-2e8a-4808-b11d-0fa06baf8254";

dpc-attribution/src/test/java/gov/cms/dpc/attribution/resources/PractitionerResourceTest.java

@@ -15,7 +15,6 @@
 import gov.cms.dpc.fhir.validations.profiles.PractitionerProfile;
 import org.hl7.fhir.dstu3.model.*;
 import org.junit.jupiter.api.AfterEach;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 import java.util.*;
@@ -248,10 +247,12 @@ void testPractitionerRemoval() {
         assertThrows(ResourceNotFoundException.class, getRequest::execute, "Should not have resource");
     }
 
-    @Disabled
     @Test
-    void testPractitionerSubmitWhenPastLimit() {
+    void testPractitionerSubmitWhenPastLimit() throws Exception {
 
+        // Restart so update takes effect
+        APPLICATION.after();
+        APPLICATION.before();
         //Currently 4 providers are created in the seed for the test
         APPLICATION.getConfiguration().setProviderLimit(5);
 
@@ -279,9 +280,11 @@ private ICreateTyped submitPractitioner(Practitioner practitioner) {
     }
 
     @Test
-    void testPractitionerSubmitWhenLimitIsSetToNegativeOne() {
+    void testPractitionerSubmitWhenLimitIsSetToNegativeOne() throws Exception {
 
-        //Currently 4 providers are created in the seed for the test
+        // Restart so update takes effect
+        APPLICATION.after();
+        APPLICATION.before();
         APPLICATION.getConfiguration().setProviderLimit(-1);
 
         final Practitioner practitioner = AttributionTestHelpers.createPractitionerResource(NPIUtil.generateNPI());

dpc-bluebutton/src/main/java/gov/cms/dpc/bluebutton/health/BlueButtonHealthCheck.java

@@ -1,19 +1,19 @@
 package gov.cms.dpc.bluebutton.health;
 
-import com.codahale.metrics.health.HealthCheck;
 import gov.cms.dpc.bluebutton.client.BlueButtonClient;
 import org.hl7.fhir.dstu3.model.CapabilityStatement;
 import org.hl7.fhir.dstu3.model.Enumerations;
+import ru.vyarus.dropwizard.guice.module.installer.feature.health.NamedHealthCheck;
 
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
 /**
- * {@link HealthCheck} class that verifies whether or not the Blue Button endpoint is accessible.
+ * {@link NamedHealthCheck} class that verifies whether or not the Blue Button endpoint is accessible.
  * This simply makes a request to the /metadata endpoint and verifies that the returned {@link CapabilityStatement} has an {@link org.hl7.fhir.dstu3.model.Enumerations.PublicationStatus#ACTIVE} status.
  */
 @Singleton
-public class BlueButtonHealthCheck extends HealthCheck {
+public class BlueButtonHealthCheck extends NamedHealthCheck {
 
     static final String INVALID_MESSAGE = "BlueButton endpoint returned invalid FHIR Metadata";
     private final BlueButtonClient client;
@@ -37,4 +37,9 @@ protected Result check() {
             return Result.unhealthy(e.getMessage());
         }
     }
+
+    @Override
+    public String getName() {
+        return "blue-button-client";
+    }
 }

dpc-common/src/main/java/gov/cms/dpc/fhir/FHIRExtractors.java

@@ -175,7 +175,13 @@ public static Identifier parseIDFromQueryParam(String queryParam) {
             system = system.substring(0, system.length() - 1);
         }
         identifier.setSystem(system);
-        identifier.setValue(stringPair.getRight());
+        // Strip off any trailing '\' characters.
+        // These might come in when parsing an ID that was separated by comma
+        String value = stringPair.getRight();
+        if (value.endsWith("\\")) {
+            value = value.substring(0, value.length() - 1);
+        }
+        identifier.setValue(value);
         return identifier;
     }
 

dpc-common/src/test/java/gov/cms/dpc/fhir/FHIRExtractorTests.java

@@ -232,6 +232,10 @@ void testIDExtraction() {
         final Identifier trailingSystem = parseIDFromQueryParam(String.format("%s\\|%s", DPCIdentifierSystem.HICN.getSystem(), "nada9"));
         assertAll(() -> assertEquals(DPCIdentifierSystem.HICN.getSystem(), trailingSystem.getSystem(), "Should have correct system"),
                 () -> assertEquals("nada9", trailingSystem.getValue(), "Should have empty value"));
+
+        final Identifier trailingValue = parseIDFromQueryParam(String.format("%s|%s\\", DPCIdentifierSystem.HICN.getSystem(), "nada9"));
+        assertAll(() -> assertEquals(DPCIdentifierSystem.HICN.getSystem(), trailingValue.getSystem(), "Should have correct system"),
+                () -> assertEquals("nada9", trailingValue.getValue(), "Should have empty value"));
     }
 
     @Test

dpc-consent/src/main/java/gov/cms/dpc/consent/cli/CreateConsentRecord.java

@@ -16,8 +16,6 @@
 import org.jooq.conf.Settings;
 import org.jooq.exception.DataAccessException;
 import org.jooq.impl.DSL;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import java.sql.Connection;
 import java.sql.SQLException;
@@ -39,8 +37,6 @@
  */
 public class CreateConsentRecord extends ConsentCommand {
 
-    private static final Logger logger = LoggerFactory.getLogger(CreateConsentRecord.class);
-
     private static final String IN_OR_OUT_ARG = "inOrOut";
 
     private final Settings settings;
@@ -95,8 +91,6 @@ protected void run(Bootstrap<DPCConsentConfiguration> bootstrap, Namespace names
         ce.setPolicyCode(inOrOut);
 
         saveEntity(bootstrap, dpcConsentConfiguration, ce);
-
-        logger.info("Created {} consent entry. Consent entry id: {}, effective {}",ce.getPolicyCode(), ce.getId().toString(), ce.getEffectiveDate());
     }
 
     private void saveEntity(Bootstrap<DPCConsentConfiguration> bootstrap, DPCConsentConfiguration dpcConsentConfiguration, ConsentEntity entity) throws DataAccessException, SQLException {

dpc-consent/src/main/java/gov/cms/dpc/consent/resources/ConsentResource.java

@@ -72,31 +72,24 @@ public List<Consent> search(
 
         // Priority order for processing params. If multiple params are passed, we only pay attention to one
         if (id.isPresent()) {
-
             final Optional<ConsentEntity> consentEntity = this.dao.getConsent(id.get());
-            entities = consentEntity.map(List::of).orElseGet(() -> List.of(ConsentEntity.defaultConsentEntity(id, Optional.empty(), Optional.empty())));
+            entities = consentEntity.map(List::of).orElse(entities);
 
         } else if (identifier.isPresent()) {
             // not sure we should support this
             final Optional<ConsentEntity> consentEntity = this.dao.getConsent(identifier.get());
-            entities = consentEntity.map(List::of).orElseGet(() -> List.of(ConsentEntity.defaultConsentEntity(id, Optional.empty(), Optional.empty())));
+            entities = consentEntity.map(List::of).orElse(entities);
 
         } else if (patientId.isPresent()) {
-
             for (String pId : Splitter.on(',').split(patientId.get())) {
                 final Identifier patientIdentifier = FHIRExtractors.parseIDFromQueryParam(pId);
                 entities.addAll(getEntitiesByPatient(patientIdentifier));
             }
 
         } else {
-
             throw new WebApplicationException("Must have some form of Consent Resource ID or Patient ID", Response.Status.BAD_REQUEST);
         }
 
-        if (entities.isEmpty()) {
-            throw new WebApplicationException("Cannot find patient with given ID", Response.Status.NOT_FOUND);
-        }
-
         return entities
                 .stream()
                 .map(e -> ConsentEntityConverter.toFhir(e, fhirReferenceURL))
@@ -136,31 +129,21 @@ public Consent update(@ApiParam(value = "Consent resource ID", required = true)
     }
 
     private List<ConsentEntity> getEntitiesByPatient(Identifier patientIdentifier) {
-        List<ConsentEntity> entities;
-        Optional<String> hicnValue = Optional.empty();
-        Optional<String> mbiValue = Optional.empty();
         String field;
 
         // we have been asked to search for a patient id defined by one among two (soon three!) coding systems
         // we need to determine which database field that system's value is stored in
         switch (DPCIdentifierSystem.fromString(patientIdentifier.getSystem())) {
             case MBI:
-                mbiValue = Optional.of(patientIdentifier.getValue());
                 field = "mbi";
                 break;
             case HICN:
-                hicnValue = Optional.of(patientIdentifier.getValue());
                 field = "hicn";
                 break;
             default:
                 throw new WebApplicationException("Unknown Patient ID code system", Response.Status.BAD_REQUEST);
         }
 
-        entities = this.dao.findBy(field, patientIdentifier.getValue());
-
-        if (entities.isEmpty()) {
-            entities = List.of(ConsentEntity.defaultConsentEntity(Optional.empty(), hicnValue, mbiValue));
-        }
-        return entities;
+        return this.dao.findBy(field, patientIdentifier.getValue());
     }
 }

dpc-consent/src/test/java/gov/cms/dpc/consent/cli/ConsentCommandsTest.java

@@ -1,5 +1,6 @@
 package gov.cms.dpc.consent.cli;
 
+import ch.qos.logback.classic.LoggerContext;
 import gov.cms.dpc.consent.DPCConsentConfiguration;
 import gov.cms.dpc.consent.DPCConsentService;
 import gov.cms.dpc.testing.IntegrationTest;
@@ -21,12 +22,10 @@
 
 @TestInstance(TestInstance.Lifecycle.PER_CLASS)
 @IntegrationTest
-@Disabled
 class ConsentCommandsTest {
 
     private final PrintStream originalOut = System.out;
     private final PrintStream originalErr = System.err;
-    private final InputStream originalIn = System.in;
 
     private final ByteArrayOutputStream stdOut = new ByteArrayOutputStream();
     private final ByteArrayOutputStream stdErr = new ByteArrayOutputStream();
@@ -51,9 +50,6 @@ public void run(DPCConsentConfiguration configuration, Environment environment)
 
     @BeforeAll
     void cliSetup() throws Exception {
-
-        app.run("db", "migrate", "ci.application.conf");
-
         // Redirect stdout and stderr to our byte streams
         System.setOut(new PrintStream(stdOut));
         System.setErr(new PrintStream(stdErr));
@@ -64,11 +60,15 @@ void cliSetup() throws Exception {
         cli = new Cli(location, bs, stdOut, stdErr);
     }
 
+    @BeforeEach
+    void stopLogging() {
+        ((LoggerContext)org.slf4j.LoggerFactory.getILoggerFactory()).stop();
+    }
+
     @AfterAll
     void teardown() {
         System.setOut(originalOut);
         System.setErr(originalErr);
-        System.setIn(originalIn);
     }
 
     @AfterEach
@@ -81,15 +81,15 @@ void resetIO() {
     final void pertinentHelpMessageDisplayed() throws Exception {
         final Optional<Throwable> t1 = cli.run("consent", "create", "-h");
         String errorMsg = String.format("Should have pertinent help message, got: %s", stdOut.toString());
-        assertAll(() -> assertTrue(t1.isPresent(), "Should have succeeded"),
+        assertAll(() -> assertFalse(t1.isPresent(), "Should have succeeded"),
                 () -> assertEquals("", stdErr.toString(), "Should not have errors"),
                 () -> assertTrue(stdOut.toString().contains("Create a new consent record"), errorMsg));
     }
 
     @Test
     final void onlyAllowsInOrOut() throws Exception {
         final Optional<Throwable> t1 = cli.run("consent", "create", "-p", "t2-mbi", "-d", "2019-11-22", "-i", "-o", "--host", "http://localhost:3500/v1");
-        assertAll(() -> assertFalse(t1.isPresent(), "Should have failed"),
+        assertAll(() -> assertTrue(t1.isPresent(), "Should have failed"),
                 () -> assertEquals("", stdOut.toString(), "Should not have output"),
                 () -> assertNotEquals("", stdErr.toString(), "Should have errors"),
                 () -> assertTrue(stdErr.toString().contains("argument -o/--out: not allowed with argument -i/--in"), "Should have '-o not allowed with -i' help message"));
@@ -98,7 +98,7 @@ final void onlyAllowsInOrOut() throws Exception {
     @Test
     final void detectsInvalidDate() throws Exception {
         final Optional<Throwable> t5 = cli.run("consent", "create", "-p", "tA-mbi", "-d", "Nov 22 2019", "-i", "--host", "http://localhost:3500/v1");
-        assertAll(() -> assertFalse(t5.isPresent(), "Should have failed"),
+        assertAll(() -> assertTrue(t5.isPresent(), "Should have failed"),
                 () -> assertEquals("", stdOut.toString(), "Should not have output"),
                 () -> assertNotEquals("", stdErr.toString(), "Should have errors"),
                 () -> assertTrue(stdErr.toString().contains("java.time.format.DateTimeParseException"), "Should have date parsing error"));
@@ -107,14 +107,14 @@ final void detectsInvalidDate() throws Exception {
     @Test
     final void createDefaultOptInRecord() throws Exception {
         final Optional<Throwable> t2 = cli.run("consent", "create", "-p", "t2-mbi", "-d", "2019-11-22", "-i", "--host", "http://localhost:3500/v1");
-        assertAll(() -> assertTrue(t2.isPresent(), "Should have succeeded"),
+        assertAll(() -> assertFalse(t2.isPresent(), "Should have succeeded"),
                 () -> assertEquals("", stdErr.toString(), "Should not have errors"));
     }
 
     @Test
     final void createDefaultOptOutRecord() throws Exception {
         final Optional<Throwable> t3 = cli.run("consent", "create", "-p", "t3-mbi", "-d", "2019-11-23", "-o", "--host", "http://localhost:3500/v1");
-        assertAll(() -> assertTrue(t3.isPresent(), "Should have succeeded"),
+        assertAll(() -> assertFalse(t3.isPresent(), "Should have succeeded"),
                 () -> assertEquals("", stdErr.toString(), "Should not have errors"));
     }
 }

dpc-consent/src/test/java/gov/cms/dpc/consent/resources/ConsentResourceTest.java

@@ -23,6 +23,7 @@
 import java.util.Date;
 import java.util.List;
 
+import static gov.cms.dpc.fhir.DPCIdentifierSystem.MBI;
 import static org.junit.jupiter.api.Assertions.*;
 
 /**
@@ -196,16 +197,14 @@ final void searchConsentResource_finds_validPatientParam(String system, String p
                 .execute();
 
         final Consent found = (Consent) sut.getEntryFirstRep().getResource();
-        System.out.println(ctx.newJsonParser().setPrettyPrint(true).encodeResourceToString(found));
 
         assertEquals(ConsentEntityConverter.OPT_IN_MAGIC, found.getPolicyRule());
         assertEquals(TEST_CONSENT_REF, found.getId());
     }
 
     @Test
-    @Disabled
     final void searchConsentResource_multiple_ids_for_one_patient() {
-        String patientIds = "mbi_1,mbi_2";
+        String patientIds = String.format("%s|%s,%s|%s", MBI.getSystem(), "mbi_1", MBI.getSystem(), "mbi_no_record");
         final IGenericClient client = createFHIRClient(ctx, getServerURL());
 
         final Bundle sut = client
@@ -219,7 +218,6 @@ final void searchConsentResource_multiple_ids_for_one_patient() {
         assertEquals(1, sut.getTotal(), "Should only find one consent record.");
 
         final Consent found = (Consent) sut.getEntryFirstRep().getResource();
-        System.out.println(ctx.newJsonParser().setPrettyPrint(true).encodeResourceToString(found));
 
         assertEquals(ConsentEntityConverter.OPT_IN_MAGIC, found.getPolicyRule());
         assertEquals(TEST_CONSENT_REF, found.getId());