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 usage of view models #1442

Closed
19 of 20 tasks
thekaveman opened this issue Jun 22, 2023 · 4 comments · Fixed by #1563
Closed
19 of 20 tasks

Refactor usage of view models #1442

thekaveman opened this issue Jun 22, 2023 · 4 comments · Fixed by #1563
Assignees
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

Comments

@thekaveman
Copy link
Member

thekaveman commented Jun 22, 2023

As shown in #1436, adding or moving around copy driven by translations requires changes in multiple places:

  • models, to reference the PO entries
  • PO files, to hold the translations
  • view view models, to get the data into the view

We need to look into how to simplify this, ideally cutting out extraneous view models that already have a corresponding model, and sending that directly to the template instead.

Plan

After some different iterations, this is the approach we landed on:

  • Move all translated strings out of models, and as close to the templates (HTML) as possible
  • Use fields in the models to reference template variants when needed (e.g. for TransitAgency and EligibilityVerifier)
  • Simplify the views as much as possible: remove usage of viewmodels, create basic dictionary context that has exactly the fields needed for that view/page

Acceptance Criteria

The following viewmodels and helpers are removed:

@thekaveman
Copy link
Member Author

We could move most of what view models are doing to using Django template tags. This seems like a good approach because:

  • Removes need for view model classes, can pass needed data to each tag or get it from context
  • Moves us closer to the concept of "components", will simplify and clarify templates and help reuse
  • Likely simplifies view functions as well
  • Tags should be straightforwardly testable

Django makes two types available OOTB:

  • simple tags return a string to be wrapped by other HTML in a template
  • inclusion tags use their own named template file, like includes do in the HTML

Django also has a way to write more advanced custom tags, probably more than we need for now.

@machikoyasuda
Copy link
Member

machikoyasuda commented Jun 27, 2023

Putting these notes here:

There are 3 places in the new Veteran flow app that requires logic in the views with knowledge of EligibilityVerifier-specific information:

  • Eligibility Start (show 3 bullets vs. 4 bullets in the first identity MediaItem)
  • Sign Out link (show Sign Out link or not -- could have some sort of EligibilityVerifier.supports_signout boolean on the model) Refactor sign-out link logic for AuthProvider's that support it #1456
  • Use Button.login() (which sets the button's CSS ID to login and specifies a hard-coded fallback text of Login.gov). The Veteran flow would use a regular button without any special CSS ID or fallback text.

@thekaveman
Copy link
Member Author

thekaveman commented Jul 11, 2023

We have a pretty solid plan:

  • Move all translated strings out of models, and as close to the templates (HTML) as possible
  • Use fields in the models to reference template variants when needed (e.g. for TransitAgency and EligibilityVerifier)
  • Simplify the views as much as possible: remove usage of viewmodels, create basic dictionary context that has exactly the fields needed for that view/page

And some different pieces of this implementation:

@thekaveman thekaveman reopened this Jul 12, 2023
@thekaveman thekaveman removed their assignment Jul 12, 2023
@thekaveman thekaveman changed the title Investigate/refactor usage of view models Refactor usage of view models Jul 12, 2023
@thekaveman thekaveman added chore Chores and tasks for code cleanup, dev experience, admin/configuration settings, etc. front-end HTML/CSS/JavaScript and Django templates back-end Django views, sessions, middleware, models, migrations etc. labels Jul 14, 2023
@thekaveman thekaveman self-assigned this Jul 23, 2023
@thekaveman thekaveman linked a pull request Jul 25, 2023 that will close this issue
@thekaveman
Copy link
Member Author

Blocked: #1563 (comment)

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
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants