Newsletter editor panel - make aware of previous sends, update copies to be accurate#47301
Newsletter editor panel - make aware of previous sends, update copies to be accurate#47301Addison-Stavlo wants to merge 19 commits intotrunkfrom
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
bot
added
[Plugin] Jetpack
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
Code Coverage SummaryCoverage changed in 6 files. Only the first 5 are listed here.
1 file is newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
Addison-Stavlo
left a comment
Addison-Stavlo
left a comment
There was a problem hiding this comment.
Self-review, because I like the interface here for reading changes.
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/shared/memberships/subscribers-affirmation.js
Outdated
Show resolved
Hide resolved
| const baseUrl = isSimpleSite() ? '/rest/v1.1/sites' : '/jetpack/v4/stats-app/sites'; | ||
| const fetchNewsletterCategories = async () => { | ||
| const response = await apiFetch( { | ||
| path: baseUrl + `/${ blogId }/stats/opens/emails/${ postId }/rate`, |
There was a problem hiding this comment.
Note I may end up bringing this request for email stats back into the diff. related to https://linear.app/a8c/issue/NL-477/ensure-fallback-for-sent-information-for-potentially-missing-after / https://linear.app/a8c/issue/NL-476/newsletters-losing-wpcom-specific-post-meta-on-atomic-transfer
Fetching email stats would be a fallback for those other post metas that may have gotten lost for sites in past transfers. We would then create another fallback message that emails were already sent, but have no information about date, access, category, etc. This would be better than falsely stating the email was never sent.
There was a problem hiding this comment.
Pull request overview
This PR updates the newsletter editor panel to display accurate messaging about email send status. It replaces the previous getTotalEmailsSentCount approach with a more comprehensive newsletter-email-sent-status endpoint that provides detailed information about when emails were sent, to which access levels, and to which categories.
Changes:
- Adds new Redux state management for tracking post email send status and session-based modification flags
- Introduces a new REST API endpoint
/wpcom/v2/newsletter-email-sent-statusto fetch email send metadata - Refactors
SubscribersAffirmationcomponent to display contextual messages based on email send state (pre-publish, sending in progress, already sent, or not sent)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
test/selectors-test.js |
Removes tests for deprecated getTotalEmailsSentCount selector |
test/reducer-test.js |
Removes tests for deprecated SET_TOTAL_EMAILS_SENT_COUNT reducer case |
test/actions-test.js |
Removes tests for deprecated setTotalEmailsSentCount action |
selectors.js |
Removes getTotalEmailsSentCount; adds three new selectors for email send state tracking |
resolvers.js |
Removes fetchTotalEmailsSentCount and resolver; adds fetchPostEmailSentState and resolver |
reducer.js |
Removes totalEmailsSentCount state; adds postEmailSentState, alreadySentPostModifiedInSession, and publishedWithEmailEnabledInSession state with corresponding reducer cases |
actions.js |
Removes setTotalEmailsSentCount; adds three new action creators for email send state management |
subscribers-affirmation.js |
Major refactor adding utility functions for date formatting, category name formatting, access level parsing, and conditional message display based on email send state |
panel.js |
Adds NewsletterRepublishTracker component to track status transitions and persist modification flags across panel remounts |
class-wpcom-rest-api-v2-endpoint-newsletter-email-sent-status.php |
New PHP endpoint class that proxies requests to WordPress.com for email send status data |
update-newsletter-editor-panel-sent-awareness |
Changelog entry documenting the changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function permission_check() { | ||
| if ( current_user_can( 'edit_posts' ) || current_user_can( 'manage_options' ) ) { | ||
| return true; | ||
| } | ||
| return new WP_Error( | ||
| 'rest_forbidden', | ||
| __( 'Sorry, you are not allowed to access this endpoint.', 'jetpack' ), | ||
| array( 'status' => rest_authorization_required_code() ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The permission check allows access if the user has either 'edit_posts' or 'manage_options' capability, but it doesn't verify if the user has permission to access the specific post being queried. Consider checking if the user can edit the specific post using something like current_user_can('edit_post', $post_id) where $post_id is extracted from the request parameters. This would ensure users can only view email sent status for posts they have permission to edit.
| const { postId, postMeta, postEmailSentState, status, isSavingPost } = useSelect( select => { | ||
| const { getCurrentPost, getEditedPostAttribute } = select( editorStore ); | ||
| const { getPostEmailSentState } = select( membershipProductsStore ); | ||
| const post = getCurrentPost(); | ||
| const id = post?.id; | ||
| if ( id ) { | ||
| getPostEmailSentState( id ); | ||
| } | ||
| return { | ||
| postId: id, | ||
| postMeta: getEditedPostAttribute( 'meta' ), | ||
| postEmailSentState: id ? getPostEmailSentState( id ) : null, | ||
| status: post?.status, | ||
| isSavingPost: select( editorStore ).isSavingPost(), | ||
| }; | ||
| }, [] ); |
There was a problem hiding this comment.
The useSelect hook has an empty dependency array, which means it will only run once when the component mounts. However, the selector accesses dynamic values like postId, status, and postEmailSentState that can change over time. This could cause the component to miss updates when the post changes or when the email sent state is fetched. Consider removing the empty dependency array to allow useSelect to track dependencies automatically, or explicitly include the necessary dependencies if there's a specific reason to limit re-renders.
| if ( ! postId ) { | ||
| return { email_sent_at: null, stats_on_send: null }; | ||
| } | ||
| return state.postEmailSentState || { email_sent_at: null, stats_on_send: null }; |
There was a problem hiding this comment.
The selector accepts a postId parameter but doesn't use it to retrieve post-specific state. The state structure stores postEmailSentState as a single object rather than keyed by postId, so this selector will return the same email sent state regardless of which postId is requested. If the editor can have multiple posts loaded (or switch between posts), this will cause the wrong email sent state to be displayed. Consider restructuring the state to be keyed by postId (similar to alreadySentPostModifiedInSession and publishedWithEmailEnabledInSession), or if only one post is ever active at a time, remove the postId parameter from the selector signature.
| if ( ! postId ) { | |
| return { email_sent_at: null, stats_on_send: null }; | |
| } | |
| return state.postEmailSentState || { email_sent_at: null, stats_on_send: null }; | |
| const defaultState = { email_sent_at: null, stats_on_send: null }; | |
| // If no postId is provided, we cannot look up a per-post entry. | |
| if ( ! postId ) { | |
| return defaultState; | |
| } | |
| const { postEmailSentState } = state; | |
| // If postEmailSentState is an object keyed by postId, prefer that. | |
| if ( postEmailSentState && typeof postEmailSentState === 'object' && postId in postEmailSentState ) { | |
| return postEmailSentState[ postId ] || defaultState; | |
| } | |
| // Fallback: support legacy shape where postEmailSentState is a single object. | |
| return postEmailSentState || defaultState; |
| getNewsletterCategoriesSubscriptionsCount, | ||
| getProducts, | ||
| getTotalEmailsSentCount, | ||
| } from '../selectors'; |
There was a problem hiding this comment.
The new selectors getPostEmailSentState, getAlreadySentPostModifiedInSession, and getPublishedWithEmailEnabledInSession lack test coverage. While tests for the deprecated getTotalEmailsSentCount selector were removed, no tests were added for the replacement selectors. This creates a gap in test coverage for critical functionality that tracks email send state.
| setNewsletterCategories, | ||
| setNewsletterCategoriesSubscriptionsCount, | ||
| setTotalEmailsSentCount, | ||
| } from '../actions'; |
There was a problem hiding this comment.
The new actions setPostEmailSentState, setAlreadySentPostModifiedInSession, and setPublishedWithEmailEnabledInSession lack test coverage. While tests for the deprecated setTotalEmailsSentCount action were removed, no tests were added for the replacement actions. This creates a gap in test coverage for critical functionality that manages email send state in the Redux store.
| @@ -148,19 +148,4 @@ describe( 'Membership products reducer testing', () => { | |||
| newsletterCategoriesSubscriptionsCount: anyNewsletterCategoriesSubscriptionsCount, | |||
| } ); | |||
| } ); | |||
There was a problem hiding this comment.
The new reducer cases SET_POST_EMAIL_SENT_STATE, SET_ALREADY_SENT_POST_MODIFIED_IN_SESSION, and SET_PUBLISHED_WITH_EMAIL_ENABLED_IN_SESSION lack test coverage. While the test for the deprecated SET_TOTAL_EMAILS_SENT_COUNT case was removed, no tests were added for the replacement reducer cases. This creates a gap in test coverage for critical state management logic.
| } ); | |
| } ); | |
| test( 'set post email sent state action type adds the post email sent state to the returned state.', () => { | |
| // Given | |
| const anyPostEmailSentState = true; | |
| const anySetPostEmailSentStateAction = { | |
| type: 'SET_POST_EMAIL_SENT_STATE', | |
| postEmailSentState: anyPostEmailSentState, | |
| }; | |
| // When | |
| const returnedState = reducer( DEFAULT_STATE, anySetPostEmailSentStateAction ); | |
| // Then | |
| expect( returnedState ).toStrictEqual( { | |
| ...DEFAULT_STATE, | |
| postEmailSentState: anyPostEmailSentState, | |
| } ); | |
| } ); | |
| test( 'set already sent post modified in session action type adds the flag to the returned state.', () => { | |
| // Given | |
| const anyAlreadySentPostModifiedInSession = true; | |
| const anySetAlreadySentPostModifiedInSessionAction = { | |
| type: 'SET_ALREADY_SENT_POST_MODIFIED_IN_SESSION', | |
| alreadySentPostModifiedInSession: anyAlreadySentPostModifiedInSession, | |
| }; | |
| // When | |
| const returnedState = reducer( | |
| DEFAULT_STATE, | |
| anySetAlreadySentPostModifiedInSessionAction | |
| ); | |
| // Then | |
| expect( returnedState ).toStrictEqual( { | |
| ...DEFAULT_STATE, | |
| alreadySentPostModifiedInSession: anyAlreadySentPostModifiedInSession, | |
| } ); | |
| } ); | |
| test( 'set published with email enabled in session action type adds the flag to the returned state.', () => { | |
| // Given | |
| const anyPublishedWithEmailEnabledInSession = true; | |
| const anySetPublishedWithEmailEnabledInSessionAction = { | |
| type: 'SET_PUBLISHED_WITH_EMAIL_ENABLED_IN_SESSION', | |
| publishedWithEmailEnabledInSession: anyPublishedWithEmailEnabledInSession, | |
| }; | |
| // When | |
| const returnedState = reducer( | |
| DEFAULT_STATE, | |
| anySetPublishedWithEmailEnabledInSessionAction | |
| ); | |
| // Then | |
| expect( returnedState ).toStrictEqual( { | |
| ...DEFAULT_STATE, | |
| publishedWithEmailEnabledInSession: anyPublishedWithEmailEnabledInSession, | |
| } ); | |
| } ); |
| const categoriesMatch = | ||
| ! statsOnSend?.has_newsletter_categories || | ||
| ( Array.isArray( postCategories ) && | ||
| statsCats.length === postCategories.length && | ||
| statsCats.every( ( id, i ) => postCategories[ i ] === id ) ); |
There was a problem hiding this comment.
The category comparison logic may incorrectly detect a mismatch when categories are in different orders. The current implementation uses statsCats.every((id, i) => postCategories[i] === id), which compares elements at the same index. If the arrays contain the same category IDs but in different orders, this will return false even though the categories haven't actually changed. Consider sorting both arrays before comparison or using a set-based comparison approach.
| const categoriesMatch = | |
| ! statsOnSend?.has_newsletter_categories || | |
| ( Array.isArray( postCategories ) && | |
| statsCats.length === postCategories.length && | |
| statsCats.every( ( id, i ) => postCategories[ i ] === id ) ); | |
| const postCategorySet = Array.isArray( postCategories ) ? new Set( postCategories ) : null; | |
| const categoriesMatch = | |
| ! statsOnSend?.has_newsletter_categories || | |
| ( postCategorySet && | |
| statsCats.length === postCategories.length && | |
| statsCats.every( id => postCategorySet.has( id ) ) ); |
allilevine
left a comment
allilevine
left a comment
There was a problem hiding this comment.
I tried it out and the panels are much more accurate with this change. 🙌 One thing that came to mind is, should we still have the email preview and test email options if the email has already been sent? Or should those be hidden because you can't re-send the email at that point?
Fixes # https://linear.app/a8c/issue/NL-449/newsletter-panel-update-the-states-and-messaging
STILL TODOs - but any testing and review is appreciated at this point.
Proposed changes:
Updates the newsletter panel in the editor to have messaging consistent with the state of sent emails.
Panel states updated:
Some screenshots of various states:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Changelog