Fix some issues in quick form when a field has been configured (Field…

…sConfig) as 'required'.

The form raised an error at validation event if the field was filled
A crash happened when the field an enumerable FK.

CHANGELOG.txt

@@ -4,6 +4,10 @@
 
   Users side :
   ------------
+    # There were several issues with fields configured as required.
+      It could notably appeared in quick forms:
+        - Fields which were added (because they are not in the default forms) cause a validation error.
+        - Fields corresponding to a ForeignKey caused an error 500.
     # The block which displays all relationships, & the configurable blocks related to
       one type of relationship, have been fixed to order more correctly entities.
       It fixes some pagination issues with PostgreSQL.
@@ -32,6 +36,13 @@
                 - The view 'views.strategy.BaseEvaluatedOrganisationView' has been improved to be easier to modify.
                 - The class-view 'views.strategy.MatrixBricksReloading' now checks permissions.
 
+    Internal breaking changes :
+    ---------------------------
+    (they should not cause problem if you have not deeply modified the source code of Creme)
+
+        # The method 'creme_core.models.fields_config.FieldsConfig.update_form_fields()',
+          now takes a form instance, and can set the '_meta' attribute.
+
 
 == Version 2.5.5 ==
 

creme/creme_core/apps.py

@@ -684,10 +684,13 @@ def new_fk_formfield(self, **kwargs):
                     help_text=self.help_text,
                 )
             elif self.get_tag(FieldTag.ENUMERABLE):
+                required = kwargs.pop('required', False)
+
                 return config_fields.CreatorEnumerableModelChoiceField(
                     model=self.model,
                     field_name=self.name,
-                    required=not self.blank,
+                    # required=not self.blank,
+                    required=not self.blank or required,
                     label=self.verbose_name,
                     help_text=self.help_text,
                     **kwargs

creme/creme_core/forms/base.py

@@ -1,6 +1,6 @@
 ################################################################################
 #    Creme is a free/open-source Customer Relationship Management software
-#    Copyright (C) 2009-2023  Hybird
+#    Copyright (C) 2009-2024  Hybird
 #
 #    This program is free software: you can redistribute it and/or modify
 #    it under the terms of the GNU Affero General Public License as published by
@@ -525,7 +525,8 @@ def __init__(self, user, *args, **kwargs):
     def _build_required_fields(self):
         # NB: not <type(self.instance)> because it returns an instance of
         #     SimpleLazyObject for User, which causes an error.
-        self.fields_configs.get_for_model(self.instance.__class__).update_form_fields(self.fields)
+        # self.fields_configs.get_for_model(self.instance.__class__).update_form_fields(self.fields)
+        self.fields_configs.get_for_model(self.instance.__class__).update_form_fields(self)
 
     def clean(self):
         res = super().clean()

creme/creme_core/models/base.py

@@ -1,6 +1,6 @@
 ################################################################################
 #    Creme is a free/open-source Customer Relationship Management software
-#    Copyright (C) 2009-2022  Hybird
+#    Copyright (C) 2009-2024  Hybird
 #
 #    This program is free software: you can redistribute it and/or modify
 #    it under the terms of the GNU Affero General Public License as published by

creme/creme_core/models/fields_config.py

@@ -1,6 +1,6 @@
 ################################################################################
 #    Creme is a free/open-source Customer Relationship Management software
-#    Copyright (C) 2015-2022  Hybird
+#    Copyright (C) 2015-2024  Hybird
 #
 #    This program is free software: you can redistribute it and/or modify
 #    it under the terms of the GNU Affero General Public License as published by
@@ -19,6 +19,7 @@
 from __future__ import annotations
 
 import logging
+from copy import deepcopy
 from functools import partial
 from itertools import chain
 from typing import TYPE_CHECKING
@@ -427,19 +428,45 @@ def is_fieldname_required(self, field_name: str) -> bool:
 
         return self.is_field_required(field)
 
-    def update_form_fields(self, form_fields) -> None:
+    # def update_form_fields(self, form_fields) -> None:
+    #     for field_name in self._get_hidden_field_names():
+    #         form_fields.pop(field_name, None)
+    #
+    #     for field_name in self.required_field_names:
+    #         try:
+    #             form_fields[field_name].required = True
+    #         except KeyError:
+    #             logger.critical(
+    #                 'The field "%s" has been configured to be required '
+    #                 'but the current form does not use this field ; '
+    #                 'please contact your administrator.',
+    #                 field_name,
+    #             )
+    #             form_fields[field_name] = (
+    #                 self.content_type
+    #                     .model_class()
+    #                     ._meta
+    #                     .get_field(field_name)
+    #                     .formfield(required=True)
+    #             )
+    def update_form_fields(self, form) -> None:
+        form_fields = form.fields
+
         for field_name in self._get_hidden_field_names():
+            # TODO: remove from meta too?
             form_fields.pop(field_name, None)
 
+        missing_field_names = []
+
         for field_name in self.required_field_names:
             try:
                 form_fields[field_name].required = True
             except KeyError:
                 # TODO: we need the form class for a better message
-                logger.critical(
+                logger.info(
                     'The field "%s" has been configured to be required '
                     'but the current form does not use this field ; '
-                    'please contact your administrator.',
+                    'so we add it.',
                     field_name,
                 )
                 # TODO: is it possible that field does not exist any more ?
@@ -450,6 +477,17 @@ def update_form_fields(self, form_fields) -> None:
                         .get_field(field_name)
                         .formfield(required=True)
                 )
+                missing_field_names.append(field_name)
+
+        if missing_field_names:
+            # NB: we add the missing fields in the options/meta, because the
+            #     model forms only fill the instance fields (from cleaned data)
+            #     corresponding to meta.fields (so if we do not complete the
+            #     field list the model validation will fail with an error like
+            #     "The field XXX is configured as required")
+            new_meta = deepcopy(form._meta)
+            new_meta.fields = (*new_meta.fields, *missing_field_names)
+            form._meta = new_meta
 
     def natural_key(self):
         return self.content_type.natural_key()

creme/creme_core/tests/models/test_fields_config.py

@@ -478,10 +478,41 @@ def test_form_update_required01(self):
             ],
         )
 
-        fields = FakeContactForm(user=user).fields
+        # fields = FakeContactForm(user=user).fields
+        form1 = FakeContactForm(user=user)
+        fields = form1.fields
         self.assertFalse(fields['mobile'].required)
         self.assertTrue(fields['phone'].required)
 
+        # ---
+        last_name = 'Senjougahara'
+        data = {'user': user.id, 'last_name': last_name}
+
+        form2 = FakeContactForm(user=user, data=data)
+        self.assertDictEqual(
+            {
+                'phone': [
+                    _('This field is required.'),
+                    _('The field «{}» has been configured as required.').format(_('Phone')),
+                ],
+            },
+            form2.errors,
+        )
+
+        # ---
+        phone = '123965'
+        form3 = FakeContactForm(
+            user=user, data={**data, 'phone': phone},
+        )
+        self.assertTrue(form3.is_valid())
+
+        contact = form3.save()
+        self.assertIsInstance(contact, FakeContact)
+        self.assertIsNotNone(contact.pk)
+        self.assertEqual(user,      contact.user)
+        self.assertEqual(last_name, contact.last_name)
+        self.assertEqual(phone,     contact.phone)
+
     def test_form_update_required02(self):
         "Field not present => added."
         user = self.get_root_user()
@@ -498,13 +529,42 @@ class Meta(FakeContactForm.Meta):
             ],
         )
 
-        form = LightFakeContactForm(user=user)
-        first_name_f = form.fields.get(required)
+        form1 = LightFakeContactForm(user=user)
+        first_name_f = form1.fields.get(required)
         self.assertIsInstance(first_name_f, CharField)
         self.assertEqual(_('First name'), first_name_f.label)
         self.assertEqual(100,             first_name_f.max_length)
         self.assertTrue(first_name_f.required)
 
+        # ---
+        last_name = 'Senjougahara'
+        data = {'user': user.id, 'last_name': last_name}
+
+        form2 = LightFakeContactForm(user=user, data=data)
+        self.assertDictEqual(
+            {
+                required: [
+                    _('This field is required.'),
+                    _('The field «{}» has been configured as required.').format(_('First name')),
+                ],
+            },
+            form2.errors,
+        )
+
+        # ---
+        required_value = 'Hitagi'
+        form3 = LightFakeContactForm(
+            user=user, data={**data, required: required_value},
+        )
+        self.assertFalse(form3.errors)
+
+        contact = form3.save()
+        self.assertIsInstance(contact, FakeContact)
+        self.assertIsNotNone(contact.pk)
+        self.assertEqual(user,           contact.user)
+        self.assertEqual(last_name,      contact.last_name)
+        self.assertEqual(required_value, getattr(contact, required))
+
     def test_descriptions_setter01(self):
         "Auto-repair invalid fields."
         h_field = 'phone'

creme/creme_core/tests/views/test_quick_forms.py

@@ -6,6 +6,7 @@
 
 from creme.creme_core.models import (
     CustomField,
+    FakeCivility,
     FakeContact,
     FakeInvoice,
     FakeOrganisation,
@@ -50,26 +51,26 @@ def test_create_contact(self):
         count = FakeContact.objects.count()
 
         url = self._build_quickform_url(FakeContact)
-        response = self.assertGET200(url)
-        self.assertTemplateUsed(response, 'creme_core/generics/form/add-popup.html')
-        self.assertEqual('SAMEORIGIN', response.get('X-Frame-Options'))  # allows iframe
+        response1 = self.assertGET200(url)
+        self.assertTemplateUsed(response1, 'creme_core/generics/form/add-popup.html')
+        self.assertEqual('SAMEORIGIN', response1.get('X-Frame-Options'))  # allows iframe
 
-        context = response.context
+        context = response1.context
         self.assertEqual(FakeContact.creation_label, context.get('title'))
         self.assertEqual(FakeContact.save_label,     context.get('submit_label'))
 
         # ---
         last_name = 'Kirika'
         email = '[email protected]'
-        response = self.assertPOST200(
+        response2 = self.assertPOST200(
             url,
             data={
                 'last_name': last_name,
                 'email':     email,
                 'user':      user.id,
             },
         )
-        self.assertEqual('SAMEORIGIN', response.get('X-Frame-Options'))  # allows iframe
+        self.assertEqual('SAMEORIGIN', response2.get('X-Frame-Options'))  # allows iframe
         self.assertEqual(count + 1, FakeContact.objects.count())
 
         contact = self.get_object_or_fail(FakeContact, last_name=last_name, email=email)
@@ -78,7 +79,7 @@ def test_create_contact(self):
                 'added': [[contact.id, str(contact)]],
                 'value': contact.id,
             },
-            response.json(),
+            response2.json(),
         )
 
     def test_get_not_superuser(self):
@@ -164,12 +165,64 @@ def test_fields_config_required(self):
         )
 
         url = self._build_quickform_url(FakeContact)
-        response = self.assertGET200(url)
+        response1 = self.assertGET200(url)
 
         with self.assertNoException():
-            fields = response.context['form'].fields
+            fields = response1.context['form'].fields
 
         self.assertNotIn(not_required, fields)
         self.assertIn(required, fields)
 
+        # ---
+        last_name = 'Kirika'
+        required_value = '1594862'
+        self.assertNoFormError(self.client.post(
+            url,
+            data={
+                'last_name': last_name,
+                'user': user.id,
+                required: required_value,
+                not_required: 'whatever',
+            },
+        ))
+
+        contact = self.get_object_or_fail(FakeContact, last_name=last_name)
+        self.assertEqual(required_value, getattr(contact, required))
+        self.assertFalse(getattr(contact, not_required))
+
+    def test_fields_config_required__enumerable_fk(self):
+        user = self.login_as_root_and_get()
+        required = 'civility'
+
+        vanilla_fields = FakeContactQuickForm(user=user).fields
+        self.assertNotIn(required, vanilla_fields)
+
+        FieldsConfig.objects.create(
+            content_type=FakeContact,
+            descriptions=[(required, {FieldsConfig.REQUIRED: True})],
+        )
+
+        url = self._build_quickform_url(FakeContact)
+        response1 = self.assertGET200(url)
+
+        with self.assertNoException():
+            fields = response1.context['form'].fields
+
+        self.assertIn(required, fields)
+
+        # ---
+        last_name = 'Kirika'
+        required_value = FakeCivility.objects.first()
+        self.assertNoFormError(self.client.post(
+            url,
+            data={
+                'last_name': last_name,
+                'user': user.id,
+                required: required_value.id,
+            },
+        ))
+
+        contact = self.get_object_or_fail(FakeContact, last_name=last_name)
+        self.assertEqual(required_value, getattr(contact, required))
+
     # TODO: test_quickform_with_custom_sync_data