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

Add A/B testing for "From:" address #648

Merged
merged 6 commits into from
May 6, 2024

Conversation

totten
Copy link
Collaborator

@totten totten commented May 1, 2024

Overview

Expands support for A/B testing.

ping @kurund

Before

  • You may add an alternate "Subject" and/or "Design". This enables A/B testing. (By default, it copies your current subject/design.)

After

  • You may add an alternate "From", "Subject", and/or "Design". This enables A/B testing. (By default, it copies your current subject/design.)

  • The "New Mailing" screen starts with one "From" field:
    Screenshot 2024-04-30 at 4 43 01 PM

  • If you click "+", it adds another "From" field:
    Screenshot 2024-04-30 at 4 42 49 PM

Comments

In my r-run, it does generate messages with the alternate "From" addresses. (This is expected, given that AbDemux anticipates variations on these fields. I added a bit more coverage in AbDemuxTest to show this.)

Here are a couple issues to review/consider before merging....

  1. Select2 Widget:
    • The "From" widget was previously declared with select2 (crm-ui-select="{dropdownAutoWidth : true, allowClear: false}").
    • But I'm finding it hard to figure out markup which meets all these goals: (a) be simple/standard, (b) render with a reasonable width, (c) display the dropdown alongside the "+" button, and (d) use select2.
    • For the moment, I've got it using a standard <select>, which satisfies (a)+(b)+(c) but not (d).
    • Probably someone with more select2/css experience could make select2 work.
  2. Reply-To Edge-Cases:
    • Each Mailing record has three related fields. Typical values look like this:
      {
       "from_name": "Albert Albertson",
       "from_email": "albert@example.org",
       "replyto_email": "\"Albert Albertson\" <albert@example.org>",
      }
    • This data-model is weird. The pre-existing web UI hides the weirdness in a couple ways. First, it simulates a single UI widget ("From") based on two API fields (from_name,from_email). Second, it may autofill replyto_email... but it depends on a configuration option (enableReplyTo aka replyTo):
      • If replyTo is enabled, then it displays "Reply-To" as a separate field (independent of "From").
      • If replyTo is disabled, then it quietly autofills replyto_email based on from_name/from_email.
    • This patch should preserve these behaviors, as long as the setting enableReplyTo remains static (which would be true in normal/day-to-day usage).
    • However, if you allow replyTo to get reconfigured, then existing data may get pulled into some funny edge-case. (Ex: Create a mailing; split the "From" values; save draft; change the setting; resume editing.) I have not tested these edges. Guess: Some would feel OK; others would feel wrong (esp when the variants defines 2x replyto_email values, but the UI wants to show one "Reply-To" widget);
    • I think there are a few options, but I'm not a big fan of any of them.
      • Leave be. It'll work in normal usage.
      • Add A/B support for the "Reply-To" field (for systems with enableReplyTo=true). More verbose code. Not a feature anyone particularly asked for. But probably the best functionality.
      • Move all responsibility for sync'ing replyto_email<=>from_name+from_email to a different layer. (Only sync when you submit.) Eventual code should still be concise. (But it feels a bit more risky -- I suspect there's some reason why it was put in its current spot.)
    • The lazy part of me likes the first option more. The maintainer part of me likes the second option more.

totten added 5 commits April 30, 2024 14:58
This maybe isn't so compelling as a feature.  But we kinda get boxed-in,
because the "From" field gets "A/B", and there's conditional sync'ing
between "From"<=>"Reply-To" (based on the `replyTo` setting).

This makes it safer to handle reconfigurations of `replyTo`, as in:

1. Start with replyTo=false (default)
2. Make a mailing. Split the "From". Save draft.
3. Reconfigure replyTo=true
4. Resume draft or re-use mailing with A/B test on "From"

Without this patch, the UI would present a singular "Reply-To", but the
underlying "variants.X.replyto_email" would have two (invisible/uneditable)
overrides.

With this patch, in that scenario, the values of "variants.X.replyto_email"
would be apparent.
@totten
Copy link
Collaborator Author

totten commented May 1, 2024

  • Well, I added support for A/B test of "Reply-To", which should make some of the edge-cases more agreeable. (Described in commit comment -- alas, a workflow with the opposite toggling would still have a quirk...)
  • As with the "From:" field, it has the issue with <select> / select2 / width / wrapping. (This consideration is aggravated a bit more by the fact that... the default value of replyto_email on a new mailing doesn't actually appear in the list of email-address options. Presumably, we never noticed because select2 is more forgiving about that... Erg...)

@kurund
Copy link
Contributor

kurund commented May 1, 2024

@totten

Thanks, I will check and get back to you.

@totten
Copy link
Collaborator Author

totten commented May 2, 2024

Chatted a bit with Coleman about select2 widths and found an incantation for something workable. ✅

Copy link
Contributor

@kurund kurund left a comment

Choose a reason for hiding this comment

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

@totten

Tested the functionality and is working as expected. Thank you. Good to merge.

@mattwire mattwire merged commit a464630 into veda-consulting-company:2.x May 6, 2024
1 check passed
@totten totten deleted the 2.x-ab-from branch May 6, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants