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

Refactor: agency links context #1390

Merged
merged 7 commits into from May 4, 2023
Merged

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented May 2, 2023

Closes #1054

This PR implements the proposed solution on the ticket so that for the three pages that show agency links (help, unverified, and retry):

Testing locally

/help and /unverified are easy to test by going through the flow (use Courtesy Card with ineligible user credentials).

To test the /enrollment/retry view, you can modify the code as follows. Then, make sure to populate an agency in your session (either through the agency selector modal or going directly to an agency's slug) so that you can go directly to the endpoint and see that agency's contact links.

-@decorator_from_middleware(EligibleSessionRequired)
+# @decorator_from_middleware(EligibleSessionRequired)
    """View handler for a recoverable failure condition."""
-    if request.method == "POST":
+    if request.method == "GET":

@angela-tran angela-tran requested a review from a team as a code owner May 2, 2023 19:15
@angela-tran angela-tran self-assigned this May 2, 2023
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. labels May 2, 2023
@machikoyasuda
Copy link
Member

Testing locally - all looks good

  • Help page without choosing an agency

image

  • Help page after selecting MST

image

  • Unverified

image

Testing locally - enrollment/retry

image

I changed my local code at https://github.com/cal-itp/benefits/blob/145ebc126b48fb78739c61557892c3feb0e1cedc/benefits/enrollment/views.py#LL146C2-L146C2 according to your notes, and then routing to http://localhost:11369/enrollment/retry directly. I get the screenshot above. No agency links. Not sure if this is a bug on my configuration side, or the PR itself

return TemplateResponse(request, TEMPLATE_UNVERIFIED, page.context_dict())
ctx = page.context_dict()
ctx["agency_links"] = agency_links
ctx["back_button"] = back_button
Copy link
Member

Choose a reason for hiding this comment

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

Great solution 🎊 So much better than my hacky past logic!

@angela-tran
Copy link
Member Author

I get the screenshot above. No agency links. Not sure if this is a bug on my configuration side, or the PR itself

Ah, yeah I forgot to mention a required step. You'll want to have an agency in your session, so that means you should start from the homepage, choose a transit provider, and then go to /enrollment/retry.

@machikoyasuda
Copy link
Member

@angela-tran Got it! I went through the flow properly (entered in an eligible Courtesy Card user) and got to the LP screen, then manually went to http://localhost:11369/enrollment/retry and it worked:

image

@machikoyasuda machikoyasuda self-requested a review May 3, 2023 22:20
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Agreed this code looks much clearer! Nice work 👍

@angela-tran
Copy link
Member Author

Thanks for the reviews @machikoyasuda and @thekaveman! ☺️

@angela-tran angela-tran merged commit c3dd537 into dev May 4, 2023
17 checks passed
@angela-tran angela-tran deleted the refactor/agency-links-context branch May 4, 2023 01:25
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. deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make agency links context explicit
3 participants