Fix teacher can see all students on the Students screen #7367
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## trunk #7367 +/- ##
============================================
+ Coverage 50.77% 50.88% +0.11%
- Complexity 11145 11149 +4
============================================
Files 613 613
Lines 47042 47064 +22
Branches 404 404
============================================
+ Hits 23884 23950 +66
+ Misses 22831 22787 -44
Partials 327 327
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
LGTM 👍 Works as expected
Create a probable relevant issue here https://github.com/Automattic/sensei-pro/issues/2501
includes/class-sensei-teacher.php
Outdated
|
|
||
| return str_replace( | ||
| 'WHERE 1=1', | ||
| 'WHERE 1=1 AND u.ID IN (' . implode( ',', array_map( 'absint', $teacher_learner_ids ) ) . ')', |
There was a problem hiding this comment.
Just a thought about it I wanted to share just in case you feel the same, not too concerned about it, usually a site doesn't have thousands of course or lessons, but students can in many cases be in the thousands. Do you think it might be a problem in some cases as the query may become too big and exceed the size limit?
There was a problem hiding this comment.
Thanks for pointing this out. I agree, this could be an issue in some cases. The default query size (max_allowed_packet) on newer MySQL versions is 16 MB, but on older ones, it is only 1 MB. I will check if this can be done with a join.
There was a problem hiding this comment.
Done! Could you please take another look? 🙏
I've also added HPPS support. 😄 cc @merkushin
|
There's a small linter issue here after I merged from |
|
Fixed, thanks. 🙂 |
There was a problem hiding this comment.
Looks good and woks as expected. I've left some minor comments. I've found only one issue during my testing that I think can be worth looking at.
In the same page (SenseiLMS -> Students), I removed a user from the course using the dropdown action on the top. But the the student was still showing up in the student list, with Enrolled Courses as N/A. If it's relevant, maybe we can think about fixing it here. If you think it's not relevant to this PR and should be handled in a separate one, feel free to do that too in which case we can merge this one.
Hm, this is an interesting problem. In the current state, it's showing all students who have progress in the course, it doesn't check the enrollment. The student is removed from the list only if you remove the user from the course and then reset the progress. I guess it makes some sense to keep showing students-with-progress because they could have data on the grading screen. A potential fix could be to reset the progress when a user is removed from a course. The funny part is that we use the same check-the-progress logic in reports to count the enrolled users (1, 2) and I assume we do it because there are multiple ways to enroll the user and it can get complicated. For now, I will open a separate issue to illustrate the problem. 👍 Thanks for the awesome feedback, btw! 💯 |
There was a problem hiding this comment.
Looks great! Left a minor comment about a comment, feel free to merge if you chose to address it. Approved anyway 😁
For now, I will open a separate issue to illustrate the problem. 👍
Awesome! Maybe adding a mention of this PR on that issue would help us get the reference.
Thanks for the awesome feedback, btw! 💯
Thank you! <3
Resolves #6044
Proposed Changes
Testing Instructions
Pre-Merge Checklist