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 bulk_update() on I18n_JSON fields for homogenous values #10618 #10620

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

jacobtylerwalls
Copy link
Member

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Before, there was silent data loss when using bulk_update() on I18n_JSON fields such as node, datatype, widget, plugin configs.

Now, we at least allow bulk_updates to homogenous values. Heterogenous values (not yet supported) throw an error to prevent a silent failure.

The situation may improve in Django 5.1 if someone submits a PR with the patch identified in this ticket. Maybe that someone is one of us? :D

Issues Solved

Closes #10618

Opened a followup #10619 for the issue demo'd in the skipped test.

Checklist

  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Ticket Background

@jacobtylerwalls jacobtylerwalls changed the title Fix bulk_update() on I18n_JSON fields for heterogenous values Fix bulk_update() on I18n_JSON fields for homogenous values Feb 20, 2024
@jacobtylerwalls jacobtylerwalls linked an issue Feb 20, 2024 that may be closed by this pull request
@jacobtylerwalls jacobtylerwalls changed the title Fix bulk_update() on I18n_JSON fields for homogenous values Fix bulk_update() on I18n_JSON fields for homogenous values #10618 Feb 20, 2024
Copy link
Member

@apeters apeters left a comment

Choose a reason for hiding this comment

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

looks good to me. How should we remind ourselves to remove this code once we move to Django 5.1? A deprecation warning?

@jacobtylerwalls
Copy link
Member Author

Good call. I guess I'm hopeful it will be fixed in 5.1, but we don't know.

I can take the test that currently has an expected failure decorator and make it more specific to assert against NotImplementedError, and then if the underlying issue of sending Cast objects is fixed, that test will start failing because we won't reach that code block, and I'll leave a comment telling future us what to do.

@jacobtylerwalls jacobtylerwalls changed the base branch from dev/7.5.x to dev/7.6.x February 27, 2024 13:46
@jacobtylerwalls
Copy link
Member Author

Retargeted 7.6, as this isn't really important enough to go back to 7.5, just adding safety for development.

Copy link
Member

@apeters apeters left a comment

Choose a reason for hiding this comment

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

looks good

@apeters apeters merged commit 91f86b6 into dev/7.6.x Mar 25, 2024
4 checks passed
@apeters apeters deleted the i18n-json-bulk-update branch March 25, 2024 21:22
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.

bulk_update() not allowed for I18n_JSON fields
2 participants