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

Migrate JSONField to Django builtin JSONField #919

Merged
merged 7 commits into from
Oct 10, 2024
Merged

Conversation

ericholscher
Copy link
Member

This runs a migration that converts from text to jsonb.

In my local testing it converted the field properly:

ethicaladserver=# select targeting_parameters from adserver_flight;
    targeting_parameters
----------------------------
 {                         +
     "niche_targeting": 0.9+
 }
(1 row)

ethicaladserver=# select targeting_parameters from adserver_flight;
   targeting_parameters
--------------------------
 {"niche_targeting": 0.9}
(1 row)

This runs a migration that converts from text to jsonb.

In my local testing it converted the field properly:

```
ethicaladserver=# select targeting_parameters from adserver_flight;
    targeting_parameters
----------------------------
 {                         +
     "niche_targeting": 0.9+
 }
(1 row)

ethicaladserver=# select targeting_parameters from adserver_flight;
   targeting_parameters
--------------------------
 {"niche_targeting": 0.9}
(1 row)

```
Copy link
Collaborator

@davidfischer davidfischer left a comment

Choose a reason for hiding this comment

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

Not sure what the deal is with simple history, but sans that this looks good.

@davidfischer
Copy link
Collaborator

Not sure what the deal is with simple history, but sans that this looks good.

I'll take a look at this and see if I can't resolve the issue.

@davidfischer
Copy link
Collaborator

The test failures seem to be a result of jazzband/django-simple-history#1181 which is still open. We may have to either ignore historical changes on jsonfields, stop adding a change reason (remove calls to update_change_reason), or wait on updating our JSONFields.

@ericholscher
Copy link
Member Author

FWIW, I never look at update_change_reason, though that's mostly because simple history doesn't do diffs, so the history is mostly useless to understand what changed.

This is a temporary fix until the underlying issue is fixed.
@davidfischer
Copy link
Collaborator

I created a PR #924 to remove update_change_reason so we can move forward with this.

@ericholscher
Copy link
Member Author

Cool. Let's plan to get it merged and deployed next week. 👍

…change-reason-removal

Remove simple history change reason
@davidfischer davidfischer merged commit 6227400 into main Oct 10, 2024
1 check passed
@davidfischer davidfischer deleted the migrate-jsonfield branch October 10, 2024 22:09
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