Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BFD-3550: Add cross-reference ID and cross-reference switch to CCW pipeline #2391

Draft
wants to merge 17 commits into
base: feature/xref
Choose a base branch
from

Conversation

aschey-forpeople
Copy link
Contributor

@aschey-forpeople aschey-forpeople commented Jul 29, 2024

JIRA Ticket:
BFD-3550

What Does This PR Do?

  • Adds the xref_grp_id and xref_sw columns to the database and the CCW pipeline
  • For now, we will need to support both files with and without these columns, so the test changes here are mostly additive to assure we can still support both scenarios. Once we only need to support files with the xref columns included, we can see about consolidating these extra tests and test files.
  • One bug fix is included here which is to check if a CSV field contains a header before trying to get the value - this prevents an exception from being thrown
  • Some test setup logic was based on the assumption that the CSV header and the order of the fields in the YAML RIF schemas should match. Places that make this assumption have been fixed so this is no longer the case.
  • The withHeader method for creating CSVs has been throwing deprecation warnings for a while. I replaced all instances of these with the builder pattern that they now want you to use.

What Should Reviewers Watch For?

If you're reviewing this PR, please check for these things in particular:

What Security Implications Does This PR Have?

Please indicate if this PR does any of the following:

  • Adds any new software dependencies

  • Modifies any security controls

  • Adds new transmission or storage of data

  • Any other changes that could possibly affect security?

  • I have considered the above security implications as it relates to this PR. (If one or more of the above apply, it cannot be merged without the ISSO or team security engineer's (@sb-benohe) approval.)

Validation

Created an ephemeral environment from test, ran database migrations, and re-ran an old pipeline load that contained a beneficiary file.

public void verifySampleAColumns() {
verifyColumns(StaticRifResourceGroup.SAMPLE_A);
public void verifySampleBColumns() {
verifyColumns(StaticRifResourceGroup.SAMPLE_B);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the sample A files alone for now so we don't mess with the existing tests. Sample B contains the new xref columns which we need to use for this test since we're checking that all columns in the file are in the schema.

@@ -56,20 +58,17 @@ private void verifyColumns(StaticRifResourceGroup sampleGroup) {
Enum<?>[] columnsInEnum = getColumnsInEnum(sampleFile.getRifFileType());

// Use a CSVParser to parse the header out of the sample file.
CSVFormat parserFormat = CSVFormat.DEFAULT.withDelimiter('|');
CSVFormat parserFormat = CSVFormat.DEFAULT.builder().setDelimiter('|').build();
Copy link
Contributor Author

@aschey-forpeople aschey-forpeople Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes to .builder() are to fix the deprecation warnings on withDelimiter

toHeaderFormat(columnsInSample, c -> c),
toHeaderFormat(columnsInEnum, c -> c.name())));

HashSet<String> sampleSet = new HashSet<>(Arrays.asList(columnsInSample));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here are to prevent the order from mattering. Instead of checking that the columns have the same index, we only care that all columns are included.

@@ -493,7 +493,7 @@ private void tweakIfBeneficiary(
* state as a BeneficiaryHistory record. (Note: this has to be done AFTER the secret hashes
* have been updated, as per above.
*/
updateBeneficaryHistory(
updateBeneficiaryHistory(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just fixing a typo

@@ -869,6 +871,19 @@ static boolean isBeneficiaryHistoryEqual(
newBeneficiaryRecord.getMbiEffectiveDate(), oldBeneficiaryRecord.getMbiEffectiveDate())) {
return false;
}

if (!Objects.equals(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will instruct the pipeline to add a new history record if the xref id or switch has changed

.toArray(String[]::new);
CSVFormat csvFormat = RifParsingUtils.CSV_FORMAT.withHeader(csvHeader);
// Get the header from the original file
List<String> csvHeader = RifParsingUtils.createCsvParser(inputFile).getHeaderNames();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of trying to create a new header with everything in the schema, we instead pull the header from the original file since the order could be different.

@aschey-forpeople aschey-forpeople marked this pull request as ready for review August 5, 2024 20:57
@@ -0,0 +1,128 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a runbook that explains how to use this script for re-running a pipeline load: https://github.com/CMSgov/beneficiary-fhir-data/wiki/How-to-clean%E2%80%90up-and-reload-test-data-from-a-CCW-pipeline-load

@aschey-forpeople aschey-forpeople changed the base branch from master to feature/xref August 14, 2024 19:51
@aschey-forpeople aschey-forpeople marked this pull request as draft August 16, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant