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 #1521

Merged
merged 6 commits into from Jul 13, 2023
Merged

Refactor: agency links #1521

merged 6 commits into from Jul 13, 2023

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Jul 12, 2023

Part of #1520

The eligibility:unverified view uses the agency links construct. They also show up in two other views:

  • core:help
  • enrollment:retry

In eligibility and enrollment, we assume an agency has been selected by the user.

For core:help, the user may navigate there before selecting an agency, so we show all "agency links" for active agencies.

This PR moves the variation and content to a new template field: TransitAgency.links_template, referencing an includes file specific for the agency. The include is used explicitly where it is needed in templates.

This PR removes unused TransitAgency fields as well as deleting the Button.agency_contact_links() viewmodel helper.

@github-actions github-actions bot added migrations [auto] Review for potential model changes/needed data migrations updates back-end Django views, sessions, middleware, models, migrations etc. front-end HTML/CSS/JavaScript and Django templates tests Related to automated testing (unit, UI, integration, etc.) labels Jul 12, 2023
@thekaveman thekaveman changed the title Refactor/agency links Refactor: agency links Jul 12, 2023
@thekaveman thekaveman force-pushed the refactor/eligibility-index branch 2 times, most recently from 8f3f977 to 479ed3e Compare July 12, 2023 04:43
@thekaveman thekaveman self-assigned this Jul 12, 2023
@thekaveman thekaveman added this to the Veterans milestone Jul 12, 2023
@thekaveman thekaveman mentioned this pull request Jul 12, 2023
20 tasks
@thekaveman thekaveman force-pushed the refactor/agency-links branch 3 times, most recently from 064d732 to c1d847a Compare July 12, 2023 05:52
@thekaveman thekaveman force-pushed the refactor/agency-links branch 2 times, most recently from fb33b6c to 37e2943 Compare July 13, 2023 17:00
Base automatically changed from refactor/eligibility-index to dev July 13, 2023 17:05
@thekaveman thekaveman marked this pull request as ready for review July 13, 2023 17:06
@thekaveman thekaveman requested a review from a team as a code owner July 13, 2023 17:06
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.

I think the removal of viewmodel helper code in this PR looks great!

A separate thing I'm thinking about is that by moving the fields from TransitAgency into the templates, it's not as easy for a non-developer to update that information from an admin interface - it would instead require a code change in a template. This came from me thinking about the documentation for configuring a new transit agency. The tradeoff I guess is between being able to customize what shows up in an agency's links vs. saying every agency should show the same info which we can then edit in the database.

@thekaveman
Copy link
Member Author

thekaveman commented Jul 13, 2023

....it's not as easy for a non-developer to update that information from an admin interface

The tradeoff I guess is between being able to customize what shows up in an agency's links vs. saying every agency should show the same info which we can then edit in the database.

This is a good point. Alternative idea:

  • Keep the info_url and phone fields, drop the template field for now
  • Update the context_processor, render all agency links the same using context
  • Later we can override the template if we need to

Basically just removing the viewmodel layer, but still using the normal context and default rendering template.

@angela-tran
Copy link
Member

Alternative idea:

  • Keep the info_url and phone fields, drop the template field for now
  • Update the context_processor, render all agency links the same using context
  • Later we can override the template if we need to

Basically just removing the viewmodel layer, but still using the normal context and default rendering template.

This sounds good to me 👍

@thekaveman thekaveman marked this pull request as draft July 13, 2023 18:33
@thekaveman thekaveman marked this pull request as ready for review July 13, 2023 19:22
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.

Looks good!

@thekaveman thekaveman merged commit dcf77a9 into dev Jul 13, 2023
13 checks passed
@thekaveman thekaveman deleted the refactor/agency-links branch July 13, 2023 19:54
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. 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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants