Merge pull request #7468 from Automattic/fix/reports-memory-issue

Fix memory issue on the student courses reports screen

changelog/fix-reports-memory-issue

@@ -0,0 +1,4 @@
+Significance: patch
+Type: fixed
+
+Memory issue on the student reports screen

includes/class-sensei-analysis.php

@@ -919,9 +919,8 @@ private function check_course_lesson( $course_id, $lesson_id, $user_id ) {
 
 		if (
 			$user_id
-			&& (
-				! in_array( $user_id, Sensei()->teacher->get_learner_ids_for_courses_with_edit_permission(), true )
-			)
+			&& ! current_user_can( 'manage_options' ) // Admins can access any user.
+			&& ! in_array( $user_id, Sensei()->teacher->get_learner_ids_for_courses_with_edit_permission(), true )
 		) {
 			wp_die( esc_html__( 'Invalid user', 'sensei-lms' ), 404 );
 		}

includes/class-sensei-teacher.php

@@ -1205,51 +1205,35 @@ public function get_learner_ids_for_courses_with_edit_permission() {
 			return [];
 		}
 
-		$learner_ids_for_courses_with_edit_permission = [];
+		$course_ids             = wp_list_pluck( $courses_with_edit_permission, 'ID' );
+		$course_ids_placeholder = implode( ',', array_fill( 0, count( $course_ids ), '%d' ) );
 
-		foreach ( $courses_with_edit_permission as $course ) {
-
-			$course_learner_ids = array();
-			$activity_comments  = Sensei_Utils::sensei_check_for_activity(
-				array(
-					'post_id' => $course->ID,
-					'type'    => 'sensei_course_status',
-					'field'   => 'user_id',
-				),
-				true
+		global $wpdb;
+		if ( Progress_Storage_Settings::is_tables_repository() ) {
+			// phpcs:disable WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- Using a direct query for performance reasons.
+			$learner_ids = (array) $wpdb->get_col(
+				$wpdb->prepare(
+					"SELECT DISTINCT user_id
+					FROM {$wpdb->prefix}sensei_lms_progress
+					WHERE type = 'course'
+					AND post_id IN ( $course_ids_placeholder )", // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared, WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare -- Placeholders created dynamically.
+					$course_ids
+				)
+			);
+		} else {
+			// phpcs:disable WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- Using a direct query for performance reasons.
+			$learner_ids = (array) $wpdb->get_col(
+				$wpdb->prepare(
+					"SELECT DISTINCT user_id
+					FROM {$wpdb->comments}
+					WHERE comment_type = 'sensei_course_status'
+					AND comment_post_ID IN ( $course_ids_placeholder )", // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared, WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare -- Placeholders created dynamically.
+					$course_ids
+				)
 			);
-
-			if ( empty( $activity_comments ) || ( is_array( $activity_comments ) && ! ( count( $activity_comments ) > 0 ) ) ) {
-				continue; // skip to the next course as there are no users on this course
-			}
-
-			// it could be an array of comments or a single comment
-			if ( is_array( $activity_comments ) ) {
-
-				foreach ( $activity_comments as $comment ) {
-
-					$user = get_userdata( $comment->user_id );
-
-					if ( empty( $user ) ) {
-						// next comment in this array
-						continue;
-					}
-
-					$course_learner_ids[] = $user->ID;
-				}
-			} else {
-
-				$user                 = get_userdata( $activity_comments->user_id );
-				$course_learner_ids[] = $user->ID;
-
-			}
-
-			// add learners on this course to the all courses learner list
-			$learner_ids_for_courses_with_edit_permission = array_merge( $learner_ids_for_courses_with_edit_permission, $course_learner_ids );
-
 		}
 
-		return $learner_ids_for_courses_with_edit_permission;
+		return array_map( 'intval', $learner_ids );
 	}
 
 	/**

tests/framework/trait-sensei-hpps-helpers.php

@@ -7,7 +7,13 @@
 
 // phpcs:disable WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid -- Using PHPUnit conventions.
 
+use Sensei\Internal\Quiz_Submission\Answer\Repositories\Answer_Repository_Factory;
+use Sensei\Internal\Quiz_Submission\Grade\Repositories\Grade_Repository_Factory;
+use Sensei\Internal\Quiz_Submission\Submission\Repositories\Submission_Repository_Factory;
 use Sensei\Internal\Services\Progress_Storage_Settings;
+use Sensei\Internal\Student_Progress\Course_Progress\Repositories\Course_Progress_Repository_Factory;
+use Sensei\Internal\Student_Progress\Lesson_Progress\Repositories\Lesson_Progress_Repository_Factory;
+use Sensei\Internal\Student_Progress\Quiz_Progress\Repositories\Quiz_Progress_Repository_Factory;
 
 if ( ! defined( 'ABSPATH' ) ) {
 	exit;
@@ -21,9 +27,30 @@
 trait Sensei_HPPS_Helpers {
 	private function enable_hpps_tables_repository() {
 		Sensei()->settings->settings['experimental_progress_storage_repository'] = Progress_Storage_Settings::TABLES_STORAGE;
+
+		$this->_course_progress_repository = Sensei()->course_progress_repository;
+		$this->_lesson_progress_repository = Sensei()->lesson_progress_repository;
+		$this->_quiz_progress_repository   = Sensei()->quiz_progress_repository;
+		$this->_quiz_submission_repository = Sensei()->quiz_submission_repository;
+		$this->_quiz_answer_repository     = Sensei()->quiz_answer_repository;
+		$this->_quiz_grade_repository      = Sensei()->quiz_grade_repository;
+
+		Sensei()->course_progress_repository = ( new Course_Progress_Repository_Factory( true, true ) )->create();
+		Sensei()->lesson_progress_repository = ( new Lesson_Progress_Repository_Factory( true, true ) )->create();
+		Sensei()->quiz_progress_repository   = ( new Quiz_Progress_Repository_Factory( true, true ) )->create();
+		Sensei()->quiz_submission_repository = ( new Submission_Repository_Factory( true, true ) )->create();
+		Sensei()->quiz_answer_repository     = ( new Answer_Repository_Factory( true, true ) )->create();
+		Sensei()->quiz_grade_repository      = ( new Grade_Repository_Factory( true, true ) )->create();
 	}
 
 	private function reset_hpps_repository() {
 		Sensei()->settings->settings['experimental_progress_storage_repository'] = Progress_Storage_Settings::COMMENTS_STORAGE;
+
+		Sensei()->course_progress_repository = $this->_course_progress_repository;
+		Sensei()->lesson_progress_repository = $this->_lesson_progress_repository;
+		Sensei()->quiz_progress_repository   = $this->_quiz_progress_repository;
+		Sensei()->quiz_submission_repository = $this->_quiz_submission_repository;
+		Sensei()->quiz_answer_repository     = $this->_quiz_answer_repository;
+		Sensei()->quiz_grade_repository      = $this->_quiz_grade_repository;
 	}
 }

tests/unit-tests/test-class-sensei-analysis.php

@@ -21,12 +21,7 @@ public function setUp(): void {
 		Sensei_Test_Events::reset();
 
 		// Disable `wp_die`.
-		add_filter(
-			'wp_die_handler',
-			function() {
-				return '__return_false';
-			}
-		);
+		add_filter( 'wp_die_handler', [ $this, 'disable_wp_die' ] );
 	}
 
 	public static function setUpBeforeClass(): void {
@@ -257,6 +252,41 @@ public function testAnalysisPage_WhenUserCoursesView_LogsUserCoursesEvent() {
 		$this->assertSame( 'user-courses', $events[0]['url_args']['view'] );
 	}
 
+	public function testAnalysisPage_WhenNotAllowedToViewUser_ThrowsException() {
+		/* Arrange */
+		$this->login_as_teacher();
+		$analysis_mock = $this->createMockWithExcludedMethod( Sensei_Analysis::class, 'analysis_page' );
+		remove_filter( 'wp_die_handler', [ $this, 'disable_wp_die' ] );
+
+		$_GET = [
+			'user_id' => 1,
+		];
+
+		/* Assert */
+		$this->expectException( 'WPDieException' );
+		$this->expectExceptionMessage( 'Invalid user' );
+
+		/* Act */
+		$analysis_mock->analysis_page();
+	}
+
+	public function testAnalysisPage_WhenViewingUserAsAdmin_LoadsPage() {
+		/* Arrange */
+		$this->login_as_admin();
+		$analysis_mock = $this->createMockWithExcludedMethod( Sensei_Analysis::class, 'analysis_page' );
+		remove_filter( 'wp_die_handler', [ $this, 'disable_wp_die' ] );
+
+		$_GET = [
+			'user_id' => 1,
+		];
+
+		/* Act */
+		$analysis_mock->analysis_page();
+
+		/* Assert */
+		$this->expectNotToPerformAssertions();
+	}
+
 	/**
 	 * Returns a partial mock object for the specified class
 	 * with all of its methods mocked but one.
@@ -274,4 +304,13 @@ private function createMockWithExcludedMethod( string $class_name, string $metho
 			array_diff( $class_methods, [ $method ] )
 		);
 	}
+
+	/**
+	 * Disable the `wp_die` handler.
+	 *
+	 * @return string
+	 */
+	public function disable_wp_die() {
+		return '__return_false';
+	}
 }

tests/unit-tests/test-class-teacher.php

@@ -577,4 +577,43 @@ public function testFilterLearnersQuery_WhenTheTeacherHasNoCourses_ReturnsCourse
 AND comments.comment_type = 'sensei_course_status'";
 		$this->assertSame( $expected, $sql );
 	}
+
+	public function testGetLearnerIdsForCoursesWithEditPermission_WhenHPPSIsDisabled_ReturnsCorrectLearnerIds() {
+		// Arrange.
+		$course_1 = $this->factory->course->create();
+		$course_2 = $this->factory->course->create();
+		$user_1   = $this->factory->user->create();
+		$user_2   = $this->factory->user->create();
+
+		Sensei_Utils::start_user_on_course( $user_1, $course_1 );
+		Sensei_Utils::start_user_on_course( $user_2, $course_2 );
+
+		// Act.
+		$learner_ids = Sensei()->teacher->get_learner_ids_for_courses_with_edit_permission();
+
+		// Assert.
+		$this->assertSame( [ $user_1, $user_2 ], $learner_ids );
+	}
+
+	public function testGetLearnerIdsForCoursesWithEditPermission_WhenHPPSIsEnabled_ReturnsCorrectLearnerIds() {
+		// Arrange.
+		$this->enable_hpps_tables_repository();
+
+		$course_1 = $this->factory->course->create();
+		$course_2 = $this->factory->course->create();
+		$user_1   = $this->factory->user->create();
+		$user_2   = $this->factory->user->create();
+
+		Sensei_Utils::start_user_on_course( $user_1, $course_1 );
+		Sensei_Utils::start_user_on_course( $user_2, $course_2 );
+
+		// Act.
+		$learner_ids = Sensei()->teacher->get_learner_ids_for_courses_with_edit_permission();
+
+		// Assert.
+		$this->assertSame( [ $user_1, $user_2 ], $learner_ids );
+
+		// Reset.
+		$this->reset_hpps_repository();
+	}
 }