Skip to content

Commit

Permalink
improve handling of (potentially) invalid attempt ids when opening th…
Browse files Browse the repository at this point in the history
…e view page
  • Loading branch information
Glutamat42 committed Jun 2, 2024
1 parent 4cc6755 commit 5e11984
Show file tree
Hide file tree
Showing 4 changed files with 276 additions and 16 deletions.
8 changes: 4 additions & 4 deletions classes/local/db/moodle_core_repository.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,17 @@ public function create_question_reference(stdClass $question_reference): int {
/**
* Retrieves the course module ID (cmid) for a given question usage ID.
*
* @param int $quid The ID of the question usage.
* @param int $question_usage_id The ID of the question usage.
* @return int The course module ID (cmid) associated with the question usage.
* @throws dml_exception If there's an error with the database query.
* @throws dml_missing_record_exception If the expected records are not found.
*/
public function get_cmid_by_question_usage_id(int $quid): int {
public function get_cmid_by_question_usage_id(int $question_usage_id): int {
// First, retrieve the contextid from the question_usages table using the question usage ID
$contextid = $this->db->get_field('question_usages', 'contextid', ['id' => $quid], MUST_EXIST);
$contextid = $this->db->get_field('question_usages', 'contextid', ['id' => $question_usage_id]);

if (!$contextid) {
throw new dml_missing_record_exception('context not found for the provided question usage ID');
throw new dml_missing_record_exception('no context found for the provided question usage ID');
}

// Now, use the contextid to find the corresponding cmid in the context table
Expand Down
42 changes: 31 additions & 11 deletions classes/local/output/pages/view_page.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
use coding_exception;
use context_module;
use dml_exception;
use dml_missing_record_exception;
use invalid_parameter_exception;
use local_logging\logger;
use mod_adleradaptivity\local\db\adleradaptivity_repository;
use mod_adleradaptivity\local\db\moodle_core_repository;
use mod_adleradaptivity\moodle_core;
use moodle_exception;
use moodle_page;
use question_engine;
Expand All @@ -38,7 +40,7 @@ class view_page {
private adleradaptivity_task_repository $task_repository;
private adleradaptivity_question_repository $question_repository;
private adleradaptivity_attempt_repository $adleradaptivity_attempt_repository;
private adleradaptivity_repository $adleradaptivity_repository;
protected adleradaptivity_repository $adleradaptivity_repository;
private moodle_core_repository $moodle_core_repository;
private logger $logger;

Expand All @@ -63,7 +65,11 @@ public function __construct() {
$this->check_basic_permissions($course, $cm, $module_context);

// setup attempt variables (and create new attempt if required)
$quba = $this->load_or_create_question_usage_by_attempt_id($attempt_id, $cm);
try {
$quba = $this->load_or_create_question_usage_by_attempt_id($attempt_id, $cm);
} catch (invalid_parameter_exception $e) {
throw new moodle_exception('invalidattemptid', 'adleradaptivity');
}
$adleradaptivity_attempt = $this->adleradaptivity_attempt_repository->get_adleradaptivity_attempt_by_quba_id($quba->get_id());

// now having enough information to check permissions to access/edit the attempt
Expand Down Expand Up @@ -147,21 +153,27 @@ private function render_page(array $tasks, question_usage_by_activity $quba, std
}

/**
* @param int $attempt_id The attempt id as specified in the request, -1 if no attempt id was provided
* @param int|null $attempt_id The attempt id as specified in the request, null if no attempt id was provided
* @param stdClass $cm course module (in moodle form)
* @return question_usage_by_activity
* @throws dml_exception
* @throws invalid_parameter_exception
* @throws moodle_exception
*/
private function load_or_create_question_usage_by_attempt_id(int $attempt_id, stdClass $cm): question_usage_by_activity {
// Load quiz attempt if attempt parameter is not -1, otherwise create new attempt
if ($attempt_id === -1) {
private function load_or_create_question_usage_by_attempt_id(int|null $attempt_id, stdClass $cm): question_usage_by_activity {
// Load quiz attempt if attempt parameter is not null, otherwise create new attempt
if ($attempt_id === null) {
$this->logger->trace('No attempt specified, loading existing attempt if exists or creating new one');
$quba = helpers::load_or_create_question_usage($cm->id);
} else {
// Load the attempt
$this->logger->trace('Loading existing attempt. Attempt ID: ' . $attempt_id);
if ($cm->id == $this->moodle_core_repository->get_cmid_by_question_usage_id($attempt_id)) {
try {
$cmid_of_question_usage = $this->moodle_core_repository->get_cmid_by_question_usage_id($attempt_id);
} catch (dml_missing_record_exception $e) {
throw new invalid_parameter_exception('Specified attempt does not exist. Attempt ID: ' . $attempt_id);
}
if ($cm->id == $cmid_of_question_usage) {
// can only happen if attempt id was specified, otherwise only a valid one will be loaded
$quba = question_engine::load_questions_usage_by_activity($attempt_id);
} else {
Expand All @@ -183,13 +195,21 @@ private function process_request_parameters(): array {
throw new moodle_exception('invalidcoursemodule', 'adleradaptivity');
}

$attempt_id = optional_param('attempt', -1, PARAM_INT);
if ($attempt_id == 0) {
$attempt_id = optional_param('attempt', null, PARAM_RAW);
if ($attempt_id !== null &&
!is_int($attempt_id) &&
!ctype_digit($attempt_id)) {
throw new moodle_exception('invalidattemptid', 'adleradaptivity');
}
if ($attempt_id !== null) {
$attempt_id = intval($attempt_id);
if ($attempt_id < 0) {
throw new moodle_exception('invalidattemptid', 'adleradaptivity');
}
}

$cm = get_coursemodule_from_id('adleradaptivity', $cmid, 0, false, MUST_EXIST);
$course = get_course($cm->course);
$cm = moodle_core::get_coursemodule_from_id('adleradaptivity', $cmid, 0, false, MUST_EXIST);
$course = moodle_core::get_course($cm->course);
$module_instance = $this->adleradaptivity_repository->get_instance_by_instance_id($cm->instance);
return array($attempt_id, $cm, $course, $module_instance);
}
Expand Down
241 changes: 241 additions & 0 deletions tests/local/output/pages/view_page_test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
<?php

namespace mod_adleradaptivity\local\output\pages;

use context_module;
use Mockery;
use mod_adleradaptivity\lib\adler_testcase;
use mod_adleradaptivity\moodle_core;
use moodle_exception;
use question_engine;
use ReflectionClass;

global $CFG;
require_once($CFG->dirroot . '/mod/adleradaptivity/tests/lib/adler_testcase.php');
require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php');

class view_page_test extends adler_testcase {
public function provider_test_request_parameters(): array {
return [
'null' => [null],
'1' => [1],
'7' => [7],
'string "9"' => ['9'],
];
}


/**
* @dataProvider provider_test_request_parameters
*/
public function testProcessRequestParameters($attempt_id) {
// Arrange
$_GET['attempt'] = $attempt_id; // positive value for attempt
$_GET['id'] = 1; // positive value for cmid
$mockAdleradaptivityRepository = Mockery::mock('mod_adleradaptivity\local\db\adleradaptivity_repository');
$mockAdleradaptivityRepository->shouldReceive('get_instance_by_instance_id')
->andReturn((object)['instance' => 1]);

$viewPage = Mockery::mock(view_page::class)->makePartial()->shouldAllowMockingProtectedMethods();

// Mock MoodleCore functions
$mockMoodleCore = Mockery::mock('alias:' . moodle_core::class);
$mockMoodleCore->shouldReceive('get_coursemodule_from_id')
->andReturn((object)['course' => 1, 'instance' => 1]);
$mockMoodleCore->shouldReceive('get_course')
->andReturn((object)['fullname' => 'Test Course']);

// Use reflection to set the private property
$reflection = new ReflectionClass(view_page::class);
$property = $reflection->getProperty('adleradaptivity_repository');
$property->setAccessible(true);
$property->setValue($viewPage, $mockAdleradaptivityRepository);

// Use reflection to call the private method
$method = $reflection->getMethod('process_request_parameters');
$method->setAccessible(true);

// Act
$result = $method->invoke($viewPage);

// Assert
$this->assertIsArray($result);
$this->assertCount(4, $result);
$this->assertEquals($attempt_id, $result[0]); // attempt_id
$this->assertEquals(1, $result[1]->instance); // cm
$this->assertEquals('Test Course', $result[2]->fullname); // course
$this->assertEquals(1, $result[3]->instance); // module_instance
}

public function invalidAttemptProvider(): array {
return [
'-7' => [-7],
'abc' => ['abc'],
'7.4' => [7.4],
];
}

/**
* @dataProvider invalidAttemptProvider
*/
public function testProcessRequestParametersWithInvalidAttempt($invalidAttempt) {
// Arrange
$_GET['attempt'] = $invalidAttempt;
$_GET['id'] = 1; // positive value for cmid
$mockAdleradaptivityRepository = Mockery::mock('mod_adleradaptivity\local\db\adleradaptivity_repository');
$mockAdleradaptivityRepository->shouldReceive('get_instance_by_instance_id')
->andReturn((object)['instance' => 1]);

$viewPage = Mockery::mock(view_page::class)->makePartial()->shouldAllowMockingProtectedMethods();

// Mock MoodleCore functions
$mockMoodleCore = Mockery::mock('alias:' . moodle_core::class);
$mockMoodleCore->shouldReceive('get_coursemodule_from_id')
->andReturn((object)['course' => 1, 'instance' => 1]);
$mockMoodleCore->shouldReceive('get_course')
->andReturn((object)['fullname' => 'Test Course']);

// Use reflection to set the private property
$reflection = new ReflectionClass(view_page::class);
$property = $reflection->getProperty('adleradaptivity_repository');
$property->setAccessible(true);
$property->setValue($viewPage, $mockAdleradaptivityRepository);

// Use reflection to call the private method
$method = $reflection->getMethod('process_request_parameters');
$method->setAccessible(true);

// Assert
$this->expectException(moodle_exception::class);
$this->expectExceptionMessage('invalidattemptid');

// Act
$method->invoke($viewPage);
}

/**
* @runInSeparateProcess
*/
public function testProcessRequestParametersWithNonExistingAttempt() {
// create two users
$user1 = $this->getDataGenerator()->create_user();

// create a course
$course = $this->getDataGenerator()->create_course();

// enrol the users in the course
$this->getDataGenerator()->enrol_user($user1->id, $course->id);

// create adler adaptivity instance
$adleradaptivity = $this->getDataGenerator()->create_module('adleradaptivity', ['course' => $course->id]);

// create an attempt
// first create a moodle attempt
$module_context = context_module::instance($adleradaptivity->cmid);
$moodle_attempt = question_engine::make_questions_usage_by_activity('mod_adleradaptivity', $module_context);
$moodle_attempt->set_preferred_behaviour(1);
question_engine::save_questions_usage_by_activity($moodle_attempt);
// then create an adleradaptivity attempt
$attempt = $this->getDataGenerator()->get_plugin_generator('mod_adleradaptivity')->create_mod_adleradaptivity_attempt($moodle_attempt->get_id(), $user1->id);

// login as user1
$this->setUser($user1);

// Arrange
$_GET['id'] = $adleradaptivity->cmid;
// Access non existent attempt
$_GET['attempt'] = $moodle_attempt->get_id() + 1;

// expect exception
$this->expectException(moodle_exception::class);
$this->expectExceptionMessage('invalidattemptid');

// Load the page to test
new view_page();
}

/**
* @runInSeparateProcess
*/
public function testProcessRequestParametersWithAttemptOfOtherUser() {
// create two users
$user1 = $this->getDataGenerator()->create_user();
$user2 = $this->getDataGenerator()->create_user();

// create a course
$course = $this->getDataGenerator()->create_course();

// enrol the users in the course
$this->getDataGenerator()->enrol_user($user1->id, $course->id);
$this->getDataGenerator()->enrol_user($user2->id, $course->id);

// create adler adaptivity instance
$adleradaptivity = $this->getDataGenerator()->create_module('adleradaptivity', ['course' => $course->id]);

// create an attempt
// first create a moodle attempt
$module_context = context_module::instance($adleradaptivity->cmid);
$moodle_attempt = question_engine::make_questions_usage_by_activity('mod_adleradaptivity', $module_context);
$moodle_attempt->set_preferred_behaviour(1);
question_engine::save_questions_usage_by_activity($moodle_attempt);
// then create an adleradaptivity attempt
$attempt = $this->getDataGenerator()->get_plugin_generator('mod_adleradaptivity')->create_mod_adleradaptivity_attempt($moodle_attempt->get_id(), $user1->id);

// login as user2
$this->setUser($user2);

// Arrange
$_GET['id'] = $adleradaptivity->cmid;
// Access non existent attempt
$_GET['attempt'] = $moodle_attempt->get_id();

// expect exception
$this->expectException(moodle_exception::class);
$this->expectExceptionMessage('do not currently have permissions');

// Load the page to test
new view_page();
}

/**
* @runInSeparateProcess
*/
public function testProcessRequestParametersWithAttemptOfOtherCm() {
// create two users
$user1 = $this->getDataGenerator()->create_user();

// create a course
$course = $this->getDataGenerator()->create_course();

// enrol the users in the course
$this->getDataGenerator()->enrol_user($user1->id, $course->id);

// create adler adaptivity instance
$adleradaptivity = $this->getDataGenerator()->create_module('adleradaptivity', ['course' => $course->id]);
$adleradaptivity2 = $this->getDataGenerator()->create_module('adleradaptivity', ['course' => $course->id]);

// create an attempt
// first create a moodle attempt
$module_context = context_module::instance($adleradaptivity->cmid);
$moodle_attempt = question_engine::make_questions_usage_by_activity('mod_adleradaptivity', $module_context);
$moodle_attempt->set_preferred_behaviour(1);
question_engine::save_questions_usage_by_activity($moodle_attempt);
// then create an adleradaptivity attempt
$attempt = $this->getDataGenerator()->get_plugin_generator('mod_adleradaptivity')->create_mod_adleradaptivity_attempt($moodle_attempt->get_id(), $user1->id);

// login as user2
$this->setUser($user1);

// Arrange
$_GET['id'] = $adleradaptivity2->cmid;
// Access non existent attempt
$_GET['attempt'] = $moodle_attempt->get_id();

// expect exception
$this->expectException(moodle_exception::class);
$this->expectExceptionMessage('invalidattemptid');

// Load the page to test
new view_page();
}
}
1 change: 0 additions & 1 deletion view.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,5 @@

// TODO: attempts report (likely h5p)
// TODO permission check (only own latest attempt, except for admin, also in processattempt)
// TODO: better handling of invalid, not existing, or otherwise incorrect attempt parameter value

new view_page();

0 comments on commit 5e11984

Please sign in to comment.