fix: use built-in Angular email validator for eperson form#4999
Conversation
|
Thanks @pzlakowski. I agree that we should not be maintaining our own validation logic for something common like emails when Angular provides one that is likely better tested. Having said that, I tested this on DSpace 8 and noticed that the email |
|
@alanorth this is desired behaviour https://angular.dev/api/forms/Validators#email given WHATWG spec. Angular recommends to use custom regex pattern if business needs are different. There should be no difference. I think if we wanted to support to require top-level domain we could use regex like this: ^(?=.{1,254}$)(?=.{1,64}@)[-!#$%&'*+/0-9=?A-Z^_`a-z{|}~]+(\.[-!#$%&'*+/0-9=?A-Z^_`a-z{|}~]+)*@[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?)+$This is derived from Angular regex, simply swapped from |
|
Thanks @pzlakowski. I don't understand the upstream use case of allowing +1 by me by review and testing. |
|
Which email address validation is performed by the backend? Both validation rules (of frontend and backend) should be identical. Do we need to adapt the backend validation rule? |
tdonohue
left a comment
There was a problem hiding this comment.
👍 Thanks @pzlakowski ! This looks reasonable to me, and I agree with @alanorth that'd I'd rather we adhere to rules established elsewhere (than to create our own custom regex). I gave this some light testing and it appears to work.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-7_x
git worktree add -d .worktree/backport-4999-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-4999-to-dspace-7_x
git switch --create backport-4999-to-dspace-7_x
git cherry-pick -x 1be9d852dc1d2e64cbf0362a356d986faf981a7d |
|
Successfully created backport PR for |
|
Successfully created backport PR for |
|
@pzlakowski : Just a note that this was only able to be auto-backported to 9.x and 8.x. However, we only guarantee backports to 9.x anyways, so that's good enough. That said, if you or anyone else wants to see this also fixed in 7.x then someone will need to create a manual backport PR against the |
References
Fixes Not able to create an account with an email containing uppercase letters from eperson form #4338
Description
I have replaced regex pattern to use built-in one from Angular when we create e-person via form.
I think we should refrain from using our own regex when Angular provides one similar. Unless there is a reason which I do not know about yet.
e.g. issue shows to use pattern from
src/app/register-email-form/register-email-form.component.tswhere Angular tries to adhere to WHATWG spec too https://github.com/angular/angular/blob/4755bbd94964f27921ec203fc1a9233a01c84792/packages/forms/src/validators.ts#L151
Angular adds 3 different constraints compared to WHATWG:
local-partto begin or end with a period (.).local-partlength to exceed 64 characters.Instructions for Reviewers
List of changes in this PR:
Steps test this PR:
TEST@example.comChecklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.