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

2.4.0 Release - remove recipient count #312

Merged
merged 2 commits into from Mar 28, 2023
Merged

Conversation

rgomezp
Copy link
Contributor

@rgomezp rgomezp commented Mar 25, 2023

1 sentence summary

Removes the recipient count used in the editor notices.

Overview

In an effort to ease server computational costs, we will no longer return the recipient count as part of the response body to POST notification.

Thus we must remove it from the plugin notices.

Other changes

Update tested up to value to 6.3


This change is Reviewable

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Due to the sleep causing delays in the posting response, and also that the notification might not be done sending within 10 seconds what do you think about moving this REST API GET /notiications/(id) call to the has_metadata ?

What if in that has_metadata method if get_post_meta($post_id, 'recipients'); returns null we make the REST API request to get the recipients from there?

onesignal-admin.php Outdated Show resolved Hide resolved
onesignal-admin.php Outdated Show resolved Hide resolved
@rgomezp
Copy link
Contributor Author

rgomezp commented Mar 27, 2023

Due to the sleep causing delays in the posting response, and also that the notification might not be done sending within 10 seconds what do you think about moving this REST API GET /notiications/(id) call to the has_metadata ?

What if in that has_metadata method if get_post_meta($post_id, 'recipients'); returns null we make the REST API request to get the recipients from there?

has_metadata is only called from the Gutenberg editor, so that wouldn't work for Classic.

It's true that the post would appear to take longer to successfully post, although this is not exactly true. The post publish will be successful, kick off the lifecycle event that fires the notification send and that is what would make the post appear longer to publish from the editor perspective.

The notification is sent immediately, however.

While this is not ideal, there aren't many good options. Some options I've already considered:

  • WP cron job: would require some extra setup
  • Fork the process, run in the background (e.g: pcntl_fork()) - not guaranteed to be supported on client's server
  • Ajax round trip: we modify the existing notice.js script to wait 10 secs and send a command to the server to make the get notification request after it detects the 200 status on the notification create call. (This is the best option, but still a bit hacky). We would need to refactor the script so that it also runs on the classic editor.

notice.js Outdated Show resolved Hide resolved
readme.txt Outdated Show resolved Hide resolved
@rgomezp rgomezp changed the title 2.4.0 Release - migrate to use view notification to get recipient count 2.4.0 Release - remove recipient count Mar 28, 2023
Stop reading the `recipients` value from the notification create response.

Update the `notice.js` to stop using `recipients`
@rgomezp rgomezp merged commit 8540956 into main Mar 28, 2023
1 check passed
@rgomezp rgomezp deleted the update-recipient-count branch March 28, 2023 18:30
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.

None yet

4 participants