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

Extend update_fields with translation fields in Model.save() #687

Merged

Conversation

andrei-shabanski
Copy link
Contributor

An alternative approach to resolving #682

Override QuerySet._update() to update translation fields if only an original field is passed in save() in argument update_fields

@last-partizan last-partizan marked this pull request as draft June 4, 2023 07:38
@andrei-shabanski andrei-shabanski marked this pull request as ready for review June 4, 2023 11:15
@andrei-shabanski andrei-shabanski changed the title [DRAFT] Extend update_fields with translation fields in Model.save() Extend update_fields with translation fields in Model.save() Jun 4, 2023
@last-partizan last-partizan self-requested a review June 5, 2023 06:19
@andrei-shabanski
Copy link
Contributor Author

@last-partizan Tests for django < 4.2 fail because we don't add the original field in update_field.

Hence the following code updates only title_de field in django 4.2, and both fields title & title_de in django < 4.2

instance, created = models.TestModel.objects.update_or_create(
     pk=obj.pk, defaults={'title_de': 'NEW DE TITLE 2'}
)

Should we update the original field in the code below?

@last-partizan
Copy link
Collaborator

Are you sure? Looking at update_or_create which you removed, it doesn't seem to rewrite original key when passing translated key.

But, for the sake of consistency, i think it's fine to update 'title' when passing title_de to the function. But, only when 'de' is the default language. Otherwise it would be unexpected behaviour.

@last-partizan
Copy link
Collaborator

last-partizan commented Jun 8, 2023

4.0 tests are also failing, as you can see. So your original suggestion may be incorrect.

Oh, hevermind, i read it other way. You're correct.

@andrei-shabanski
Copy link
Contributor Author

The same errors (or behavior) were relevant for the removed implementation of update_or_create. We just didn't have tests to catch them

@andrei-shabanski
Copy link
Contributor Author

andrei-shabanski commented Jun 8, 2023

But, for the sake of consistency, i think it's fine to update 'title' when passing title_de to the function. But, only when 'de' is the default language. Otherwise it would be unexpected behaviour.

The current django-modeltranslation version saves the last assigned value to the original field into "title" column even when a secondary language is active currently. Check the sample below

>>> obj = TestModel(title='default')
... with override('en'):
...     obj.title = 'English variant'
...     obj.save()
... 
... TestModel.objects \
...     .rewrite(False) \
...     .filter(pk=obj.pk) \
...     .values('title', 'title_de', 'title_en') \
...     .first()

{
    "title": "English variant",
    "title_de": "default",
    "title_en": "English variant",
}

So "title" might store not only the default translation (-- that looks strange to me).

Should we check the currently active language to determine when the original field (title) must be updated? I mean to update "title" even if "de" isn't default language

@last-partizan
Copy link
Collaborator

So "title" might store not only the default translation (-- that looks strange to me).

AFAIK, docs are suggesting "not rely on the original field anymore", becouse it can be incosistent.

If we can make it consistent with minimal changes, then yes, let's do it. But if that would be too much rewriting, we should leave this for separate refactoring - if you want to fix it.

Right now we should focus on solving "Extend update_fields with translation variants", without breaking update_or_create. Maybe we even should exclude "title" from assertions, to get tests to pass.

@andrei-shabanski
Copy link
Contributor Author

I excluded "title" from the assertion. Not sure I can find the right approach now due to my limited knowledge of the codebase

@last-partizan
Copy link
Collaborator

I excluded "title" from the assertion. Not sure I can find the right approach now due to my limited knowledge of the codebase

@andrei-shabanski please, add a note about this in the test.

@last-partizan
Copy link
Collaborator

Please reabse it on latest master, CI was broken because latest mariadb was failing to start.

@last-partizan last-partizan self-requested a review June 25, 2023 07:54
@last-partizan last-partizan merged commit d86c6de into deschler:master Jul 16, 2023
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