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

CORE Overwrite empty language fields with default value (SH-90) #754

Closed

Conversation

hrayr-artunyan
Copy link
Contributor

@hrayr-artunyan hrayr-artunyan commented Aug 24, 2016

When a user completes a form that includes translatable fields (e.g.
Product) and only fill out the default language portion (e.g. English)
the form will fill out the other language fields with that of the
english. This fix is meant to address limitations in django-parlers own
fallback mechanism.

This commit also includes a new management command
shuup_fallback_language that will perform the same functionality on
existing models. It's meant to be ran only once as a migration step.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2303/
Test PASSed.

@Pikkupomo
Copy link
Contributor

I think the management command should be a datamigration.

The reason I think so is that:

  • IMO management commands are commands that should be run many times (like collect translations etc) or ones that can be run only once but have re-usability on clean installations (shuup_init)
  • This is a one time script that should be run when this update is being run on the shuup. After this update it's useless to even have this script since all translated fields will be replaced with values on saving the object. And it has no use in clean installs after this update.

@tulimaki tulimaki changed the title CORE Overwrite empty language fields with default value CORE Overwrite empty language fields with default value (SH-90) Aug 25, 2016
@hrayr-artunyan hrayr-artunyan force-pushed the fix-fallback-language branch 2 times, most recently from 65aba15 to ff8dde0 Compare August 25, 2016 18:52
@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2312/
Test FAILed.

@hrayr-artunyan
Copy link
Contributor Author

Retest this please

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2313/
Test FAILed.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2314/
Test FAILed.

@tulimaki
Copy link
Contributor

retest this please

for obj in objects:
# model being translatable does not guarantee translations exist
if obj.translations.count():
default_translation = obj.get_translation(default_lang)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail if the object doesn't have translation in PARLER_DEFAULT_LANGUAGE_CODE. That happens some times since I think only currently active language is required most of the MultiLanguageModelForm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default_language is a required field in MultiLanguageModelForm isn't it?
self.default_language = kwargs.pop("default_language", getattr(self, 'language', get_language()))

Where is active language set?

Copy link
Contributor

@tulimaki tulimaki Aug 25, 2016

Choose a reason for hiding this comment

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

I think not. Can you test it with for few admin modules like products, categories and shops? I think that most of the cases only the currently active language is required. That's probably a bug though. But maybe not for this task. But the migration should work in those "corrupted" databases too.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I am right here on solution probably would be to check the default translation for one field with safe_translation_getter

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2323/
Test FAILed.

# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.apps import apps as django_apps
Copy link
Contributor

@Pikkupomo Pikkupomo Aug 25, 2016

Choose a reason for hiding this comment

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

wouldn't from django import apps suffice? or import django.apps or something else. Shouldn't be needed to use as.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pikkupomo, the function in which I reference apps, already passes apps as an argument, so there's a conflict. apps passed by the function is different from django.apps.apps

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2326/
Test FAILed.

@hrayr-artunyan
Copy link
Contributor Author

retest this please

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2327/
Test FAILed.

@hrayr-artunyan
Copy link
Contributor Author

retest this please

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2328/
Test FAILed.

@hrayr-artunyan
Copy link
Contributor Author

retest this please

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2330/
Test FAILed.

@hrayr-artunyan
Copy link
Contributor Author

retest this please

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2331/
Test FAILed.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2334/
Test FAILed.

When a user completes a form that includes translatable fields (e.g.
Product) and only fill out the default language portion (e.g. English)
the form will fill out the other language fields with that of the
english. This fix is meant to address limitations in django-parlers own
fallback mechanism.

This commit also includes a data migration that will perform the same
functionality on existing translation models.

Refs SH-90
@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/2335/
Test FAILed.

@tulimaki
Copy link
Contributor

tulimaki commented Aug 26, 2016

For this we will be using different approach. We really need to follow parler here and copying the default taranslation is not the right way.

The "right" solution is already planned. Let's get back to this issue in a few days.

Close for now at least.

@tulimaki tulimaki closed this Aug 26, 2016
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.

None yet

4 participants