fix: avoid race condition between post-save sync & test

[dkoo]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Avoids a potential race condition when sending a test email. Sending a test email triggers a save, but because the campaign is synced after a save instead of on save (so that we can regenerate MJML first), and the test endpoint also triggers a sync, it's possible that the test request can fire its sync and send the test email before the post-save sync completes. This can result in the test email being sent with old data and content.

Until this fix is deployed, editors can avoid this race condition by manually saving before sending a test email.

How to test the changes in this Pull Request:

1. To make the race condition more likely to happen, try adding sleep( 5 ); before this line in whichever ESP class you're testing with. This will force the post-save-sync to take 5 seconds longer to start.
2. On trunk, make some changes to a newsletter draft's content but don't save. Then, send yourself a test email without saving first. The content in the sent test should match the content from before you clicked "Send a Test Email".

• If you open your dev tools > Network tab, you can see the race condition happening in real time:
  a. After clicking "Send a Test Email" there should be an XHR POST request to an endpoint with the post ID of the post you're editing. This is the "save" request.
  b. Immediately after the save request finishes, you should see another POST request to the test endpoint to trigger a test email.
  c. Sometime after that (since you added the sleep delay above) you should see a POST request to post-mjml, then GET requests to retrieve and sync-errors. This series of requests is the post-save sync.

3. Check out this branch and repeat step 2. This time, confirm that the test email's content always matches the state of the content even if you sent a test without saving first.

• In the dev tools > Network tab, you should now see that the POST request to the test endpoint doesn't start until after the last sync-errors request, which means that we're now waiting until the post-save sync completes before sending a test email. Because of this we can remove the extra sync that gets triggered by the test endpoint handler (MC only).

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully ran tests with your changes locally?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.32%. Comparing base (57d83fc) to head (08a4f09).
Report is 3 commits behind head on trunk.

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk    #1679      +/-   ##
============================================
+ Coverage     20.30%   20.32%   +0.01%     
+ Complexity     2663     2661       -2     
============================================
  Files            48       48              
  Lines         10616    10608       -8     
============================================
  Hits           2156     2156              
+ Misses         8460     8452       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adekbadek]

Didn't work for me – I can recreate the issue (with sleep(5)) on this branch. What I observe is the post update, /post-mjml, /test sequence (as described), but when I add a logline after the the sleep and the sync method called, I see it's done after all these requests are resolved.

[dkoo]

@adekbadek Apologies, I pasted a link to the wrong line to add the sleep( 5 ); line. It should go before this line in the sync method, to delay the post-save sync. The series of requests should always happen in this order:

1. save (POST to post ID)
2. post-mjml (POST)
3. retrieve (GET)
4. sync-error (GET)
5. test (POST)

I've updated the testing instructions above. After moving the sleep( 5 ); line, are you still able to replicate the issue with the test email being sent with outdated content?

[adekbadek]

Works as described now!