The grouping methods within a category have been designed to be plugins.
When a custom plugin is added , the recipients across the grouping methods in a category are not being identified properly.

For example, with the current role grouping method and a custom grouping method based on say user creation date, currently
the code gathers all users with the role AND the users satisfying the user creation date condition.

But what is expected is that users that have the chosen role, whose user creation date satisfies the condition chosen should be gathered,

Also a check needs to be added on the category form that a value has been chosen for atleast one grouping method.

CommentFileSizeAuthor
#9 interdiff.txt4.36 KBsukanya.ramakrishnan
#9 2885074-multiple-grouping-methods-issue-09.patch5.44 KBsukanya.ramakrishnan
#4 interdiff.txt518 bytessukanya.ramakrishnan
#4 2885074-multiple-grouping-methods-issue-04.patch3.75 KBsukanya.ramakrishnan
#2 2885074-multiple-grouping-methods-issue-02.patch3.79 KBsukanya.ramakrishnan

Comments

sukanya.ramakrishnan created an issue. See original summary.

sukanya.ramakrishnan’s picture

sukanya.ramakrishnan commented
StatusFileSize
new 2885074-multiple-grouping-methods-issue-02.patch3.79 KB

Submitting a patch for this issue!

Thanks,
Sukanya

sukanya.ramakrishnan’s picture

sukanya.ramakrishnan commented
Status: Active » Needs review
sukanya.ramakrishnan’s picture

sukanya.ramakrishnan commented
Issue summary: View changes
StatusFileSize
new 2885074-multiple-grouping-methods-issue-04.patch3.75 KB
new interdiff.txt518 bytes

Changed the message when no recipient types are chosen on the category form to a simpler one. Submitting a patch and interdiff from #2

Status: Needs review » Needs work

The last submitted patch, 4: 2885074-multiple-grouping-methods-issue-04.patch, failed testing. View results

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented 
  1. +++ b/src/Form/CategoryForm.php
@@ -94,6 +94,25 @@ class CategoryForm extends EntityForm {
+      $form_state->setErrorByName('', $this->t('Atleast one recipient is required.'), 'error');

    Typo here in 'Atleast' :) 

  2. +++ b/src/MassContact.php
@@ -231,10 +231,22 @@ class MassContact implements MassContactInterface {
+            $category_recipients[$plugin_id] = $this->getAllActiveUsers();

    I thought it wasn't possible to create categories without recipients... When it says some other grouping should have categories, then why do we need to select all users here?

sukanya.ramakrishnan’s picture

sukanya.ramakrishnan commented

@jhedstrom. Thanks for your comments.

While it is correct that we cant create categories without recipients, there can be a grouping in a category without any selections.
The getAllActiveUsers() is needed because if we intersect an empty grouping with another grouping with selections, we are going to get an empty resultset.

sukanya.ramakrishnan’s picture

sukanya.ramakrishnan commented

Hmm, now i think a better approach would be to just not add the recipients before doing the intersect, as simple as that.

Will fix that :)

sukanya.ramakrishnan’s picture

sukanya.ramakrishnan commented
StatusFileSize
new 2885074-multiple-grouping-methods-issue-09.patch5.44 KB
new interdiff.txt4.36 KB

Submitting patch based ob jhedstrom's review. Also added a test for the categoryform to ensure that there is atleast one recipient selected.

Thanks,
Sukanya

sukanya.ramakrishnan’s picture

sukanya.ramakrishnan commented
Status: Needs work » Needs review

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Fixed

Awesome! Also, thanks for the test coverage!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.