Bug/double opt in triggered despite being disabled in settings #115

MaxwellGarceau · 2025-01-09T01:01:44Z

Description of the Change

This PR bundles a two related issues.

Double Opt-In triggered despite being disabled in settings: Fixes a bug causing double opt-in to occur every time regardless of Mailchimp WP settings on new user sign ups
Existing subscriber can sign up even if "Update Existing Subscriber?" is turned off: Fixed a bug that caused the "Update Existing Subscriber?" setting to essentially always return as true. This caused the FE Mailchimp WP form to always return a success when subscribers would sign up with the same email.

Closes #113
Closes #114

How to test the Change

Double Opt-In triggered despite being disabled in settings

double-opt-in-triggered-despite-being-disabled-testing.mp4

Ensure that Double Opt-In is disabled in the Mailchimp WP settings page
Navigate to the FE and submit a new form submission with an email address you control (e.g. max.garceau+newsubmission@10up.com)
Check your email and ensure that you haven't received an email
Check your Mailchimp account to ensure the contact has been created

If Double Opt-In is enabled and a `pending` user tries to update their information, then their status will still be `pending` and they will have to confirm the original confirmation email (bug fix/logic change)

Previously, the conditional check was attempting to skip Double Opt-in for any emails with an existing status.

double-opt-in-enabled-with-pending-user.mp4

Turn on Double Opt-In in the Mailchimp WP settings
Turn on "Update Existing Subscribers?" in the Mailchimp WP settings
Sign up with a new subscriber
DO NOT CONFIRM THE EMAIL
Check your Mailchimp account to confirm that the contact has not been created
Sign up and change some information with that same email
You WILL NOT receive another confirmation email. Confirm the original confirmation email.
Verify that the new contact is created in the Mailchimp account with the changes you've entered

Fix: Existing subscriber could sign up even if "Update Existing Subscriber?" is turned off

update-existing-subscriber-is-off-should-prevent-subscriber-from-signing-up-multiple-times.mp4

Turn "Update Existing Subscriber?" off in the Mailchimp WP settings
Sign up with a new subscriber on the FE submission form
Sign up a second time
You should receive an error stating "This email address has already been subscribed to this list"

If "Update Existing Subscriber?" is disabled and the email does not belong to a new subscriber then don't let them sign up (changed logic)

Previously, the conditional check would only check for subscribed emails to throw an error. Now, any existing email type will throw the error.

existing-emails-can-not-signup-if-update-existing-subscribers-is-not-enabled.mp4

Turn "Update Existing Subscriber?" off in the Mailchimp WP settings
subscribed - Sign up with an already subscribed email on the FE submission form. You should see "This email address has already been subscribed to this list."
unsubscribed - Sign up with an email that has been unsubscribed from this list. You should see "This email address has already been subscribed to this list."
Enable Double Opt-In
Sign up with a new email
DO NOT CONFIRM THE EMAIL
pending - Try signing up again with the email. You should see "This email address has already been subscribed to this list."

Other statuses include cleaned, transactional, and archived, but I don't think we need to worry about testing those.

Changelog Entry

Fixed - Fix bug causing all new subscribers to receive a double opt-in email
Fixed - Fix bug causing any contact with a Mailchimp status (subscribed, unsubscribed, pending, etc) to be able to submit the sign up form even if "Update Existing Subscriber?" was disabled
Fixed - Pending contacts will now still be required to confirm their original confirmation email if they try to update their contact while "Update Existing Subscribers?" is enabled and "Double Opt-in" is enabled

Credits

Props Nathan Tetzlaff, @MaxwellGarceau, pluckvermont.

Checklist:

I agree to follow this project's Code of Conduct.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests pass.

Remove the user status from double opt-in since it's not clear that logic affects why a user should receive a confirmation email or not

…sting user check get_option( 'mc_update_existing' ) returns '1' or '', but never false

…g subscribers is off

MaxwellGarceau · 2025-01-09T01:04:27Z

mailchimp.php

+	// If update existing is turned off and the subscriber is not new, error out.
+	$is_new_subscriber = false === $status;
+	if ( ! get_option( 'mc_update_existing' ) && ! $is_new_subscriber ) {
+		$msg   = esc_html__( 'This email address has already been subscribed to this list.', 'mailchimp' );


‼️ ‼️ I changed the functionality slightly here according to my interpretation of the use case.

If "Update Existing Subscriber?" is disabled and the email does not belong to a new subscriber then don't let them sign up.

The previously functionality only checked for emails with a subscribed status. This missed unsubscribed, pending, and the other possible Mailchimp statuses.

‼️ Question: Do we want to update unsubscribed and pending users as well? If we don't update unsubscribed users then it will be functionally impossible for any user to resubscribe once unsubscribed. This directly relates to #93.

I do think we'll want to allow other statuses to work here besides just subscribed. As a user, I think I'd consider it a bug if someone that has unsubscribed can't resubscribe if that setting is on. The way I think this should work is if someone is subscribed or is in a pending state (I would assume that means they subscribed but haven't verified that yet), we shouldn't allow them to resubscribe if this option is enabled. But if they have never subscribed or if they have unsubscribed, they should be allowed to subscribe

The way I think this should work is if someone is subscribed or is in a pending state, we shouldn't allow them to resubscribe if this option is enabled mc_update_existing

I 100% agree with you.

In the case of subscribed, the sign up request will update the existing contact instead of creating a duplicate.

In the case of pending (subscribed, but haven't verified), their information will also be updated. However, the user must still confirm the email before the contact is created. The exception to this is if single opt-in has been enabled then the pending contact will be created immediately without needing to verify the original email.

[If mc_update_existing is enabled or disabled] But if they have never subscribed they should be allowed to subscribe

Yes! Current functionality.

[If mc_update_existing is disabled] If they have unsubscribed they should be allowed to subscribe

I agree as well. The current Mailchimp functionality is that contacts can not be resubscribed through the API. However, there's a solution in #117 to still address this scenario.

I added a comment directly after "Update Existing Subscriber?" check to make an API request for the native Mailchimp sign up form if the contact has a status of unsubscribed and "Update Existing Subscriber?" is enabled.

wordpress/mailchimp.php

Lines 916 to 917 in 8e1e3f4

// TODO: If get_option( 'mc_update_existing' ) && 'unsubscribed' === $status then

// make an API request to fetch Mailchimp hosted sign up form and display to user

mailchimp.php

MaxwellGarceau · 2025-01-09T04:18:54Z

Some of the logic around setting a subscribers status as well as what type of subscribers should be allowed to update their contact info in Mailchimp are a bit hard to remember. I think those would be good candidates for unit tests. What does everyone else think?

dkotter · 2025-01-09T15:55:16Z

I think those would be good candidates for unit tests. What does everyone else think?

I'm fine with adding unit tests here

MaxwellGarceau · 2025-01-10T23:20:21Z

Update on the unit tests. I've been prioritizing wrapping up other in progress tickets. The unit tests may not be ready before I move onto another project next week.

MaxwellGarceau added 7 commits January 8, 2025 18:53

Ignore vscode settings

fd66b47

Update comments to clarify functionality

e38f0b0

Early return subscribed users

e0b5cc5

Make double opt-in the only factor in a confirmation email

2865c28

Remove the user status from double opt-in since it's not clear that logic affects why a user should receive a confirmation email or not

Add comments

f7e0992

Fix bug causing existing users to sign up regardless of subscribe exi…

3fea7b1

…sting user check get_option( 'mc_update_existing' ) returns '1' or '', but never false

Restrict any previous subscriber from signing up while update existin…

bb3b577

…g subscribers is off

github-actions bot assigned MaxwellGarceau Jan 9, 2025

github-actions bot added this to the 1.7.0 milestone Jan 9, 2025

MaxwellGarceau commented Jan 9, 2025

View reviewed changes

MaxwellGarceau added 2 commits January 8, 2025 20:36

Add location where Mailchimp sign up form should display

f30dc64

Clarified logic for double opt-in

01c00b9

MaxwellGarceau commented Jan 9, 2025

View reviewed changes

mailchimp.php Show resolved Hide resolved

MaxwellGarceau added 2 commits January 8, 2025 22:08

Simplify todo note

6181802

Fix lint errors

8e1e3f4

MaxwellGarceau marked this pull request as ready for review January 9, 2025 04:08

github-actions bot added the needs:code-review This requires code review. label Jan 9, 2025

MaxwellGarceau requested a review from dkotter January 9, 2025 04:08

jeffpaul requested a review from iamdharmesh January 9, 2025 15:15

jeffpaul mentioned this pull request Jan 9, 2025

Mailchimp WP Double Opt-In settings are not syncronized with Mailchimp account Double Opt-In settings #116

Open

1 task

dkotter approved these changes Jan 10, 2025

View reviewed changes

dkotter requested a review from qasumitbagthariya January 10, 2025 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/double opt in triggered despite being disabled in settings #115

Bug/double opt in triggered despite being disabled in settings #115

MaxwellGarceau commented Jan 9, 2025 •

edited by jeffpaul

Loading

MaxwellGarceau Jan 9, 2025

dkotter Jan 9, 2025

MaxwellGarceau Jan 9, 2025 •

edited

Loading

MaxwellGarceau commented Jan 9, 2025

dkotter commented Jan 9, 2025

MaxwellGarceau commented Jan 10, 2025

	// TODO: If get_option( 'mc_update_existing' ) && 'unsubscribed' === $status then
	// make an API request to fetch Mailchimp hosted sign up form and display to user

Bug/double opt in triggered despite being disabled in settings #115

Are you sure you want to change the base?

Bug/double opt in triggered despite being disabled in settings #115

Conversation

MaxwellGarceau commented Jan 9, 2025 • edited by jeffpaul Loading

Description of the Change

How to test the Change

Double Opt-In triggered despite being disabled in settings

If Double Opt-In is enabled and a pending user tries to update their information, then their status will still be pending and they will have to confirm the original confirmation email (bug fix/logic change)

Fix: Existing subscriber could sign up even if "Update Existing Subscriber?" is turned off

If "Update Existing Subscriber?" is disabled and the email does not belong to a new subscriber then don't let them sign up (changed logic)

Changelog Entry

Credits

Checklist:

MaxwellGarceau Jan 9, 2025

Choose a reason for hiding this comment

dkotter Jan 9, 2025

Choose a reason for hiding this comment

MaxwellGarceau Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

MaxwellGarceau commented Jan 9, 2025

dkotter commented Jan 9, 2025

MaxwellGarceau commented Jan 10, 2025

MaxwellGarceau commented Jan 9, 2025 •

edited by jeffpaul

Loading

If Double Opt-In is enabled and a `pending` user tries to update their information, then their status will still be `pending` and they will have to confirm the original confirmation email (bug fix/logic change)

MaxwellGarceau Jan 9, 2025 •

edited

Loading