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

Allow multiemail input for ServiceGroup EMAIL #146

Merged
merged 4 commits into from Aug 17, 2020

Conversation

nikoloutsa
Copy link
Contributor

@nikoloutsa nikoloutsa commented Apr 5, 2019

For our use-case it would make sense to allow more than one emails for ServiceGroup contact , since it's not a person it's a group.

- test1@mail.com valid
- test1@mail.com;test2@mail.com valid
- test1@mail.com; invalid
- test1@mail.com;test2@mail.com invalid

	modified:   config/gocdb_schema.xml
	modified:   htdocs/web_portal/views/service_group/edit_service_group.php
config/gocdb_schema.xml Outdated Show resolved Hide resolved
config/gocdb_schema.xml Outdated Show resolved Hide resolved
@gregcorbett
Copy link
Member

* [test1@mail.com](mailto:test1@mail.com);[test2@mail.com](mailto:test2@mail.com) valid
* [test1@mail.com](mailto:test1@mail.com);[test2@mail.com](mailto:test2@mail.com) invalid

What is the difference between these two cases? To me, they look the same.

@jrha
Copy link
Contributor

jrha commented Apr 9, 2019

Slightly off-topic but maybe something to consider for @gregcorbett as a later improvement: The base regex may be too restrictive as it definitely does not allow for Unicode characters in addresses, which may or may not be allowed anyway.

It might also be worth seeing whether PHP's built-in regex filter would be good enough:

filter_var($email, FILTER_SANITIZE_EMAIL);

The HTML5 spec says:

/^[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[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])?)*$/

So… 🤷‍♂️

Copy link
Member

@tofu-rocketry tofu-rocketry left a comment

Choose a reason for hiding this comment

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

Looks ok. Just that question over the lazy matching (+?).

@gregcorbett
Copy link
Member

Slightly off-topic but maybe something to consider for @gregcorbett as a later improvement: The base regex may be too restrictive as it definitely does not allow for Unicode characters in addresses, which may or may not be allowed anyway.

It might also be worth seeing whether PHP's built-in regex filter would be good enough:

filter_var($email, FILTER_SANITIZE_EMAIL);

The HTML5 spec says:

/^[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[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])?)*$/

So… 🤷‍♂️

Feel free to open an issue :)

@tofu-rocketry
Copy link
Member

@jrha opened an issue (#147). Do we just need to sort the lazy matching out?

@gregcorbett
Copy link
Member

@jrha opened an issue (#147). Do we just need to sort the lazy matching out?

And what the difference between these two test cases is suppose to be. @nikoloutsa?.

@nikoloutsa
Copy link
Contributor Author

nikoloutsa commented Apr 18, 2019

the difference is not to allow a semicolon at the end of the mail field.

It looked cleaner to me when someone inputs just a single email, to trim out the last semicolon as it was previously without my addition.

@tofu-rocketry
Copy link
Member

the difference is not to allow a semicolon at the end of the mail field.

@nikoloutsa, There doesn't seem to be a semicolon at the end of the last test case though. I can see that there is a semicolon at the end of the invalid case that only has one email address, so was that just a mistype for the invalid two address case?

Change to proposed regex to allow a trailing semicolon and remove newly proposed lazy matching in the regex.
@GRyall
Copy link
Contributor

GRyall commented Feb 3, 2020

As discussed above I have removed the lazy matching. Following in person discussions with the devlopment team I have also reworked the regex to allow trailing semicolons. @gregcorbett this is ready to be merged!

GRyall
GRyall previously approved these changes Feb 3, 2020
Co-Authored-By: gregcorbett <gregcorbett@users.noreply.github.com>
@gregcorbett gregcorbett merged commit d181cf6 into GOCDB:master Aug 17, 2020
gregcorbett added a commit that referenced this pull request Aug 18, 2020
Merge of master to dev (covering #146)
gregcorbett added a commit to gregcorbett/gocdb that referenced this pull request Oct 14, 2021
- This reverts commit d181cf6,
  reversing changes made to
  5d6dea3.
- PR GOCDB#146 was erroneously merged into master.
- I don't want this feature (Allow multi email input for
  Service Group EMAIL) in 5.7.6 as it is not part of that
  release branch so has not been tested in preproduction.
- PR GOCDB#247 ensures the feature is in the dev branch. The feature is
  already part of the release-5.8.0 branch.
gregcorbett added a commit that referenced this pull request Oct 15, 2021
- This reverts commit d181cf6,
  reversing changes made to
  5d6dea3.
- PR #146 was erroneously merged into master.
- I don't want this feature (Allow multi email input for
  Service Group EMAIL) in 5.7.6 as it is not part of that
  release branch so has not been tested in preproduction.
- PR #247 ensures the feature is in the dev branch. The feature is
  already part of the release-5.8.0 branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants