Sending to nextcloud groups by myrho · Pull Request #810 · nextcloud/mail

myrho · 2018-02-27T15:31:59Z

This PR overrides #800.

Autocomplete nextcloud groups in recipient input fields.
Prefix nextcloud group recipients with "nextcloud:" to have a means of distinguishing from yet-to-implement Contacts groups recipients (Sending to groups [$150 awarded] #41).
Expands groups to email addresses of group members.

Fixes #41

ChristophWurst

Thanks!

ChristophWurst · 2018-02-27T15:46:15Z

 		$container->registerAlias('ErrorMiddleware', ErrorMiddleware::class);
 		$container->registerMiddleWare('ErrorMiddleware');
+
+		$container->registerService('OCA\Mail\Service\GroupsIntegration', function($c) {


I don't think you have to register this here. The DI container should be able to resolve this class automatically.

How should the container know which AbstractGroupServices GroupsIntegration should be constructed with?

See below. Use an interface instead of this abstract class and register the implementation as alias of the interface here (see other services in this file).

Please use $container->registerAlias(IGroupService::class, NextcloudGroupService::class); instead

ChristophWurst · 2018-02-27T15:48:20Z


 	public function findMatches($term) {
 		$recipientsFromContacts = $this->contactsIntegration->getMatchingRecipient($term);
+		$recipientGroups = $this->groupsIntegration->getMatchingGroups($term);


I'd prefer to have this logic moved to a service class, maybe even introduce a facade for that. But that's a minor enhancement - this can be done later on 😉

ChristophWurst · 2018-02-27T15:49:44Z

+
+namespace OCA\Mail\Service\Group;
+
+abstract class AbstractGroupService {


Looks like an interface would be the better choice here.

ChristophWurst · 2018-02-27T15:50:53Z

+
+		return array_map(
+			function($g) {
+				return [


Looks like a good candidate to implement PHP's JsonSerializable interface for this.

I ported this pattern from Contacts\IManager::search. It's just returning key/value pairs. Where would JsonSerializable fit in?

Nvm, just realized that $g is some data structure from the Nextcloud server, so nothing we can change. In that case that's fine 👍

ChristophWurst · 2018-02-27T15:51:06Z

+		$users = $this->groupManager->get($groupId)->getUsers();
+		return array_map(
+			function($user) {
+				return [


ChristophWurst · 2018-02-27T15:53:11Z

+	 */
+	private $groupServices = [];
+
+	public function __construct(AbstractGroupService ...$groupServices) {


For simplicity, I'd recommend to only inject a single group service (there's only one so far). You don't have to use the concrete impl here but use the interface (as proposed above) and register the impl as alias for the interface in Application.

I designed this so it will be extensible for Contacts groups.
But I could reserve this feature for later when Contacts groups integration will be ready.

ChristophWurst · 2018-02-27T15:53:48Z

+	/**
+	 * The services to get groups from
+	 *
+	 * @var IGroupService[]


Does this interface really exist?

It did :-)
Missed to update the docs here.

But AbstractGroupService should become IGroupService again anyways.

ChristophWurst · 2018-02-27T15:55:29Z

+	protected function setUp() {
+		parent::setUp();
+
+		$this->groupService1 = $this->getMockBuilder(NextcloudGroupService::class)


use $this->createMock(...::class);.

ChristophWurst · 2018-02-27T15:56:41Z

+	 *
+	 * @return string
+	 */
+	abstract public function getNamespace();


Do we really need these namespaces? This kinda looks like an unnecessary generalization.

Yes, I think so. There are two reasons:

When GroupIntegration once would be extended for Contacts groups, we need a means to tell apart whether it's a Nextcloud or a Contacts group, since both applications can have equally named groups. Eg. nextcloud:mygroup vs. contacts:mygroup.

We need to distinguish nextcloud groups from single recipients, since Nextcloud groups could look like email addresses. Eg. nextcloud:group@domain.com vs. group@domain.com ...

ChristophWurst · 2018-02-28T11:20:26Z

 		$this->appName = 'mail';
 		$this->request = $this->createMock(IRequest::class);
 		$this->accountService = $this->createMock(AccountService::class);
+		$this->groupsIntegration = new GroupsIntegration();


use a mock instead please

fixed

ChristophWurst

🚀

ChristophWurst · 2018-03-12T09:26:17Z

 		$container->registerAlias('ErrorMiddleware', ErrorMiddleware::class);
 		$container->registerMiddleWare('ErrorMiddleware');
+
+		$container->registerService('OCA\Mail\Service\GroupsIntegration', function($c) {


Please use $container->registerAlias(IGroupService::class, NextcloudGroupService::class); instead

ChristophWurst · 2018-03-12T09:27:26Z

+	protected function setUp() {
+		parent::setUp();
+
+		$this->groupsManager = $this->createMock('OCP\IGroupManager');


use the ::class notation

ChristophWurst · 2018-03-12T09:28:37Z

+
+	public function getUsers($groupId) {
+		if(!$this->groupManager->groupExists($groupId)) {
+			throw new \Exception("$groupId ({$this->getNamespace()}) does not exist");


throw a OCA\Mail\Exception\ServiceException instead

ChristophWurst · 2018-03-12T09:29:10Z

+	 */
+	public function servicePrefix(IGroupService $gs) {
+		if(empty($gs->getNamespace())) {
+			throw new \Exception('GroupService has no namespace');


use ServiceException instead

ChristophWurst · 2018-03-12T09:29:53Z

 		$this->contactsIntegration = $this->getMockBuilder('\OCA\Mail\Service\ContactsIntegration')
 			->disableOriginalConstructor()
 			->getMock();
+		$this->groupsIntegration = $this->getMockBuilder('\OCA\Mail\Service\GroupsIntegration')


->createMock and the ::class notation

ChristophWurst · 2018-03-12T09:30:36Z

+	}
+
+	public function testGetUsersWrong() {
+		$this->expectException(\Exception::class);


needs update to ServiceException

ChristophWurst · 2018-03-12T09:32:41Z

+	 */
+	private $groupServices = [];
+
+	public function __construct(IGroupService ...$groupServices) {


I'd prefer to simply inject a single instance as long as we have only a single implementation. Other implementations can be injected later.

ChristophWurst · 2018-03-12T09:34:19Z

+	}
+
+	public function testExpandEmpty() {
+		$this->expectException(\Exception::class);


ServiceException

ChristophWurst · 2018-03-12T09:34:25Z

+	}
+
+	public function testExpandWrong2() {
+		$this->expectException(\Exception::class);


ServiceException

ChristophWurst

👍 @myrho thanks a lot! I'll try to take over and integrate this PR next week

ChristophWurst · 2018-03-13T09:31:51Z

+
+}
+
+?>


remove closing tag

ChristophWurst

Squashed the commits, rebased onto current master and fixed some coding style issues. Code looks good and manual tests confirmed that the feature works as expected. There are a few edge cases that are not handled well (like groups that have no members with email addresses set - horde will throw an exception because there is no recipient).

@myrho that can be fixed in follow-up pull requests. Thanks a lot for your work on this

ChristophWurst · 2018-03-19T10:36:29Z

Scrutinizer — 3 new issues

Should be addressed before merging.

ChristophWurst

Scrutinizer is happy now 😉

…-groups-i

…utocomplete-groups-i

ChristophWurst · 2018-03-20T18:44:16Z

cc @nextcloud/mail for review. Would be great to have a second person testing/reviewing this new feature

ChristophWurst · 2018-03-26T06:55:06Z

cc @nextcloud/mail for review. Would be great to have a second person testing/reviewing this new feature

ain't nobody got time for that.

Merging.

ChristophWurst · 2018-03-26T06:56:47Z

@myrho please don't forget to claim your well-deserved bounty for fixing #41 💰

alexanderdd · 2018-03-26T14:01:51Z

I am confused.

Please see my comment in #41

ChristophWurst requested changes Feb 27, 2018

View reviewed changes

ChristophWurst added 2. developing enhancement labels Feb 27, 2018

ChristophWurst added this to the 0.7.11 milestone Feb 27, 2018

ChristophWurst previously requested changes Feb 28, 2018

View reviewed changes

ChristophWurst requested changes Mar 12, 2018

View reviewed changes

ChristophWurst reviewed Mar 13, 2018

View reviewed changes

Comment thread lib/Service/Group/IGroupService.php Outdated

}

?>

ChristophWurst Mar 13, 2018 •

edited

Loading

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

remove closing tag

ChristophWurst self-assigned this Mar 13, 2018

ChristophWurst force-pushed the autocomplete-groups-i branch from 92ab7e7 to 539966a Compare March 19, 2018 08:21

Add ability to send to groups

1aa8a4d

ChristophWurst force-pushed the autocomplete-groups-i branch from 2d2c7b6 to 1aa8a4d Compare March 19, 2018 08:55

ChristophWurst approved these changes Mar 19, 2018

View reviewed changes

ChristophWurst added 3. to review and removed 2. developing labels Mar 19, 2018

ChristophWurst requested review from Gomez, jakobsack and jancborchardt March 19, 2018 08:58

ChristophWurst removed their assignment Mar 19, 2018

ChristophWurst added 2. developing and removed 3. to review labels Mar 19, 2018

ChristophWurst assigned myrho Mar 19, 2018

fix documentation, esp. return type

a79dc2f

ChristophWurst approved these changes Mar 19, 2018

View reviewed changes

ChristophWurst added 3. to review and removed 2. developing labels Mar 19, 2018

ChristophWurst added 2 commits March 20, 2018 19:41

Merge branch 'master' of github.com:/nextcloud/mail into autocomplete…

123350d

…-groups-i

Merge branch 'autocomplete-groups-i' of github.com:/myrho/mail into a…

b9756bb

…utocomplete-groups-i

ChristophWurst removed request for Gomez, jakobsack and jancborchardt March 26, 2018 06:55

ChristophWurst merged commit f55185a into nextcloud:master Mar 26, 2018

alexanderdd mentioned this pull request Mar 26, 2018

Sending to groups [$150 awarded] #41

Closed

ChristophWurst mentioned this pull request Apr 5, 2018

Send e-mail to all users or members of a user group #861

Closed

ChristophWurst mentioned this pull request Jun 18, 2018

Mail to Contact Group #936

Closed

wdfee mentioned this pull request Jul 18, 2019

Restrict sending emails to specific groups #1862

Open

bfritscher mentioned this pull request Dec 30, 2019

Feature contact group send #2414

Merged

ChristophWurst mentioned this pull request Apr 23, 2020

Use all available group services #2972

Merged


		namespace OCA\Mail\Service\Group;

		abstract class AbstractGroupService {

Uh oh!

Conversation

myrho commented Feb 27, 2018 • edited by ChristophWurst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

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!

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!

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!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

ChristophWurst Mar 13, 2018 • 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 commented Mar 19, 2018

Uh oh!

ChristophWurst left a comment

myrho commented Feb 27, 2018 •

edited by ChristophWurst

Loading

ChristophWurst Mar 13, 2018 •

edited

Loading