Change deactivation approach #2562

renatho · 2023-08-16T18:50:24Z

Changes proposed in this Pull Request

We identified an issue that could happen in intervals between syncs (WPJM <> wpjobmanager.com <> JobTarget). It could cause a promotion stop accidentally. The discussion: p1692125471733679-slack-C053397HN30.
In order to fix it, we're implementing @fjorgemota's idea to keep all the jobs with _promoted meta key on the feed, even if it's not promoted. To represent it in the feed, we added the promote attribute, which is not currently used but it's useful if we want to iterate in the future, and it makes the endpoint more concise.

The previous issue:

User promote a job, completing the JobTarget checkout.
Before the 5 minutes polling interval (or more if they delay to complete the checkout), the user change something in another promoted jobs triggering a sync (WPJM -> wpjobmanager.com).
Their job would still have _promoted 0 because it didn't sync with the new status yet (wpjobmanager.com -> WPJM). So in the sync WPJM -> wpjobmanager.com, it would remove the job from wpjobmanager.com, removing from the feed that JobTarget will consume, which would deactivate their promotion.

Testing instructions

Make sure your env has at least one promote job.
Promote a new job, completing the JobTarget checkout.
Before the status is updated in WPJM, update another promoted job, and make sure the new job you created wasn't removed from wpjobmanager.com (Check it through WP Admin because the feed has cache, or just clear the cache).
On wpjobmanager.com, run the job to update the statuses vip dev-env -- exec wp cron event run wpjobmanager_com_sync_promoted_job_status.
Check that the job is marked as promoted on WPJM.
Deactivate the job on WPJM job listings, and make sure it was removed from wpjobmanager.com.
For another promoted job of your site, update the status to expired (see instructions on https://github.com/Automattic/wpjobmanager.com/pull/579).
Make sure the job is marked as not promoted on job listings but continues on the feed of WPJM and wpjobmanager.com.

…bmanager.com

fjorgemota

Hey there, this is looking good!

I left a couple of comments/suggestions, and I'll test the PR in the meantime. :)

includes/promoted-jobs/class-wp-job-manager-promoted-jobs-notifications.php

fjorgemota · 2023-08-17T18:44:02Z

includes/promoted-jobs/class-wp-job-manager-promoted-jobs-api.php

@@ -234,6 +233,7 @@ private function prepare_item_for_response( WP_Post $item ) {
 		return [
 			'id'           => (string) $item->ID,
 			'status'       => $item->post_status,
+			'promoted'     => WP_Job_Manager_Promoted_Jobs::is_promoted( $item->ID ),


Do you foresee any changes we need to make on WPJMCOM to use this field?

I am just thinking about the use-case here. 🤔

Currently, I don't think so. But I added it because it can be useful in the future and make the feed more concise (we don't have promoted and not promoted jobs in the feed without a differentiation).
We could use that, in case we want to list all the promoted jobs as a type of extra promotion, for example.

Sounds good, thanks!

docs/api/internal.json

includes/promoted-jobs/class-wp-job-manager-promoted-jobs.php

Co-authored-by: Fernando Jorge Mota <[email protected]>

fjorgemota

Looks good and works well! I left a comment for updating a PHPDoc, but everything else seems to be working just fine. Thanks!

includes/promoted-jobs/class-wp-job-manager-promoted-jobs-notifications.php

includes/promoted-jobs/class-wp-job-manager-promoted-jobs.php

fjorgemota · 2023-08-18T00:05:48Z

includes/admin/class-wp-job-manager-promoted-jobs-admin.php

@@ -209,6 +209,11 @@ public function handle_promote_job() {
 		$verify_endpoint_url = rest_url( '/wpjm-internal/v1/promoted-jobs/verify-token', 'https' );
 		$verify_endpoint_url = substr( $verify_endpoint_url, strlen( $site_url ) );

+		// Make sure the job contains the promoted job meta to be listed in the feed.
+		if ( ! $is_editing ) {
+			WP_Job_Manager_Promoted_Jobs::update_promotion( $post_id, false );


Good catch on setting the flag only if we're creating a new job

fjorgemota · 2023-08-18T00:06:17Z

includes/promoted-jobs/class-wp-job-manager-promoted-jobs-api.php

@@ -234,6 +233,7 @@ private function prepare_item_for_response( WP_Post $item ) {
 		return [
 			'id'           => (string) $item->ID,
 			'status'       => $item->post_status,
+			'promoted'     => WP_Job_Manager_Promoted_Jobs::is_promoted( $item->ID ),


Sounds good, thanks!

fjorgemota · 2023-08-18T00:07:23Z

includes/promoted-jobs/class-wp-job-manager-promoted-jobs.php

+	 * @param bool|string $promoted `true` to promoted, `false` to not promoted, `force_delete` to delete.
+	 *                              The deletion is used to force a removal from the feed, deactivating the promotion while syncing.


I think we should probably update this PHPDoc, right?

Yep! Good catch! I updated the previous commit just changing this. ;)
51ebc4b

…ameter

renatho added 5 commits August 16, 2023 15:32

Update API to contain promoted attribute

6e17f71

List all jobs with promoted meta key in the feed

52e3cd9

Add a meta key as not promoted as soon the user is redirected to wpjo…

0501cb6

…bmanager.com

Remove promoted meta key when manually deactivating a promotion

30e4f0a

Update jobs notification to get promoted meta deletion

8ef9d31

renatho requested a review from a team August 16, 2023 18:50

renatho self-assigned this Aug 16, 2023

renatho added the Promoted Jobs label Aug 16, 2023

fjorgemota reviewed Aug 17, 2023

View reviewed changes

renatho and others added 2 commits August 17, 2023 17:08

Enhance PHPDoc

63db340

Fix swagger documentation

fa4ba74

Co-authored-by: Fernando Jorge Mota <[email protected]>

renatho force-pushed the change/deactivation-approach branch from f0859e7 to 99c974b Compare August 17, 2023 20:38

renatho requested a review from fjorgemota August 17, 2023 20:54

fjorgemota approved these changes Aug 18, 2023

View reviewed changes

Refactor methods to avoid boolean with special string in the same par…

51ebc4b

…ameter

renatho force-pushed the change/deactivation-approach branch from 99c974b to 51ebc4b Compare August 18, 2023 13:07

renatho merged commit 8114ae1 into feature/promoted-jobs Aug 18, 2023
7 checks passed

renatho deleted the change/deactivation-approach branch August 18, 2023 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change deactivation approach #2562

Change deactivation approach #2562

renatho commented Aug 16, 2023

fjorgemota left a comment

fjorgemota Aug 17, 2023

renatho Aug 17, 2023

fjorgemota Aug 18, 2023

fjorgemota left a comment

fjorgemota Aug 18, 2023

fjorgemota Aug 18, 2023

fjorgemota Aug 18, 2023

renatho Aug 18, 2023

		* @param bool\|string $promoted `true` to promoted, `false` to not promoted, `force_delete` to delete.
		* The deletion is used to force a removal from the feed, deactivating the promotion while syncing.

Change deactivation approach #2562

Change deactivation approach #2562

Conversation

renatho commented Aug 16, 2023

Changes proposed in this Pull Request

The previous issue:

Testing instructions

fjorgemota left a comment

Choose a reason for hiding this comment

fjorgemota Aug 17, 2023

Choose a reason for hiding this comment

renatho Aug 17, 2023

Choose a reason for hiding this comment

fjorgemota Aug 18, 2023

Choose a reason for hiding this comment

fjorgemota left a comment

Choose a reason for hiding this comment

fjorgemota Aug 18, 2023

Choose a reason for hiding this comment

fjorgemota Aug 18, 2023

Choose a reason for hiding this comment

fjorgemota Aug 18, 2023

Choose a reason for hiding this comment

renatho Aug 18, 2023

Choose a reason for hiding this comment