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

Fix(models): allow blank #1979

Merged
merged 3 commits into from
Mar 25, 2024
Merged

Fix(models): allow blank #1979

merged 3 commits into from
Mar 25, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Mar 22, 2024

Closes #1977

This PR sets blank to True for all nullable fields on our models. This is so form validation on those fields will allow blank values.

@angela-tran angela-tran self-assigned this Mar 22, 2024
@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. deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Mar 22, 2024
Copy link

github-actions bot commented Mar 22, 2024

Coverage report

Click to see where and how coverage changed

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

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

in addition to allowing null in the database for this field, forms
should allow blank values for this field.
@angela-tran angela-tran marked this pull request as ready for review March 22, 2024 21:08
@angela-tran angela-tran requested a review from a team as a code owner March 22, 2024 21:08
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.

I get that these are only changes that impact the Admin forms, but I still think a migration is required.

  1. Check out dev and run bin/reset_db.sh (now DB looks like it does on dev)
  2. Check out this branch and run bin/init.sh (which is what the appcontainer will run in dev)
  3. See output:
Operations to perform:
  Apply all migrations: admin, auth, contenttypes, core, django_google_sso, sessions
Running migrations:
  No migrations to apply.
  Your models in app(s): 'core' have changes that are not yet reflected in a migration, and so won't be applied.
  Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

@thekaveman thekaveman added the bug Something isn't working label Mar 22, 2024
@angela-tran
Copy link
Member Author

I get that these are only changes that impact the Admin forms, but I still think a migration is required.

Ah, thanks for catching this

@angela-tran angela-tran marked this pull request as draft March 22, 2024 21:32
@angela-tran angela-tran marked this pull request as ready for review March 25, 2024 19:58
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.

This isn't totally related, but I feel like since you're updating models for editing in the Admin...

It seems like we forgot to list AuthProvider in the models that are registered for editing in the Admin: https://github.com/cal-itp/benefits/blob/dev/benefits/core/admin.py#L19

Do you think you can add that here so we can just be done with it?

@thekaveman thekaveman mentioned this pull request Mar 25, 2024
2 tasks
this is so AuthProvider can be edited too
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.

👍

@angela-tran angela-tran merged commit 3f33ea6 into dev Mar 25, 2024
13 checks passed
@angela-tran angela-tran deleted the fix/allow-blank branch March 25, 2024 23:30
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. bug Something isn't working deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot save EligibilityVerifier without providing an AuthProvider
2 participants