Details
Major
Description
Users may have any mix of these three permissions:
- Create mailing
- Schedule mailing
- Approve mailing
Generally, the approval flow is driven driven through Drupal rules. For example, user A creates a mailing and submits. A notification is sent to user B who reviews it and schedules. Then another notification is sent to user C who reviews and approves.
There's some preliminary support for the permissions in the new Mailing API, but we need several more pieces to support this use-case, e.g.
- The frontend needs to adapt based on permissions+status – hiding and showing various sections, e.g.
- In the wizard, hide/show an "Approval" step depending on whether permission "approve mailings" is granted.
- We need a way for client-side code to assess permissions. (Note: This is emphatically a complement and not a substitute for permissions in the API.) An adhoc call to addSetting() is probably the expedient thing. (Although.. it's really tempting to a clever scanning/preloading trick like we do with ts().)
- The interface for Mailing.submit API probably needs revision.
- Check that Drupal Rules still fires as expected.
- Check for a link in the "Scheduled and Sent Mailings" that may be pointing to standalone page. Make it something sensible.
Attachments
Activity
Discussed with dgg on IRC, and it seemed simpler to change the UI a bit rather than change the API, so that's what we did.
PR: https://github.com/civicrm/civicrm-core/pull/5275
This sometimes uses the old UI (e.g. if separate people have the permissions "schedule mailings" and "approve mailings") but sometimes uses the new UI (if one person has both permissions).
I'm not a big fan of either the old UX or this new UX, but this seems to reach the level of "don't totally break the approval feature".
Tim - spent 3 hours on QA (ignored the test failures in Jenkins and patched my local). Here's my results. I think only a few of the
{BUG}s are important enough to bother with - but don't know effort for fixes.=============
Approval Workflow
===============
Admin - c, s, a
Manager - a : bobc
Staff - c, s : Tim
Volunteer - c : Jamie
Scheduler - s : alan
QA Results
==========
User permissions: create mailings (only)
New mailing - UI has Content and Options {BUG?} - UI works as expected BUT all roles seem to require “skip IDS check” permission in order to save a mailing. Otherwise I see:
====
Sorry an error has occurred => “There is a validation error with your HTML input. Your activity is a bit suspicious … “
====
Permissions: create AND schedule mailings
Continue to Schedule a created mailing.
- UI is fine to do Schedule action{BUG?} - After scheduling, user sees Approve/Reject action in selector which they probably shouldn’t see. Clicking this link gives the expected result: “Access denied”.
Permissions: schedule mailings (only)
Continue to Schedule a created mailing.
- UI is fine to do Schedule action {BUG?} - After scheduling, user sees Approve/Reject action in selector which they probably shouldn’t see. Clicking this link gives the expected result: “Access denied”.
Permissions: approve mailings (only)
Go to Scheduled Mailings and click Approve / Reject action link
- UI loads expected Approve / Reject form. Approving gives expected results.{BUG}
- Preview Mailing pane on Approve / Reject screen is empty (no content). This is probably a significant issue since the approver needs to see what they are approving. (User with create, schedule, approve does see content there, so permissions issue on preview pane content).
{BUG} Click “continue” to view an Unscheduled mailing- UI loads Previous (active) / Next (inactive) but no other content. Clicking Previous doesn’t do anything.
- Save Draft button action creates another mailing
{BUG}
Click ‘New Mailing’ from menu or Find Mailings
- UI loads Previous (active) / Next (inactive) but no other content. Clicking Previous doesn’t do anything.
- Save Draft button action creates another mailing
Permissions: create, schedule and approve
- New mailing flow works as expected
NOTES:
1.
The Send Test Mailing to a Group seems problematic if exposed to a user w/o APPROVE permission - since they could pick a normal mailing list and just send it out. Need to see how that’s handled in older UI.
2. Reject action seems to clear Scheduled date and puts the mailing ‘back’ to where it can be worked on and scheduled again. This seems like appropriate behavior.
Re: BUG? #1 – IDS – Did you delete Config.IDS.ini? The PR included a change to how this file is generated.
(Note: I believe all the remaining bugs/issues are pre-existing - not regressions from the rewrite.)
Re: BUG? #2+3 – Approve/Reject in selector – Skimming the code in CRM_Mailing_Selector_Browse::getRows, it looks like permission "access CiviMail" implies "$allAccess" which makes the link show up. There seem to be inconsistent about ideas whether to treat "access CiviMail" as a total/overarching permission (which implies the other perms) or as a baseline permission (a pre-requisite for the others).
Re: BUG? #4 – Preview Mailing is empty – I see the expected content in there. FWIW, I'm testing with an HTML-based mailing. The user has "approve:YES,create:NO,schedule:NO,access:YES". (If I revoke "access CiviMail", then preview shows an access-denied error - but it's not empty.)
Re: BUG? #5/6 – New/Continue Mailing is empty – This seems like another variation on confusion over "access CiviMail". It doesn't make much sense for someone with "approve:YES,create:NO,schedule:NO" to go to the composition screen. You get a more sensible UX with "approve:YES,access:NO".
Let's chat over IRC about how to handle "access CiviMail".
Ooops, missed one.
Re: BUG #7 (err, #1) – Send Test Mailing to a Group – Tough to say what to do. Seems like ideally one wants different permissions for different groups (e.g. allowing sending to small/internal/test groups but not to large/constituent/real groups). FWIW, the old UI includes the option to send to a test group with "create:YES,schedule:NO,approve:NO,access:NO". IMHO: it's doing what it used to do, so it's fine.
Aside for the record - we're trying to keep track of hours on this one, but there were a number of hours spent during the main design/implementation on this topic. We might say we're missing 4-10hr.
Discussed permissions and A/B test with dgg. To keep the design+QA simple, we'll just require 'access CiviMail' for A/B tests. It's patchwelcome to add more sophisticated permissions.
Tim - just to get clear on this, did the extra permission described in OP Create mailing / Schedule mailing / Approve mailing not make it? Never seen them on a 4.6 install
peter davis, yes we did a lot of work during the 4.6.alpha/beta cycle to get those permissions working.
However, you are also correct that it never worked in a real 4.6 install. After the last 4.6.beta (but before 4.6.0), there was a small refactoring of permissions which broke it.
Thanks for the pointer to the other ticket. This may be coming on our radar. At least for Create v Create and Schedule
Initially, I added a CiviMail layout which adapts based on the workflow and permissions. The UI elements in "edit-workflow.html" were basically:
A. Wizard
1. Content (show if permission 'create mailings')
2. Options (show if permission 'create mailings')
3. Schedule (show if permission 'schedule mailings')
4. Approve (show if permission 'approve mailings')
I ran into trouble when thinking about how to wire up actions after the "Schedule" and "Approve" steps given different combinations of permissions, and I think this is because the above approach actually improves flexibility – i.e. a user with suitable permissions could go to the 'Approve' step, then go back to 'Content' to make a change, and then return to 'Approve'. In <=v4.5, this wasn't allowed.
I believe this wasn't allowed because the workflow stuff was stitched into CiviMail after-the-fact, tried to work-around some other weird artifacts in the BAO, and ultimately produced a kind'a weird data-model.
Trying to change that might be annoying, so... I figured... maybe I can just enforce the same constraints (i.e. after submission but before approval, disallow editing). This was fairly simply change to edit-workflow.html:
A. Wizard (show if mailing is unsubmitted)
1. Content (show if permission 'create mailings')
2. Options (show if permission 'create mailings')
3. Schedule (show if permission 'schedule mailings')
B. Accordion "Approve" (show if mailing is submitted && permission 'approve mailings')
This approach leads to some bugs (arguably in API+Angular side, but it's hard to say).
Now I'm going back and forth - it feels like any path requires more hours of work, and there are a couple paths that feel divergent:
PATH A: Cleanup. CRUD API.
PATH B: Enshrine the workflow.
Anyway, probably best to sleep on this and take another look with fresher eyes.