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 sensei email generating multiple queries on page load #6819

Merged

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Apr 18, 2023

Resolves #6815

Proposed Changes

  • Updated the check before initializing the email so that instead of generating multiple queries before enabling emails, we can do only one which improves the performance. It triggers meta cache queries, but they are very small and fast, unlike the join queries.

Testing Instructions

  1. You should have a way to monitor the queries (Can use the Query Monitor plugin)
  2. Check the generated queries when you reload a page
  3. You should see only one sensei_email query logged there

###Before

Screen.Recording.2023-04-18.at.2.59.54.PM.mov

###After
Screen Shot 2023-04-18 at 3 02 05 PM

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
  • 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

@Imran92 Imran92 requested a review from a team April 18, 2023 10:57
@Imran92 Imran92 self-assigned this Apr 18, 2023
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #6819 (f82de23) into trunk (ef6cee6) will decrease coverage by 0.04%.
Report is 10 commits behind head on trunk.
The diff coverage is 59.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #6819      +/-   ##
============================================
- Coverage     49.80%   49.76%   -0.04%     
- Complexity    10650    10653       +3     
============================================
  Files           586      586              
  Lines         45047    45055       +8     
  Branches        402      402              
============================================
- Hits          22434    22420      -14     
- Misses        22286    22308      +22     
  Partials        327      327              
Files Changed Coverage Δ
includes/admin/class-sensei-learners-main.php 5.30% <0.00%> (ø)
includes/internal/emails/class-email-generator.php 82.60% <0.00%> (-17.40%) ⬇️
...ils/generators/class-email-generators-abstract.php 41.66% <33.33%> (+41.66%) ⬆️
includes/internal/emails/class-email-sender.php 99.02% <100.00%> (ø)
...ernal/emails/generators/class-course-completed.php 100.00% <100.00%> (ø)
...nternal/emails/generators/class-course-created.php 100.00% <100.00%> (+6.06%) ⬆️
...nternal/emails/generators/class-course-welcome.php 100.00% <100.00%> (+4.16%) ⬆️
...al/emails/generators/class-new-course-assigned.php 96.29% <100.00%> (ø)
...s/internal/emails/generators/class-quiz-graded.php 90.62% <100.00%> (ø)
...ails/generators/class-student-completes-course.php 97.05% <100.00%> (ø)
... and 6 more

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 105592a...f82de23. Read the comment docs.

@jom
Copy link
Member

jom commented May 11, 2023

Hi @Imran92! Thanks for picking up this issue. This is an improvement, but I wonder if there is a way to still limit the meta queries... I'm still seeing ~38 related to this (without object caching). One way might be caching this, but then you'd need to handle invalidation. Did y'all consider delaying the check if an email was active until later on? One possibility would be a proxy callback in the hooks in the email generators' init methods that could check, or even just a check in the Email_Generators_Abstract::send_email_action method.

@Imran92
Copy link
Contributor Author

Imran92 commented Jul 7, 2023

Thanks a lot for the review @jom! I've updated the PR to use a intermediate callback in the middle. Checking at the time of send_email_action would be simpler, but in that case, all the processing to prepare the email data would still be done even if the email was disabled. There are some changes in pro related to this. Please take a look again when possible.
Thanks!

donnapep
donnapep previously approved these changes Jul 17, 2023
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.

Seems to work! 👍🏻

@donnapep donnapep added this to the 4.16.1 milestone Jul 17, 2023
@donnapep
Copy link
Collaborator

This one is pending a fix. @Imran92 would you mind describing what's left to do for this one so that it's not lost? Thx!

@Imran92
Copy link
Contributor Author

Imran92 commented Aug 7, 2023

Suppose someone has both sensei and sensei-pro and updates only sensei but not sensei pro or delays updating, as the check for being active has been moved from core to individual email generator. In that case, their pro emails will still be active even when deactivated during that time frame. We need to put a check in place to prevent that from happening.

@Imran92 Imran92 modified the milestones: 4.16.1, 4.16.2 Aug 10, 2023
@donnapep donnapep self-requested a review August 28, 2023 18:52
@Imran92
Copy link
Contributor Author

Imran92 commented Sep 7, 2023

Both of the issues mentioned #6819 (comment) and #6819 (comment) are taken care of in d9722c2 and 083af2d :)

@donnapep donnapep modified the milestones: 4.17.0, 4.17.1 Sep 12, 2023
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.

Emails are still working, and I no longer see 1 query per email. 🎉 The 1 query I do see on the emails page isn't the same as the screenshot in the PR description though:

Screenshot 2023-09-19 at 2 40 15 PM

@Imran92
Copy link
Contributor Author

Imran92 commented Sep 20, 2023

The 1 query I do see on the emails page isn't the same as the screenshot in the PR description though:

Yeah, I believe it's the one that fetches the emails to populate the list we see when we're on Sensei LMS -> Settings -> Emails -> Student Emails. When we're on any other page, it shouldn't generate any query ☺️

@Imran92 Imran92 merged commit 44e5f50 into trunk Sep 20, 2023
23 of 24 checks passed
@Imran92 Imran92 deleted the fix/sensei-email-generating-multiple-queries-on-page-load branch September 20, 2023 14:21
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.

Multiple Email queries running on every page load
4 participants