Merge pull request #221 from spectras/feature/form_refactor

Refactor TranslatableModelForm

Author: spectras

docs/internal/forms.rst

@@ -4,7 +4,6 @@
 
 .. module:: hvad.forms
 
-
 *******************************
 TranslatableModelFormMetaclass
 *******************************
@@ -14,48 +13,169 @@ TranslatableModelFormMetaclass
     Metaclass of :class:`TranslatableModelForm`.
 
     .. method:: __new__(cls, name, bases, attrs)
-    
-        The main thing happening in this metaclass is that the declared and base
-        fields on the form are built by calling
-        :func:`django.forms.models.fields_for_model` using the correct model
-        depending on whether the field is translated or not. This metaclass also
-        enforces the translations accessor and the master foreign key to be
-        excluded.
+
+        Uses Django's internal ``fields_for_model`` to get translated fields
+        for model and fields declarations, then lets Django handle the other
+        fields. Once it is done, it merges the translated fields, preserving order.
+
+        Special handling is done to:
+
+        * Prevent ``language_code`` from being used in any way by a field. This is
+          because the form uses the ``language_code`` key in the ``cleaned_data``
+          dictionary.
+        * Prevent ``master`` from being recognized as a translated field. It is
+          still a valid field name though.
+        * Prevent the translations accessor from being used as a field.
 
 
 **********************
 TranslatableModelForm
 **********************
 
-.. class:: TranslatableModelForm(ModelForm)
+.. class:: BaseTranslatableModelForm(BaseModelForm)
+
+        The actual class supporting the features and methods, but lacking metaclass
+        sugar. Inherited by :class:`~TranslatableModelForm` to attach the metaclass.
+        Details are documented on that class.
+
+.. class:: TranslatableModelForm(BaseTranslatableModelForm)
+
+        Main form for editing :class:`~hvad.models.TranslatableModel` instances. As with
+        regular django :class:`~django.forms.Form` classes, it can be used either
+        directly or by passing it to :func:`~translatable_modelform_factory`.
+
+        As an extension to regular forms, it handles translation and can be bound
+        to a language. Binding to a language is done by setting :attr:`language`
+        on the class (not the instance), either by inheriting it manually or
+        using the factory function. Once bound to a language, the form is in
+        **enforce** mode: all manipulations will be done using that language
+        exclusively.
 
     .. attribute:: __metaclass__
-    
+
         :class:`TranslatableModelFormMetaclass`
 
+    .. attribute:: language
+
+        The language the form is bound to. This is a class attribute. If present,
+        the form is in **enforce** mode and will only deal with the specified
+        language. See each method for the exact effects.
+
     .. method:: __init__(self, data=None, files=None, auto_id='id_%s', prefix=None, initial=None, error_class=ErrorList, label_suffix=':', empty_permitted=False, instance=None)
 
-        If this class is initialized with an instance, it updates ``initial`` to
-        also contain the data from the :term:`Translations Model` if it can be
-        found.
+        If this class is initialized with an instance, that has a translation
+        loaded, it updates ``initial`` to also contain the data from the
+        :term:`Translations Model`.
+
+        If the form is not bound to a language, it will use the data from the
+        instance. If the instance has no translation loaded, an attempt will be
+        made at loading the current language, and if that fails the fields will
+        be blank.
+
+        If the form is in **enforce** mode and the instance does not have the
+        correct translation loaded, then:
+
+        * it will attempt to load it from the database.
+        * if that fails, it will try to use the loaded translation on the instance.
+        * if that fails (instance is untranslated), it will use default values.
+
+        This process results in new translations being pre-populated with data
+        from another language. Simply pass an instance in that language, or an
+        untranslated instance if the behavior is not desired.
+
+    .. method:: clean(self)
+
+        If the form is in **enforce** mode, namely if it has a
+        ``language`` property, apply the it to ``cleaned_data``. As usual, the
+        special value ``None`` is replaced by current language.
+
+        If the form is not bound to a language, this method does nothing. It is
+        then possible to either use :meth:`save` in unbound mode or set the
+        language code manually in ``cleaned_data['language_code']``.
+
+        .. note:: A missing language is not the same as ``None``. While ``None``
+                  will be replaced by current language and applied to ``cleaned_data``,
+                  a missing language will not apply any language at all.
+
+    .. method:: _post_clean(self)
+
+        Loads a translation appropriate to the form mode. It is the very same that
+        will be loaded by :meth:`save`. Doing it twice is needed because:
+
+        * it must be done in ``_post_clean`` so that the correct translation is
+          available for modifications. For instance, if the view updates some
+          translated fields in between the call to ``is_valid()`` and ``save()``,
+          or if a form defines a custom ``save()``.
+        * it must also be done in ``save`` to ensure the language is correctly
+          enforced when in **enforce** mode.
+
+        This double check has no cost: unless the instance is changed by the view,
+        the ``save()`` check will see the translation is correct and do nothing.
 
     .. method:: save(self, commit=True)
-    
+
         Saves both the :term:`Shared Model` and :term:`Translations Model` and
-        returns a combined model. The :term:`Translations Model` is either
-        altered if it already exists on the :term:`Shared Model` for the current
-        language (which is fetched from the ``language_code`` field on the form
-        or the current active language) or newly created.
-        
-        .. note:: Other than in a normal :class:`django.forms.ModelForm`, this
-                  method creates two queries instead of one. 
+        returns a combined model.
 
-    .. method:: _post_clean(self)
+        The target language is determined as follows:
+
+        * If a language is defined in ``cleaned_data``, that language is used.
+        * Else, if the instance has a translation loaded, its language is used.
+        * Else, the current language is used.
+
+        Once the language is determined, the following happen:
+
+        * If the object does not exist, it is created.
+        * If the object exists but not in the target language, its shared fields
+          are updated and a new translation is created.
+        * If the object exists in the target language, it is updated.
+
+        .. note:: The **enforce** mode has no direct impact on this method. Rather,
+                  it affects the behavior of :meth:`clean`, which places relevant
+                  language (or lack thereof) in ``cleaned_data``.
+
+    .. method:: _get_translation(self, instance, language, enforce)
+
+        The internal method :meth:`_post_clean` and :meth:`save` delegate
+        translation checks to. Note that ``enforce`` here refers to the presence
+        of a ``language_code`` in ``cleaned_data`` and not whether the form is
+        in **enforce** mode or not.
+
+        This method must gurantee that calling it on its result is a no-op.
+        Namely, in this example the second call must do nothing and return
+        the translation as is::
+
+            translation = self._get_translation(instance, language, enforce)
+            instance = combine(translation, instance.__class__)
+            translation = self._get_translation(instance, language, enforce)
+
+
+.. function:: translatable_modelform_factory(language, model, form=TranslatableModelForm, **kwargs)
+
+    Attaches a language and a model class to the specified form and returns the
+    resulting class. Additional arguments are any arguments accepted by Django's
+    :func:`~django.forms.models.modelform_factory`, including ``fields`` and
+    ``exclude``.
+
+    Having a language attached, the returned form is in **enforce** mode.
+
+.. function:: translatable_modelformset_factory(language, model, form=TranslatableModelForm, **kwargs)
+
+    Creates a formset class, allowing edition a collection of instances of ``model``,
+    all of them in the specified ``language``. Additional arguments are any
+    argument accepted by Django's :func:`~django.forms.models.modelformset_factory`.
+
+    Having a language attached, the returned formset is in **enforce** mode.
+
+.. function:: translatable_inlineformset_factory(language, parent_model, model, form=TranslatableModelForm, **kwargs)
+
+    Creates an inline formset, allowing edition of a collection of instances of
+    ``model`` attached to an instance of ``parent_model``, all of those objects
+    being in the specified ``language``. Additional arguments are any argument
+    accepted by Django's :func:`~django.forms.models.inlineformset_factory`.
+
+    Having a language attached, the returned formset is in **enforce** mode.
 
-        Ensures the correct translation is loaded into **self.instance**.
-        It tries to load the language specified in the form's **language_code**
-        field from the database, and calls
-        :meth:`~hvad.models.TranslatableModel.translate` if it does not exist yet.
 
 **********************
 BaseTranslationFormSet

docs/public/forms.rst

@@ -31,12 +31,11 @@ TranslatableModelForm
 *********************
 
 TranslatableModelForms work like :class:`~django.forms.ModelForm`, but can
-display and edit translatable fields as well. There use is very similar,
+display and edit translatable fields as well. Their use is very similar,
 except the form must subclass :class:`~hvad.forms.TranslatableModelForm` instead of
 :class:`~django.forms.ModelForm`::
 
     class ArticleForm(TranslatableModelForm):
-        # language = 'en'       # See below
         class Meta:
             model = Article
             fields = ['pub_date', 'headline', 'content', 'reporter']
@@ -46,23 +45,26 @@ There is none but for the parent class. This ``ArticleForm`` will allow editing
 of one ``Article`` in one language, correctly introspecting the model to know
 which fields are translatable.
 
-The language the form uses is computed this way:
+The form can work in either normal mode, or **enforce** mode. This affects the
+way the form chooses a language for displaying and committing.
 
-- if the form is given a model instance, it will use the language that instance
-  was loaded with.
-- if this fails, it will look for a ``language`` attribute set on the form.
-- if this fails, it will use the current language, as returned by
-  :func:`~django.utils.translation.get_language`. If a request is being
-  processed, that will be the language of the request.
+* A form is in normal mode if it has no language set. This is the default. In
+  this mode, it will use the language of the ``instance`` it is given, defaulting
+  to current language if not ``instance`` is specified.
+* A form is in **enforce** mode if is has a language set. This is usually achieved
+  by calling :ref:`translatable_modelform_factory <translatablemodelformfactory>`.
+  When in **enforce** mode, the form will always use its language, disregarding
+  current language and reloading the ``instance`` it is given if it has another
+  language loaded.
+* The language can be overriden manually by providing a
+  :meth:`custom clean() method <django.forms.Form.clean>`.
 
-In all cases, any ``language_code`` field sent with form data will be ignored.
-It is the reponsibility of calling code to ensure the data matches the language
-of the form.
+In all cases, the language is not part of the form seen by the browser or sent
+in the POST request. If you need to change the language based on some user
+input, you must override the ``clean()`` method with your own logic, and set
+:attr:`~django.forms.Form.cleaned_data` ``['language_code']`` with it.
 
-All features of Django's form work as usual. Just be careful while overriding
-the :meth:`~hvad.forms.TranslatableModelForm.save` or
-:meth:`~hvad.forms.TranslatableModelForm._post_clean` methods, as they are
-crucial parts for the form to work.
+All features of Django forms work as usual.
 
 .. _translatablemodelformfactory:
 
@@ -78,6 +80,11 @@ eases the generation of uncustomized forms by providing a factory::
 The translation-aware version works exactly the same way as the original one,
 except it takes the language the form should use as an additional argument.
 
+The returned form class is in **enforce** mode.
+
+.. note:: If using the ``form=`` parameter, the given form class must inherit
+          :ref:`TranslatableModelForm <translatablemodelform>`.
+
 .. _translatablemodelformset:
 
 *************************
@@ -89,20 +96,28 @@ provides a factory to create formsets of translatable models::
 
     AuthorFormSet = translatable_modelformset_factory('en', Author)
 
-It is also possible to override the queryset, the same way you would do it for
-a regular formset. In fact, it is recommended, as the default will not prefetch
-translations::
+This formset allows edition a collection of ``Author`` instances, all of them
+being in English.
+
+All arguments supported by Django's :func:`~django.forms.models.modelformset_factory`
+can be used.
+
+For instance, it is possible to override the queryset, the same way it is done for
+a regular formset. In fact, it is recommended for performance, as the default
+queryset will not prefetch translations::
 
     BookForm = translatable_modelformset_factory(
         'en', Book, fields=('author', 'title'),
-        queryset=Book.objects.language().filter(name__startswith='O'),
+        queryset=Book.objects.language('en').all(),
     )
 
-Using :meth:`~hvad.manager.TranslationManager.language` ensures translations will
-be loaded at once, and allows filtering on translated fields.
+Here, using :meth:`~hvad.manager.TranslationManager.language` ensures translations
+will be loaded at once, and allows filtering on translated fields is needed.
+
+The returned formset class is in **enforce** mode.
 
 .. note:: To override the form by passing a ``form=`` argument to the factory,
-          the custom form must inherit :class:`~hvad.forms.TranslatableModelForm`.
+          the custom form must inherit :ref:`TranslatableModelForm <translatablemodelform>`.
 
 .. _translatableinlineformset:
 
@@ -115,8 +130,18 @@ provides a factory to create inline formsets of translatable models::
 
     BookFormSet = translatable_inlineformset_factory('en', Author, Book)
 
+This creates an inline formset, allowing edition of a collection of instances of
+``Book`` attached to a single instance of ``Author``, all of those objects
+being editted in English. It does not allow editting other languages; for this,
+please see :ref:`translationformset_factory <translationformset>`.
+
+Any argument accepted by Django's :func:`~django.forms.models.inlineformset_factory`
+can be used with ``translatable_inlineformset_factory`` as well.
+
+The returned formset class is in **enforce** mode.
+
 .. note:: To override the form by passing a ``form=`` argument to the factory,
-          the custom form must inherit :class:`~hvad.forms.TranslatableModelForm`.
+          the custom form must inherit :ref:`TranslatableModelForm <translatablemodelform>`.
 
 .. _translationformset:
 

docs/public/release_notes.rst

@@ -20,6 +20,14 @@ Python and Django versions supported:
 
 New features:
 
+- :ref:`TranslatableModelForm <translatablemodelform>` has been refactored to make
+  its behavior more consistent. As a result, it exposes two distinct language
+  selection modes, *normal* and *enforce*, and has a clear API for manually
+  overriding the language — :issue:`221`.
+- The new features of :func:`~django.forms.models.modelform_factory` introduced by
+  Django 1.6 and 1.7 are now available on
+  :ref:`translatable_modelform_factory <translatablemodelformfactory>` as
+  well — :issue:`221`.
 - :ref:`TranslationQueryset <TranslationQueryset-public>` now has a
   :ref:`fallbacks() <fallbacks-public>` method when running on
   Django 1.6 or newer, allowing the queryset to use fallback languages while
@@ -29,6 +37,18 @@ New features:
 - It is now possible to use :ref:`TranslationQueryset <TranslationQueryset-public>`
   as default queryset for translatable models. — :issue:`207`.
 
+Compatibility warnings:
+
+- :ref:`TranslatableModelForm <translatablemodelform>` has been refactored to make
+  its behavior more consistent. The core API has not changed, but edge cases are
+  now clearly specified and some inconsistencies have disappeared, which could
+  create issues, especially:
+
+  - Direct use of the form class, without passing through the
+    :ref:`factory method <translatablemodelformfactory>`. This used to have an
+    unspecified behavior regarding language selection. Behavior is now
+    well-defined. Please ensure it works the way you expect it to.
+
 Fixes:
 
 - :ref:`TranslatableModelForm <translatablemodelform>`'s