Fixes Unvalidated Email Succeeding to group addition

[rohnsha0]

fixes #933

[mister-roboto]

@rohnsha0 thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[rohnsha0]

@jenkins-plone-org please run jobs

[ale-rt]

Please check my comments. The only thing that I consider a blocker is to not introduce new code for email validation.

[rohnsha0]

Please check my comments. The only thing that I con sider a blocker is to not introduce new code for email validation.

I understand, it was my oversee... However pushed an commit with validation using PloneTool

[rohnsha0]

@jenkins-plone-org please run jobs

[mauritsvanrees]

@rohnsha0 Did you push everything? I now get an AttributeError for validate_group_emails.

[rohnsha0]

@rohnsha0 Did you push everything? I now get an AttributeError for validate_group_emails.

Ah, I renamed validate_group_emails -> extract_invalid_emails... missed renaming usage of the same.. but its fixed now! please recheck now!

[mauritsvanrees]

Thanks for working on this.

There is a problem when the new email validation fails: the form switches to a form for creating a new group. This is caused by the line self.group = None which is already in the code on master.
You should move this line after your validation. Actually, it looks like you could actually remove the line.

Also, even with that fixed, any changes that the user has filled in, are gone when there is a validation error: the form shows the current group data. It should show the data from the request. One way to do this, would be to change the get_group_property method to take the prop_id from the request, but only if it is a POST request.

[rohnsha0]

Thanks for working on this.

There is a problem when the new email validation fails: the form switches to a form for creating a new group. This is caused by the line self.group = None which is already in the code on master. You should move this line after your validation. Actually, it looks like you could actually remove the line.

I've removed self.group=None

[rohnsha0]

Also, even with that fixed, any changes that the user has filled in, are gone when there is a validation error: the form shows the current group data. It should show the data from the request. One way to do this, would be to change the get_group_property method to take the prop_id from the request, but only if it is a POST request.

I've pushed a PR that retains the form data after an unsuccessful attempt. However, the name field is not getting retained!

[ale-rt]

@jenkins-plone-org please run jobs

[ale-rt]

This PR prevents:

• setting multiple mail addresses separated by commas+space, see COMMASPACE in https://python.readthedocs.io/fr/hack-in-language/library/email-examples.html.
• setting an email + display name/

This might be a breaking change for somebody.
I am not sure, I want that.

CC @plone/framework-team

[mauritsvanrees]

Since when do we want to support multiple emails here? That would surprise me.

[ale-rt]

Since when do we want to support multiple emails here? That would surprise me.

I am just saying that it is actually possible with the current code base.

[rohnsha0]

the form now validates multiple mail id(s) as well along with displayName! @ale-rt @mauritsvanrees

[rohnsha0]

@jenkins-plone-org please run jobs

[ale-rt]

I appreciate your efforts, but this seems like a breaking change without significant benefits. If another core developer agrees with your approach, I will anyway not veto it.

I believe the PR has gone off track.
The code now attempts to split a string from an input that should not allow newlines.
This form needs to be migrated to z3c.form to ensure proper validation.
However, I recommend postponing that, as we are doing a major refactoring that is moving around templates.

Instead of raising a validation error, I would prefer issuing a warning to the user, indicating that the email field contains an invalid email-like value.
That would be acceptable for me.

Try to wait if somebody else has other opinions or if they are fine with your proposed approach!

Thanks again for your patience and work!