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

Fix memory issue on the student courses reports screen #7468

Merged
merged 5 commits into from Feb 7, 2024

Conversation

m1r0
Copy link
Member

@m1r0 m1r0 commented Feb 5, 2024

Resolves #7439

This PR fixes an "out of memory" error on the student courses reports screen on sites with many students.

The main issue was when checking if the current user has access to the student's report screen. For that, the code was fetching the course status for all students for all courses. To make it worse, WP_Metadata_Lazyloader tries to fetch all the comment metas at once and fills the memory of the server.

Before:

dCI64X.png

After:

image

Proposed Changes

  • Optimize the Sensei_Teacher::get_learner_ids_for_courses_with_edit_permission method so it will fetch the needed data in a more efficient way.
  • Prepare the Sensei_Teacher::get_learner_ids_for_courses_with_edit_permission method for HPPS. This method is not used on the front-end so the HPPS logic is not in use currently.
  • Bypass checking for screen access permission if the current user is an admin. This skips the unnecessary call to Sensei_Teacher::get_learner_ids_for_courses_with_edit_permission for admin users.

Testing Instructions

  1. Enable the WP Query plugin.
  2. Go to Sensei LMS -> Reports -> Students and select a student.
  3. The screen should be loading as before.
  4. Notice a decrease in the memory usage on that screen compared to trunk. For me, the decrease was around 5% because my local database is not that big.
  5. If you have a big database at hand, test with that.

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

@m1r0 m1r0 added the [Type] Bug label Feb 5, 2024
@m1r0 m1r0 added this to the 4.20.2 milestone Feb 5, 2024
@m1r0 m1r0 self-assigned this Feb 5, 2024
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (85fdd5f) 51.20% compared to head (b86b965) 51.26%.
Report is 7 commits behind head on trunk.

❗ Current head b86b965 differs from pull request most recent head 7910c7f. Consider uploading reports for the commit 7910c7f to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7468      +/-   ##
============================================
+ Coverage     51.20%   51.26%   +0.05%     
+ Complexity    11189    11184       -5     
============================================
  Files           614      614              
  Lines         47230    47229       -1     
  Branches        405      405              
============================================
+ Hits          24186    24211      +25     
+ Misses        22717    22691      -26     
  Partials        327      327              
Files Coverage Δ
includes/class-sensei-analysis.php 20.94% <100.00%> (+0.19%) ⬆️
includes/class-sensei-teacher.php 53.79% <100.00%> (+3.79%) ⬆️

... 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 1933acc...7910c7f. Read the comment docs.

@m1r0 m1r0 marked this pull request as ready for review February 5, 2024 16:57
@m1r0 m1r0 requested a review from a team February 5, 2024 17:21
Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

I didn't notice a performance improvement, but I did check that the data was still the same when logged in as an admin or as a teacher. Looking at the code, I guess this should also be tested with HPPS on and off?

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 too 👍

@m1r0
Copy link
Member Author

m1r0 commented Feb 7, 2024

I guess this should also be tested with HPPS on and off?

Hm, I thought covering it with unit tests should be enough but just in case I did a few more live HPPS tests as well. I'll start adding HPPS test instructions in the future. 👍

@m1r0 m1r0 merged commit 02e3436 into trunk Feb 7, 2024
22 checks passed
@m1r0 m1r0 deleted the fix/reports-memory-issue branch February 7, 2024 10: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.

Out of memory error on the student courses reports screen for sites with many students
3 participants