Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accumulated backports for Mosaico support #9555

Closed
wants to merge 19 commits into from
Closed

Conversation

totten
Copy link
Member

@totten totten commented Dec 17, 2016

This branch contains a number backports to support the updated Mosaico extension on 4.6. There are a lot of them, so let me try to structure them a bit more:

I'm submitting this as a large branch/PR to facilitate both personal-usage and automated-testing of the combined work. However, I wouldn't expect all of these to go into canonical 4.6. Rather, I'll submit the first one (Mailing data model) because it feels like the most important.

If anyone is interested in engaging with testing/refinement of other specific batches/patches, I'll happily submit separate 4.6 PRs.

(Note: I've updated the description after merging batch 1and rebasing. If we need it, the old description is https://gist.github.com/totten/bce6aebdd5a30cbd372311cd98b253c8.)

--

@totten totten force-pushed the 4.6-19690 branch 2 times, most recently from c043e33 to 00f20ec Compare January 5, 2017 03:04
@totten totten changed the title WIP - CRM-19690 - CiviMail - Allow alternative email templating systems (backport #9497) Accumulated backports for Mosaico support Jan 5, 2017
This adds the TokenProcessor and associated classes to the container in 4.6.
These are not *used* at runtime -- but they're published so that extensions
may use them.

This folds in a few more recent patches to TokenProcessor subsystem provided
by civicrm#9497 and removes the parts defined for scheduled reminders.
…public

These two functions will be useful in FlexMailer's DefaultEngine.
… `static`

This function is only called once, and it previously lacked test coverage,
so the `static` warning previously snuck through. Fix it!
If you install an extension under `$cmsRoot/vendor/org.example.foo`, the
resulting URL contains an extraneous `/` (eg
`http://example.org//vendor/org.example.foo`).  (In Windows, I suspect it's
even worse because it uses DIRECTORY_SEPARATOR in the URl -- eg
`http://example.org/\vendor/org.example.foo`.)

This patch checks for and removes the extraneous slash -- and always
constructs the URL with the appropriate delimiter (`/`).

Problem observed in `dmaster`.
General notes:

 1. Template types can be declared (along with preferred editor) using
    hook_civicrm_mailingTemplateTypes.
 2. When creating a mailing, use the path `civicrm/a/#/mailing/new` to create
    a mailing with the default `template_type` (aka first-preferred, by weight).
 3. When creating a mailing, use the path `civicrm/a/#/mailing/new/{template_type}`
    to create a mailing with a specific `template_type`.
 4. When editing a mailing, check the `template_type` and load the
    appropriate editor.
 5. Some of hte boilerplate from `2step.html`, `unified.html`, etal has been
    moved to `base.html`.

Note that this breaks a hidden feature -- before, you could switch among a
few different layouts (`2step`, `unified`, `unified2`, `wizard`) by manually
editing the URL (e.g.  `civicrm/a/#/mailing/2/unified`). Now, to change
the layout of the traditional-style mailings, you can implement a hook, e.g.

```
function mymod_civicrm_mailingTemplateTypes(&$types) {
  foreach ($types as &$typeDef) {
    if ($typeDef['name'] === 'traditional') {
      $typeDef['editorUrl'] = '~/crmMailing/EditMailingCtrl/unified.html';
    }
  }
}
```
If you perform a contact search and create a new mailing, it would use
`template_type=traditional`, even if another template type had greater
priority. With this patch,  it respects the priority.

Note: I considered changing the default in Mailing.create API to always
match most-preferred template-type.  However, that would break some existing
API consumers (e.g.  headless consumers or ones who define their own UI).
For external API-based integrations, we should preserve the default
semantics of `body_text`/`body_html` by defaulting to
`template_type=traditional`.

The preference in CRM-19690 is that any use-case based on the screen
`civicrm/a/#/mailing/{id}` should have its default determined by weight.
FlexMailer (https://github.com/civicrm/org.civicrm.flexmailer/) is a
refactoring of the email-delivery logic from the Mailing BAOs.  The primary
goal is to make the email-delivery logic more extensible by exposing a
better set of events for extension-authors.  Sadly, the original code is a
bit toxic (originally lacking in tests; thick with many features; using some
quirky dataflows), which means:

 1. Any refactoring of it poses a high risk.
 2. The refactoring should ideally be done with iteration/validation as
    an optional extension.

This patch aims to be the bare-minimum core patch required to facilitate a
better 'leap by extension'.  The main priorities are:

 1. Minimize risk -- no impact on existing users who can continue using existing logic.
 2. Enable iteration/testing/deployment of an optional extension in real-world scenarios.
 3. Keep any core hacks clear and isolated - don't rashly commit to new, public APIs.
Note: These classes are not actually referenced by anything in core v4.6;
however, we provide so that extensions can use them.
The motivation here is to support these two different CLI commands:

```
cv dl myextension
cv dl myextension --dev
```

The two commands differ in that they pull from different feeds -- e.g.  the
stable feed `https://civicrm.org/extdir/ver={ver}|cms={uf}` vs the
developmental feed `https://civicrm.org/extdir/ver={ver}|cms={uf}|status=`.
However, this creates the real possibility that the user might go back/forth
among different feeds (omitting/enabling the `--dev` option per whim).
totten added a commit to totten/uk.co.vedaconsulting.mosaico that referenced this pull request Feb 25, 2017
To run on CiviCRM v4.6, you need to specifically use v4.6.26+ along with the
patches from [PR #9555](civicrm/civicrm-core#9555).
Although I do expect some folks will use the patches, it doesn't feel
accurate to list it for general consumption on 4.6 sites.

Note: You can enable/use extensions regardless of what's in the
`<compatibility>` / `<ver>` tags.  The tag's main purpose is to filter the
list of extensions advertised in the public directory.
@jackrabbithanna
Copy link
Contributor

I think we'll try to get this into 4.6.30

@jackrabbithanna
Copy link
Contributor

@totten we're taking a hard look at this for an upcoming release. How do you "feel" about it? Should it pretty much work, is it a WIP? @guanhuan any strong objections to having Mosaico in 4.6?

@guanhuan
Copy link
Contributor

guanhuan commented Aug 24, 2017

@jackrabbithanna I am not against it, in fact I think it will make sense to have it in 4.6 if it doesn't pose a threat to anything at all.

@jackrabbithanna
Copy link
Contributor

We still think this could get into 4.6, but not for the October release.... @eileenmcnaughton you'd prefer I close this now, and reopen later when we choose to start integration into 4.6 ?

@eileenmcnaughton
Copy link
Contributor

Yeah - let's do that! Let's try to keep open PRs to things that are progressing & just close & re-open more liberally

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

Successfully merging this pull request may close these issues.

5 participants