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.

Sign up for GitHub

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

Jump to bottom

add facilities check on mobile appts #18486

Merged
merged 12 commits into from
Sep 23, 2024
Merged

add facilities check on mobile appts #18486

merged 12 commits into from
Sep 23, 2024

Conversation

aherzberg
Copy link
Contributor

@aherzberg aherzberg commented Sep 17, 2024 •
edited
Loading

Summary

Add Facilities check to VAOS policy. This will not effect VAOS controller.

created separate ticket to merge VAOS policy logic into same place: https://app.zenhub.com/workspaces/va-mobile-60f1a34998bc75000f2a489f/issues/gh/department-of-veterans-affairs/va-mobile-app/9617

Related issue(s)

department-of-veterans-affairs/va-mobile-app#9415

Testing done

  • New code is covered by unit tests
  • Describe what the old behavior was prior to the change
  • Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing
  • If this work is behind a flipper:
    • Tests need to be written for both the flipper on and flipper off scenarios. Docs.
    • What is the testing plan for rolling out the feature?

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas)

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

@va-vfs-bot va-vfs-bot temporarily deployed to 9415-vaos-policy/main/main September 17, 2024 22:23 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 9415-vaos-policy/main/main September 17, 2024 23:39 Inactive
@aherzberg aherzberg marked this pull request as ready for review September 18, 2024 18:19
@aherzberg aherzberg requested review from a team as code owners September 18, 2024 18:19
aherzberg
aherzberg commented Sep 18, 2024
View reviewed changes
modules/vaos/app/policies/vaos_policy.rb
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 a separate check so we can return a unique error that FE can eventually use to trigger a banner saying that they don't have any facilities.

aherzberg
aherzberg commented Sep 18, 2024
View reviewed changes
modules/mobile/spec/requests/mobile/v0/appointments/create_spec.rb
@@ -16,6 +16,7 @@
let!(:user) { sis_user(icn: '1012846043V576341') }

before do
allow_any_instance_of(User).to receive(:va_patient?).and_return(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just adding the facilities was not working in this spec as it was transforming facilities into test facilities 984. This change is unique to testing env so not worried about effecting prod performance.

@va-vfs-bot va-vfs-bot temporarily deployed to 9415-vaos-policy/main/main September 18, 2024 18:25 Inactive
aherzberg
aherzberg commented Sep 18, 2024
View reviewed changes
modules/mobile/app/controllers/mobile/v0/appointments_controller.rb Outdated
@@ -137,6 +137,7 @@ def appointments_cache_interface
def authorize
raise_access_denied unless current_user.authorize(:vaos, :access?)
raise_access_denied_no_icn if current_user.icn.blank?
raise_access_denied_no_facilities unless current_user.authorize(:vaos, :facilities?)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kpethtel , I'm unsure if this warrants a flipper. It seems simple enough but let me know if you feel differently.

kpethtel
kpethtel previously approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@kpethtel kpethtel left a comment

Choose a reason for hiding this comment

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

Looks good.

I don't think this merits a feature flag. We've been told this is the right thing to do and it makes conceptual sense. If it causes issues, those issues won't be 500s. They'll be user complaints. If we receive enough of those, we can stop checking the new policy and keep asking questions about why this isn't the right thing to do.

Is vaos also going to use the new policy?

modules/vaos/app/policies/vaos_policy.rb Outdated
@@ -4,4 +4,8 @@
def access?
Flipper.enabled?('va_online_scheduling', user) && user.loa3?
end

def facilities?
Copy link
Contributor

Choose a reason for hiding this comment

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

Would facilities_access? make sense here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah thats better. Updated.

@va-vfs-bot va-vfs-bot temporarily deployed to 9415-vaos-policy/main/main September 18, 2024 22:53 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 9415-vaos-policy/main/main September 20, 2024 18:57 Inactive
@ryan-mcneil
Copy link
Contributor

@aherzberg please pull master to take care of the CVE

@va-vfs-bot va-vfs-bot temporarily deployed to 9415-vaos-policy/main/main September 23, 2024 15:48 Inactive
ryan-mcneil
ryan-mcneil previously approved these changes Sep 23, 2024
View reviewed changes
@ryan-mcneil
Copy link
Contributor

@aherzberg let me know when you're ready to merge - I might have to override because of that EOF error

@aherzberg
Copy link
Contributor Author

@ryan-mcneil should be good to merge now. Looks like prod deploy has already gone out for the day so I should have the rest of the day to test it. Thanks!

@ryan-mcneil
Copy link
Contributor

@aherzberg Ope maybe I missed it before, but there are also failing specs... are any of those potentially related to your changes? Otherwise let's take note of which ones are failing so I can talk to the respective teams and get eyes on them. Thanks!

@ryan-mcneil ryan-mcneil dismissed their stale review September 23, 2024 17:37

Dismissing until code checks are passing again

@aherzberg
Copy link
Contributor Author

aherzberg commented Sep 23, 2024 •
edited
Loading

@ryan-mcneil I wasn't able to reproduce the errors locally on all 3 failures so was assuming it was all environment issues. I've been seeing a lot of that in the last week or so. The errors also effect specs that are unrelated to the code being changed

@ryan-mcneil ryan-mcneil merged commit 5c10857 into master Sep 23, 2024
20 of 23 checks passed
@ryan-mcneil ryan-mcneil deleted the 9415-vaos-policy branch September 23, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpethtel kpethtel kpethtel approved these changes

@ryan-mcneil ryan-mcneil ryan-mcneil approved these changes

Assignees
No one assigned
Labels
require-backend-approval test-passing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aherzberg @ryan-mcneil @kpethtel @va-vfs-bot