Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

(mosaico#247) (dev/core#588) Apply prefix to subject of test mailings #28

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

totten
Copy link
Member

@totten totten commented Dec 13, 2018

This is was previously fixed in civicrm/civicrm-core#12758, but that fix had a
couple undesired side-effect:

  • For non-Mosaico mailings, the prefix would be applied twice (https://lab.civicrm.org/dev/core/issues/588);
    once in the JS layer and once in the BAO delivery engine.
  • The saved value of the subject would temporarily store the prefix as part of the subject.
    (In normal usage, it would be fixed later by an autosave; but there could be edge-cases/races.)

This is an alternative fix which makes flexmailer more precisely imitate the
behavior in the BAO delivery engine.

Note: If one doesn't like this behavior, then you can disable or replace the service, e.g.

Civi::service('civi_flexmailer_test_prefix')->setActive(FALSE);

This is was previously fixed in civicrm/civicrm-core#12758, but that fix had a
couple undesired side-effect:

* For non-Mosaico mailings, the prefix would be applied twice (https://lab.civicrm.org/dev/core/issues/588);
  once in the JS layer and once in the BAO delivery engine.
* The saved value of the `subject` would temporarily store the prefix as part of the subject.
  (In normal usage, it would be fixed later by an autosave; but there could be edge-cases/races.)

This is an alternative fix which makes flexmailer more precisely imitate the
behavior in the BAO delivery engine.

Note: If one doesn't like this behavior, then you can disable or replace the service, e.g.

```php
Civi::service('civi_flexmailer_test_prefix')->setActive(FALSE);
```
@civibot civibot bot added the master label Dec 13, 2018
@totten
Copy link
Member Author

totten commented Dec 13, 2018

I've r-run this locally (along with a revert of 12758) and confirmed the expected subject lines for:

  • Normal mailing => Send test => specific email (with prefix)
  • Normal mailing => Send test => group (with prefix)
  • Normal mailing => Actual delivery (*without prefix)
  • Mosaico mailing => Send test => specific email (with prefix)
  • Mosaico mailing => Send test => group (with prefix)
  • Mosaico mailing => Actual delivery (*without prefix)

Copy link
Contributor

@seamuslee001 seamuslee001 left a comment

Choose a reason for hiding this comment

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

Changes look good

<?php
/*
+--------------------------------------------------------------------+
| CiviCRM version 4.7 |
Copy link
Contributor

Choose a reason for hiding this comment

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

@totten nitpcik this should be civicrm 5 and copyright to 2018

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. The copyright header should be changed. I'm inclined to do that a separate PR with a global search/replace.

@seamuslee001
Copy link
Contributor

@totten i have merged the other PR so i think this is good now

@totten
Copy link
Member Author

totten commented Dec 13, 2018

/test

@totten
Copy link
Member Author

totten commented Dec 13, 2018

Thanks, @seamuslee001.

Tests have passed here. Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants