chore: Relax agency card form class check#3481
Conversation
|
This PR updates |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
thekaveman
left a comment
There was a problem hiding this comment.
Hmm, I guess I didn't realize how far-reaching it would be to remove agency_card as an option from SystemName. Not that I disagree!
Part of the reason we set up CST was to have a "language neutral" agency for demos etc. that wouldn't favor/throw shade on a real agency, and this takes us away from that idea while majorly simplifying the overall codebase, so I think this is the right direction.
I'll just recommend that we also remove the CSTAgencyCard form and its associated copy as well, since it seems that is no longer relevant with these changes.
jgravois
left a comment
There was a problem hiding this comment.
nice work @Scotchester. i'm all for nuking agency_card. 💣
| f"This agency/flow combination does not support Eligibility API verification: " | ||
| f"{view.agency.slug}, {view.flow.system_name}" | ||
| ) | ||
| expected_error_msg = "The senior flow does not support Eligibility API verification." |
There was a problem hiding this comment.
my (slight) preference would be to reuse, but no offense taken if you disagree though.
its very unlikely that someone could change the variable above and miss this reference immediately below.
| expected_error_msg = "The senior flow does not support Eligibility API verification." | |
| expected_error_msg = f"The {view.flow.system_name} flow does not support Eligibility API verification." |
We no longer worry about matching agency slugs, only the flow's `system_name`.
Co-authored-by: John Gravois <jgravois@compiler.la>
Co-authored-by: Kegan Maher <kegan@compiler.la>
8c3c050 to
837fa05
Compare
|
Rebased and feedback addressed! Ready for final review. |
There was a problem hiding this comment.
this code all looks reasonable to me, i didn't have any trouble running the migration locally. afterward the canned Agency card self-serve enrollment flow worked a treat 😄
Your card information may not have been entered correctly.
The number and last name must be entered exactly as they appear on your MST Courtesy Card. Please check your card and try again, or contact your transit agency for help.
i'm under the impression that is expected because the flow isn't configured correctly for e2e testing on main locally either.
we can confirm that all is still well on https://dev-benefits.calitp.org after merging. no offense taken if anyone else wants other eyes on this PR before you merge it though.
Did you follow the full set of testing instructions I put in the OP? No shame if you overlooked them, I just want to understand why they didn't work if you did. |
Co-authored-by: john gravois <jagravois@gmail.com>
i did not and i disagree with you. shame on me! 🤦♂️ |
jgravois
left a comment
There was a problem hiding this comment.
RTFM and confirmed this works e2e. nice work @Scotchester 💪
Requested change has been made.
|
Thanks @jgravois! Could you also give this a review so I can merge them at the same time? cal-itp/eligibility-server#597 |
There was a problem hiding this comment.
This looks good to me too. We just need @thekaveman's review and then will be good to go Oh nvm, didn't realize you could dismiss the review like that 😅
|
I'll hold on just the same to make sure he confirms I removed what he asked for :) |
this removes the usage of AgencySlug from the ConfirmView
this removes the usage of AgencySlug from the ConfirmView
thekaveman
left a comment
There was a problem hiding this comment.
I didn't go through the QA steps, but my requested change was addressed!
Thanks everyone for the work on this 👍
Closes #3398
We no longer want to worry about matching agency slugs, only the flow's
system_name. This will allow agencies to honor other agencies' programs.Testing instructions
.devcontainer/server/settings.pyto changeIMPORT_FILE_PATHtohttps://raw.githubusercontent.com/cal-itp/eligibility-server/cd5fed032524e4753a97573a7caef6fa17ea398b/data/server.csvflask drop-dbfollowed byflask init-db