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: centralize management of Django routes #2308

Merged
merged 12 commits into from
Aug 20, 2024
Merged

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Aug 16, 2024

Closes #2303

Part of #2252

Context setting

Right now we have Django route strings, e.g. app_name:view_name all over the place: in views, in middleware, in models, in templates, in URL config, in tests... it is truly a mess and difficult to get a handle on.

This PR refactors every route string in the app into a top-level benefits/routes.py module. Everywhere else mentioned above that needs a route string, uses this central module to get it.

Implementation details

  • Create a class benefits.routes.Routes with an @property for every route in the app
  • Property names are exactly like the current ROUTE_NAME variables, cleaned up and organized by app e.g.
    • INDEX for the homepage
    • HELP for the help page
    • ELIGIBILITY_INDEX for the entry to eligibility phase
    • ENROLLMENT_INDEX for the entry to enrollment phase
    • etc.
  • Note: I decided against CORE_ routes, it felt a little extra... but happy to add that for consistency.
  • Property docstrings """Doc strings""" are used to describe what the route is for, handy for devs using these props
  • Define an instance of the class in the module: routes = Routes()
  • This instance is imported and used elsewhere e.g. in views:
    from benefits.routes import routes
    
    redirect(reverse(routes.INDEX))
  • Helper function on the class Routes.name(route: str) -> str gets the name portion, e.g. name from app:name
  • urls.py uses this Routes.name() helper when registering URLs, avoiding duplication
  • New benefits.core.context_processors.routes() puts every route in the template context, so all hardcoded routes could be removed from templates:
    <a href="{% url routes.INDEX %}">Go home</a>

@thekaveman thekaveman added the chore Chores and tasks for code cleanup, dev experience, admin/configuration settings, etc. label Aug 16, 2024
@thekaveman thekaveman self-assigned this Aug 16, 2024
@github-actions github-actions bot added tests Related to automated testing (unit, UI, integration, etc.) front-end HTML/CSS/JavaScript and Django templates migrations [auto] Review for potential model changes/needed data migrations updates back-end Django views, sessions, middleware, models, migrations etc. labels Aug 16, 2024
Copy link

github-actions bot commented Aug 16, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  routes.py
  benefits/core
  context_processors.py
  middleware.py
  models.py
  session.py
  urls.py
  views.py
  benefits/eligibility
  forms.py
  urls.py
  views.py 32
  benefits/enrollment
  forms.py
  urls.py
  views.py 246
  benefits/in_person
  urls.py
  views.py
  benefits/oauth
  middleware.py
  redirects.py
  urls.py
  views.py
Project Total  

This report was generated by python-coverage-comment-action

@thekaveman thekaveman force-pushed the refactor/routes branch 3 times, most recently from 013a964 to e5d86d2 Compare August 16, 2024 20:22
@lalver1 lalver1 force-pushed the refactor/eligibilitytype-enrollmentflow branch 2 times, most recently from e807f90 to 903f76f Compare August 16, 2024 20:58
Base automatically changed from refactor/eligibilitytype-enrollmentflow to main August 16, 2024 21:18
@thekaveman thekaveman marked this pull request as ready for review August 16, 2024 21:37
@thekaveman thekaveman requested a review from a team as a code owner August 16, 2024 21:37
lalver1
lalver1 previously approved these changes Aug 19, 2024
Copy link
Member

@lalver1 lalver1 left a comment

Choose a reason for hiding this comment

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

The code looks good to me and I didn't see any dangling routes (searched for the regex "[A-Za-z0-9_]*:[A-Za-z0-9_]*" and the results are all in routes.py). I only saw one place where the route changed but I think the change makes sense 👍

@@ -37,7 +29,7 @@ def index(request, agency=None):
else:
session.update(request, eligible=False, origin=agency.index_url)
else:
session.update(request, agency=agency, eligible=False, origin=reverse(ROUTE_CORE_INDEX))
session.update(request, agency=agency, eligible=False, origin=agency.index_url)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is correct, since if the session has an agency, the origin is agency.index_url --> routes.AGENCY_INDEX --> "core:agency_index" (url "/cst"). But originally origin was set to "core:index" via ROUTE_CORE_INDEX (url "/") so just wanted to make sure this change was intended.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks for noticing this and asking.

I did intentionally make this change because it seemed wrong to direct the user to core:index if they had an agency selected already.

@thekaveman thekaveman requested a review from angela-tran August 19, 2024 20:24
@thekaveman
Copy link
Member Author

I think it would be good if @angela-tran takes a look before this is merged.

angela-tran
angela-tran previously approved these changes Aug 19, 2024
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

This refactor looks good to me. I left a question about how the routes context processor works, which I think is more just for my understanding rather than pointing out a needed change.

benefits/core/context_processors.py Outdated Show resolved Hide resolved
@thekaveman thekaveman dismissed stale reviews from angela-tran and lalver1 via b2db54f August 19, 2024 22:32
@thekaveman thekaveman force-pushed the refactor/routes branch 3 times, most recently from 374d3e6 to c37ef28 Compare August 19, 2024 22:52
@thekaveman thekaveman requested a review from angela-tran August 19, 2024 22:54
@thekaveman thekaveman merged commit 7664917 into main Aug 20, 2024
15 checks passed
@thekaveman thekaveman deleted the refactor/routes branch August 20, 2024 19:20
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. chore Chores and tasks for code cleanup, dev experience, admin/configuration settings, etc. front-end HTML/CSS/JavaScript and Django templates migrations [auto] Review for potential model changes/needed data migrations updates tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chore: update route for re-enrollment error
3 participants