Make asRequired conditional on binding.setAsRequiredEnabled(..) #7098

TatuLund · 2019-12-05T11:23:50Z

It is a very common use case in complex form that whether a field is required or not, it depends on input on other fields. Hypothetical use case sample could be that we have form for a Product and price of the product is needed except in case the Product's type is Sample. So in that kind of scenarios it would be needed to turn off asRequired() validation easily. The purpose of this enhancement and new binding.setAsRequiredEnabled(..) API is to help implementation of this kind of use cases more easily.

There is more generic ticket about conditional validation, and this PR is partially addressing it #10709

Cherry pick from vaadin/framework#11834

Requested in #5030

This change is

It is a very common use case in complex form that whether a field is required or not, it depends on input on other fields. Hypothetical use case sample could be that we have form for a Product and price of the product is needed except in case the Product's type is Sample. So in that kind of scenarios it would be needed to turn off asRequired() validation easily. The purpose of this enhancement and new binding.setAsRequiredEnabled(..) API is to help implementation of this kind of use cases more easily.

vaadin-bot · 2019-12-06T16:28:26Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

+         *            {@code false} if asRequired validator should
+         *            be disabled, {@code true} otherwise (default)
+         */
+        public void setAsRequiredEnabled(boolean asRequiredEnabled);


"public" is redundant in this context.

vaadin-bot · 2019-12-06T16:28:27Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

+         */
+        public void setAsRequiredEnabled(boolean asRequiredEnabled);
+
+        /**


First sentence of Javadoc is incomplete (period is missing) or not present.

vaadin-bot · 2019-12-06T16:28:29Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

+         * @return {@code false} if asRequired validator is disabled
+         *         {@code true} otherwise (default)
+         */
+        public boolean isAsRequiredEnabled();


"public" is redundant in this context.

denis-anisimov

Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @TatuLund)

a discussion (no related file):
It looks like this is a shorthand for already existing functionality.
BindingBuilder has method asRequired(Validator) method.
So it's only a question of providing a good implementation for Validator: if you don't want some logic to be involved in the validation then you may change the iml of Validator.

So two reasons I see why this is extra functionality:

you treat asRequired as a special case. You are right that it's a common case when validation for one field depends on values in other fields. But the only change which is done here is related to required validation. I see this is as generic usecase for any validator.
the functionality already exists: it's possible to change the validator behavior based on values in other fields. This is generic functionality which works for any validator and required is just a specific usecase. I don't see a reason to treat it specifically.

denis-anisimov

Reviewed 1 of 2 files at r1.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @TatuLund)

TatuLund

Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @TatuLund)

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

It looks like this is a shorthand for already existing functionality.
BindingBuilder has method asRequired(Validator) method.
So it's only a question of providing a good implementation for Validator: if you don't want some logic to be involved in the validation then you may change the iml of Validator.

So two reasons I see why this is extra functionality:

you treat asRequired as a special case. You are right that it's a common case when validation for one field depends on values in other fields. But the only change which is done here is related to required validation. I see this is as generic usecase for any validator.

the functionality already exists: it's possible to change the validator behavior based on values in other fields. This is generic functionality which works for any validator and required is just a specific usecase. I don't see a reason to treat it specifically.

This feature, i.e. possibility of more easily to toggle whether asRequired() is enabled or not has been requested by customers many times. I know it is possible to do it using e.g. custom validator, but my understanding is that the need is so common place, that this shorthand and improved DX is justified. Also asRequired() is a special use case already in Binder. Also the feedback I have received is mostly concerning asRequired, not so much the other validators, hence I did this first. I have other idea regarding other validators, which I will probably try to implement later.

denis-anisimov

Reviewed 1 of 2 files at r1.
Reviewable status: 10 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @TatuLund)

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 226 at r1 (raw file):

setAsRequiredEnabled

The wording in the name is confusing for me.
But I can't suggest anything better.....

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 237 at r1 (raw file):

isAsRequiredEnabled

This name is even more confusing. But again: can't suggest a better one.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 919 at r1 (raw file):

return ValidationResult.ok();

curly braces around .
We require them by code style in if end else.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 921 at r1 (raw file):

return customRequiredValidator.apply(value, context);

same here.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 1028 at r1 (raw file):

private boolean asRequiredSet;

is it final ?

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 1298 at r1 (raw file):

Quoted 5 lines of code…

            if (!asRequiredSet) {
                throw new IllegalStateException(
                 "Unable to toggle asRequired validation since "
                         + "asRequired has not been set.");
            }

No unit tests for this.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 1302 at r1 (raw file):

validate();

Something wrong with formatting ? (the indentation is too high on this line )

flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java, line 522 at r1 (raw file):

        binding.setAsRequiredEnabled(false);
        assertFalse(textField.isRequiredIndicatorVisible());

That tests only change for setRequiredIndicatorVisible.
There is also a call to validate. This is not tested anyhow.

TatuLund

Reviewable status: 10 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @TatuLund)

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 226 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

setAsRequiredEnabled
The wording in the name is confusing for me.
But I can't suggest anything better.....

I had some discussion about that with my peers and Leif, but we did not figure out better one.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 919 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

return ValidationResult.ok();
curly braces around .
We require them by code style in if end else.

Fixed

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 1028 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

private boolean asRequiredSet;

is it final ?

Yes, fixed

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 1302 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

validate();
Something wrong with formatting ? (the indentation is too high on this line )

Done.

vaadin-bot · 2019-12-18T10:15:44Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

+            }
+            if (asRequiredEnabled != isAsRequiredEnabled()) {
+                field.setRequiredIndicatorVisible(asRequiredEnabled);
+                validate();


Prefix this call to "validate" with "super.".

denis-anisimov

Reviewed 1 of 1 files at r2.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @TatuLund)

- Testing that if binding.setAsRequiredEnabled(false);, empty value is not invalid - Testing that binding.setAsRequiredEnabled(false); when field is not asRequired() throws exception

TatuLund

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @vaadin-bot)

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 226 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

"public" is redundant in this context.

Done.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 228 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

First sentence of Javadoc is incomplete (period is missing) or not present.

Done.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 237 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

"public" is redundant in this context.

Done.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 921 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

return customRequiredValidator.apply(value, context);

same here.

Done

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 1298 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

            if (!asRequiredSet) {
                throw new IllegalStateException(
                 "Unable to toggle asRequired validation since "
                         + "asRequired has not been set.");
            }

No unit tests for this.

Addded the test case

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java, line 1303 at r2 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Prefix this call to "validate" with "super.".

Done.

flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java, line 522 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

        binding.setAsRequiredEnabled(false);
        assertFalse(textField.isRequiredIndicatorVisible());
That tests only change for setRequiredIndicatorVisible.
There is also a call to validate. This is not tested anyhow.

Added the test case

* Avoid clean up if PNPM is used (#7095) Fixes #7083 * Iterate over a copy for UI internals listeners (#7103) Avoids ConcurrentModificationException if firing an event causes listeners to be added or removed. * Convert lambda exps to simple loops to fix Java 11 build (#7102) * Fix command expectations to expect a local install (#7100) Test run against a local installation of pnpm test was expecting a global installation to be used. * Highlight drop target when dragged on child Fixes #7108 ..but does not fix case where child is a drop target too and drag moves back to parent drop target -> the class name for parent element is not reapplied. * Make nested drop target highlighting work Fixes #7109 * Allow adding a local js module through Page.addJsModule (#6933) * Deny adding a local js module through Page.addJsModule * Deprecate Page.addJsModule(url, loadMode) * Remove old "flow-deps" dependency/package-lock if PNPM is used (#7117) Part of #7113 * Fix using host value from the request (#7115) When using the host value from the request header we need to add the transport as host only contains the server and port. In the off case that we get the scheme as a header we should use that. * Exclude pnpm from managing itself (#7125) * Use recommended API to get chrome capabilities (#7138) DesiredCapabilities.chrome will be deprecated and removed. Current usage results in logged warnings "Using `new ChromeOptions()` is preferred to `DesiredCapabilities.chrome()`" https://github.com/SeleniumHQ/selenium/blob/selenium-3.141.59/java/client/src/org/openqa/selenium/remote/DesiredCapabilities.java#L115-L118 * Add explicit "crossorigin" attribute to bundle script tag in bootstrap (#7124) * Use Element API for traversing children to call the onEnabledStateChanged method (#7131) Fixes #7085 * Deprecate isIOS method and make it in ExtendedClientDetails (#7139) Fixes #6671 * Added missing constructor with scanNestedDefinitions option (#7099) Cherry pick from: vaadin/framework#11801 * Upgrade webpack plugins to fix npm audit warnings (#7122) Upgrade compression-webpack-plugin dependency Upgrade copy-webpack-plugin version Upgrade webpack dependencies versions * Only sending to server should be postponed - Update value in StateTree on the client side immediately - Defer execution only sending a new value to the server Fixes #7128 * Add IT test * Remove IT test which doesn't reproduce the issue * Handle possible array (#7159) If building sourcemaps for webpack the assets array will contain an array of bundles instead of a single string. Fixes #7158 * Workarounds for Safari 10 <script nomodule> and scope bugs (#7165) * Unregister listener when disconnecting web component (#7061) * Make links focusable (#7155) * Fix broken mobile dnd polyfill for ipad on iOS13 Fixes #7123 * Copy when jar (#7175) If packaging the project as a jar then we will copy the frontend files during prepare-frontend. This way running the development mode jar should have all frontend files available. Fixes #6994 * Fix running pnpm for a local installation (#7185) * Fix ElementStateProvider serialization (#7192) Add readResolve method for singleton class handing during (de)serialization of state providers. Fixes #7190 * Pin pnpm verion to 4.5 and remove workaround for 4.3.3 (#7191) Fixes #7183 * Pass query parameters to the new location (#7189) * Update chromedriver to v79 (#7181) * Assert chrome version 79 (#7201) * Only clear all if value changes. (#7212) If we remove all when the innerHTML value stays the same we will loose the innerHTML on the client as the value is not re-sent, but is still cleared from the client. Fixes #4644 * Don't add abstract error targets (#7215) Filter out any abstract classes from error navigation targets as they can't be instantiated anyway. Fixes vaadin/spring#530 * Add onclick to error window (#7217) Now the webpack error window can be closed by clicking it. Fixes #7211 * Error message for failing imports should be tool dependent (#7216) Fixes #7182 * Combine all constructors together Fixes #7148 * log: debug the list of dependencies and imports found in classes (#7221) Co-authored-by: Denis <denis@vaadin.com> Co-authored-by: Leif Åstrand <legioth@gmail.com> Co-authored-by: Mehdi Javan <32511762+mehdi-vaadin@users.noreply.github.com> Co-authored-by: caalador <mikael.grankvist@gmail.com> Co-authored-by: Pekka Hyvönen <pekka@vaadin.com> Co-authored-by: Bogdan Udrescu <bogdanudrescu@users.noreply.github.com> Co-authored-by: Guille <alvarezguille@users.noreply.github.com> Co-authored-by: Johannes Eriksson <joheriks@vaadin.com> Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com> Co-authored-by: Muammer Yucel <muammer.yucel.82@gmail.com> Co-authored-by: Giovanni Lovato <giovanni@lova.to> Co-authored-by: Manuel Carrasco Moñino <manolo@apache.org>

TatuLund · 2019-12-27T10:37:45Z

Superced by #7232

vaadin-bot · 2019-12-27T10:47:58Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateImports.java

@@ -296,14 +308,17 @@ File getGeneratedFallbackFile() {
     *            a directory with project's frontend files
     * @param tokenFile
     *            the token (flow-build-info.json) path, may be {@code null}
+     * @param disablePnpm
+     *            if {@code true} then npm is used instead of pnpm, otherwise
+     *            pnpm is used
     */
    TaskUpdateImports(ClassFinder finder,


Constructor has 8 parameters, which is greater than 7 authorized.

vaadin-bot · 2019-12-27T10:47:59Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateImports.java

+        if (!disablePnpm) {
+            note = "\nMake sure first that `pnpm` command is installed, otherwise you should install it using npm: `npm add -g pnpm@4.5.0`";
+        }
+        return String.format(


Format string should use %n rather than \n in com.vaadin.flow.server.frontend.TaskUpdateImports.getAbsentPackagesMessage()

vaadin-bot · 2019-12-27T10:48:01Z

SonarQube analysis reported 6 issues

1 critical
3 major
2 minor

Watch the comments in this conversation to review them.

TatuLund added 2 commits December 5, 2019 13:19

Add test for binding.setAsRequiredEnabled(..)

4798a11

TatuLund added 3 commits December 5, 2019 13:25

Fixing typo

30e4177

Fixing JavaDoc links

bdabf65

Fixing typo

8f5d369

vaadin-bot reviewed Dec 6, 2019

View reviewed changes

vaadin-bot added the +0.1.0 label Dec 6, 2019

denis-anisimov suggested changes Dec 18, 2019

View reviewed changes

denis-anisimov reviewed Dec 18, 2019

View reviewed changes

TatuLund commented Dec 18, 2019

View reviewed changes

denis-anisimov suggested changes Dec 18, 2019

View reviewed changes

Fixed according to review

7cddbe7

TatuLund commented Dec 18, 2019

View reviewed changes

vaadin-bot reviewed Dec 18, 2019

View reviewed changes

vaadin-bot added +1.0.0 and removed +0.1.0 labels Dec 18, 2019

denis-anisimov reviewed Dec 18, 2019

View reviewed changes

Added test cases

8b7182e

- Testing that if binding.setAsRequiredEnabled(false);, empty value is not invalid - Testing that binding.setAsRequiredEnabled(false); when field is not asRequired() throws exception

TatuLund commented Dec 27, 2019

View reviewed changes

TatuLund and others added 2 commits December 27, 2019 09:40

Renaming test

a2ee97a

TatuLund closed this Dec 27, 2019

vaadin-bot reviewed Dec 27, 2019

View reviewed changes

vaadin-bot added +0.1.0 and removed +1.0.0 labels Dec 27, 2019

manolo deleted the setAsRequiredEnabled branch January 29, 2020 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make asRequired conditional on binding.setAsRequiredEnabled(..) #7098

Make asRequired conditional on binding.setAsRequiredEnabled(..) #7098

TatuLund commented Dec 5, 2019 •

edited

Loading

vaadin-bot Dec 6, 2019

vaadin-bot Dec 6, 2019

vaadin-bot Dec 6, 2019

denis-anisimov left a comment

denis-anisimov left a comment

TatuLund left a comment

denis-anisimov left a comment

TatuLund left a comment

vaadin-bot Dec 18, 2019

denis-anisimov left a comment

TatuLund left a comment

TatuLund commented Dec 27, 2019

vaadin-bot Dec 27, 2019

vaadin-bot Dec 27, 2019

vaadin-bot commented Dec 27, 2019

Make asRequired conditional on binding.setAsRequiredEnabled(..) #7098

Make asRequired conditional on binding.setAsRequiredEnabled(..) #7098

Conversation

TatuLund commented Dec 5, 2019 • edited Loading

vaadin-bot Dec 6, 2019

Choose a reason for hiding this comment

vaadin-bot Dec 6, 2019

Choose a reason for hiding this comment

vaadin-bot Dec 6, 2019

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

TatuLund left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

TatuLund left a comment

Choose a reason for hiding this comment

vaadin-bot Dec 18, 2019

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

TatuLund left a comment

Choose a reason for hiding this comment

TatuLund commented Dec 27, 2019

vaadin-bot Dec 27, 2019

Choose a reason for hiding this comment

vaadin-bot Dec 27, 2019

Choose a reason for hiding this comment

vaadin-bot commented Dec 27, 2019

TatuLund commented Dec 5, 2019 •

edited

Loading