From 947ba773c747a36f46fa18389304454de21ea2be Mon Sep 17 00:00:00 2001 From: Guillaume Englert Date: Wed, 28 Feb 2024 10:32:02 +0100 Subject: [PATCH] Fix some issues in quick form when a field has been configured (FieldsConfig) 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 | 11 +++ creme/creme_core/apps.py | 5 +- creme/creme_core/forms/base.py | 5 +- creme/creme_core/models/base.py | 2 +- creme/creme_core/models/fields_config.py | 46 ++++++++++-- .../tests/models/test_fields_config.py | 66 ++++++++++++++++- .../tests/views/test_quick_forms.py | 71 ++++++++++++++++--- 7 files changed, 186 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 803c90c62c..fe6ac4d909 100644 --- a/CHANGELOG.txt +++ b/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 == diff --git a/creme/creme_core/apps.py b/creme/creme_core/apps.py index e5aafdec91..12283ba43f 100644 --- a/creme/creme_core/apps.py +++ b/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 diff --git a/creme/creme_core/forms/base.py b/creme/creme_core/forms/base.py index 3945d6030a..b66cf7cd95 100644 --- a/creme/creme_core/forms/base.py +++ b/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 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() diff --git a/creme/creme_core/models/base.py b/creme/creme_core/models/base.py index 5a285cfc98..5e537fb3a6 100644 --- a/creme/creme_core/models/base.py +++ b/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 diff --git a/creme/creme_core/models/fields_config.py b/creme/creme_core/models/fields_config.py index 3b592b0460..662c368496 100644 --- a/creme/creme_core/models/fields_config.py +++ b/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() diff --git a/creme/creme_core/tests/models/test_fields_config.py b/creme/creme_core/tests/models/test_fields_config.py index f943f7eb34..943799550f 100644 --- a/creme/creme_core/tests/models/test_fields_config.py +++ b/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' diff --git a/creme/creme_core/tests/views/test_quick_forms.py b/creme/creme_core/tests/views/test_quick_forms.py index de8ec941d5..2f2e6e9190 100644 --- a/creme/creme_core/tests/views/test_quick_forms.py +++ b/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,18 +51,18 @@ 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 = 'admin@hello.com' - response = self.assertPOST200( + response2 = self.assertPOST200( url, data={ 'last_name': last_name, @@ -69,7 +70,7 @@ def test_create_contact(self): '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