-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feat: Agency card direct route #2312
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
back-end
Django views, sessions, middleware, models, migrations etc.
tests
Related to automated testing (unit, UI, integration, etc.)
migrations
[auto] Review for potential model changes/needed data migrations updates
and removed
tests
Related to automated testing (unit, UI, integration, etc.)
labels
Aug 20, 2024
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
thekaveman
force-pushed
the
feat/agencycard-route
branch
2 times, most recently
from
August 20, 2024 15:49
28f740b
to
7fc1309
Compare
thekaveman
force-pushed
the
feat/agencycard-route
branch
from
August 20, 2024 19:21
7fc1309
to
68ab430
Compare
thekaveman
force-pushed
the
feat/agencycard-route
branch
2 times, most recently
from
August 22, 2024 23:18
388b9db
to
49a27f6
Compare
The code looks good to me and it was helpful to see how to normalize the structure of all agency redirects 👍 |
* one default generic, not fully configured * one configured for claims validation * one configured for eligibility verification this makes it clearer which one to use for a given case, and cleans up a number of other fixtures and tests
view function placeholder
initialize user's session for agency, and then for the last (only) eligibility API flow found (the normal case) "last" here is ordering by pk, which is implicitly ordering on when flow objects were created in the database, so in the (very edge, currently unsupported) case of multiple eligibility API flows, this picks the most recent one that was defined
normalize the structure of all agency redirects into the form: /{agency.slug}/{redirect} e.g. /cst/agencycard /cst/eligibility vs. the inconsistent mix before: /cst/agencycard /eligibility/cst
thekaveman
force-pushed
the
feat/agencycard-route
branch
from
August 26, 2024 16:53
49a27f6
to
69175d6
Compare
angela-tran
approved these changes
Aug 26, 2024
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.
This looks good to me. Tested it locally too
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
back-end
Django views, sessions, middleware, models, migrations etc.
migrations
[auto] Review for potential model changes/needed data migrations updates
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2252
Edited the URL to be
/{slug}/agency-card
per #2252 (comment)In addition to the tasks of #2252, this also refactors the one other agency-specific shortcut route to normalize it into the same pattern as the now (2) other agency-specific shortcut routes.
Instead of:
/cst/agency-card
/cst/publickey
/eligibility/cst
We now have:
/cst/agency-card
/cst/publickey
/cst/eligibility
This route is used on the agency selector modal: it is the route the user goes through to get to
eligibility:index
when they start at thecore:index
(which stores the transit agency selection in the user's session).This differs slightly from the agency index page
core:agency_index
; in that case, although the transit agency is also stored in the user's session, the user's CTA on the page is a single button that does not use this shortcut, but goes directly to the/eligibility
page (since the transit agency is already configured in the session).I also refactored the pytest model fixtures a bit to get them into a shape that I could more easily use to test this new functionality.
Testing
F5
/cst/agency-card
--> you should be redirected to theeligibility:confirm
page/cst/agency-card
--> you should be redirected to theeligibility:confirm
page with the newer flow selected/cst/agency-card
--> you should be redirected to the user error page