[FIX] website: prevent validation of conditionally hidden fields

Steps to reproduce the issue (see opw):
- Add a form to a page
- Add a checkbox
- Add a second checkbox that is required and visible only if the first
  checkbox is not checked
- Save
- Do not check anything and try to send => Good, you can't, the required
  checkbox is not set
- Check the second checkbox and try to send => Good, you can, the
  required checkbox is set
- Go back to the empty form, check the first checkbox and try to send
=> Bad, you can't but you should: the required checkbox is hidden, it
   is not supposed to be required anymore.

(Not that arguably, that two checkboxes setup should be replaced by a
required selection field but the issue of the OPW would remain as there
is another required conditionally-hidden field (a file upload) when the
second checkbox is checked.)

Conditionally-hidden fields should not require validation while they are
hidden. Indeed, their purpose is to be able to enter additional data
when some condition is fulfilled. If such a field is required, it is
only required when visible. The problem only occurred with checkboxes,
the other fields were already working in that case. Although there could
be issues with dates too, this commit added a series of FIXME and TODO
comments as well as some things need to be investigated.

opw-3003952

closes odoo/odoo#105870

Signed-off-by: Romain Derie (rde) <rde@odoo.com>

addons/website/static/src/snippets/s_website_form/000.js

@@ -385,6 +385,9 @@ odoo.define('website.s_website_form', function (require) {
             // Loop on all fields
             this.$target.find('.form-field, .s_website_form_field').each(function (k, field) { // !compatibility
                 var $field = $(field);
+                // FIXME that seems broken, "for" does not contain the field
+                // but this is used to retrieve errors sent from the server...
+                // need more investigation.
                 var field_name = $field.find('.col-form-label').attr('for');
 
                 // Validate inputs for this field
@@ -394,17 +397,27 @@ odoo.define('website.s_website_form', function (require) {
                     // field as it seems checkValidity forces every required
                     // checkbox to be checked, instead of looking at other
                     // checkboxes with the same name and only requiring one
-                    // of them to be checked.
+                    // of them to be valid.
                     if (input.required && input.type === 'checkbox') {
                         // Considering we are currently processing a single
                         // field, we can assume that all checkboxes in the
                         // inputs variable have the same name
+                        // TODO should be improved: probably do not need to
+                        // filter neither on required, nor on checkbox and
+                        // checking the validity of the group of checkbox is
+                        // currently done for each checkbox of that group...
                         var checkboxes = _.filter(inputs, function (input) {
                             return input.required && input.type === 'checkbox';
                         });
-                        return !_.any(checkboxes, checkbox => checkbox.checked);
+                        return !_.any(checkboxes, checkbox => checkbox.checkValidity());
 
                     // Special cases for dates and datetimes
+                    // FIXME this seems like dead code, the inputs do not use
+                    // those classes, their parent does (but it seemed to work
+                    // at some point given that https://github.com/odoo/odoo/commit/75e03c0f7692a112e1b0fa33267f4939363f3871
+                    // was made)... need more investigation (if restored,
+                    // consider checking the date inputs are not disabled before
+                    // saying they are invalid (see checkValidity used here))
                     } else if ($(input).hasClass('s_website_form_date') || $(input).hasClass('o_website_form_date')) { // !compatibility
                         if (!self.is_datetime_valid(input.value, 'date')) {
                             return true;
@@ -414,6 +427,16 @@ odoo.define('website.s_website_form', function (require) {
                             return true;
                         }
                     }
+
+                    // Note that checkValidity also takes care of the case where
+                    // the input is disabled, in which case, it is considered
+                    // valid (as the data will not be sent anyway).
+                    // This takes care of conditionally-hidden fields (whose
+                    // inputs are disabled while they are hidden) which should
+                    // not require validation while they are hidden. Indeed,
+                    // their purpose is to be able to enter additional data when
+                    // some condition is fulfilled. If such a field is required,
+                    // it is only required when visible for example.
                     return !input.checkValidity();
                 });
 

addons/website/static/tests/tours/website_form_editor.js

@@ -9,6 +9,26 @@ odoo.define('website.tour.form_editor', function (require) {
     const HIDDEN = 'Hidden';
     const CONDITIONALVISIBILITY = 'Visible only if';
 
+    const NB_NON_ESSENTIAL_REQUIRED_FIELDS_IN_DEFAULT_FORM = 2;
+    const ESSENTIAL_FIELDS_VALID_DATA_FOR_DEFAULT_FORM = [
+        {
+            name: 'email_from',
+            value: 'admin@odoo.com',
+        },
+        {
+            name: 'subject',
+            value: 'Hello, world!',
+        }
+    ];
+    const essentialFieldsForDefaultFormFillInSteps = [];
+    for (const data of ESSENTIAL_FIELDS_VALID_DATA_FOR_DEFAULT_FORM) {
+        essentialFieldsForDefaultFormFillInSteps.push({
+            content: "Enter data in model-required field",
+            trigger: `.s_website_form_model_required .s_website_form_input[name="${data.name}"]`,
+            run: `text ${data.value}`,
+        });
+    }
+
     const selectButtonByText = function (text) {
         return [{
             content: "Open the select",
@@ -618,5 +638,123 @@ odoo.define('website.tour.form_editor', function (require) {
         },
     ]);
 
+    tour.register('website_form_conditional_required_checkboxes', {
+        test: true,
+        url: '/',
+    }, [
+        // Create a form with two checkboxes: the second one required but
+        // invisible when the first one is checked. Basically this should allow
+        // to have: both checkboxes are visible by default but the form can
+        // only be sent if one of the checkbox is checked.
+        {
+            content: "Enter edit mode",
+            trigger: 'a[data-action=edit]',
+        }, {
+            content: "Add the form snippet",
+            trigger: '#oe_snippets .oe_snippet:has(.s_website_form) .oe_snippet_thumbnail',
+            run: 'drag_and_drop #wrap',
+        }, {
+            content: "Select the form by clicking on an input field",
+            extra_trigger: '.s_website_form_field',
+            trigger: 'section.s_website_form input',
+            run: function (actions) {
+                actions.auto();
+
+                // The next steps will be about removing non essential required
+                // fields. For the robustness of the test, check that amount
+                // of field stays the same.
+                const requiredFields = this.$anchor.closest('[data-snippet]').find('.s_website_form_required');
+                if (requiredFields.length !== NB_NON_ESSENTIAL_REQUIRED_FIELDS_IN_DEFAULT_FORM) {
+                    console.error('The amount of required fields seems to have changed');
+                }
+            },
+        },
+        ...((function () {
+            const steps = [];
+            for (let i = 0; i < NB_NON_ESSENTIAL_REQUIRED_FIELDS_IN_DEFAULT_FORM; i++) {
+                steps.push({
+                    content: "Select required field to remove",
+                    trigger: '.s_website_form_required .s_website_form_input',
+                });
+                steps.push({
+                    content: "Remove required field",
+                    trigger: '.oe_overlay .oe_snippet_remove',
+                });
+            }
+            return steps;
+        })()),
+        ...addCustomField('boolean', 'checkbox', 'Checkbox 1', false),
+        ...addCustomField('boolean', 'checkbox', 'Checkbox 2', true, {visibility: CONDITIONALVISIBILITY}),
+        {
+            content: "Open condition item select",
+            trigger: 'we-select[data-name="hidden_condition_opt"] we-toggler',
+        }, {
+            content: "Choose first checkbox as condition item",
+            trigger: 'we-button[data-set-visibility-dependency="Checkbox 1"]',
+        }, {
+            content: "Open condition comparator select",
+            trigger: 'we-select[data-attribute-name="visibilityComparator"] we-toggler',
+        }, {
+            content: "Choose 'not equal to' comparator",
+            trigger: 'we-button[data-select-data-attribute="!selected"]',
+        }, {
+            content: 'Save the page',
+            trigger: 'button[data-action=save]',
+        },
+
+        // Check that the resulting form behavior is correct
+        {
+            content: "Wait for page reload",
+            trigger: 'body:not(.editor_enable) [data-snippet="s_website_form"]',
+            run: function (actions) {
+                // The next steps will be about removing non essential required
+                // fields. For the robustness of the test, check that amount
+                // of field stays the same.
+                const essentialFields = this.$anchor.find('.s_website_form_model_required');
+                if (essentialFields.length !== ESSENTIAL_FIELDS_VALID_DATA_FOR_DEFAULT_FORM.length) {
+                    console.error('The amount of model-required fields seems to have changed');
+                }
+            },
+        },
+        ...essentialFieldsForDefaultFormFillInSteps,
+        {
+            content: 'Try sending empty form',
+            trigger: '.s_website_form_send',
+        }, {
+            content: 'Check the form could not be sent',
+            trigger: '#s_website_form_result.text-danger',
+            run: () => null,
+        }, {
+            content: 'Check the first checkbox',
+            trigger: 'input[type="checkbox"][name="Checkbox 1"]',
+        }, {
+            content: 'Check the second checkbox is now hidden',
+            trigger: '.s_website_form:has(input[type="checkbox"][name="Checkbox 2"]:not(:visible))',
+            run: () => null,
+        }, {
+            content: 'Try sending the form',
+            trigger: '.s_website_form_send',
+        }, {
+            content: "Check the form was sent (success page without form)",
+            trigger: 'body:not(:has([data-snippet="s_website_form"])) .fa-check-circle',
+            run: () => null,
+        }, {
+            content: "Go back to the form",
+            trigger: 'a.navbar-brand.logo',
+        },
+        ...essentialFieldsForDefaultFormFillInSteps,
+        {
+            content: 'Check the second checkbox',
+            trigger: 'input[type="checkbox"][name="Checkbox 2"]',
+        }, {
+            content: 'Try sending the form again',
+            trigger: '.s_website_form_send',
+        }, {
+            content: "Check the form was again sent (success page without form)",
+            trigger: 'body:not(:has([data-snippet="s_website_form"])) .fa-check-circle',
+            run: () => null,
+        }
+    ]);
+
     return {};
 });

addons/website/tests/test_website_form_editor.py

@@ -28,3 +28,6 @@ class TestWebsiteFormEditor(odoo.tests.HttpCase):
             mail.email_to,
             self.env.company.email,
             'The email was not edited, the form should still have been sent to the company email')
+
+    def test_website_form_conditional_required_checkboxes(self):
+        self.start_tour('/', 'website_form_conditional_required_checkboxes', login="admin")