(REF; dev/core#873) MailingAB - Migrate "copy winner" logic from JS to PHP #14045
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a "winner" is selected for an A/B test, several fields must be copied over. This provides the "copy" logic on the server-side. Before ------- The copy logic is only available in Javascript, so it's hard to reference in a unit-test. After ----- The copy logic is available as an extra option for the MailingAB.submit API. By default, the API works as before. If you add the option `winner_id`, then it will also copy-over the details from the winning experiment.
…ilingMgr.mergeInto` In this context, the user has run an A/B experiment and chosen a specific variant (the "winner") to use in the full blast. The purpose of the code is to copy the "winner" content to the final mailing - and send the final mailing. This replaces some client-side logic with a server-side equivalent.
|
(Standard links)
|
|
@seamuslee001 are you in a position to test this? The bulk of the change is unit tests :-) |
|
Code makes sense & unit tests are solid - I understand you have work building on this. I discussed with @seamuslee001 who also looked through the code. Merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
In CiviMail, a user may run an A/B test, compare the performance of each variant, and choose a "winner". The mailing details (
subject,body_text, etc) are copied from the winner to the full, final mailing.This refactoring does not seek to change the usage or behavior. It just moves the "copy winner" logic from in-situ JS to a PHP API. It aims to allow better unit-testing and to allow more alternative workflows.
Before
crmMailingMgr.mergeInto(finalMailing, winnerMailing...)).After
MailingAB.submit winner_id=...)Mailing.createto save any changes. But the client-side still winds up with the right data because it reloads after submission.)Comments
The new test-coverage is for the new API parameter. To show that it has the overall intended effect (e.g. old code and new code produce similar outcomes for user), I used this procedure:
mailcatcherorfakesmtp)Job.process_mailings. Check that a subset of people receive both mailings.Job.process_mailings