Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/393 Match api email validation #402

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

GermanSaracca
Copy link
Contributor

@GermanSaracca GermanSaracca commented May 20, 2024

What this PR does / why we need it:
Fixes a mismatch between the email validation of the SPA Dataset creation form and the email validation of the dataverse api.
Also improves the UX to display the alert error banner only when creations fail, with messages specific to both the client-javascript api wrapper and the dataverse api itself (from the latter extracting only the field-related part of the error and not the part of the error related to Java invalid value exceptions).

Which issue(s) this PR closes:

Closes #393

Special notes for your reviewer:

Suggestions on how to test this:
Enter an invalid email (test@test.e) and check that the error message is displayed below the field.
It is a bit difficult to test the error messages in the banners as the validations are being fulfilled from the form before it is submitted.
One option could be to test locally, modifying the code so that no fields are required within the form and submitting the form, in useDefineRules.tsx set the required property to false.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
The same error banner is maintained, only now when the user submits the form, if there is an error and this banner appears, it is automatically focused with an specific message about the error.

Error thrown from the dataverse api 👇
Screenshot 2024-05-20 at 11 01 07

Error thrown from the client-javascript api wrapper 👇
Screenshot 2024-05-20 at 11 03 05

How the error banner is focused 👇
In this example the client-side validations were removed just to show the focus case.
alert-error-behaviour.webm

Is there a release notes update needed for this change?:
No.

Additional documentation:
No.

@GermanSaracca GermanSaracca added bug Something isn't working Size: 3 A percentage of a sprint. 2.1 hours. pm.GREI-d-2.7.1 NIH, yr2, aim7, task1: R&D UI modules for creating datasets and supporting publishing workflows pm.GREI-d-2.7.2 NIH, yr2, aim7, task2: Implement UI modules for creating datasets and publishing workflows SPA: Create Dataset Form labels May 20, 2024
@GermanSaracca GermanSaracca changed the title Fix/393 match api email validation2 Fix/393 Match api email validation May 21, 2024
@GermanSaracca GermanSaracca self-assigned this May 22, 2024
@coveralls
Copy link

coveralls commented May 22, 2024

Coverage Status

coverage: 97.848% (+0.04%) from 97.805%
when pulling f34e56a on fix/393-match-api-email-validation2
into 0d1d97e on develop.

@GermanSaracca GermanSaracca removed their assignment May 22, 2024
@ekraffmiller ekraffmiller self-assigned this May 23, 2024
@ekraffmiller
Copy link
Contributor

@GermanSaracca There is still a difference between the form validation and the API validation - from my testing, I think the API validation is requiring that the email address be from a list of top level domains, for example: https://data.iana.org/TLD/tlds-alpha-by-domain.txt.

When I try an address that is not in the TLD list, I get an error:
Screenshot 2024-05-23 at 9 23 46 PM
The API uses an apache commons validator: https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/validation/EMailValidator.java#L13
I think it is using this underlying domain validator:
https://commons.apache.org/proper/commons-validator/apidocs/org/apache/commons/validator/routines/DomainValidator.html

I did some searches and couldn't find a javascript library that does something similar, so I'm not sure the best way to handle this, what do you think?

@GermanSaracca
Copy link
Contributor Author

@GermanSaracca There is still a difference between the form validation and the API validation - from my testing, I think the API validation is requiring that the email address be from a list of top level domains, for example: https://data.iana.org/TLD/tlds-alpha-by-domain.txt.

When I try an address that is not in the TLD list, I get an error: Screenshot 2024-05-23 at 9 23 46 PM The API uses an apache commons validator: https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/validation/EMailValidator.java#L13 I think it is using this underlying domain validator: https://commons.apache.org/proper/commons-validator/apidocs/org/apache/commons/validator/routines/DomainValidator.html

I did some searches and couldn't find a javascript library that does something similar, so I'm not sure the best way to handle this, what do you think?

Hi @ekraffmiller , your findings are totally correct, there is no Javascript library that does the same validations as the Apache library ( at least not a maintained one ).
One option, would be to hardcode all possible TLDS in the SPA but I really don like it because they are always changing or new ones are added.
I think this is just a client side validation layer and after receiving the error we are showing in the alert so I think it is enough for the user to know what is happening. What are your thoughts?

@ekraffmiller
Copy link
Contributor

@GermanSaracca There is still a difference between the form validation and the API validation - from my testing, I think the API validation is requiring that the email address be from a list of top level domains, for example: https://data.iana.org/TLD/tlds-alpha-by-domain.txt.
When I try an address that is not in the TLD list, I get an error: Screenshot 2024-05-23 at 9 23 46 PM The API uses an apache commons validator: https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/validation/EMailValidator.java#L13 I think it is using this underlying domain validator: https://commons.apache.org/proper/commons-validator/apidocs/org/apache/commons/validator/routines/DomainValidator.html
I did some searches and couldn't find a javascript library that does something similar, so I'm not sure the best way to handle this, what do you think?

Hi @ekraffmiller , your findings are totally correct, there is no Javascript library that does the same validations as the Apache library ( at least not a maintained one ). One option, would be to hardcode all possible TLDS in the SPA but I really don like it because they are always changing or new ones are added. I think this is just a client side validation layer and after receiving the error we are showing in the alert so I think it is enough for the user to know what is happening. What are your thoughts?

@GermanSaracca There is still a difference between the form validation and the API validation - from my testing, I think the API validation is requiring that the email address be from a list of top level domains, for example: https://data.iana.org/TLD/tlds-alpha-by-domain.txt.
When I try an address that is not in the TLD list, I get an error: Screenshot 2024-05-23 at 9 23 46 PM The API uses an apache commons validator: https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/validation/EMailValidator.java#L13 I think it is using this underlying domain validator: https://commons.apache.org/proper/commons-validator/apidocs/org/apache/commons/validator/routines/DomainValidator.html
I did some searches and couldn't find a javascript library that does something similar, so I'm not sure the best way to handle this, what do you think?

Hi @ekraffmiller , your findings are totally correct, there is no Javascript library that does the same validations as the Apache library ( at least not a maintained one ). One option, would be to hardcode all possible TLDS in the SPA but I really don like it because they are always changing or new ones are added. I think this is just a client side validation layer and after receiving the error we are showing in the alert so I think it is enough for the user to know what is happening. What are your thoughts?

I think this is just a client side validation layer and after receiving the error we are showing in the alert so I think it is enough for the user to know what is happening. What are your thoughts?

Yes, I think so too, it would be very cumbersome to maintain a list in our code. The new alert messages work very well to inform the user 👍

@ekraffmiller
Copy link
Contributor

@GermanSaracca There are some warnings due to the new lint tests...I know we don't need to fix them all, but maybe this PR could fix the ones related to Create Dataset?

/Users/ellenkraffmiller/Documents/GitHub/dataverse-frontend2024/src/sections/create-dataset/CreateDataset.tsx
  44:6  warning  React Hook useEffect has a missing dependency: 'setIsLoading'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/Users/ellenkraffmiller/Documents/GitHub/dataverse-frontend2024/src/sections/create-dataset/MetadataBlockFormFields/MetadataFormField/Fields/PrimitiveMultiple.tsx
  51:5  warning  React Hook useMemo has a missing dependency: 'builtFieldNameWithIndex'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

@GermanSaracca
Copy link
Contributor Author

I completely agree, I will fix those eslint warnings, thanks @ekraffmiller

@GermanSaracca
Copy link
Contributor Author

@ekraffmiller Eslint warnings were addressed 👍🏼 , thanks!

Copy link
Contributor

@ekraffmiller ekraffmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@GPortas GPortas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

fixemail.mov

@GPortas GPortas merged commit 0e12edc into develop Jun 6, 2024
20 checks passed
@GPortas GPortas deleted the fix/393-match-api-email-validation2 branch June 6, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pm.GREI-d-2.7.1 NIH, yr2, aim7, task1: R&D UI modules for creating datasets and supporting publishing workflows pm.GREI-d-2.7.2 NIH, yr2, aim7, task2: Implement UI modules for creating datasets and publishing workflows Size: 3 A percentage of a sprint. 2.1 hours. SPA: Create Dataset Form
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Dataset: email validation does not match validation in the Dataverse API
4 participants