-
Notifications
You must be signed in to change notification settings - Fork 66
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
API-25814: Update api_version
to "V0" for Segmented Appeals APIs
#12443
API-25814: Update api_version
to "V0" for Segmented Appeals APIs
#12443
Conversation
when 'V2' | ||
when 'v0' | ||
V0_STATUSES | ||
when 'v2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could I get an extra set of eyes on this? Before this change, api_version.downcase
would never have matched "V2," so the V1 statuses would have been returned every time. If you look at the bottom of this file, the status
field is validated based on the results of this method, so I want to be sure that we have it right. Will also add some specs for this once we clarify the desired behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you've said here sounds right to me, too - it seems like it would be nice to settle on consistently using upcase or downcase for all v*
strings we work with and convert them all at some point if possible (but not as part of this ticket/PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I went back and forth on upcase vs. downcase a few times since we use both.
I investigated the supported statuses a bit more and am comfortable with this change now. Before this change, we were technically allowing the "uploaded", "received", and "expired" statuses for V2 and V0 HLRs, even though those statuses are not supported in V2 or V0 according to our documentation. To be extra sure, I also confirmed in Production that we don't have any V2 or V0 HLR records that are in those statuses.
Supported HLR Statuses – Before:
V1 HLR: ["pending", "submitting", "submitted", "processing", "error", "uploaded", "received", "success", "expired", "complete"]
V2 HLR: ["pending", "submitting", "submitted", "processing", "error", "uploaded", "received", "success", "expired", "complete"]
V0 HLR: ["pending", "submitting", "submitted", "processing", "error", "uploaded", "received", "success", "expired", "complete"]
Supported HLR Statuses – After:
V1 HLR: ["pending", "submitting", "submitted", "processing", "error", "uploaded", "received", "success", "expired", "complete"]
V2 HLR: ["pending", "submitting", "submitted", "processing", "success", "complete", "error"]
V0 HLR: ["pending", "submitting", "submitted", "processing", "success", "complete", "error"]
Also, I just added some specs to test that the correct supported HLR statuses are returned for each API version.
err_hlrs = HigherLevelReview.v2.where(status: 'error') | ||
stuck_hlrs = HigherLevelReview.v2.where(status: stuck_hlr_statuses) | ||
err_hlrs = HigherLevelReview.v2_or_v0.where(status: 'error') | ||
stuck_hlrs = HigherLevelReview.v2_or_v0.where(status: stuck_hlr_statuses) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I anticipate that we'll be making more changes to reporting in order to report V2 and V0 appeals separately, but for now, I'm including V0 appeals in all existing reporting.
Generated by 🚫 Danger |
Summary
This PR updates the Segmented Appeals APIs to save an
api_version
of "V0" rather than "V2" onHigherLevelReviews
,NoticeOfDisagreements
, andSupplementalClaims
records created from those APIs, since the API versions for the Segmented APIs are all V0.Related issue(s)
API-25814
Testing done
I updated the controller specs to test for the correct
api_version
and tested the V0 and V2create
endpoints locally.Screenshots
None
What areas of the site does it impact?
This PR impacts all Appeals API Segmented (V0) API
create
endpoints. The V2 Decision Reviews HLR/SC/NODcreate
endpoints were also updated, but the changes there were refactors only.Acceptance criteria
Requested Feedback
Would like an extra set of eyes on the change to
modules/appeals_api/app/models/concerns/appeals_api/hlr_status.rb
, since that code didn't look like it was working correctly before.