dev/mail#90 - Allow re-use mailing on draft mailings and adhoc mailings

[larssandergreen]

See also : https://lab.civicrm.org/dev/mail/-/issues/90

Before

• "Re-use" was shown only on scheduled/sent and archived mailings pages.
• "Re-use" was shown only on mailings with standard groups (omitting maliings that used adhoc targets).

After

• "Re-use" is also shown on draft/unscheduled mailings (i.e. on all mailings).
• "Re-use" is also shown on mailings with adhoc targets.
• The list of actions has been reordered, so that "Continue" comes before "Re-use"/"Copy".
  
  
• The action "Re-use" has been renamed to "Copy".

NOTE: Description updated April 19 to indicate that the PR also affects adhoc mailings.

[civicrm-builder]

Can one of the admins verify this patch?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

add to whitelist

[totten]

On a functionality level, I think this is a good idea. 👍

• General standards
  • (r-explain) Pass
  • (r-user) Discuss:
    • I was looking at two buttons ("Continue", "Re-use") and clicked "Re-use". After clicking, I wasn't really sure that it had made a copy. (Basically, the UX of clicking "Continue" or "Re-use" is exactly the same.) I had to put on my developer-hat, look at the URL to see the mailing ID#, and reassure myself. It would be nice to have some cue -- like changing the mailing.name ("Copy of {$oldName}") or showing a status-alert (CRM_Core_Session::setStatus(...ts('Created a new copy of mailing "%1"', ...)...)).
  • (r-doc) Pass
  • (r-run) Pass
• Developer standards
  • (r-tech) Pass
  • (r-code) Pass: Looks pretty clean and nicely matches other code in the same context.
  • (r-maint) Pass
  • (r-test) Pass

[totten]

How hard would it be to make this label adapt?

This is maybe a bit nitpicky, but "Re-Use" suggests that the mailing has been "used" before. A draft hasn't been used before.

If it's not too hard, maybe something like this would be more apropos:

  'name' => $thisItemIsADraft ? ts('Copy') : ts('Re-Use'),

[larssandergreen]

Agreed, I suggested in the issue changing the text to "Copy". My thought is that it would be better to keep the label consistent for all mailings and just change it to "Copy" throughout. Otherwise, users may wonder what the difference is between "Copy" and "Re-use". Copy is also consistent with events, contribution pages, price sets, etc which all use some variation of Copy, Make a Copy, Copy Price Set, etc.

[totten]

Yup. "Copy" is a small UI change in near term (and we might want to tweak the docs after this PR), but I think you're right - "Copy" is more consistent in the big picture. It also matches the Copy of ... text.

[magnolia61]

love this! before you had to sent a mailing in order to reuse it. Nice one :-)

[mlutfy]

+1 on this, I often had to use the hidden URLs to do this.

Warning to those using multi-lingual and using this process to translate mailings: https://lab.civicrm.org/dev/mail/-/issues/87

[larssandergreen]

@totten Yes, good idea to give the user some feedback to indicate this is a copy. I think adding "- Copy" or something like that to the mailing name makes the most sense and is consistent with how this works for events, price sets, contribution pages, etc.
But how the mailing copy works is a bit beyond me right now, so someone would need to point me in the right direction. I see how this works for price sets, for example, but I don't see anything analogous for mailings.

[totten]

But how the mailing copy works is a bit beyond me right now, so someone would need to point me in the right direction.

Good question. It can be tricky to trace the actions for some of these forms. Here's how one can locate this one:

• The link declared 'url' => 'civicrm/mailing/send',, so that's our focal point.

• Traditionally, I would've just done a global text search for civicrm/mailing/send. Most IDEs have some hotkey for this, but I'm a CLI fan, so I'd do grep:

  $ grep -r civicrm/mailing/send CRM/
  CRM/Core/Block.php:          'path' => 'civicrm/mailing/send',
  CRM/Utils/Token.php:        $value = CRM_Utils_System::url('civicrm/mailing/send',
  CRM/Mailing/Selector/Browse.php:          'url' => 'civicrm/mailing/send',
  CRM/Mailing/Selector/Browse.php:          'url' => 'civicrm/mailing/send',
  CRM/Mailing/xml/Menu/Mailing.xml:    <path>civicrm/mailing/send</path>
  CRM/Mailing/Controller/Send.php:    // New:            civicrm/mailing/send?reset=1
  CRM/Mailing/Controller/Send.php:    // Re-use:         civicrm/mailing/send?reset=1&mid=%%mid%%
  CRM/Mailing/Controller/Send.php:    // Continue:       civicrm/mailing/send?reset=1&mid=%%mid%%&continue=true
  

  The key result is the "Menu" XML file, which declares that <path>civicrm/mailing/send</path> corresponds to <page_callback>CRM_Mailing_Controller_Send</page_callback>.

• Alternatively, instead of a text-search, now-a-days I'll do:

  $ cv api4 Route.get +w path=civicrm/mailing/send
  [
      {
          "path": "civicrm/mailing/send",
          "title": "New Mailing",
          "page_callback": "CRM_Mailing_Controller_Send",
  ...
  

• Either way, we land at CRM_Mailing_Controller_Send. The key bit appears to be:

  civicrm-core/CRM/Mailing/Controller/Send.php

  Lines 50 to 53 in bc77d7c

  	if ($mid && !$continue) {
  	$clone = civicrm_api3('Mailing', 'clone', ['id' => $mid]);
  	CRM_Utils_System::redirect(CRM_Utils_System::url('civicrm/a/', NULL, TRUE, '/mailing/' . $clone['id']));
  	}

The easiest thing is to probably to keep the call to civicrm_api3('Mailing', 'clone', ...). Afterward, write the new name, call CRM_Core_Session::setStatus(), and then redirect() at the end.

(NOTE: The $clone variable has all the info you need to make a change - e.g. $clone['id'] and $clone['values'][...]['name']. Just call civicrm_api3('Mailing', 'create', ...) with the new id and the preferred name.)

[larssandergreen]

Thanks @totten, that helped a lot. Now adds "Copy of" to the cloned mailing.

What are your thoughts on changing to "Copy" instead of "Re-use" on all mailing pages (see note above)?

[totten]

Re-tested and still looks good. If you can update the label to "Copy", that'll be perfect. Flagging "merge ready" (ie it safe to merge now, but we can wait a day or so in case you push the update).

[larssandergreen]

Changed Re-use to Copy here and added a documentation pull request for the same.

[larssandergreen]

Test failure is a different contact checksum from a token.

[larssandergreen]

jenkins, test this please

[larssandergreen]

@ufundo has helped me figure out that this patch does something else as well: Before, mailings created from searches could not be re-used/copied. Now, they can be copied, the same as any other mailing.
That's nice, but I just want to check if there is any reason we should be preventing mailings from search results from being copied. Is the concern that the hidden smart group will no longer be present?

[totten]

Yeah, that's a bit curious - why would it block Copy/Re-use of search-based mailings (CRM_Mailing_Form_Task_AdhocMailing; aka mailings with hidden groups; aka mailings where the targets are group_type=Base)?

I agree with @ufundo @larssandergreen that this seems like an improvement. It is a fairly distinct change, so I've updated the r-explain.

I'm not sure why it was hidden, but - as an exercise in due diligence - I can imagine two plausible reasons. (Neither seems particularly compelling to me right now.) They are:

1. Adhoc mailings require special bits in the editor UI (e.g. handling of the "Recipients" field and the "Unsubscribe" field). This has evolved over time, and it's fairly isolated in Civi 5.x (it's only two fields that change), and these two fields seem to be working. But circa Civi 2.x/3.x (when this was added), the whole screen worked differently, and it may have been buggy if you tried to re-use an adhoc mailing.
2. If you think of the mailing as "ad hoc", then the selection of contacts is a fleeting decision of the moment. The list of recipients that you chose on Tuesday may no longer be correct for a future mailing on Friday.
   • (a) Example: You search for event participants and cherry-pick a few who you know to be donors. Then you send the first message. Two weeks later, you copy/reuse and send another message. But in the intervening time, the various statuses (registered participants; existing donors) may have changed. If you re-use the mailing, then you're acting with stale list of contacts.
   • (b) Counter-example: You had an informal chat with a group of people after a meetup. You decide to send those people a follow-up by email. Two weeks later, you send them another follow-up message. In this case, you might re-use the mailing to avoid needing to re-select people.
   • Opinion: I think (2b) is legit enough. It's true that (2a) could be mis-use, but even with (2a)... there may be legit reasons to re-use the original target-list. It really depends on what/why you're communicating. The main thing is to communicate clearly so that the user makes an informed decision about copying/reusing values.

Aside: When you actually re-use a mailing, you see this "Hidden group NNN", which is mysterious. But you can inspect/drill-down on the full "recipients" list, so I don't think it's a hard block. (Maybe a label like "Adhoc group NNN" would be a little more apropos... but that's getting really outside the scope of the issue.)

Anyway, this still looks fine. Merging.

[larssandergreen]

Great, thanks @totten. Regarding the hidden smart group aside, I actually have an issue open about this. With help from @tschuettler and some testing and digging through the code , it looks like the the hidden group can in fact be smart or not smart, so you could actually make a hidden smart group which would presumably update for the next mailing (but I certainly could be wrong about the next mailing case in particular).
For some reason, the hidden smart group isn't updated between when you create the mailing and when you send it or come back to it later - unless you clear the smart group cache manually or via cron. Normal smart groups don't behave that way. I don't know why these are different and don't really know the first place to start with this one. But if you have thoughts, that might help clarify what is happening here.