BCDA-8443|8444: Update attribution to support csv files #1012
Conversation
## 🎫 Ticket https://jira.cms.gov/browse/BCDA-8414 ## 🛠 Changes Adding unit test
laurenkrugen-navapbc
had a problem deploying
to
test
GitHub Actions
Failure
laurenkrugen-navapbc
had a problem deploying
to
test
GitHub Actions
Failure
laurenkrugen-navapbc
temporarily deployed
to
test
GitHub Actions
Inactive
laurenkrugen-navapbc
temporarily deployed
to
test
GitHub Actions
Inactive
laurenkrugen-navapbc
temporarily deployed
to
test
GitHub Actions
Inactive
bcda/cclf/csv.go
Outdated
|
|
||
| tx, err := conn.BeginEx(ctx, nil) | ||
| if err != nil { | ||
| err = fmt.Errorf("failed to start transaction: %w", err) |
There was a problem hiding this comment.
Same here (and a couple of places below)
|
|
||
| rtx := postgres.NewRepositoryPgxTx(tx) | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
We could change this to handle panic, error, and commit.
defer func() {
if p := recover(); p != nil {
tx.Rollback()
panic(p)
} else if err != nil {
tx.Rollback()
} else {
err = tx.Commit()
}
}()
There was a problem hiding this comment.
there's two other places that are currently using the existing method of handling rollbacks/transaction errors. I am going to ticket this one as a separate item so we can update those outside of the PR.
bcda/cclf/csv.go
Outdated
| err = nil | ||
| break | ||
| } | ||
| // should there be additional validation here so that we know the mbi is a valid mbi? |
There was a problem hiding this comment.
I think it wouldn't hurt to add validation for MBI. We can create a utility function. At a minimum, we can check if it is 11 characters long. If we want to spend a day with regex (or if BFD has it) we can do the full logic of numbers and characters.
There was a problem hiding this comment.
Yeah, I think this would be good to add some validation here and within our CCLF import as well. Maybe we can discuss the implications of adding that validation step - should we always ingest no matter what so that we don't suppress an MBI that is correct but isn't valid?
I'm going to create a ticket for this and hold off on this change for now.
| } | ||
|
|
||
| filenameDate := subMatches[6] | ||
| t, err := time.Parse("D060102.T150405", filenameDate) |
There was a problem hiding this comment.
Should we declare these and reference dates as constants? #nomagicstrings
There was a problem hiding this comment.
we definitely should -- I pulled this from some other places that could use updating as well. I will put this one in a separate ticket and put out a smaller PR.
laurenkrugen-navapbc
temporarily deployed
to
test
GitHub Actions
Inactive
laurenkrugen-navapbc
merged commit c347ca3
into
epic/attribution-lambda-generalization
🎫 Ticket
https://jira.cms.gov/browse/BCDA-4443
https://jira.cms.gov/browse/BCDA-4444
🛠 Changes
handlecsvimport, where there is logging already. this reduces log noiseℹ️ Context
Changes were made to support attribution files in a csv file format. Follows the flow and structure of the CCLF file importer and was adapted to fit expected csv file format.
🧪 Validation
Added new tests and current tests passing.