BFD-3663: Populate tags for claim / EOB on ingestion from CCW #2497
Conversation
dondevun
requested review from
mjburling,
aschey-forpeople and
malessi
as code owners
apps/bfd-model/bfd-model-rif/src/main/java/gov/cms/bfd/model/rif/samhsa/CcwTagKey.java
Outdated
Show resolved
Hide resolved
…if/samhsa/CcwTagKey.java Co-authored-by: aschey-forpeople <[email protected]>
...ine/bfd-pipeline-shared-utils/src/main/java/gov/cms/bfd/pipeline/sharedutils/SamhsaUtil.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,706 @@ | |||
| - claimClass: CarrierClaim | |||
There was a problem hiding this comment.
Clever solution! My only concern is that we're giving up a lot of compile-time safety by invoking all of this dynamically. Alternatively, we could have an adapter class for each claim type - HHASamhsaAdapter, InpatientSamhsaAdapter, etc. that would wrap each claim object and implement some interface or inherit from some base class that we could operate on using common methods just like what you've done in these yaml files. It would be a bit more boilerplate than using reflection, but I think it would be a bit less prone to accidental breakage. Thoughts?
There was a problem hiding this comment.
If your concern about compile-time safety is that the methods may not exist, then the integration tests should alleviate that -- Every method in the yaml file is exercised. Or did you have a different concern?
The adapter solution would work, but I think it would be a bit more tightly coupled than the yaml solution. What I mean is that if, in the future, we add new SAMHSA fields, we would need only add two lines into the YAML file (column and method).
There was a problem hiding this comment.
If we leave this code as is and never change it, then yes, the tests will catch it. My main concern with these kinds of things is that it's more difficult to refactor and extend safely in the future. I think the concept here makes sense - with the adapter pattern we can still use getFields() as you have here and we should only need to add 1-2 lines of code to add the new field there.
There was a problem hiding this comment.
Made some changes using the adapter pattern, like we talked about. I still think reflection is the way to go here; otherwise, we'd have to set method references in the base adapter class, which would require each adapter to pass each of it's relevant methods (since, while the entities share a base class, the base class does not define most of the methods). This can be up to 66 methods in some entities, which defeats the purpose of this streamlining process.
Another idea I've been thinking about is annotating all of the samhsa field get methods in the entities with @samhsa (or something similar), which would allow us to programmatically grab a list of all of the annotated methods at once, without having to explicitly reference them by name; however, I'm not quite sure how this would work with our generated entities -- it would be a bit more complex than if the entities were hand woven.
There was a problem hiding this comment.
Been thinking about this, and I've come to agree with what you have been trying to get at: passing the methods along from the adapter with a Supplier object may be a better solution for the sake of maintainability Reflection would make the code a lot more compact, but:
- Too easy to make a mistake, and
- I'm not convinced that the execution time of invoking a method with reflection is advisable due to the volume of claims that we process at a time.
| case RdaMcsClaim mcsClaim -> { | ||
| Optional<List<McsTag>> tags = Optional.of(checkAndProcessMcsClaim(mcsClaim)); | ||
| persistTags(tags, entityManager); | ||
| public <TClaim> boolean processClaim(TClaim claim, EntityManager entityManager) { |
There was a problem hiding this comment.
Do we need the return value here? Doesn't seem like it's used right now.
| * @param claim The claim to check. | ||
| * @return A list of tag entities to persist. | ||
| */ | ||
| private List<HhaTag> checkAndProcessHhaClaim(HHAClaim claim) |
There was a problem hiding this comment.
I think this logic can operate on an input of SamhsaAdapterBase so it doesn't need to be duplicated for each claim type.
JIRA Ticket:
BFD-3663
What Does This PR Do?
Populate SAMHSA tags for claims on ingestion from CCW.
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
Have you fully verified and tested these changes? Is the acceptance criteria met? Please provide reproducible testing instructions, code snippets, or screenshots as applicable.
Unit tests and integration tests have been created.