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

Feat: prevent agency from being saved if it doesn't have copy for index #2713

Closed
wants to merge 1 commit into from

Conversation

angela-tran
Copy link
Member

While working on #2705, I realized that it would help prevent errors if TransitAgency.clean checks that the agency has all the copy it will need. That is, the clean method should try to call each @property method for getting the context dictionary for whatever page and make sure calling that method doesn't raise an error.

This is similar to how EnrollmentFlow.clean checks that the flow is able to get its in_person_eligibility_context if it is an in-person flow.

Basically if somehow we've forgotten to add copy for some slug, or if we accidentally remove copy from one of the mappings, then upon saving, we'll realize it.

@angela-tran angela-tran added this to the Simplify templates milestone Feb 28, 2025
@angela-tran angela-tran self-assigned this Feb 28, 2025
@github-actions github-actions bot added tests Related to automated testing (unit, UI, integration, etc.) back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev and removed tests Related to automated testing (unit, UI, integration, etc.) labels Feb 28, 2025
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/core/models
  transit.py
Project Total  

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

@angela-tran
Copy link
Member Author

angela-tran commented Feb 28, 2025

I realized while writing the PR description that this doesn't fully protect us from errors. For example, if we removed an entry for an existing TransitAgency, an error could still happen.

It would be better to have unit tests that can be more comprehensive.
(A unit test could iterate over the AgencySlug values and check that the core_context.agency_index mapping contains all of them. Something like that.)

@angela-tran angela-tran deleted the feat/validate-agency-index-copy-found branch February 28, 2025 03:33
@thekaveman thekaveman removed this from the Simplify templates milestone Feb 28, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants