-
Notifications
You must be signed in to change notification settings - Fork 62
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
MHV-61122: Medical records access updated with new eligibility check #18641
base: master
Are you sure you want to change the base?
MHV-61122: Medical records access updated with new eligibility check #18641
Conversation
MHVMedicalRecordsPolicy = Struct.new(:user, :mhv_medical_records) do | ||
MR_ACCOUNT_TYPES = %w[Premium].freeze | ||
|
||
def access? | ||
MR_ACCOUNT_TYPES.include?(user.mhv_account_type) && user.va_patient? | ||
if Flipper.enabled?(:mhv_medical_records_new_eligibility_check) | ||
client = UserEligibility::Client.new(session: { user_id: user.mhv_correlation_id, icn: user.icn }) |
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.
Seems like there should be a rescue clause here incase of issue in the client requests performed here
Sorry ,I see the validate_client method is in place, but should we also consider logging additional details around the error_message? This could help capture cases where the error originates from the client or a service, especially if the error doesn't fit within the predefined responses you're currently handling (lines 22-24)
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.
Hi Lindsey, thank you for your review. I moved the error handling block to the access method so it would catch client failures as well, and added the error message to the logged details. Let me know if you see anything else that could be improved. Thanks!
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.
Great job Matt -- this looks good. Don't forget to make the modifications in devops
and vsp-infra-application-manifests
before merging.
base_request_headers.merge({ | ||
'x-api-key': Settings.mhv.medical_records.x_api_key, | ||
appToken: Settings.mhv.rx.app_token | ||
}) |
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.
Consider in the future this approach for readability
base_request_headers.merge({ | |
'x-api-key': Settings.mhv.medical_records.x_api_key, | |
appToken: Settings.mhv.rx.app_token | |
}) | |
mhv_headers = { | |
'x-api-key': Settings.mhv.medical_records.x_api_key, | |
appToken: Settings.mhv.rx.app_token | |
} | |
base_request_headers.merge(mhv_headers) |
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.
Good call!
8fa00d3
Summary
Related issue(s)
https://jira.devops.va.gov/browse/MHV-61122
Implement findings on: Update MR policy to not rely on the old eligibility check
Description
Implement findings on: Update MR policy to not rely on the old eligibility check.
See https://jira.devops.va.gov/browse/MHV-55523
SM is already looking into a new method for determining whether a user is Basic or Premium, so we can potentially use their method, if feasible.
Here are some notes about this:
Currently, to determine MHV account type (basic, advanced, premium), VA.gov is utilizing the bluebutton/geteligibledataclass endpoint. Currently, SM is using this to determine eligibility for users of their app.
However I just spoke with Muazzam and there is some recent concern that this is the wrong endpoint to use. Since the elimination of the "advanced" account type, there are now edge cases for which this endpoint may return wrong results.
There is apparently another endpoint, "sm eligibility" that is more accurate and is the one we should be using. As a side benefit, it accounts for the fact that an eligible user must have a treatment facility, so it satisfies all the acceptance criteria for this ticket.
User Story: As a Medical Records User, I want to be able to easily determine if a user is premium or basic on the backend and implement that solution on va.gov.
Testing done
Tests written for a user having access, not having access, missing parameters, an invalid ICN, and not having the feature flag enabled.
Screenshots
No UI changes.
Acceptance criteria
AC1 Implement the new solution of determining if user is premium or basic from ticket https://jira.devops.va.gov/browse/MHV-55523
Requested Feedback
Do I need to write tests for the mhv_medical_records_policy.rb file? I don't see any tests for other similar policy files.