Profiles: Add activity when added as a presenter for Learn workshop #84

renintw · 2022-07-10T21:11:07Z

Issue: WordPress/five-for-the-future#179 - Add activity when added as a presenter for Learn workshop

Question

If an author published a post, then switched back to draft, and published again, there'll be two identical activities on assigned presenters' profiles, is it expected behavior?
The icon before the assigned presenter activity is also changed as the screencast below shows, but I can't locate the style.css of profiles in the repo. How do we usually modify this file, do we directly modify themes/profiles.wordpress.org/style.css on the sandbox?

Screencast of results

Screen.Capture.on.2022-07-11.at.05-02-17.mov

renintw · 2022-07-10T21:12:27Z

...-content/plugins/wporg-profiles-wp-activity-notifier/wporg-profiles-wp-activity-notifier.php

@@ -150,6 +151,71 @@ public function maybe_notify_new_published_post( $new_status, $old_status, $post
 		$this->notify_new_blog_post( $post );
 	}

+	public function insert_post ($postId, $post) {
+		if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {


Avoid the second firing action triggered by Gutenberg.
(For the case: Publish->Switch to Draft->Publish Again)

Is REST_REQUEST a reliable way to detect that sequence of events? And is that a common occurrence?

At a quick glance it seems some of the other notification types don't try to prevent duplicate activities when something is published twice. It might be overkill, but if it's common then as a general solution I would consider using postmeta to store a list of notified actions to prevent dupes. Perhaps that's better done on the profile API side - @dd32 do you have an opinion on this?

Since wp_insert_post would be fired twice because of Gutenberg (according to these two posts: post1, post2) and causes the issue showed in the screencast below. (You can see that there is another updating... after published, making the account rentest have duplicate activities for the assigned presenter)

Screen.Capture.on.2022-07-12.at.04-04-42.mp4

I first tried the suggested solution brought up in post1, which is to use wp_after_insert_post instead, but the result didn't change.

So I then use the method mentioned in post2, which is using REST_REQUEST to detect the REST API fired by the Gutenberg editor.

It does smell though... not sure if there's a better way to handle this.

using postmeta to store a list of notified actions to prevent dupes

I might need to ask you more about this.

Since wp_insert_post would be fired twice because of Gutenberg

You should be able to look at the metadata of the insert I believe, and see the state change somehow (I'm guessing post_status: auto-draft -> draft -> published maybe?)

It might be overkill, but if it's common then as a general solution I would consider using postmeta to store a list of notified actions to prevent dupes.

Dupes should be able to be handled on the profiles side, every activity entry has a contextual link/id/etc.. but avoiding sending duplicate data in the same pageload is obviously beneficial from a performance standpoint..

Hm, that's interesting about the extra call to wp_insert_post. I wonder if this is the cause:

https://wordpress.stackexchange.com/questions/395666/why-is-save-post-hook-being-called-twice-despite-all-my-efforts#comment576840_395672

Some plugins incorrectly make a second REST API request to save meta after a post has been saved because the author doesn't understand the core data API and doesn't realise Gutenberg will send the meta for them if they register it and update the posts meta directly. This is where that comes from, and it's bad practice

I don't know how the presenter metadata is handled, but it's possible that (or other postmeta) is causing the duplicate call. That might explain why you were having trouble fetching the postmeta from the transition_post_status hook - maybe the meta is only stored in the second call, after the post status has changed. I'll do some testing and see what I can turn up.

I found some discussion here, I guess it explains why this happened.
WordPress/gutenberg#12903 (comment)

As we know that Gutenberg sends two requests when we hit the Publish/Update button:
a) Request to WP REST API to create/update the post
b) Request to post.php to save/submit the metaboxes

a) Gutenberg saves data via one or two requests; the second save request only fires if there are meta boxes, so a plugin cannot depend on this request happening.
b) If there are going to be two requests, the plugin can't necessarily depend on the first request, because the first doesn't contain all the data for the post.

renintw · 2022-07-10T21:18:10Z

...-content/plugins/wporg-profiles-wp-activity-notifier/wporg-profiles-wp-activity-notifier.php

+
+		foreach ($presenter_wporg_username as $username) {
+
+			sleep(1);


It seems we got the rate limit, so sleep a second before the next call out.
(For the case that author and presenter are the same person)

I don't think sleep() is a good solution in production, though it's useful for diagnosing the problem. It could theoretically lead to unexpected UI delays when publishing a post. Better to find a more robust solution: either find out why the timeout is occurring and solve that, or schedule the function to run in a cron job.

I should remove this. It was added because at first I was putting the logic in transition_post_status.
Now that the logic is under wp_insert_post, the problem of writing data to DB twice in a short time no longer exists.
(It would write the Wrote a new workshop activity first during transition_post_status and then after the post is published, the Assigned presenter is sent to DB. Previously, these two would be fired almost at the same time, causing weird behavior of notifying activity.)

I'm still curious what's the practice for us to handle such case (write DB in a row in a short time) since the logic might be changed back to use transition_post_status.
cron job sounds like a good idea.

either find out why the timeout is occurring and solve that

What does timeout here refer to, timeout from sleep? or timeout of writing DB? Thanks!

either find out why the timeout is occurring and solve that
What does timeout here refer to

I believe Alex meant rate-limit here, not timeout.

There shouldn't be any rate-limits in production that would catch this - if there is, we should probably get those lifted for self-calls.

Yep sorry Dion's right, I meant the rate limit.

tellyworth · 2022-07-11T05:14:45Z

...-content/plugins/wporg-profiles-wp-activity-notifier/wporg-profiles-wp-activity-notifier.php

+
+		foreach ($presenter_wporg_username as $username) {
+
+			sleep(1);


I don't think sleep() is a good solution in production, though it's useful for diagnosing the problem. It could theoretically lead to unexpected UI delays when publishing a post. Better to find a more robust solution: either find out why the timeout is occurring and solve that, or schedule the function to run in a cron job.

tellyworth · 2022-07-11T05:18:14Z

...-content/plugins/wporg-profiles-wp-activity-notifier/wporg-profiles-wp-activity-notifier.php

@@ -150,6 +151,71 @@ public function maybe_notify_new_published_post( $new_status, $old_status, $post
 		$this->notify_new_blog_post( $post );
 	}

+	public function insert_post ($postId, $post) {
+		if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {


Is REST_REQUEST a reliable way to detect that sequence of events? And is that a common occurrence?

At a quick glance it seems some of the other notification types don't try to prevent duplicate activities when something is published twice. It might be overkill, but if it's common then as a general solution I would consider using postmeta to store a list of notified actions to prevent dupes. Perhaps that's better done on the profile API side - @dd32 do you have an opinion on this?

tellyworth · 2022-07-11T05:27:06Z

...-content/plugins/wporg-profiles-wp-activity-notifier/wporg-profiles-wp-activity-notifier.php

@@ -51,6 +51,7 @@ private function __construct() {

 	public function init() {
 		add_action( 'transition_post_status',    array( $this, 'maybe_notify_new_published_post'   ), 10, 3 );
+		add_action( 'wp_insert_post',         	 array( $this, 'insert_post' ),                       10, 2 );


I'm curious why you used the wp_insert_post action rather than transition_post_status (as used by maybe_notify_new_published_post().

I used transition_post_status in the beginning, but since the action is triggered before saving data to DB, I can't get the post meta with the function get_post_meta.
The problem would be gone if transition_post_status can be used here, are there any simple ways to get the post_meta before saving it to DB here?

After thinking about it some more: you're on the right track, wp_insert_post is (almost) the right action to use here. If it was only on transition_post_status then we'd miss seeing presenters that were added to the post after it was published.

I think what's needed is actually the wp_insert_post_data filter. That's triggered during wp_insert_post(), but before the post is inserted or updated:

https://github.com/WordPress/wordpress-develop/blob/9b105d92a4b769f396ba798db1f106abab75001f/src/wp-includes/post.php#L4361-L4374

If you hook that one, the $postarr array should contain all the data being inserted, including the meta_input data that gets inserted as postmeta. And the $update param should tell you if it's an update or an insert. So in the case of an insert, you'd add activity for all of those users. If it's an update, you could use get_post_meta() to figure out if any new presenters are being added, and only add activity for those.

Does that make sense? I think that would mostly deal with the issue of duplicates too, since you'd only notify when a new username is found in $postarr.

wp_insert_post_data -> This is nice!
It could deal with the case:
[run1] presenter: ren -> send activity to ren
[run2] presenter: ren, ren2 -> send activity to ren2

But if it's the following case, it still duplicates, but I believe it's acceptable.
[run1] presenter: ren, ren2 -> send activity to ren, ren2
[run2] presenter: ren -> send activity to no one
[run3] presenter: ren, ren2 -> send activity to ren2 again

Yeah I agree - handling of that edge case isn't quite right, but it's good enough for now.

If we also add some basic dupe handling on the API side then that should be pretty solid. Let's make that a separate issue.

There's also the issue of removal, but since I don't think there's a delete API there's nothing we can do about it for now. We could consider handling that in a separate issue later if it becomes an issue.

renintw · 2022-07-18T19:10:17Z

Changed back to use transition_post_status.
@dd32 @tellyworth Could you help review it again? thanks!

This issue and The weird behavior caused by sending two activities (Wrote a new workshop & Assigned presenter) together to the same user in a short time WON'T happen anymore because $this->notify_new_blog_post( $post ); would be executed during the first request, and $presenter_wporg_username would have value only when Gutenberg makes the second request, so its following logic will be skipped during the first request due to if ( empty( $presenter_wporg_username ) ) { return; }.
For now, both a) Updating a post and b) Switching back to draft and publishing a post again will trigger notify_workshop_presenter and cause duplicate activities. After several tries, it seems to make more sense and efficient to implement the logic on the Profile API side. Issue opened.
What's the flow of modifying files that are not on Github repos?

The icon before the assigned presenter activity is also changed as the screencast below shows, but I can't locate the style.css of profiles in this repo. How do we usually modify this kind of file, do we directly modify themes/profiles.wordpress.org/style.css on the sandbox and commit it?

iandunn

I haven't tested, but the code LGTM 👍🏻

I left a few suggestions, but they're not blockers.

It might also help to add a case to tests/e2e.php. I've found that easier for testing during development than doing it manually each time. It also avoids disrupting production data with testing.

...-content/plugins/wporg-profiles-wp-activity-notifier/wporg-profiles-wp-activity-notifier.php

Only change "workshop" in message but not in function name or other places as in WordPress/Learn#819, there's only text being changed. Should wait until the changes on learn side have been made before changing the remaining "workshop" here to avoid confusion.

iandunn · 2022-09-14T18:37:06Z

merged in https://meta.trac.wordpress.org/changeset/12062/

renintw commented Jul 10, 2022

View reviewed changes

tellyworth self-requested a review July 11, 2022 02:24

tellyworth reviewed Jul 11, 2022

View reviewed changes

renintw force-pushed the add/send-activity-when-presenters-added-for-Learn-workshop branch 3 times, most recently from f008c96 to 41ee344 Compare July 18, 2022 17:57

renintw mentioned this pull request Jul 18, 2022

Handle duplicate activities - added as a presenter activity (and other activities?) WordPress/five-for-the-future#204

Open

renintw force-pushed the add/send-activity-when-presenters-added-for-Learn-workshop branch from e296138 to 73cf1ae Compare July 18, 2022 22:11

iandunn reviewed Jul 18, 2022

View reviewed changes

renintw force-pushed the add/send-activity-when-presenters-added-for-Learn-workshop branch 3 times, most recently from 1e017ab to ced910b Compare July 25, 2022 13:00

iandunn mentioned this pull request Aug 2, 2022

Tracking Contributions: Training team WordPress/five-for-the-future#179

Open

4 tasks

renintw force-pushed the add/send-activity-when-presenters-added-for-Learn-workshop branch from ced910b to 7ee00f4 Compare September 13, 2022 16:29

renintw added 6 commits September 14, 2022 23:08

Profiles: Add notifier for Learn workshop presenter

9467e52

Change back to use transition_post_status

26f3cd9

Fix some PHPCS errors/warnings.

df45eae

Revert comparison operator

50aee30

Change activity message

5073479

renintw force-pushed the add/send-activity-when-presenters-added-for-Learn-workshop branch from 7ee00f4 to e85ca40 Compare September 14, 2022 16:17

Rebase trunk

57c5490

renintw force-pushed the add/send-activity-when-presenters-added-for-Learn-workshop branch 2 times, most recently from e157564 to 57c5490 Compare September 14, 2022 16:46

renintw closed this Sep 14, 2022

renintw deleted the add/send-activity-when-presenters-added-for-Learn-workshop branch September 14, 2022 17:25

iandunn mentioned this pull request Sep 14, 2022

Move presenter notification to Learn-specific file WordPress/Learn#940

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Profiles: Add activity when added as a presenter for Learn workshop #84

Profiles: Add activity when added as a presenter for Learn workshop #84

renintw commented Jul 10, 2022

renintw Jul 10, 2022 •

edited

Loading

tellyworth Jul 11, 2022

renintw Jul 11, 2022 •

edited

Loading

dd32 Jul 12, 2022

dd32 Jul 12, 2022

tellyworth Jul 12, 2022

renintw Jul 12, 2022

renintw Jul 10, 2022

tellyworth Jul 11, 2022

renintw Jul 11, 2022 •

edited

Loading

dd32 Jul 12, 2022

tellyworth Jul 12, 2022

tellyworth Jul 11, 2022

tellyworth Jul 11, 2022

tellyworth Jul 11, 2022

renintw Jul 11, 2022 •

edited

Loading

tellyworth Jul 12, 2022

renintw Jul 12, 2022 •

edited

Loading

tellyworth Jul 12, 2022

renintw commented Jul 18, 2022 •

edited

Loading

iandunn left a comment

iandunn commented Sep 14, 2022

Profiles: Add activity when added as a presenter for Learn workshop #84

Profiles: Add activity when added as a presenter for Learn workshop #84

Conversation

renintw commented Jul 10, 2022

Question

Screencast of results

renintw Jul 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw commented Jul 18, 2022 • edited Loading

iandunn left a comment

Choose a reason for hiding this comment

iandunn commented Sep 14, 2022

renintw Jul 10, 2022 •

edited

Loading

renintw Jul 11, 2022 •

edited

Loading

renintw Jul 11, 2022 •

edited

Loading

renintw Jul 11, 2022 •

edited

Loading

renintw Jul 12, 2022 •

edited

Loading

renintw commented Jul 18, 2022 •

edited

Loading