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

Make the transfer status fields nullable #143

Merged
merged 3 commits into from
Jan 13, 2022

Conversation

bjester
Copy link
Member

@bjester bjester commented Jan 13, 2022

Summary

  • Prevents Django setting "" default for new transfer status related fields

TODO

  • Have tests been updated for the new code?
  • Has documentation been written/updated?
  • New dependencies (if any) added to requirements file

Reviewer guidance

Tested both migration pathways, with previous migration already applied and without any migrations

Issues addressed

List the issues solved or partly solved by the PR

Documentation

If the PR has documentation, link the file here (either .rst in your repo or if built on Read The Docs)

Copy link
Member

@jamalex jamalex left a comment

Choose a reason for hiding this comment

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

Testing both upgrades pathways, which you did, was my main question. Besides that, I couldn't see anything from a skim of the rest of the code the seemed to rely on these fields being str instances (mostly truthiness or equivalence), and the tests all pass, so looks good to me! Thanks!

@bjester bjester merged commit 7eb702f into learningequality:release-v0.6.x Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants