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

Integrates django-modeltranslation as an optional app à la south #1019

Merged
merged 34 commits into from Mar 5, 2015

Conversation

Projects
None yet
8 participants
@Kniyl
Collaborator

Kniyl commented Apr 27, 2014

I finally decided to take the time to integrate my wrapper shown in #106 into mezzanine. The code is far better that way.

One downside though:

======================================================================
ERROR: test_mulisite (mezzanine.core.tests.CoreTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/fields/related.py", line 303, in __get__
    rel_obj = getattr(instance, self.cache_name)
AttributeError: 'RichTextPage' object has no attribute '_page_ptr_cache'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/mathias/PkgBuilding/mezzanine_devel/mezzanine/mezzanine/core/tests.py", line 238, in test_mulisite
    site1.delete()
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/base.py", line 694, in delete
    collector.collect([self])
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/deletion.py", line 197, in collect
    field.rel.on_delete(self, field, sub_objs, self.using)
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/deletion.py", line 17, in CASCADE
    source_attr=field.name, nullable=field.null)
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/deletion.py", line 197, in collect
    field.rel.on_delete(self, field, sub_objs, self.using)
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/deletion.py", line 17, in CASCADE
    source_attr=field.name, nullable=field.null)
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/deletion.py", line 181, in collect
    parent_objs = [getattr(obj, ptr.name) for obj in new_objs]
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/deletion.py", line 181, in <listcomp>
    parent_objs = [getattr(obj, ptr.name) for obj in new_objs]
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/fields/related.py", line 320, in __get__
    rel_obj = qs.get()
  File "/home/mathias/PkgBuilding/mezzanine_devel/lib/python3.4/site-packages/django/db/models/query.py", line 307, in get
    self.model._meta.object_name)
mezzanine.pages.models.DoesNotExist: Page matching query does not exist.

I tried to investigate why this is happening but I lack knowledge about the internals of mezzanine and django-modeltranslation. It can be that when models are registered for translation, their managers are not correctly patched but I’m very unsure about this.

Anyway, this issue has a workaround since it happens only when one tries to delete a Site with remaining Pages attached to it. If all pages are deleted beforehand, the Site delete smoothly.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Apr 28, 2014

Collaborator

Ok, found out that when dealing with

AttributeError: 'RichTextPage' object has no attribute '_page_ptr_cache'

django uses a queryset to retrieve pages associated to a site. (Line 312 of the file django/db/models/fields/related.py)
When modeltranslation is activated, it uses modeltranslation.translator.add_manager.<locals>.NewMultilingualManager to create a queryset which happens to be empty. Thus being unable to get the one page we are trying to delete.

I’ll try to dig a bit further before submitting an issue to the django-modeltranslation project. But this issue should resolve itself from a mezzanine point of view.

Collaborator

Kniyl commented Apr 28, 2014

Ok, found out that when dealing with

AttributeError: 'RichTextPage' object has no attribute '_page_ptr_cache'

django uses a queryset to retrieve pages associated to a site. (Line 312 of the file django/db/models/fields/related.py)
When modeltranslation is activated, it uses modeltranslation.translator.add_manager.<locals>.NewMultilingualManager to create a queryset which happens to be empty. Thus being unable to get the one page we are trying to delete.

I’ll try to dig a bit further before submitting an issue to the django-modeltranslation project. But this issue should resolve itself from a mezzanine point of view.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Apr 28, 2014

Owner

Thanks a lot for your work on this. It's a tough decision to merge it in - it's very badly needed, but it brings with it a huge dependency that will increase the load on maintaining Mezzanine.

That aside we'll probably merge it in, the need for it outweighs the extra effort. We actually did something like this before with modeltranslation and a bool setting, and it was full of bugs, so we removed it. That said, this looks a lot more complete, but we're going to have to test it ridiculously.

I'm extremely busy this month, so considering this won't be a quick merge, I probably won't have the space to look at it properly until late may, but in the meantime if you could really test it as much as possible, working through as many different scenarios with Mezzanine as possible (going over all its capabilities), that'd be a huge help.

Thanks again - it'll be great to have this merged and stable.

Owner

stephenmcd commented Apr 28, 2014

Thanks a lot for your work on this. It's a tough decision to merge it in - it's very badly needed, but it brings with it a huge dependency that will increase the load on maintaining Mezzanine.

That aside we'll probably merge it in, the need for it outweighs the extra effort. We actually did something like this before with modeltranslation and a bool setting, and it was full of bugs, so we removed it. That said, this looks a lot more complete, but we're going to have to test it ridiculously.

I'm extremely busy this month, so considering this won't be a quick merge, I probably won't have the space to look at it properly until late may, but in the meantime if you could really test it as much as possible, working through as many different scenarios with Mezzanine as possible (going over all its capabilities), that'd be a huge help.

Thanks again - it'll be great to have this merged and stable.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd
Owner

stephenmcd commented Apr 28, 2014

I've posted this thread to the mailing list:

https://groups.google.com/forum/#!topic/mezzanine-users/VXVfCU8OFGk

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Apr 28, 2014

Collaborator

I figured out what is wrong with deleting a Site object. I can’t come up with a proper solution but, in my opinion, this behaviour is acceptable.

What is acceptable in “not being able to delete a Site”?

Well, first of all it is still possible to delete a Site object. Either delete all the Pages it contains before deleting it, or delete the Site when it is activated. What fails is deleting a Site while an other one is activated if the one being delete contains some Displayable.

Second, this issue arise when django-modeltranslation is activated. Which means when both USE_MODELTRANSLATION and USE_I18N are set to True which is not the default. A comment pointing to the bug tracker and associated to one of these two settings can be an acceptable warning for activating a 2-years-old-requested non-bug-free feature.

Where does the problem come from?

The short answer is: from mezzanine.core.managers.CurrentSiteManager.

Let’s take a closer look at the deletion process. When a Site object gets deleted (other as well, but I’m focusing on Site because it fails) django collects all related objects to the Site and delete them in cascade, recursivingly deleting every of their related objects. The problem with Pages is that their content_model are derived classes. So these derived classes store a page_ptr_id into the database to mark their relationship with the Page instance. To retrieve this instance, a last query is made and our problems begins. (read the FIXME at line 177 of django/db/models/deletion.py)

What happens is that this query is looking for Page instances and will later be filtered out to get the one page corresponding to the page_ptr_id value. But this query is made through mezzanine.core.managers.CurrentSiteManager.get_query_set which filters out Page instances that don’t belong to the currently activated Site. Thus the error. Deleting a Site while it is activated works because the queryset contains the Page with the correct id, otherwise it fails because the queryset does not contain such page.

But it worked before introducing django-modeltranslation into mezzanine

Yes. That is because without django-modeltranslation the query is not made through a manager. The queryset is contructed by hand and is fetching all Page instances. What makes django-modeltranslation change this behaviour is that every models registered for translation are being assigned a custom manager (line 148 of file modeltranslation/translation.py) wich is multi-inherited from modeltranslation.managers.MultilingualManager and the base manager associated to the model (mezzanine.core.DisplayableManager in the case of Page objects). And our problem comes from the fact that modeltranslation.managers.MultilingualManager.use_for_related_fields is set to True.

It might be necessary for the internals of django-modeltranslation but it is causing our issue. It is triggering mezzanine.core.CurrentSiteManager.get_query_set when we don’t want it.

Is there a way to change that?

I don’t know. The fastest, ugliest way to patch that could be adding some code into mezzanine.utils.sites like:

def delete(self):
    old_site_id = current_site_id()
    activate_site_id(self.id)
    super(Site, self).delete()
    activate_site_id(old_site_id)
Site.delete = delete

where activate_site_id is not yet written.

A far better way would be to patch django/db/models/deletion.py to avoid executing a query for each getattr of the comprehension list. But I have no idea of how to do that.

Collaborator

Kniyl commented Apr 28, 2014

I figured out what is wrong with deleting a Site object. I can’t come up with a proper solution but, in my opinion, this behaviour is acceptable.

What is acceptable in “not being able to delete a Site”?

Well, first of all it is still possible to delete a Site object. Either delete all the Pages it contains before deleting it, or delete the Site when it is activated. What fails is deleting a Site while an other one is activated if the one being delete contains some Displayable.

Second, this issue arise when django-modeltranslation is activated. Which means when both USE_MODELTRANSLATION and USE_I18N are set to True which is not the default. A comment pointing to the bug tracker and associated to one of these two settings can be an acceptable warning for activating a 2-years-old-requested non-bug-free feature.

Where does the problem come from?

The short answer is: from mezzanine.core.managers.CurrentSiteManager.

Let’s take a closer look at the deletion process. When a Site object gets deleted (other as well, but I’m focusing on Site because it fails) django collects all related objects to the Site and delete them in cascade, recursivingly deleting every of their related objects. The problem with Pages is that their content_model are derived classes. So these derived classes store a page_ptr_id into the database to mark their relationship with the Page instance. To retrieve this instance, a last query is made and our problems begins. (read the FIXME at line 177 of django/db/models/deletion.py)

What happens is that this query is looking for Page instances and will later be filtered out to get the one page corresponding to the page_ptr_id value. But this query is made through mezzanine.core.managers.CurrentSiteManager.get_query_set which filters out Page instances that don’t belong to the currently activated Site. Thus the error. Deleting a Site while it is activated works because the queryset contains the Page with the correct id, otherwise it fails because the queryset does not contain such page.

But it worked before introducing django-modeltranslation into mezzanine

Yes. That is because without django-modeltranslation the query is not made through a manager. The queryset is contructed by hand and is fetching all Page instances. What makes django-modeltranslation change this behaviour is that every models registered for translation are being assigned a custom manager (line 148 of file modeltranslation/translation.py) wich is multi-inherited from modeltranslation.managers.MultilingualManager and the base manager associated to the model (mezzanine.core.DisplayableManager in the case of Page objects). And our problem comes from the fact that modeltranslation.managers.MultilingualManager.use_for_related_fields is set to True.

It might be necessary for the internals of django-modeltranslation but it is causing our issue. It is triggering mezzanine.core.CurrentSiteManager.get_query_set when we don’t want it.

Is there a way to change that?

I don’t know. The fastest, ugliest way to patch that could be adding some code into mezzanine.utils.sites like:

def delete(self):
    old_site_id = current_site_id()
    activate_site_id(self.id)
    super(Site, self).delete()
    activate_site_id(old_site_id)
Site.delete = delete

where activate_site_id is not yet written.

A far better way would be to patch django/db/models/deletion.py to avoid executing a query for each getattr of the comprehension list. But I have no idea of how to do that.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Apr 30, 2014

Collaborator

Dammit, I’m an idiot. I have this working:

def delete(self):
    old_site_id = current_site_id()
    activate_site_id(self.id)
    super(Site, self).delete()
    activate_site_id(old_site_id)
Site.delete = delete

and it makes all the tests pass again. But it is of no use since the admin uses bulk deletion through a queryset and never calls Site.delete directly. So back to the point where Site objects have to be deleted one by one. And they have to be the one who is activated when they are deleted.

Collaborator

Kniyl commented Apr 30, 2014

Dammit, I’m an idiot. I have this working:

def delete(self):
    old_site_id = current_site_id()
    activate_site_id(self.id)
    super(Site, self).delete()
    activate_site_id(old_site_id)
Site.delete = delete

and it makes all the tests pass again. But it is of no use since the admin uses bulk deletion through a queryset and never calls Site.delete directly. So back to the point where Site objects have to be deleted one by one. And they have to be the one who is activated when they are deleted.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl May 12, 2014

Collaborator

Latest version of django-model translation fixes the Site issue. See bug repport and associated commit.

Collaborator

Kniyl commented May 12, 2014

Latest version of django-model translation fixes the Site issue. See bug repport and associated commit.

@Romamo

This comment has been minimized.

Show comment
Hide comment
@Romamo

Romamo May 14, 2014

Please check my patch to django-modeltranslation. May be it will help you to remove duplicates
Romamo/django-modeltranslation@2e68811

Also you can review my standalone app https://github.com/Romamo/mezzanine-modeltranslation

Romamo commented May 14, 2014

Please check my patch to django-modeltranslation. May be it will help you to remove duplicates
Romamo/django-modeltranslation@2e68811

Also you can review my standalone app https://github.com/Romamo/mezzanine-modeltranslation

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl May 14, 2014

Collaborator

Duplicates are a result of mezzanine auto-adding fields in the admin if no fieldsets is provided. It adds the base field (which gets converted to several languages instances by modeltranslation) and it also add the per-language fields that modeltranslation added when models were registered for translation. Thus duplicating the widgets in admin.

I think that since it is mezzanine that creates the duplicates, it is better to fix it in mezzanine (this is what Kniyl/mezzanine@f89ebcd does for inlines) rather than in modeltranslation.

Collaborator

Kniyl commented May 14, 2014

Duplicates are a result of mezzanine auto-adding fields in the admin if no fieldsets is provided. It adds the base field (which gets converted to several languages instances by modeltranslation) and it also add the per-language fields that modeltranslation added when models were registered for translation. Thus duplicating the widgets in admin.

I think that since it is mezzanine that creates the duplicates, it is better to fix it in mezzanine (this is what Kniyl/mezzanine@f89ebcd does for inlines) rather than in modeltranslation.

@jerivas jerivas referenced this pull request May 23, 2014

Closed

Add Multilanguage support #106

@Kniyl Kniyl referenced this pull request May 23, 2014

Closed

enable multilingual website #29

@fguimara

This comment has been minimized.

Show comment
Hide comment
@fguimara

fguimara Jan 6, 2015

Any on this front? Is it supposed to be any merge soon?

This feature would be awesome on the production version

fguimara commented Jan 6, 2015

Any on this front? Is it supposed to be any merge soon?

This feature would be awesome on the production version

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Feb 3, 2015

Collaborator

Looking very good! The only issue I see right now (at least on the Admin side) is that inlines are not tabbed (for example, in Galleries and Forms). I'm not sure if Model Translation provides classes for them.

Collaborator

jerivas commented Feb 3, 2015

Looking very good! The only issue I see right now (at least on the Admin side) is that inlines are not tabbed (for example, in Galleries and Forms). I'm not sure if Model Translation provides classes for them.

@Lumpusz

This comment has been minimized.

Show comment
Hide comment
@Lumpusz

Lumpusz Feb 3, 2015

Hi Mathias, thank you for taking care of this long awaited feature request! Looking forward for this work to me merged into the official mezzanine repository.

Lumpusz commented Feb 3, 2015

Hi Mathias, thank you for taking care of this long awaited feature request! Looking forward for this work to me merged into the official mezzanine repository.

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Feb 3, 2015

Collaborator

So, off the top of my head, this is the list of things that need to be done:

  • Decide on the correct approach for doing i18n for the slugs.
  • Add some language switching mechanism to Inlines in the Admin.
  • Add some language switching mechanism to localizable settings in the Settings Admin.
  • Catch up with master to account for all changes in Mezzanine so far.
  • Write tests for translation related stuff.

@stephenmcd, what do you think about these points? Anything else that needs to be done before merge? :)

Collaborator

jerivas commented Feb 3, 2015

So, off the top of my head, this is the list of things that need to be done:

  • Decide on the correct approach for doing i18n for the slugs.
  • Add some language switching mechanism to Inlines in the Admin.
  • Add some language switching mechanism to localizable settings in the Settings Admin.
  • Catch up with master to account for all changes in Mezzanine so far.
  • Write tests for translation related stuff.

@stephenmcd, what do you think about these points? Anything else that needs to be done before merge? :)

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Feb 3, 2015

Collaborator

Just for reference, Model Translation DOES include Admin classes for inline Admins. https://django-modeltranslation.readthedocs.org/en/latest/admin.html#admin-inlines

Collaborator

jerivas commented Feb 3, 2015

Just for reference, Model Translation DOES include Admin classes for inline Admins. https://django-modeltranslation.readthedocs.org/en/latest/admin.html#admin-inlines

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 3, 2015

Collaborator

It does but these are not tabbed. Tabbed ones are only regular ones : https://django-modeltranslation.readthedocs.org/en/latest/admin.html#tabbed-translation-fields-admin-classes

Maybe a feature request to the modeltranslation team will avoid us to duplicate what they could come up with some day.

Collaborator

Kniyl commented Feb 3, 2015

It does but these are not tabbed. Tabbed ones are only regular ones : https://django-modeltranslation.readthedocs.org/en/latest/admin.html#tabbed-translation-fields-admin-classes

Maybe a feature request to the modeltranslation team will avoid us to duplicate what they could come up with some day.

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Feb 3, 2015

Collaborator

Hmm, according to this issue in model-translation tabbed inlines are supported (both the stacked and tabular flavors). Perhaps Mezzanine's custom inlines are messing something up?

Collaborator

jerivas commented Feb 3, 2015

Hmm, according to this issue in model-translation tabbed inlines are supported (both the stacked and tabular flavors). Perhaps Mezzanine's custom inlines are messing something up?

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Feb 10, 2015

Collaborator

Tried it out and it's working great. Only problem I see is that the language selector appears over the admin action buttons. Perhaps the action buttons need to be offset down or to the left to make room for the floating drop down.

Collaborator

jerivas commented Feb 10, 2015

Tried it out and it's working great. Only problem I see is that the language selector appears over the admin action buttons. Perhaps the action buttons need to be offset down or to the left to make room for the floating drop down.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Feb 10, 2015

Owner

Just tried it and noticed the same thing - but what I was getting at earlier was, do we really need separate language selectors in the admin for both the UI itself and content, and can we somehow have the translation fields leverage the UI admin selector?

Owner

stephenmcd commented Feb 10, 2015

Just tried it and noticed the same thing - but what I was getting at earlier was, do we really need separate language selectors in the admin for both the UI itself and content, and can we somehow have the translation fields leverage the UI admin selector?

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Feb 10, 2015

Collaborator

Only downside I see to coupling both selectors (as Matthias noticed) is that model translation and the UI languages might not be the same, and some users might not enable L10N, which disables the UI lang selector.

Collaborator

jerivas commented Feb 10, 2015

Only downside I see to coupling both selectors (as Matthias noticed) is that model translation and the UI languages might not be the same, and some users might not enable L10N, which disables the UI lang selector.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 10, 2015

Collaborator

Thing is, I don't really know where to put this selector. I had it in the header next to the L10N one but it was confusing.
But I agree that its position should be improved. And, imo, it should have a fixed one so the users can access it when they are editing translation fields down below.

Collaborator

Kniyl commented Feb 10, 2015

Thing is, I don't really know where to put this selector. I had it in the header next to the L10N one but it was confusing.
But I agree that its position should be improved. And, imo, it should have a fixed one so the users can access it when they are editing translation fields down below.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 10, 2015

Collaborator

I moved the selector down there. Is it better?
2015-02-10-215208_1366x768_scrot

Collaborator

Kniyl commented Feb 10, 2015

I moved the selector down there. Is it better?
2015-02-10-215208_1366x768_scrot

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Feb 10, 2015

Owner

Honestly I think it's a really confusing user experience to have two different language selectors, and more often than not the list of ui languages would match the list of content languages.

Owner

stephenmcd commented Feb 10, 2015

Honestly I think it's a really confusing user experience to have two different language selectors, and more often than not the list of ui languages would match the list of content languages.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 11, 2015

Collaborator

I agree. And the fact that I don't really know where to put the selector to be both visible and clearly identifiable tends to confirm that.
However, there are currently two issues of using only one selector:

  1. users who set USE_L10N to False won't be able to use a selector at all;
  2. their behavior is completely different. The L10N one redirects to django's set_language on a click, discarding changes when the page is shown again; and the modeltranslation's one just hide/show some divs on the same page on a click.

1 can be easily solved by using {% if settings.USE_L10N or settings.USE_MODELTRANSLATION %} in the template instead of just {% if settings.USE_L10N %}; but for 2... I still didn't have the epiphany to manage that.

Collaborator

Kniyl commented Feb 11, 2015

I agree. And the fact that I don't really know where to put the selector to be both visible and clearly identifiable tends to confirm that.
However, there are currently two issues of using only one selector:

  1. users who set USE_L10N to False won't be able to use a selector at all;
  2. their behavior is completely different. The L10N one redirects to django's set_language on a click, discarding changes when the page is shown again; and the modeltranslation's one just hide/show some divs on the same page on a click.

1 can be easily solved by using {% if settings.USE_L10N or settings.USE_MODELTRANSLATION %} in the template instead of just {% if settings.USE_L10N %}; but for 2... I still didn't have the epiphany to manage that.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 11, 2015

Collaborator

Ok… tried to get back on using tabs instead of the global switch to reduce confusion. I combined all tabs from TabularInline into a single one, though, so it is not ugly anymore. Does it seems good enought to be pushed or should I still try something else?

  • TabularInline:
    2015-02-11-224110_1366x768_scrot
  • Settings:
    2015-02-11-223928_1366x768_scrot
Collaborator

Kniyl commented Feb 11, 2015

Ok… tried to get back on using tabs instead of the global switch to reduce confusion. I combined all tabs from TabularInline into a single one, though, so it is not ugly anymore. Does it seems good enought to be pushed or should I still try something else?

  • TabularInline:
    2015-02-11-224110_1366x768_scrot
  • Settings:
    2015-02-11-223928_1366x768_scrot
@Lumpusz

This comment has been minimized.

Show comment
Hide comment
@Lumpusz

Lumpusz Feb 11, 2015

@Kniyl this is the best so far. Do you think that instead of tabs, using radios, only for inlines, would look more appealing? http://jqueryui.com/button/#radio

Lumpusz commented Feb 11, 2015

@Kniyl this is the best so far. Do you think that instead of tabs, using radios, only for inlines, would look more appealing? http://jqueryui.com/button/#radio

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 11, 2015

Collaborator

I rely on the activate (or select for jQuery-UI 1.8) event to trigger all hidden tabs at once. Unfortunately, buttons doesn't seems to provide such hooks.

Collaborator

Kniyl commented Feb 11, 2015

I rely on the activate (or select for jQuery-UI 1.8) event to trigger all hidden tabs at once. Unfortunately, buttons doesn't seems to provide such hooks.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Feb 11, 2015

Owner

I think the tabbed approach is still really poor from a user perspective.

Visually the tabs are just hanging there and don't seem to flow from the field itself in a natural way, but please don't give that too much thought - the real problem here is that it's most likely that none of us are particularly great UI designers, so we're really just going around in circles here with all these attempts, in futility to some extent.

More importantly, I think it's quite poor from a user interaction perspective. When I land on a form with 27 translatable fields, I now need to click 27 tabs just to be able to edit the content for one alternative language. If you think about how an actual translator will do this, they'll most likely be translating to a single language, one piece of content at a time - ideally there should only be one selection needed.

Both these issues are solved by the idea I proposed earlier, around leveraging the UI language selector. A translator would login to the system, and can actually use the language selector on the login screen to the same effect. Two issues were raised with this:

  1. USE_L10N may not be configured.

This is actually solved trivially. We pre-set a ton of different settings, based on the values of other settings, inside mezzanine.utils.conf.set_dynamic_settings. If USE_MODELTRANSLATION is set, we can set USE_L10N to True.

  1. List of UI languages mightn't match translation languages.

I think more often than not this won't be the case. A project might have two or three translation languages, and presumably there'd be no real reason to not have those available as UI languages as well. And if there was an edge-case project where more UI languages were required than what can be translated, the translatable language could always fall back to a default in the case of a UI language being selected that translations aren't required for. Not sure how well I've described that, it feels a bit confusing, so I can certainly clarify further there if needed.

As for the actual implementation - surely it's not too much of a stretch to hide the translation selection widgets (tabs, dropdowns, whatever they currently are), and trigger default selection based on grabbing the current UI language?

Owner

stephenmcd commented Feb 11, 2015

I think the tabbed approach is still really poor from a user perspective.

Visually the tabs are just hanging there and don't seem to flow from the field itself in a natural way, but please don't give that too much thought - the real problem here is that it's most likely that none of us are particularly great UI designers, so we're really just going around in circles here with all these attempts, in futility to some extent.

More importantly, I think it's quite poor from a user interaction perspective. When I land on a form with 27 translatable fields, I now need to click 27 tabs just to be able to edit the content for one alternative language. If you think about how an actual translator will do this, they'll most likely be translating to a single language, one piece of content at a time - ideally there should only be one selection needed.

Both these issues are solved by the idea I proposed earlier, around leveraging the UI language selector. A translator would login to the system, and can actually use the language selector on the login screen to the same effect. Two issues were raised with this:

  1. USE_L10N may not be configured.

This is actually solved trivially. We pre-set a ton of different settings, based on the values of other settings, inside mezzanine.utils.conf.set_dynamic_settings. If USE_MODELTRANSLATION is set, we can set USE_L10N to True.

  1. List of UI languages mightn't match translation languages.

I think more often than not this won't be the case. A project might have two or three translation languages, and presumably there'd be no real reason to not have those available as UI languages as well. And if there was an edge-case project where more UI languages were required than what can be translated, the translatable language could always fall back to a default in the case of a UI language being selected that translations aren't required for. Not sure how well I've described that, it feels a bit confusing, so I can certainly clarify further there if needed.

As for the actual implementation - surely it's not too much of a stretch to hide the translation selection widgets (tabs, dropdowns, whatever they currently are), and trigger default selection based on grabbing the current UI language?

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 11, 2015

Collaborator

I’ve been thinking about this lately and I pretty much reached the same conclusion: L10N is not an issue and, since the translation languages should be a subset of the site languages, the differences between the two should not be difficult to handle.

But, the main issue that this approach gives is that the UI language selector triggers a redirect and can potentially lose data that weren't saved beforehand, unlike the selector from modeltranslation which hide/reveals fields on the same page.

I’ll push this solution, though. So we can check how much of a big deal it is compared to its advantages.

Collaborator

Kniyl commented Feb 11, 2015

I’ve been thinking about this lately and I pretty much reached the same conclusion: L10N is not an issue and, since the translation languages should be a subset of the site languages, the differences between the two should not be difficult to handle.

But, the main issue that this approach gives is that the UI language selector triggers a redirect and can potentially lose data that weren't saved beforehand, unlike the selector from modeltranslation which hide/reveals fields on the same page.

I’ll push this solution, though. So we can check how much of a big deal it is compared to its advantages.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Feb 11, 2015

Owner

That data loss is a hugely important point - we could add some confirmation javascript to the language selector that warns the user and allows them to cancel changing the language, much like gmail does (or did) when you close the browser tab without sending an email that's been written.

Owner

stephenmcd commented Feb 11, 2015

That data loss is a hugely important point - we could add some confirmation javascript to the language selector that warns the user and allows them to cancel changing the language, much like gmail does (or did) when you close the browser tab without sending an email that's been written.

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Feb 11, 2015

Collaborator

Some alternatives off the top of my head, which would require creating a Mezzanine specific implementation of set_language view:

  • Trigger save() before doing the redirect to avoid data loss. This will hit the DB every time the user changes language and might be slow.
  • Persist the form data in the request (as POST data) or in the user's sessionStorage. Notice that set_language already uses POST data so we might not want to mess with it.

As a side note: @stephenmcd remember the global model translation switch is available in the current implementation, so a translator won't have to switch each field individually.

Collaborator

jerivas commented Feb 11, 2015

Some alternatives off the top of my head, which would require creating a Mezzanine specific implementation of set_language view:

  • Trigger save() before doing the redirect to avoid data loss. This will hit the DB every time the user changes language and might be slow.
  • Persist the form data in the request (as POST data) or in the user's sessionStorage. Notice that set_language already uses POST data so we might not want to mess with it.

As a side note: @stephenmcd remember the global model translation switch is available in the current implementation, so a translator won't have to switch each field individually.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 12, 2015

Collaborator

I pushed a version that uses the active language to determine which fields to show/hide so you guys can try it and see how it feels. Nothing is done yet to handle the possible data loss.

But (as always, there is a but) a new problem shown up in the name of required fields. Modeltranslation does apply required attributes on the default language for fields that are defined with blank=False. Which means, the default language should be populated before any other otherwise validation errors appears while attempting to save a page. I’ll check later in the documentation of modeltranslation if it is possible to change this behavior but it breaks the natural flow of create, edit, save for whatever language is suitable to the user. However, one may argue that translators shouldn't have to create a page and must only edit those which already are saved with the default language.

Anyway, it adds to the "save" issue and we should consider wether we fix it and keep the nice looking interface or we get back to the terrible looking tabs which work out of the box.
Since modeltranslation won't be a default feature, my personal taste would be to use the tabs (and I think I can manage to get all the tabs synchronize without the need of a global switch, if needed), open an issue saying that the admin looks bad with modeltranslation enabled and hopefully someone with greater design skill will provide a solution.

I think I will cool my brain on that for the next few days, maybe it will help to come up with something that works best.

Collaborator

Kniyl commented Feb 12, 2015

I pushed a version that uses the active language to determine which fields to show/hide so you guys can try it and see how it feels. Nothing is done yet to handle the possible data loss.

But (as always, there is a but) a new problem shown up in the name of required fields. Modeltranslation does apply required attributes on the default language for fields that are defined with blank=False. Which means, the default language should be populated before any other otherwise validation errors appears while attempting to save a page. I’ll check later in the documentation of modeltranslation if it is possible to change this behavior but it breaks the natural flow of create, edit, save for whatever language is suitable to the user. However, one may argue that translators shouldn't have to create a page and must only edit those which already are saved with the default language.

Anyway, it adds to the "save" issue and we should consider wether we fix it and keep the nice looking interface or we get back to the terrible looking tabs which work out of the box.
Since modeltranslation won't be a default feature, my personal taste would be to use the tabs (and I think I can manage to get all the tabs synchronize without the need of a global switch, if needed), open an issue saying that the admin looks bad with modeltranslation enabled and hopefully someone with greater design skill will provide a solution.

I think I will cool my brain on that for the next few days, maybe it will help to come up with something that works best.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Feb 12, 2015

Owner

Thanks for all the effort @Kniyl - really awesome.

Owner

stephenmcd commented Feb 12, 2015

Thanks for all the effort @Kniyl - really awesome.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Feb 15, 2015

Collaborator

The more I try to improve the "one language selector to rule them all" solution, the more I feel that tabs are the way to go. The data loss on language change and the required fields issue are too much of a hassle to handle properly. The approach provides a neat UI, though, and seems intuitive at first. I wish I had found a way to make it smooth for the user, because in its state it can be painful to use.

An other detail that tabs are improving is the ability to easily spot translatable fields. Having a bunch of language names on top of a field will make you know that you can provide translations for them even if you are not aware that the site is supposed to be multi-lingual.

As such, I pushed a version with a new design for tabs, hopping that they will feel better. This way, anyone can fork my repo and try to improve either the tabs version (last commit) or the language selector version (previous commit). This new version keeps in sync all tabs on a page so that when you change a language for a field, all other translatable fields are changed to the same language.

Example by image:

  • before click:
    2015-02-15-002958_1366x768_scrot
  • after click:
    2015-02-15-003002_1366x768_scrot
Collaborator

Kniyl commented Feb 15, 2015

The more I try to improve the "one language selector to rule them all" solution, the more I feel that tabs are the way to go. The data loss on language change and the required fields issue are too much of a hassle to handle properly. The approach provides a neat UI, though, and seems intuitive at first. I wish I had found a way to make it smooth for the user, because in its state it can be painful to use.

An other detail that tabs are improving is the ability to easily spot translatable fields. Having a bunch of language names on top of a field will make you know that you can provide translations for them even if you are not aware that the site is supposed to be multi-lingual.

As such, I pushed a version with a new design for tabs, hopping that they will feel better. This way, anyone can fork my repo and try to improve either the tabs version (last commit) or the language selector version (previous commit). This new version keeps in sync all tabs on a page so that when you change a language for a field, all other translatable fields are changed to the same language.

Example by image:

  • before click:
    2015-02-15-002958_1366x768_scrot
  • after click:
    2015-02-15-003002_1366x768_scrot
@Lumpusz

This comment has been minimized.

Show comment
Hide comment
@Lumpusz

Lumpusz Feb 15, 2015

I think this approach combines the bests of all alternatives, of all discussed solutions (switching language on a page with only one click, indicate translatable fields), and eliminates all the worst (confusion with multiple language selectors, as well as combining two similar, but not identical features under one selector.) It also looks better than the previos solution with tabs. Thanks @Kniyl!

Lumpusz commented Feb 15, 2015

I think this approach combines the bests of all alternatives, of all discussed solutions (switching language on a page with only one click, indicate translatable fields), and eliminates all the worst (confusion with multiple language selectors, as well as combining two similar, but not identical features under one selector.) It also looks better than the previos solution with tabs. Thanks @Kniyl!

@Lumpusz

This comment has been minimized.

Show comment
Hide comment
@Lumpusz

Lumpusz Feb 25, 2015

@stephenmcd @Kniyl What is still missing for modeltranslation integration to be merged into master?

Lumpusz commented Feb 25, 2015

@stephenmcd @Kniyl What is still missing for modeltranslation integration to be merged into master?

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Feb 25, 2015

Owner

Free time.

Owner

stephenmcd commented Feb 25, 2015

Free time.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Mar 4, 2015

Owner

Just reviewed this again and I think the UI works well.

One question - how does caching come into play? Currently with a production deploy, we enable caching of all pages using URLs as keys - it seems like the default setup with modeltranslation at the moment doesn't prefix URLs with language, eg foo.com/en/some-page, so would we end up with cached content for one language showing when another language has been selected?

We'll also need to squash the 30+ commits into one once this is ready, as each commit message gets put into the changelog.

Owner

stephenmcd commented Mar 4, 2015

Just reviewed this again and I think the UI works well.

One question - how does caching come into play? Currently with a production deploy, we enable caching of all pages using URLs as keys - it seems like the default setup with modeltranslation at the moment doesn't prefix URLs with language, eg foo.com/en/some-page, so would we end up with cached content for one language showing when another language has been selected?

We'll also need to squash the 30+ commits into one once this is ready, as each commit message gets put into the changelog.

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Mar 5, 2015

Collaborator

I'll set up a site based on this branch on my VPS to test the caching. In the meantime, how does caching work with logged in users in both the public site and the admin? Because the URLs are the same, but depending on your credentials you'll see different things.

Collaborator

jerivas commented Mar 5, 2015

I'll set up a site based on this branch on my VPS to test the caching. In the meantime, how does caching work with logged in users in both the public site and the admin? Because the URLs are the same, but depending on your credentials you'll see different things.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Mar 5, 2015

Owner

Cool Ed. Caching only comes into play for unauthenticated users, so no problem there.

Owner

stephenmcd commented Mar 5, 2015

Cool Ed. Caching only comes into play for unauthenticated users, so no problem there.

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Mar 5, 2015

Collaborator

Okay, it's up and running (http://dev.jerivas.com). As far as I can tell, pages are being correctly rendered for different languages. I've added this string to all templates, so you can see the last time Django parsed the templates: Generated at: {% now "jS F Y H:i:s" %}.

Collaborator

jerivas commented Mar 5, 2015

Okay, it's up and running (http://dev.jerivas.com). As far as I can tell, pages are being correctly rendered for different languages. I've added this string to all templates, so you can see the last time Django parsed the templates: Generated at: {% now "jS F Y H:i:s" %}.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Mar 5, 2015

Owner

Thanks Ed. I was surprised it worked, but looking at how the cache keys are generated, they actually take the current language into account, which totally explains it:

https://github.com/stephenmcd/mezzanine/blob/master/mezzanine/utils/cache.py#L91

Owner

stephenmcd commented Mar 5, 2015

Thanks Ed. I was surprised it worked, but looking at how the cache keys are generated, they actually take the current language into account, which totally explains it:

https://github.com/stephenmcd/mezzanine/blob/master/mezzanine/utils/cache.py#L91

@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Mar 5, 2015

Collaborator

Huh, that's nice!

Collaborator

jerivas commented Mar 5, 2015

Huh, that's nice!

@stephenmcd stephenmcd merged commit 71c7494 into stephenmcd:master Mar 5, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@jerivas

This comment has been minimized.

Show comment
Hide comment
@jerivas

jerivas Mar 5, 2015

Collaborator

So nice to see this becoming part of Mezzanine :)

While getting the live installation ready, I noticed createdb will fail if you have settings.USE_MODELTRANSLATION = True. The workaround is to activate Model Translation only after the initial database installation, which then requires running migrations for the translation fields. Of course, I'm no Model Translation expert, so I wanted to ask @Kniyl what is your recommended approach to:

  • Start a Mezzanine project with Model Translation serving several languages from the get-go
  • Add Model Translation to an existing Mezzanine project
Collaborator

jerivas commented Mar 5, 2015

So nice to see this becoming part of Mezzanine :)

While getting the live installation ready, I noticed createdb will fail if you have settings.USE_MODELTRANSLATION = True. The workaround is to activate Model Translation only after the initial database installation, which then requires running migrations for the translation fields. Of course, I'm no Model Translation expert, so I wanted to ask @Kniyl what is your recommended approach to:

  • Start a Mezzanine project with Model Translation serving several languages from the get-go
  • Add Model Translation to an existing Mezzanine project
@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Mar 5, 2015

Owner

Thanks everyone who contributed to this, it's merged now - I've announced it here:

https://groups.google.com/forum/#!topic/mezzanine-users/imE1GEwlvLI

Owner

stephenmcd commented Mar 5, 2015

Thanks everyone who contributed to this, it's merged now - I've announced it here:

https://groups.google.com/forum/#!topic/mezzanine-users/imE1GEwlvLI

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Mar 5, 2015

Collaborator

@jerivas To start a Mezzanine project with modeltranslation integration from the start, one has just to set settings.USE_MODELTRANSLATION = True and provide at least two languages in settings.LANGUAGES and it’s done. However, as you said you had an error trying that, I checked again. Everything went fine with Django 1.6, but there is an issue with Django 1.7. Something to do with the manage command update_translation_fields being run too late, I guess. I’ll fix that as soon as possible.

As for adding modeltranslation afterwhile, one has to manually run manage.py update_translation_fields in addition to the two prerequisites mentionned above. And that’s it.

@stephenmcd How do I provide only one big commit ? rebase ?

Collaborator

Kniyl commented Mar 5, 2015

@jerivas To start a Mezzanine project with modeltranslation integration from the start, one has just to set settings.USE_MODELTRANSLATION = True and provide at least two languages in settings.LANGUAGES and it’s done. However, as you said you had an error trying that, I checked again. Everything went fine with Django 1.6, but there is an issue with Django 1.7. Something to do with the manage command update_translation_fields being run too late, I guess. I’ll fix that as soon as possible.

As for adding modeltranslation afterwhile, one has to manually run manage.py update_translation_fields in addition to the two prerequisites mentionned above. And that’s it.

@stephenmcd How do I provide only one big commit ? rebase ?

@twil

This comment has been minimized.

Show comment
Hide comment
@twil

twil Mar 5, 2015

Hi Stephen,

Thanks for the merge! I'm playing with mezzanine and modeltranslation the whole week now and it plays well. The only thing I can add this far is documentation needs mentioning such a usefull setting as MODELTRANSLATION_FALLBACK_LANGUAGES. Without it you could see blank posts.

Initially I had set

LANGUAGES = (
    ('ru', _('Russian')),
    ('en', _('English')),
)

And the way modeltranslation works Russian became the default (fallback) language. Posts created in Russian were shown when viewing in both Russian and English but posts created in English shown blank lines when viewing site in Russian. After addition of

MODELTRANSLATION_FALLBACK_LANGUAGES = ('ru', 'en')

things settled and no blank lines are shown.

twil commented Mar 5, 2015

Hi Stephen,

Thanks for the merge! I'm playing with mezzanine and modeltranslation the whole week now and it plays well. The only thing I can add this far is documentation needs mentioning such a usefull setting as MODELTRANSLATION_FALLBACK_LANGUAGES. Without it you could see blank posts.

Initially I had set

LANGUAGES = (
    ('ru', _('Russian')),
    ('en', _('English')),
)

And the way modeltranslation works Russian became the default (fallback) language. Posts created in Russian were shown when viewing in both Russian and English but posts created in English shown blank lines when viewing site in Russian. After addition of

MODELTRANSLATION_FALLBACK_LANGUAGES = ('ru', 'en')

things settled and no blank lines are shown.

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Mar 5, 2015

Collaborator

@jerivas @stephenmcd The issue encountered when manage.py createdb is run under Django 1.7 has been fixed in Kniyl/mezzanine@6e03f17, should I make a new pull request for that ?

The issue was that, instead of creating tables from scratch, django 1.7 now creates tables out of migrations. Thus modeltranslation can't hook up and write its own column at creation time. manage.py sync_translation_fields has to be run before manage.py update_translation_fields and everything works fine.

Collaborator

Kniyl commented Mar 5, 2015

@jerivas @stephenmcd The issue encountered when manage.py createdb is run under Django 1.7 has been fixed in Kniyl/mezzanine@6e03f17, should I make a new pull request for that ?

The issue was that, instead of creating tables from scratch, django 1.7 now creates tables out of migrations. Thus modeltranslation can't hook up and write its own column at creation time. manage.py sync_translation_fields has to be run before manage.py update_translation_fields and everything works fine.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
@stephenmcd

stephenmcd Mar 5, 2015

Owner

Yes please do - thanks a lot!

Owner

stephenmcd commented Mar 5, 2015

Yes please do - thanks a lot!

@tabatabaei

This comment has been minimized.

Show comment
Hide comment
@tabatabaei

tabatabaei Mar 22, 2015

when relase mezzanine with multilingual support ???
they are want to write multilingual extention for mezzanine and make multilingual page content, if empty other language content translate from defult language content by google translator?

tabatabaei commented Mar 22, 2015

when relase mezzanine with multilingual support ???
they are want to write multilingual extention for mezzanine and make multilingual page content, if empty other language content translate from defult language content by google translator?

@Kniyl

This comment has been minimized.

Show comment
Hide comment
@Kniyl

Kniyl Mar 24, 2015

Collaborator

I can't answer the first question since it depends on @stephenmcd schedule.

For the second one… can you rephrase it? I don't quite understand.

Collaborator

Kniyl commented Mar 24, 2015

I can't answer the first question since it depends on @stephenmcd schedule.

For the second one… can you rephrase it? I don't quite understand.

@stephenmcd

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment