JS validation: dynamically added form inputs are now validated #260

boris-brtan · 2018-06-18T10:21:48Z

Q	A
Description, reason for the PR	problem with saving forms with dynamically added parameters
New feature	No
BC breaks	No
Fixes issues	#190
Standards and tests pass	Yes
Have you read and signed our License Agreement for contributions?	Yes

vitek-rostislav

Hi @boris-brtan, the review is done now, I have a few notes so please check them out. I really do not know what to do with the last commit so I asked @PetrHeinz for help.

vitek-rostislav · 2018-06-21T14:16:56Z

CHANGELOG.md

@@ -60,6 +60,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
    - changed domain icon in e-shop domain administration can be saved 
 - [#260 - Admin: Js validation: dynamically added parameters are now validated](https://github.com/shopsys/shopsys/pull/260)
    - validation is evaluated after dynamic parameters are added
+    - jQuery migration console error logging is no more


I do not understand this commit at all, what was the problem?

vitek-rostislav · 2018-06-21T14:27:46Z

packages/framework/src/Resources/scripts/common/validation/customizeBundle.js

@@ -4,7 +4,10 @@
    Shopsys.validation = Shopsys.validation || {};

    Shopsys.validation.addNewItemToCollection = function (collectionSelector, itemIndex) {
-        $($(collectionSelector)).jsFormValidator('addPrototype', itemIndex);
+        $(collectionSelector).jsFormValidator('addPrototype', itemIndex);


why you changed the double cast?

vitek-rostislav · 2018-06-21T14:30:20Z

packages/framework/src/Resources/scripts/common/validation/validation.js

    });

+    function invalidateForm () {


this should rather be named to something like "disableJsFormValidation" as it disables JS validation on a form.

vitek-rostislav · 2018-06-21T14:33:59Z

packages/framework/src/Resources/scripts/common/validation/validation.js

+            .toggle();
+    }
+
+    Shopsys.validation.refreshBindings = function () {


We have Shopsys.register component for that purpose - you can register a callback and that register newly added content on which the callback will be executed, see https://github.com/shopsys/shopsys/blob/master/packages/framework/src/Resources/scripts/common/register.js. I think this could serve for the purpose perfectly.

vitek-rostislav · 2018-06-21T14:36:25Z

packages/framework/src/Resources/scripts/common/validation/customizeBundle.js

@@ -4,7 +4,10 @@
    Shopsys.validation = Shopsys.validation || {};

    Shopsys.validation.addNewItemToCollection = function (collectionSelector, itemIndex) {
-        $($(collectionSelector)).jsFormValidator('addPrototype', itemIndex);
+        $(collectionSelector).jsFormValidator('addPrototype', itemIndex);
+        if (Shopsys.validation.refreshBindings instanceof Function) {


is this necessary?

vitek-rostislav · 2018-06-21T16:41:44Z

project-base/tests/ShopBundle/Acceptance/acceptance/PageObject/Admin/ProductEditPage.php

+    /**
+     * @param int $productId
+     */
+    public function saveInvalidForm($productId)


I would name this method more specifically - it saves form with invalid parameter...

vitek-rostislav · 2018-06-21T16:43:10Z

...-base/tests/ShopBundle/Acceptance/acceptance/AdminProductSaveFormWithEmptyParametersCest.php

+        $productEditPage->saveInvalidForm(5);
+        $productEditPage->assertSaveFormErrorBoxVisible();
+
+        $productEditPage->cleanUpWorkspaceAfterChanges();


why? could not you just call $this->tester->acceptPopup()?

In this test the form is changed and browser will stop execution of next tests due to modal box that wants approval. Seems that opposite order of commands doesn't work if the test is ended and next test requests for different page modal box is not canceled.

vitek-rostislav · 2018-06-21T16:45:55Z

...-base/tests/ShopBundle/Acceptance/acceptance/AdminProductSaveFormWithEmptyParametersCest.php

+        LoginPage $loginPage,
+        ProductEditPage $productEditPage
+    ) {
+        $me->wantTo('not able to save invalid form');


$me->wantTo('see that I am not able to save an invalid form');?

vitek-rostislav · 2018-06-21T16:51:16Z

project-base/tests/ShopBundle/Acceptance/acceptance/PageObject/Admin/ProductEditPage.php

+    public function saveInvalidForm($productId)
+    {
+        $this->tester->amOnPage('/admin/product/edit/' . $productId);
+        // scroll to have the button visible because of the fixed bars


is it necessary to scroll down? I thought that when you do just $this->tester->clickByCss('.js-parameters-item-add');, it does not matter that the element is not in viewport, or does it?

Yes, there is the problem with fixed elements with higher z-index. Click method will do the scroll to element like function

document.querySelector('.js-parameters-item-add').scrollIntoView()

and then client will try to click on it but there is element fixed bar above it and codeception will fail on error

Element \<a href="#" class="js-parameters-item-add btn btn--plus">...\</a> is not clickable at point (338, 15). Other element would receive the click: \<header class="web__header">...\</header>

vitek-rostislav · 2018-06-21T16:55:36Z

CHANGELOG.md

@@ -61,6 +61,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - [#260 - Admin: Js validation: dynamically added parameters are now validated](https://github.com/shopsys/shopsys/pull/260)
    - validation is evaluated after dynamic parameters are added
    - jQuery migration console error logging is no more
+    - support for multiple option of Flag and Brand items were added for form to be validated and not submitted if it is invalid


I honestly do not know what to do with this problem, the implementation in JsFormValidatorFactory::normalizeViewTransformers() is (already was) really confusing and fragile 😞 It should probably be refactored completely but I do not know how... @PetrHeinz could you please help us here? 👼

vitek-rostislav · 2018-06-21T16:58:33Z

While testing, I noticed yet another bug but it seems that it is already on master so I created a separate issue - #268

PetrHeinz · 2018-08-02T06:12:22Z

After discussion with @boris-brtan we've reduced scope of this PR as there are two unrelated issues atm:

The dynamically added inputs do not have JS validation. This is what we aim to fix.
On product edit page, the form is submitted even if the values are not valid, i.e. JS validation does not prevent the form from submitting. This is related to BG-2136 fixed in 3771de6, we will create a separate issue for this. Boris wanted to fix it in one of the last commits, which recieved most comment during this CR.

Can someone please do a code review of this fix with the reduced scope?

vitek-rostislav

Hi guys, I reviewed and tested the changes and everything now seems ok, you can pass the task to business validation 👍

LukasHeinz · 2018-08-02T14:31:47Z

CHANGELOG.md

@@ -78,6 +78,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - [#352 - flushes executed in loops are now executed outside of loop](https://github.com/shopsys/shopsys/pull/352)
 - [#342 - procedures are now executed only if relevant columns are changed](https://github.com/shopsys/shopsys/pull/342)
 - [#362 - guidelines-for-pull-request.md: fixed indentation of lines and code blocks](https://github.com/shopsys/shopsys/pull/362)
+- [#260 - JS validation: dynamically added form inputs are now validated](https://github.com/shopsys/shopsys/pull/260)


Please correct changelog entry because new version was released.

- Shopsys.validation modified so it applies handlers for newly registered content - code was made more consistent across usages

- avoids issues during parsing, including problems with parsing whitespace characters

boris-brtan force-pushed the bb-admin-product-detail-parameters-js-validation branch 2 times, most recently from c944a3a to 3038ada Compare June 18, 2018 14:47

boris-brtan mentioned this pull request Jun 19, 2018

Admin: Js validation: missing tests for invalidated saving of form with multiple options (checkboxes) #259

Closed

boris-brtan force-pushed the bb-admin-product-detail-parameters-js-validation branch 3 times, most recently from d04c194 to 26c595d Compare June 19, 2018 11:28

vitek-rostislav requested changes Jun 21, 2018

View reviewed changes

TomasLudvik assigned vitek-rostislav Jul 26, 2018

PetrHeinz force-pushed the bb-admin-product-detail-parameters-js-validation branch from 7db2a8d to 8e2e30f Compare August 2, 2018 06:07

PetrHeinz force-pushed the bb-admin-product-detail-parameters-js-validation branch from 8e2e30f to 6b718b4 Compare August 2, 2018 06:23

PetrHeinz changed the title ~~Admin: Js validation: dynamically added parameters are now validated~~ JS validation: dynamically added form inputs are now validated Aug 2, 2018

vitek-rostislav approved these changes Aug 2, 2018

View reviewed changes

LukasHeinz reviewed Aug 2, 2018

View reviewed changes

Boris Brtáň added 3 commits August 3, 2018 09:28

JS validation: dynamically added form inputs are now validated

32a8629

register dynamically added form elements via Shopsys.register

c1b83a4

- Shopsys.validation modified so it applies handlers for newly registered content - code was made more consistent across usages

use $.parseHTML to parse prototype HTML of dynamically added inputs

569002f

- avoids issues during parsing, including problems with parsing whitespace characters

boris-brtan force-pushed the bb-admin-product-detail-parameters-js-validation branch from 6b718b4 to 569002f Compare August 3, 2018 07:28

boris-brtan merged commit 87c13b9 into master Aug 3, 2018

boris-brtan deleted the bb-admin-product-detail-parameters-js-validation branch August 3, 2018 07:34

grossmannmartin mentioned this pull request Mar 2, 2021

JS validation does not work for empty collections #2254

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS validation: dynamically added form inputs are now validated #260

JS validation: dynamically added form inputs are now validated #260

boris-brtan commented Jun 18, 2018

vitek-rostislav left a comment

vitek-rostislav Jun 21, 2018

vitek-rostislav Jun 21, 2018

vitek-rostislav Jun 21, 2018

vitek-rostislav Jun 21, 2018

vitek-rostislav Jun 21, 2018

vitek-rostislav Jun 21, 2018

vitek-rostislav Jun 21, 2018

boris-brtan Jun 22, 2018

vitek-rostislav Jun 21, 2018

vitek-rostislav Jun 21, 2018

boris-brtan Jun 22, 2018 •

edited

vitek-rostislav Jun 21, 2018

vitek-rostislav commented Jun 21, 2018

PetrHeinz commented Aug 2, 2018

vitek-rostislav left a comment

LukasHeinz Aug 2, 2018

JS validation: dynamically added form inputs are now validated #260

JS validation: dynamically added form inputs are now validated #260

Conversation

boris-brtan commented Jun 18, 2018

vitek-rostislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boris-brtan Jun 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitek-rostislav commented Jun 21, 2018

PetrHeinz commented Aug 2, 2018

vitek-rostislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boris-brtan Jun 22, 2018 •

edited