Integrates django-modeltranslation as an optional app à la south

[Kniyl]

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]

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]

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]

I've posted this thread to the mailing list:

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

[Kniyl]

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]

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]

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

[Romamo]

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]

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.

[stephenmcd]

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.

2. 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]

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]

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]

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]

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]

Thanks for all the effort @Kniyl - really awesome.

[Kniyl]

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:
  
• after click:
  

[Lumpusz]

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]

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

[stephenmcd]

Free time.

[stephenmcd]

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]

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]

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

[jerivas]

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]

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]

Huh, that's nice!

[jerivas]

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]

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]

@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]

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]

@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]

Yes please do - thanks a lot!

[tabatabaei]

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]

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]

Next release discussed here: https://groups.google.com/d/msg/mezzanine-users/_QWfFVB3RVc/AgcYAjZ7fdkJ