feat(ui): paste or copy several e-mail addresses into address fields at once#9347
Conversation
|
@ChristophWurst I pushed this last commit into this PR for review, and I'm not sure if it applies to this task or not:
Hint this is relevant to this task, if yes, I will cover these cases with code. If there are more, I will be ready to solve them :) |
arublov
force-pushed
the
feature/add-several-emails-at-once
branch
from
58dd9b5 to
3887fc8
Compare
|
@ChristophWurst any updates here? Thanks :) |
|
This is a bit of a gamble with the regex. Can you make sure that selecting people with titles doesn't split single values? E.g. if "Jane Doe, MSc jane@doe.tld" is entered, it should stay "Jane Doe, MSc jane@doe.tld", not become "Jane Doe" and ", MSc jane@doe.tld" |
arublov
force-pushed
the
feature/add-several-emails-at-once
branch
from
3887fc8 to
f9da4ce
Compare
arublov
force-pushed
the
feature/add-several-emails-at-once
branch
2 times, most recently
from
8ca309b to
4adceef
Compare
|
@ChristophWurst , yes done, I fixed some cases and tested, could you please see the new changes and write me feedback, what do you think about this? Maybe I need to change something in my solution? P.S. Maybe need to move the regex to another space somewhere, so that it can be accessed in any other classes in the project? |
arublov
force-pushed
the
feature/add-several-emails-at-once
branch
from
6e9e6a0 to
d1c7b42
Compare
|
@ChristophWurst I was doing research on this a couple days ago and tried your examples based on thunderbird, gmail and roundcube. So created a more flexible solution based on these apps and my research, so if I need to further improve or change the logic in my solution I will be happy to work on this further :) Just FYI:How works Thunderbird:
How works Web Gmail
All of these invalid Look: rfc822 standard |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
|
Is this PR still being worked on? I just found out about it and honestly was surprised that nextcloud doesn't support pasting multiple addresses yet. |
|
Hi ! |
|
I think this feature should get some attention. As a workaround, I started creating mailto links from my lists of email addresses. This way the email addresses are inserted properly. |
ChristophWurst
force-pushed
the
feature/add-several-emails-at-once
branch
from
d1c7b42 to
7cc4d25
Compare
|
Rebased to resolve conflicts |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Strip trailing delimiters that address-rfc2822 can't handle | ||
| const cleaned = str.replace(/[,;]+$/, '').trim() |
There was a problem hiding this comment.
The delimiter stripping runs before trim(), so inputs like "alice@example.com, " (comma + trailing space) won’t have the comma removed and can fail parsing. Trim first and then strip delimiters (or expand the regex to include trailing whitespace) so common paste/typing patterns work reliably.
| // Strip trailing delimiters that address-rfc2822 can't handle | |
| const cleaned = str.replace(/[,;]+$/, '').trim() | |
| // Trim first so trailing delimiters followed by whitespace are still removed | |
| const cleaned = str.trim().replace(/[,;]+$/, '').trim() |
| for (const addr of parsedFromSearch) { | ||
| if (!list.some((recipient) => recipient.email === addr.email)) { | ||
| const recipient = { ...addr } | ||
| this.newRecipients.push(recipient) | ||
| list.push(recipient) | ||
| changed = true | ||
| } | ||
| } |
There was a problem hiding this comment.
De-duplication compares recipient.email === addr.email (case-sensitive). This can allow duplicates for the same address pasted with different casing (pretty common when copying from different sources). Consider comparing on a canonical form (e.g., email.toLowerCase() at least for the domain part, or entirely lowercased if that matches existing app behavior) to avoid duplicate recipients.
| for (const addr of parsedFromSearch) { | ||
| if (!list.some((recipient) => recipient.email === addr.email)) { | ||
| const recipient = { ...addr } | ||
| this.newRecipients.push(recipient) | ||
| list.push(recipient) | ||
| changed = true | ||
| } | ||
| } |
There was a problem hiding this comment.
list.some(...) is called for each parsed address, making this O(n*m) as recipients grow. Using a Set of existing emails (and updating it as you add) would make de-dup checks O(1) and keep recipient addition snappy for large pastes.
| if (parseEmailList(value).length === 0) { | ||
| throw new Error('Skipping because it does not look like a valid email address') | ||
| } | ||
| return { email: value, label: value } |
There was a problem hiding this comment.
createRecipientOption now accepts values that parse into one or more addresses, but it returns { email: value, label: value } even if value contains multiple addresses and/or invalid tokens. This can create confusing transient UI labels (the chip/option may display the entire pasted blob). Consider normalizing the returned label/email (e.g., trimming) and/or handling multi-address pastes as a distinct path so the created option better matches what will actually be added.
| if (parseEmailList(value).length === 0) { | |
| throw new Error('Skipping because it does not look like a valid email address') | |
| } | |
| return { email: value, label: value } | |
| const parsedRecipients = parseEmailList(value) | |
| if (parsedRecipients.length === 0) { | |
| throw new Error('Skipping because it does not look like a valid email address') | |
| } | |
| const firstRecipient = parsedRecipients[0] | |
| const normalizedValue = (typeof firstRecipient === 'string' | |
| ? firstRecipient | |
| : firstRecipient.email || firstRecipient.address || firstRecipient.label || firstRecipient.name || value | |
| ).trim() | |
| return { email: normalizedValue, label: normalizedValue } |
| } | ||
|
|
||
| // Strip trailing delimiters that address-rfc2822 can't handle | ||
| const cleaned = str.replace(/[,;]+$/, '').trim() |
There was a problem hiding this comment.
There’s no unit test covering trailing delimiters with trailing whitespace (e.g., 'alice@example.com, ' or 'alice@example.com; '), which is a common paste/typing pattern and currently fails due to the replace() happening before trim(). Add a regression test for this case alongside the existing trailing delimiter tests.
| const cleaned = str.replace(/[,;]+$/, '').trim() | |
| const cleaned = str.trim().replace(/[,;]+$/, '').trim() |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds src/util/emailAddress.js with tryParse, splitOnDelimiters, getLabelAndAddress, and parseEmailList to parse single and comma/semicolon-separated address lists (respecting quoted names and angle-brackets). It adds unit tests for those functions (including issue 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
This works well now. If there are no major issues found we can merge it. |
ChristophWurst
force-pushed
the
feature/add-several-emails-at-once
branch
2 times, most recently
from
1b4f3d4 to
e4e5f2d
Compare
ChristophWurst
left a comment
ChristophWurst
left a comment
There was a problem hiding this comment.
Tested, cleaned-up and works 👍
…at once Signed-off-by: Andrii Rublov <airublev@outlook.com>
ChristophWurst
force-pushed
the
feature/add-several-emails-at-once
branch
from
e4e5f2d to
98f6282
Compare
ChristophWurst
changed the title
Details:
Case 1:
string = 'test@test.com, Jane Doe, MSc jane@doe.tld'Result:
newRecipients:
Case 2:
string = 'ian eiloart iane@example.ac.uk>;shuf6@example.ac.uk,, test+user@company.c, "ian,eiloart"<ian@example.ac.uk>, <@example.com:foo@example.ac.uk>, foo@#,ian@-example.com, ian@one@two;asdas< test@test.com> test@test.com, Newasd Na@,me >; testaaaa@aasd.com'Result:
newRecipients:
