Skip to content

Background job taxonomy for queuing jobs#497

Merged
ankitrox merged 14 commits intofeature/regenerate-existing-imagesfrom
feature/background-job-taxonomy
Aug 30, 2022
Merged

Background job taxonomy for queuing jobs#497
ankitrox merged 14 commits intofeature/regenerate-existing-imagesfrom
feature/background-job-taxonomy

Conversation

@ankitrox
Copy link
Copy Markdown

@ankitrox ankitrox commented Aug 23, 2022 •
edited
Loading

Summary

Create the background_job taxonomy. This taxonomy is non-public and hidden in REST. This will by default be hidden in admin screen, but can be made visible using perflab_enable_background_process_queue_screen filter. ( #491 )

Fixes #478

Relevant technical choices

Taxonomy has custom capabilities

manage_jobs,
edit_jobs,
delete_jobs,
assign_jobs

User must have these capabilities assigned for the taxonomy (job queue) management. By default admin screen for this taxonomy (job queue) will be hidden.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@ankitrox ankitrox added [Focus] Images no milestone PRs that do not have a defined milestone for release [Type] Epic A high-level project / epic that will encompass several sub-issues labels Aug 23, 2022
@ankitrox ankitrox self-assigned this Aug 23, 2022
@ankitrox ankitrox linked an issue Aug 23, 2022 that may be closed by this pull request
Create Background Processing Infrastructure: Create background_job taxonomy #478
Closed
@ankitrox ankitrox marked this pull request as ready for review August 25, 2022 10:57
@ankitrox ankitrox linked an issue Aug 25, 2022 that may be closed by this pull request
Create Background Processing Infrastructure: Create Queue Admin Screen #491
Closed
felixarntz
felixarntz reviewed Aug 25, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@ankitrox This is a good start, I left some comments (mostly minor) for things to refine.

One small note, please avoid using the feature/ prefix for your development branches as those branches will need branch protection rules. Only special branches (like the overall feature/regenerate-existing-images branch) should use that prefix. 👍

@ankitrox ankitrox requested a review from felixarntz August 25, 2022 18:36
mukeshpanchal27
mukeshpanchal27 reviewed Aug 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Left some nit-pick reviews.

modules/images/regenerate-existing-images/load.php Outdated
'back_to_items' => __( 'Back to Background Jobs', 'performance-lab' ),
);

$caps = perflab_get_background_job_capabilities();
Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 Aug 26, 2022

Choose a reason for hiding this comment

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

Do you need this?

tests/modules/images/regenerate-existing-images/load-tests.php Outdated
* @group regenerate-existing-images
*/


Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 Aug 26, 2022

Choose a reason for hiding this comment

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

Remove space.

@ankitrox ankitrox added the [Plugin] Regenerate Existing Images Issues for the Regenerate Existing Images module label Aug 26, 2022
eugene-manuilov
eugene-manuilov requested changes Aug 26, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@eugene-manuilov eugene-manuilov left a comment

Choose a reason for hiding this comment

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

Thanks, @ankitrox. Added some feedback to your code that will probably will require additional discussion within our joint team.

Also, I think we need to remove the mention of the 491 ticket in the PR description because this PR doesn't implement the admin UI screen.

@ankitrox
Copy link
Copy Markdown
Author

ankitrox commented Aug 26, 2022

@eugene-manuilov We should keep #491 also as we have taken the decision to manipulate the visibility based on user capabilities.

felixarntz
felixarntz reviewed Aug 26, 2022
View reviewed changes
felixarntz
felixarntz reviewed Aug 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@ankitrox Only a few nit-picks left.

As @eugene-manuilov suggested, please remove #491 from the PR mentions. While this PR does part of introducing the default screen, it doesn't fully make it available, so let's work on that in a separate PR.

@felixarntz felixarntz mentioned this pull request Aug 26, 2022
Merged
3 tasks
@ankitrox ankitrox removed a link to an issue Aug 29, 2022
Create Background Processing Infrastructure: Create Queue Admin Screen #491
Closed
@ankitrox
Copy link
Copy Markdown
Author

ankitrox commented Aug 29, 2022

@felixarntz Addressed the feedback. Also, removed #491 from this PR.

@ankitrox ankitrox requested a review from felixarntz August 29, 2022 09:38
felixarntz
felixarntz approved these changes Aug 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@ankitrox Great work!

@eugene-manuilov Could you please re-review? I left replies to your concerns. Let's address the admin screen details in a follow up.

eugene-manuilov
eugene-manuilov approved these changes Aug 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@eugene-manuilov eugene-manuilov left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @ankitrox.

@ankitrox ankitrox merged commit 68f1e51 into feature/regenerate-existing-images Aug 30, 2022
@ankitrox ankitrox deleted the feature/background-job-taxonomy branch August 30, 2022 17:06
@felixarntz felixarntz mentioned this pull request Aug 30, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eugene-manuilov eugene-manuilov eugene-manuilov approved these changes

@felixarntz felixarntz felixarntz approved these changes

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

@getsource getsource Awaiting requested review from getsource

@JustinyAhin JustinyAhin Awaiting requested review from JustinyAhin

@jjgrainger jjgrainger Awaiting requested review from jjgrainger jjgrainger is a code owner

@mukeshpanchal27 mukeshpanchal27 Awaiting requested review from mukeshpanchal27

Assignees

@ankitrox ankitrox

Labels

no milestone PRs that do not have a defined milestone for release [Plugin] Regenerate Existing Images Issues for the Regenerate Existing Images module [Type] Epic A high-level project / epic that will encompass several sub-issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Create Background Processing Infrastructure: Create background_job taxonomy

5 participants

@ankitrox @eugene-manuilov @jjgrainger @felixarntz @mukeshpanchal27