Fix/last activity sort

[mikeyarce]

Fixes #5335

Changes proposed in this Pull Request

Previously, we sorted Students by Last Activity Date by manipulating the DB query. In #5104 we extracted that out of the query and now we add that information to the query results to each user object. However, doing that broke the sorting by last activity.

What I'm doing here is moving the logic from parsing query arguments before the query to ordering after the query has been done.

Testing instructions

• Go to Sensei LMS -> Students
• Have at least 3 students with different Last Activity times.
• Click on the Last Activity column header
• Make sure both ASC and DESC work as expected

Screenshot / Video

[merkushin]

Hey @mikeyarce 👋
Here, I think, we sort students only on the current page, because $query->get_all() returns only part of the list (limited with per_page and offset).
That will lead to surprising results :) Have you tested it when you have two or three pages of students?

[mikeyarce]

Thanks @merkushin !
What do you think about alerting the SQL query here when the orderby is set to last_activity_date?

Feels a bit messy but something like this?

$sql = "
	SELECT SQL_CALC_FOUND_ROWS
		`u`.`ID` AS 'user_id',
		`u`.`user_nicename`,
		`u`.`user_login`,
		`u`.`user_email`,
		'' AS 'course_statuses',
		0 AS 'course_count',";

if ( 'last_activity_date' === $this->order_by ) {
	$sql .= "
	COUNT(`c`.`comment_approved`) AS 'course_count', (
		SELECT MAX(cm.comment_date_gmt)
		FROM {$wpdb->comments} cm
		WHERE cm.user_id = u.ID
		AND cm.comment_approved IN ('complete', 'passed', 'graded')
		AND cm.comment_type = 'sensei_lesson_status'
	) AS last_activity_date";
}

$sql .= " FROM `{$wpdb->users}` AS `u`";

if ( 'last_activity_date' === $this->order_by ) {
	$sql .= "
		LEFT JOIN `{$wpdb->comments}` AS `c`
		ON `u`.`ID` = `c`.`user_id` AND `c`.`comment_type` = 'sensei_course_status'";
}

[mikeyarce]

I should note that I did test the above with a few pages of users and it does work.

[merkushin]

My main concern about the latest solution is related to performance.
Have you tested it on large databases (with a lot of students: thousands)?
You can try our CLI command for this purpose: https://github.com/Automattic/sensei/blob/trunk/includes/cli/class-sensei-db-seed-command.php
Maybe @Imran92 could advise here as he put a lot of time discovering performance issues in reports.

[renatho]

Hey there! I think I had the same issue opened here with some more context: #5613

I also added the origin of the problem (handling envs where we can't make relationships between users tables and other tables) and some suggestions on how to fix it.

I'm also adding the issue to this PR to be closed when this is merged.

[mikeyarce]

Thanks @renatho ! It's been on the backburner for a while - of the options you listed, which one would you go for? Just remvoing the sorting, or having a conditional query?

[renatho]

@mikeyarce, I'd say that the conditional would be the best approach, so we offer the best possible solution to each scenario, but if we want just to make a quick solution, we could just remove the sorting for now and enhance it in the future.

[m1r0]

As stated in #5613, we should come back to this once we have the custom tables. The last activity sort will be disabled until then.