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

feat: uncheck the "Send notification" checkbox when a post is published #342

Closed
wants to merge 8 commits into from

Conversation

sherwinski
Copy link
Contributor

@sherwinski sherwinski commented Jan 15, 2025

Description

After a post is published or updated, uncheck the "Send notification when post is published or updated" checkbox. This is to prevent unintentionally sending subsequent notifications if say, a post author or other plugin were to edit the post. With this change, plugin consumers will need to opt into sending notifications to users after the post is initially published.

Details

On first attempt, we tried simply unchecking the "Send notification" checkbox when a user saves a post. This quickly proved to be an issue as the checkbox state is used in determining whether a OneSignal notification request should be make. A "quick and dirty" way to get around this is to add a wait between when a post is saved and when the checkbox is unchecked. Unsurprisingly, this is not a good approach as external factors (such as network speeds) can cause these actions to happen out of sync. A better approach would ideally wait to uncheck the box once we can guarantee that the notification action has successfully completed, which requires coordination between the client and server.

This PR achieves this effect by:

  • Adding a "notification status" global that tracks if a notification has been successfully sent yet.
  • Creating an internal endpoint that is available to both the client and server via a property on the global object.
    • This endpoint incorporates security measures so that only users with permission to edit the post can create push notification requests.
  • Polling this endpoint once every 1.5 seconds whenever a post is saved, which will return the current notification status.
  • Unchecking the "Send notification" checkbox once the poll indicates that a notification has been sent, guaranteeing that these steps are taken in the correct order.
  • Resetting the notification status metadata field as a final cleanup step.

Demo

v2 Demo

uncheck.send.notif.checkbox.mov

v3 Demo Gutenberg

uncheck.send.notif.checkbox.v3.gutenberg.mov

v3 Demo Classic

uncheck.send.notif.checkbox.v3.classic.mov

This change is Reviewable

Copy link
Contributor Author

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

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

Open questions/concerns:

@marclucraft
Copy link
Contributor

marclucraft commented Jan 15, 2025

This might be better solved using PHP (in onesignal-notification.php) rather than JS client-side (since posts might be created automatically / remotely)

Something along the lines of; if this is a post update, don't do anything

Could also add another option in the Settings screen "Automatically send notifications on Post Update", so the above would become: if this is a post update, don't do anything – unless "Automatically send notifications on Post Update" is checked

@rgomezp
Copy link
Contributor

rgomezp commented Jan 15, 2025

It's fragile to rely on a 3rd party button class name to listen to. Especially since this can potentially change between WP versions.

Instead, we can use the 'core/editor' API on wp.data to listen to changes in the post status and uncheck the checkbox that way.

@rgomezp rgomezp requested review from jkasten2 and removed request for rgomezp January 15, 2025 18:45
@rgomezp rgomezp force-pushed the uncheck-after-publish branch from b9585fc to 76d2a1a Compare January 15, 2025 18:46
Copy link
Contributor Author

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

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

One small thing that I noticed is that even though the checkbox does get unchecked, the options field underneath doesn't get hidden as it would when you manually click on the checkbox.

Screen.Recording.2025-01-15.at.5.32.06.PM.mov

@sherwinski
Copy link
Contributor Author

sherwinski commented Jan 16, 2025

@rgomezp Hmm now there are different issues with the behavior:

  • A notification isn't sent the first time a post is published
    • My best guess as to why is that it could be related to this
  • A notification is sent when the post is updated but the checkbox UI doesn't update unless the page is refreshed
    • May just need a call to updateUI() when a post is updated, assuming that flow is any different than publishing for the first time
Screen.Recording.2025-01-16.at.11.33.41.AM.mov

@rfischmann
Copy link

Any ETA on this, guys? This is a huge blocker for us. We're still on plugin version 2.4.4 only because of this.

@sherwinski
Copy link
Contributor Author

Hey @rfischmann, thanks for checking in. We're pushing to release this soon, hopefully no later than in two weeks time.

@sherwinski sherwinski force-pushed the uncheck-after-publish branch from 9332a6b to 8a71bd7 Compare February 7, 2025 02:09
sherwinski and others added 8 commits February 6, 2025 18:09
Current implementation will only show the checkbox as unchecked after the page is refreshed.
…h button is clicked

Potential concerns with this approach:

- A wait is needed, otherwise the notification won't send. This may because it takes the plugin time to structure the request and immediately unchecking it will mark it as "don't send". Generally, functionality should not be dependent on waiting
- The publish is targeted from the document body (`.editor-post-publish-button__button`) but this is brittle since we don't control the class values on the button
…atus changes in JS

Motivation: it's fragile to rely on a 3rd party button class name to listen to. Especially since this can potentially change between WP versions.

Instead, we can use the 'core/editor' API on `wp.data` to listen to changes in the post status and uncheck the checkbox that way.
We accomplish this by polling for the status of the post every second for up to ten seconds. When a post has completed posting/saving, then we will uncheck the "Send notification" checkbox to ensure that the state is unaffected prior to composing & sending the notification request to OneSignal. This only applies to publishing a post for the first time.

TODO: need to investigate and implement similar logic for updating a post
…shed

Prior to this change we would run into an edge case where a notification was saved as already "sent" (via the `_onesignal_notification_sent` global). This caused notifications to not be sent on alternating instances of updating a post. This commit aims to address that by adding an AJAX endpoint to reset notification status *after* successfully sending a notification.

Additionally, this commit removes duplicate script enqueueing introduced earlier as it was causing duplicate requests to be made to the admin-ajax endpoints. It also adds some improvements to polling, such as error handling, to help with debugging
This was added due to alleviate issues with multiple save triggers however, deduping the enqueued scripts resolved this issue on its own.
@sherwinski sherwinski force-pushed the uncheck-after-publish branch from 8a71bd7 to c1c2876 Compare February 7, 2025 02:13
@sherwinski
Copy link
Contributor Author

Closing in favor of #348

@sherwinski sherwinski closed this Feb 13, 2025
@sherwinski sherwinski deleted the uncheck-after-publish branch February 13, 2025 19:33
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.

4 participants