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

Use bulk_edit_posts hook for bulk edit handler #7526

Merged
merged 6 commits into from
Mar 11, 2024

Conversation

merkushin
Copy link
Member

This PR addresses the issue with performance degradation, noticed here: #7523 (review).

Proposed Changes

  • Use bulk_edit_posts hook for bulk edit handler.

Testing Instructions

  1. Create a few courses with lessons.
  2. Go to lessons and use bulk edit to update lesson course and complexity, for example.
  3. Make sure the changes were applied to selected lessons.
  4. Make sure Quick Edit works as well.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Legacy courses (course without blocks) are tested
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@merkushin merkushin added this to the 4.21.1 milestone Mar 7, 2024
@merkushin merkushin requested a review from a team March 7, 2024 04:47
@merkushin merkushin self-assigned this Mar 7, 2024
Copy link

github-actions bot commented Mar 7, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 96.42857% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 51.93%. Comparing base (9c9bcae) to head (7ab9b3c).
Report is 22 commits behind head on trunk.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7526      +/-   ##
============================================
- Coverage     51.93%   51.93%   -0.01%     
- Complexity    11267    11268       +1     
============================================
  Files           630      630              
  Lines         47665    47666       +1     
  Branches        420      420              
============================================
  Hits          24756    24756              
- Misses        22572    22573       +1     
  Partials        337      337              
Files Coverage Δ
includes/class-sensei-lesson.php 37.12% <96.42%> (-0.02%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7059a0d...7ab9b3c. Read the comment docs.

Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

Thanks for making the change @merkushin ! Looks like bulk_edit_posts was introduced in WP 6.3.. I was wondering if we should wait until 6.3 becomes are minimum required version (25th of this month probably) or should conditionally handle it differently for lower versions (like keeping it working with save_post but detaching the hook after first call). WDYT?

@merkushin
Copy link
Member Author

@Imran92 I think it is better to add a backward-compatibility version 👍 And it shouldn't be too hard (actually, that was one of my interim versions, but I removed it as I didn't find it useful 😅).

Copy link

github-actions bot commented Mar 7, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Mar 8, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

Test the previous changes of this PR with WordPress Playground.

Imran92
Imran92 previously approved these changes Mar 11, 2024
Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -4321,38 +4325,59 @@ public function generate_all_lessons_edit_field( $title, $field ) {
);
}

/**
* Save bulk edit fields. It is a backword compatible function for the bulk edit pre WP 6.3.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in the word 'backward'

Copy link

Test the previous changes of this PR with WordPress Playground.

@merkushin merkushin merged commit 54dafaa into trunk Mar 11, 2024
25 checks passed
@merkushin merkushin deleted the fix/use-proper-hook-for-bulk-action branch March 11, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants