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.

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

Tbolt/4182 Fix issue with renewing state admin #4262

Merged
merged 19 commits into from
Sep 1, 2022

Conversation

tbolt
Copy link
Contributor

@tbolt tbolt commented Aug 4, 2022

Resolves #4182

Description

Updates the state admin matching process to allow matching on already existing matches.

Significant changes or possible side effects

State admins who have been previously approved should now be eligible "matches." Unsure if any side effects are introduced from this.

Automated test cases written

Test added to verify that state admin role will return a match

Steps to manually verify this change

  1. Login as fed admin
  2. For a already valid state admin user for this current FFY, add a new certification for next FFY
  3. Verify that the certification has a match
  4. Verify that you can make the match successfully

This pull request is ready to review when

  • Automated tests are updated (and all tests are passing)
  • Pull request has been labeled, if applicable with feature, content, bug, tests, refactor
  • Associated OpenAPI documentation has been updated
  • The experience passes a basic manual accessibility audit (keyboard nav, screenreader, text scaling) OR an exemption is documented

This pull request can be merged when

  • Code has been reviewed by someone other than the original author
  • QA has verified the functionality related to the change
  • QA has verified the accesibility using tools such as screen readers and WAVE (accessibility evaluation tool)
  • Design has approved the experience
  • Product has approved the experience

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
@tbolt tbolt added the bug needs triage, then squashing label Aug 4, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #4262 (40df2f1) into main (7f6c146) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4262      +/-   ##
==========================================
+ Coverage   86.30%   86.32%   +0.01%     
==========================================
  Files         260      260              
  Lines        5931     5932       +1     
  Branches     1240     1240              
==========================================
+ Hits         5119     5121       +2     
+ Misses        752      751       -1     
  Partials       60       60              
Impacted Files Coverage Δ
api/db/certifications.js 63.33% <100.00%> (+1.26%) ⬆️
...src/pages/admin/fed-admin/MatchStateAdminDialog.js 79.31% <100.00%> (+3.44%) ⬆️

Impacted file tree graph

Impacted Files Coverage Δ
api/db/certifications.js 63.33% <100.00%> (+1.26%) ⬆️
...src/pages/admin/fed-admin/MatchStateAdminDialog.js 79.31% <100.00%> (+3.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f6c146...40df2f1. Read the comment docs.

@tbolt tbolt requested review from thetif and Sun-Mountain August 8, 2022 15:13
@cms-eapd-bot
Copy link

cms-eapd-bot commented Aug 9, 2022

This deploy was cleaned up.

@mirano-darren mirano-darren self-assigned this Aug 10, 2022
@mirano-darren
Copy link
Contributor

Looks good to me!

Given When Then Result
Fed admin panel Testing in main (not this PR) Approving State Admin, then re-adding the same user for 2023 Cannot find a match for the 2023 user; expected bug behavior
Fed admin panel Testing in this PR Approving State Admin, then re-adding the same user for 2023 Able to find find a match despite the user already approved. PASS ✅

@tbolt tbolt removed the request for review from Sun-Mountain August 18, 2022 14:43
@thetif thetif requested review from akuas and removed request for beparticular August 31, 2022 21:12
Copy link

@akuas akuas left a comment

Choose a reason for hiding this comment

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

Works as expected. Maybe this is obvious, but a consideration is that the record a (FFY 2022) must be matched before adding a new certification for record b (FFY 2023). If you don't it will just list both without the ability the match record b. I don't see this as a bug and I wasn't able to identify any real issues with the way it work now.

@akuas
Copy link

akuas commented Sep 1, 2022

Just to clarify, as a user you just need to make sure that the status of the previous user says matched before you recertify, but it is a nonissue.

Copy link
Contributor

@jeromeleecms jeromeleecms left a comment

Choose a reason for hiding this comment

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

This works as advertised. Thank you.

@tbolt tbolt merged commit 9793115 into main Sep 1, 2022
@tbolt tbolt deleted the tbolt/4182-state-admin-renew-bug branch September 1, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs triage, then squashing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Fix issue with renewing State Admin
7 participants