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

[DPC-4299] - Update AO job so sanctions don't affect org API access #2280

Merged
merged 5 commits into from
Sep 30, 2024
Merged

[DPC-4299] - Update AO job so sanctions don't affect org API access #2280

merged 5 commits into from
Sep 30, 2024

Conversation

lukey-luke
Copy link
Contributor

@lukey-luke lukey-luke commented Sep 27, 2024 •
edited
Loading

🎫 Ticket

DPC-4299

🛠 Changes

  • remove code that updates ProviderOrganization verification status in verify_ao_job.rb

ℹ️ Context

  • an AO failing med sanction check would cause an entire organization to also be rejected. disable_rejected() would delete the api tokens for the organization
  • We no longer want this behavior, so now only the AO user is rejected

🧪 Validation

  • existing test cases already cover this sidekiq job in verify_ao_job_spec.rb
  • failing test cases updated to reflect changes

@lukey-luke lukey-luke changed the title Ls/dpc 4299 verify ao tokens draft - [DPC-4299] - Update AO job so AO sanctions do not affect org API access Sep 27, 2024
@lukey-luke lukey-luke changed the title draft - [DPC-4299] - Update AO job so AO sanctions do not affect org API access draft - [DPC-4299] - Update AO job so sanctions don't affect org API access Sep 27, 2024
@lukey-luke lukey-luke changed the title draft - [DPC-4299] - Update AO job so sanctions don't affect org API access [DPC-4299] - Update AO job so sanctions don't affect org API access Sep 27, 2024
@lukey-luke lukey-luke marked this pull request as ready for review September 27, 2024 21:51
NavaCharlesHorowitz
NavaCharlesHorowitz reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@NavaCharlesHorowitz NavaCharlesHorowitz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If your preference is to comment out the code, could you add a comment to explain the retention/reason for disabling? Or else just delete these calls if we're done with them? Thx!

@lukey-luke lukey-luke requested a review from a team September 30, 2024 20:10
ashley-weaver
ashley-weaver reviewed Sep 30, 2024
View reviewed changes
dpc-portal/spec/jobs/verify_ao_job_spec.rb
@@ -198,11 +198,8 @@ def expect_audits(link, also: [])
expect_log_for(link, 'ao_med_sanctions')
end
VerifyAoJob.perform_now
links.each do |link|
expect_audits(link, also: %i[user org])
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have all the context here, but should this still check the link and user audits, ie just remove org from the also params?

ashley-weaver
ashley-weaver reviewed Sep 30, 2024
View reviewed changes
dpc-portal/spec/jobs/verify_ao_job_spec.rb
@@ -188,8 +189,7 @@ def expect_audits(link, also: [])
expect(link.verification_reason).to eq 'ao_med_sanctions'
expect(link.user.verification_status).to eq 'rejected'
expect(link.user.verification_reason).to eq 'ao_med_sanctions'
expect(link.provider_organization.verification_status).to eq 'rejected'
expect(link.provider_organization.verification_reason).to eq 'ao_med_sanctions'
expect(link.provider_organization.verification_status).to eq 'approved'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(link.provider_organization.verification_status).to eq 'approved'
expect(link.provider_organization.verification_status).to eq 'approved'
expect(link.provider_organization.verification_reason).to be nil

ashley-weaver
ashley-weaver approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@ashley-weaver ashley-weaver left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nitpicks

@lukey-luke lukey-luke merged commit a11d37e into main Sep 30, 2024
7 checks passed
@lukey-luke lukey-luke deleted the ls/dpc-4299-verify-ao-tokens branch September 30, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NavaCharlesHorowitz NavaCharlesHorowitz NavaCharlesHorowitz left review comments

@ashley-weaver ashley-weaver ashley-weaver approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lukey-luke @ashley-weaver @NavaCharlesHorowitz