Feature contact group send by bfritscher · Pull Request #2414 · nextcloud/mail

bfritscher · 2019-12-29T00:57:43Z

Test of implementing contact group lookup and expansion
By adding a new GroupService implementation and copying some code of the ContactsIntegration.php
This is a first attempt to fix issue #41 . Some open questions I identified the following open issues:

To which email of the contact do we send? first or all?
Should we hide the expansion in the BCC field?
GroupService only seems to use email field no label?

Fixes #41

welcome · 2019-12-29T00:57:45Z

Thanks for opening your first pull request in this repository! ✌️

jancborchardt · 2019-12-30T06:47:22Z

@bfritscher hello and welcome to Nextcloud Mail! :) Could you add some screenshots so people see how to test? And it also helps a lot with design review – thanks! :)

bfritscher · 2019-12-30T09:33:46Z

@jancborchardt thanks! There is not a lot to see I used the existing group autocomplete. And used it to add contact group autocomplete. So now in the mail's To field the autocomplete also provides group names from contacts. The entry is marked as (ContactGroup) instead of (nextcloud) which is for internal groups. On send the entry is expanded to all emails by the GroupsIntegration provided by @myrho in #810.

jancborchardt · 2019-12-30T10:06:51Z

Ah right, and ContactGroup means a group as per the Contacts app, right?

Then my only feedback is to simply append "(group)" instead of the quite technical "(ContactGroup)". :)

ChristophWurst · 2020-01-07T12:35:33Z

Then my only feedback is to simply append "(group)" instead of the quite technical "(ContactGroup)". :)

That makes it ambiguous. It should be "Contacts" IMO.

ChristophWurst

Code looks good otherwise. Thanks for also adding tests!

ChristophWurst · 2020-01-07T12:36:36Z

+			if (!isset($r['EMAIL'])) {
+				continue;
+			}
+			$groups = preg_split('/,/', $r['CATEGORIES']);


this might return false

why not just use explode?

Good point missed explode when looking through the api. Long time since I coded in php.

No worries :)

jancborchardt · 2020-01-08T05:52:16Z

Then my only feedback is to simply append "(group)" instead of the quite technical "(ContactGroup)". :)

That makes it ambiguous. It should be "Contacts" IMO.

Alright, that seems good. :)

ChristophWurst · 2020-01-08T09:20:11Z

 use OCA\Mail\Service\AvatarService;
 use OCA\Mail\Service\Group\IGroupService;
 use OCA\Mail\Service\Group\NextcloudGroupService;
+use OCA\Mail\Service\Group\ContactsGroupService;


actually not used here. you may also remove the two other group service imports

I do not yet get everything about use, but if its like imports it seems to me that since d18ef16 when BootstrapSingleton was created only App is really used? And everything else was moved to Bootstrap?

Yes, that is absolutely correct :)

Ok I squashed my edits into one commit and made a second one for the cleanup of the imports in applications. Do you need it on a different pull request?

By adding a new GroupService implementation and copying some code of the ContactsIntegration.php Signed-off-by: Boris Fritscher <boris@fritscher.ch>

Signed-off-by: Boris Fritscher <boris@fritscher.ch>

ChristophWurst · 2020-01-09T13:54:50Z

To which email of the contact do we send? first or all?

Good question. Let's send to all of them.

Should we hide the expansion in the BCC field?

No, I think there can be cases where you want to use a group for BCC.

GroupService only seems to use email field no label?

IIRC we just fall back to email as label if no label is set.

ChristophWurst · 2020-01-09T13:55:33Z

+			}
+
+			$email = $r['EMAIL'];
+			// only take first email ?


as commented, I think we should just use all.

ok changed it to iterate through all emails

I wouldn't iterate over all email addresses for a specific contact, often contacts have employer mail addresses or mail addresses for different purposes. I would just use the first mail address, which is supposed to be the favourite or public mail address of the respective contact.

ChristophWurst

Looks good otherwise and works as advertised :)

Thanks a lot!

Signed-off-by: Boris Fritscher <boris@fritscher.ch>

ChristophWurst

Great work!

ChristophWurst · 2020-01-10T13:42:28Z

Will integrate once CI passes. Restarted a few jobs. Looks like it was a hiccup with the Docker service.

dieter-wilhelm · 2020-01-18T10:54:29Z

To which email of the contact do we send? first or all?

Good question. Let's send to all of them.

I would suggest the first one (supposedly the "favourite" one)

Should we hide the expansion in the BCC field?

No, I think there can be cases where you want to use a group for BCC.

yes, I think for this "group" functionality BCC is the most import field with regards to privacy

GroupService only seems to use email field no label?

IIRC we just fall back to email as label if no label is set.

Pardon, what does "label" mean in this context?

thank you for your work and please tell me when this feature will be available.

ChristophWurst · 2020-01-20T13:34:16Z

I would suggest the first one (supposedly the "favourite" one)

Open a ticket to discuss.

Pardon, what does "label" mean in this context?

It's a detail of the UI code. Like what we use for the user-visible text on each entry.

bfritscher force-pushed the feature-contact-group-send branch from 5f2fc32 to 6bdc2ec Compare December 30, 2019 09:22

ChristophWurst requested changes Jan 7, 2020

View reviewed changes

ChristophWurst reviewed Jan 8, 2020

View reviewed changes

Implement contact group lookup and expansion

ffe16e6

By adding a new GroupService implementation and copying some code of the ContactsIntegration.php Signed-off-by: Boris Fritscher <boris@fritscher.ch>

bfritscher force-pushed the feature-contact-group-send branch from e13a331 to 7de5e5f Compare January 8, 2020 16:47

remove unused imports

e1d5256

Signed-off-by: Boris Fritscher <boris@fritscher.ch>

bfritscher force-pushed the feature-contact-group-send branch from 7de5e5f to e1d5256 Compare January 8, 2020 16:49

ChristophWurst added 3. to review enhancement labels Jan 9, 2020

ChristophWurst reviewed Jan 9, 2020

View reviewed changes

ChristophWurst added 2. developing and removed 3. to review labels Jan 9, 2020

send to all emails of the contact matching a group

28357f7

Signed-off-by: Boris Fritscher <boris@fritscher.ch>

ChristophWurst added 4. to release and removed 2. developing labels Jan 10, 2020

ChristophWurst approved these changes Jan 10, 2020

View reviewed changes

ChristophWurst merged commit 9ff714a into nextcloud:master Jan 16, 2020

alexanderdd mentioned this pull request Jan 22, 2021

sending to groups > expand group name to individual addresses #2723

Open

kesselb mentioned this pull request Jun 6, 2025

Writing am mail to a contact group should only use the preferred Mail-Address #9075

Open

Uh oh!

Conversation

bfritscher commented Dec 29, 2019 • edited by ChristophWurst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welcome Bot commented Dec 29, 2019

Uh oh!

jancborchardt commented Dec 30, 2019

Uh oh!

bfritscher commented Dec 30, 2019

Uh oh!

jancborchardt commented Dec 30, 2019

Uh oh!

ChristophWurst commented Jan 7, 2020

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jancborchardt commented Jan 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Jan 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dieter-wilhelm Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Jan 10, 2020

Uh oh!

dieter-wilhelm commented Jan 18, 2020

Uh oh!

ChristophWurst commented Jan 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bfritscher commented Dec 29, 2019 •

edited by ChristophWurst

Loading

dieter-wilhelm Jan 18, 2020 •

edited

Loading