-
Notifications
You must be signed in to change notification settings - Fork 153
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
Uplift pni tristate fields #3762
Conversation
@cadecairos I've added you for review mostly because I'm also touching some of the factory stuff, so making sure that all looks fine will help out a lot! |
Awesome! I've check-off what has been done in the spreadsheet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no issues with the changes. Copied staging locally and applied the migrations without any issue, and the pni page looks fine afterwards. Let's try this on staging!
\o/ |
@@ -0,0 +1,83 @@ | |||
# Generated by Django 2.2.5 on 2019-10-09 20:34 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are all NullBoolean to Boolean(null=True) updates
network-api/networkapi/buyersguide/migrations/0028_uplift_tri_to_quadstate.py
Outdated
Show resolved
Hide resolved
network-api/networkapi/buyersguide/migrations/0028_uplift_tri_to_quadstate.py
Outdated
Show resolved
Hide resolved
network-api/networkapi/buyersguide/migrations/0028_uplift_tri_to_quadstate.py
Outdated
Show resolved
Hide resolved
@@ -1,13 +1,24 @@ | |||
# Generated by Django 2.2.5 on 2019-10-09 00:10 | |||
|
|||
from django.db import migrations, models | |||
from networkapi.buyersguide.models import Product |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change makes sure we use a Product
based on the database schema, not what's in models.py (since models.py can change in the future, and then this migration would crash...)
('buyersguide', '0027_null_boolean_field'), | ||
] | ||
|
||
operations = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is a four stage uplife:
- build new fields, with unique names, that use the new 4-state field type
- migrate the 3-state data from the old fields as 4-state data to the new fields
- delete the old fields
- rename the new fields so they use the names of the old fields
This means that at the end of this migrations, nothing has really changed except the fields types got changed from NullBoolean to ExtendedYesNo, and some of the fields got renamed as per the spreadsheet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code makes sense to me and works well locally. 👍
Builds on #3757 and uplifts all
NullBoolean
fields toBoolean(null=True)
(new recommended way to do things), save the four fields that needed to become the new ExtendedYesNoField (Yes
/No
/Unknown
/Not applicable
).New code for this PR only: a8f3ba1 + 7510170
Related issues: #3729
One thing that we probably want to make sure works is running this on top of staging data, locally. This runs "from scratch" (using
inv docker-new-env
) but the more important question is whether these migrations won't break when presented with staging's data and schema