diff --git a/classes/manager.php b/classes/manager.php index 5a50c15..1625c26 100644 --- a/classes/manager.php +++ b/classes/manager.php @@ -16,6 +16,7 @@ namespace local_sitsgradepush; +use context_course; use core_component; use core_course\customfield\course_handler; use DirectoryIterator; @@ -1438,6 +1439,37 @@ public function get_all_summative_grade_items(int $courseid): array { return $results; } + /** + * Delete assessment mapping. + * + * @param int $courseid + * @param int $mappingid + * @return void + * @throws \dml_exception + * @throws \moodle_exception + */ + public function remove_mapping(int $courseid, int $mappingid): void { + global $DB; + + // Check permission. Assume the user who has permission to map assessment is allowed to remove mapping too. + if (!has_capability('local/sitsgradepush:mapassessment', context_course::instance($courseid))) { + throw new \moodle_exception('error:remove_mapping', 'local_sitsgradepush'); + } + + // Check the mapping exists. + if (!$DB->record_exists(self::TABLE_ASSESSMENT_MAPPING, ['id' => $mappingid])) { + throw new \moodle_exception('error:assessmentmapping', 'local_sitsgradepush', '', $mappingid); + } + + // Remove mapping is not allowed if grades have been pushed. + if ($this->has_grades_pushed($mappingid)) { + throw new \moodle_exception('error:mab_has_push_records', 'local_sitsgradepush', '', 'Mapping ID: ' . $mappingid); + } + + // Everything is fine, remove the mapping. + $DB->delete_records(self::TABLE_ASSESSMENT_MAPPING, ['id' => $mappingid]); + } + /** * Save transfer log. * diff --git a/classes/output/renderer.php b/classes/output/renderer.php index afe9237..c484b42 100644 --- a/classes/output/renderer.php +++ b/classes/output/renderer.php @@ -214,14 +214,16 @@ public function render_dashboard(array $moduledeliveries, int $courseid, int $re $assessmentmapping->name = $assessmentdata->source->get_assessment_name(); $assessmentmapping->url = $assessmentdata->source->get_assessment_url(false); $assessmentmapping->transferhistoryurl = $assessmentdata->source->get_assessment_transfer_history_url(false); + $assessmentmapping->removesourceurl = $this->get_remove_source_url($courseid, $mapping->id)->out(false); // Check if there is a task running for the assessment mapping. $taskrunning = taskmanager::get_pending_task_in_queue($mapping->id); $assessmentmapping->taskrunning = !empty($taskrunning); $assessmentmapping->taskprogress = $taskrunning && $taskrunning->progress ? $taskrunning->progress : 0; - // Disable the change source button if there is a task running. - $assessmentmapping->disablechangesource = + // Disable the change source button / hide the remove source button + // if grades have been pushed or there is a task running. + $assessmentmapping->disablechangesource = $assessmentmapping->hideremovesourcebutton = !empty($taskrunning) || $this->manager->has_grades_pushed($mapping->id); $componentgrade->assessmentmapping = $assessmentmapping; @@ -445,4 +447,19 @@ private function get_warning_message_for_history_page_table(\stdClass $mapping, return $warningmessage; } + + /** + * Get the remove source URL. + * + * @param int $courseid Course ID + * @param int $mapid Assessment mapping ID + * + * @return \moodle_url + */ + private function get_remove_source_url(int $courseid, int $mapid): \moodle_url { + return new \moodle_url( + '/local/sitsgradepush/dashboard.php', + ['id' => $courseid, 'mapid' => $mapid, 'action' => 'removesource'] + ); + } } diff --git a/dashboard.php b/dashboard.php index 3a0abd5..ce44e28 100644 --- a/dashboard.php +++ b/dashboard.php @@ -26,6 +26,7 @@ namespace local_sitsgradepush; use context_course; +use core\output\notification; use moodle_exception; use moodle_url; @@ -51,6 +52,27 @@ // Check user's capability. require_capability('local/sitsgradepush:mapassessment', $context); +// Process remove source action. +if ($action = optional_param('action', '', PARAM_ALPHA)) { + if ($action === 'removesource') { + $mapid = required_param('mapid', PARAM_INT); + try { + manager::get_manager()->remove_mapping($courseid, $mapid); + // Reload the page. + redirect(new moodle_url('/local/sitsgradepush/dashboard.php', ['id' => $courseid])); + } catch (moodle_exception $e) { + // Add notification. + redirect(new moodle_url( + '/local/sitsgradepush/dashboard.php', + ['id' => $courseid]), + $e->getMessage(), + null, + notification::NOTIFY_ERROR + ); + } + } +} + $header = get_string('dashboard:header', 'local_sitsgradepush'); $param = ['id' => $courseid]; if ($reassess == 1) { diff --git a/lang/en/local_sitsgradepush.php b/lang/en/local_sitsgradepush.php index 006f844..bea483b 100644 --- a/lang/en/local_sitsgradepush.php +++ b/lang/en/local_sitsgradepush.php @@ -56,6 +56,8 @@ $string['dashboard:moduledelivery'] = 'MODULE DELIVERY'; $string['dashboard:moodle_activity'] = 'Moodle activity'; $string['dashboard:reassessment_page'] = 'Re-assessment'; +$string['dashboard:remove_btn_content'] = 'Are you sure you want to remove source {$a->sourcename} for SITS assessment ({$a->mabseq}) {$a->mabname}?'; +$string['dashboard:remove_btn_title'] = 'Remove source'; $string['dashboard:seq'] = 'SEQ'; $string['dashboard:sits_assessment'] = 'SITS assessment'; $string['dashboard:source'] = 'Source'; @@ -101,7 +103,7 @@ $string['error:invalid_source_type'] = 'Invalid source type. {$a}'; $string['error:lesson_practice'] = 'Practice lessons have no grades'; $string['error:lti_no_grades'] = 'LTI activity is set to not send grades to gradebook'; -$string['error:mab_has_push_records'] = 'Assessment component mapping cannot be updated as marks have been transfered for {$a}'; +$string['error:mab_has_push_records'] = 'Assessment component mapping cannot be updated as marks have been transferred for {$a}'; $string['error:mab_invalid_for_mapping'] = 'This assessment component is not valid for mapping due to the following reasons: {$a}.'; $string['error:mab_not_found'] = 'Assessment component not found. ID: {$a}'; $string['error:mapassessment'] = 'You do not have permission to map assessment.'; @@ -119,6 +121,7 @@ $string['error:pastcourse'] = 'It looks like this course is from a previous academic year, marks transfer is not allowed.'; $string['error:pushgradespermission'] = 'You do not have permission to transfer marks.'; $string['error:reassessmentdisabled'] = 'Re-assessment marks transfer is disabled.'; +$string['error:remove_mapping'] = 'You do not have permission to remove mapping.'; $string['error:resit_number_zero_for_reassessment'] = 'Student resit number should not be zero for a reassessment.'; $string['error:same_map_code_for_same_activity'] = 'An activity cannot be mapped to more than one assessment component with same map code'; $string['error:studentnotfound'] = 'Student with idnumber {$a->idnumber} not found for component grade {$a->componentgrade}'; diff --git a/templates/dashboard.mustache b/templates/dashboard.mustache index 016055b..da8aaa8 100644 --- a/templates/dashboard.mustache +++ b/templates/dashboard.mustache @@ -98,7 +98,8 @@ "transferhistoryurl": "http://test.m4.local:4001/local/sitsgradepush/index.php?id=22", "taskrunning": false, "taskprogress": "0", - "disablechangesource": false + "disablechangesource": false, + "hideremovesourcebutton": false } } }, @@ -169,6 +170,17 @@ {{#str}} dashboard:changesource, local_sitsgradepush {{/str}} for {{mabseq}} {{mabname}} + {{/currentacademicyear}} {{/assessmentmapping}} {{#currentacademicyear}} diff --git a/tests/behat/behat_sitsgradepush.php b/tests/behat/behat_sitsgradepush.php index 7a3c876..675ff2c 100644 --- a/tests/behat/behat_sitsgradepush.php +++ b/tests/behat/behat_sitsgradepush.php @@ -120,10 +120,10 @@ public function i_click_on_the_button_for(string $buttontext, string $rowtext): $page = $this->getSession()->getPage(); // Buttons which are button type, i.e. not links. - $buttons = ['Select', 'Transfer marks']; + $buttons = ['Select', 'Transfer marks', 'Remove source']; // Determine the XPath based on button type. - if (in_array($buttontext, ['Select source', 'Change source', 'Transfer marks'])) { + if (in_array($buttontext, ['Select source', 'Change source', 'Transfer marks', 'Remove source'])) { $xpath = "//tr[th[contains(text(), '$rowtext')]]"; } else if ($buttontext == 'Select') { $xpath = "//tr[td[contains(text(), '$rowtext')]]"; @@ -179,6 +179,22 @@ public function i_should_see_activity_is_mapped_for(string $linktext, string $si } } + /** + * Check if a SITS assessment is not mapped. + * + * @param string $sitsassessment + * @throws \Exception + * + * @Then I should see :sitsassessment is not mapped + */ + public function i_should_see_sits_assessment_is_not_mapped(string $sitsassessment): void { + global $DB; + $mab = $DB->get_record(manager::TABLE_COMPONENT_GRADE, ['mabname' => $sitsassessment]); + if (manager::get_manager()->is_component_grade_mapped($mab->id, 0)) { + throw new Exception("SITS assessment '$sitsassessment' is mapped."); + } + } + /** * Set up the course for marks transfer. * diff --git a/tests/behat/remove_source.feature b/tests/behat/remove_source.feature new file mode 100644 index 0000000..2573245 --- /dev/null +++ b/tests/behat/remove_source.feature @@ -0,0 +1,41 @@ +@local @local_sitsgradepush + +Feature: Remove Moodle source for SITS assessment component + As a teacher with the appropriate permissions + I can remove a Moodle source for a SITS assessment component + + Background: + Given the following "users" exist: + | username | firstname | lastname | idnumber | email | + | teacher1 | Teacher1 | Test | tea1 | teacher1@example.com | + And the following custom field exists for grade push: + | category | CLC | + | shortname | course_year | + | name | Course Year | + | type | text | + And the following "courses" exist: + | fullname | shortname | format | customfield_course_year | + | Course 1 | C1 | topics | ##now##%Y## | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + And the following "permission overrides" exist: + | capability | permission | role | contextlevel | reference | + | local/sitsgradepush:mapassessment | Allow | editingteacher | Course | C1 | + And the course "C1" is set up for marks transfer + And the following "activities" exist: + | activity | name | intro | course | idnumber | section | + | assign | Assign 1 | A1 desc | C1 | assign1 | 1 | + | quiz | Quiz 1 | Q1 desc | C1 | quiz1 | 1 | + And the course "C1" is regraded + And the "mod" "assign1" is mapped to "72hr take-home examination (3000 words)" + + @javascript + Scenario: Remove source for a SITS assessment component + Given I am on the "Course 1" course page logged in as teacher1 + And I click on "More" "link" in the ".secondary-navigation" "css_element" + And I select "SITS Marks Transfer" from secondary navigation + And I should see "Assign 1" is mapped to "72hr take-home examination (3000 words)" + And I click on the "Remove source" button for "72hr take-home examination (3000 words)" + And I click on "Confirm" "button" in the "Remove source" "dialogue" + Then I should see "72hr take-home examination (3000 words)" is not mapped diff --git a/tests/manager_test.php b/tests/manager_test.php index 6954300..e678f0b 100644 --- a/tests/manager_test.php +++ b/tests/manager_test.php @@ -26,6 +26,7 @@ use local_sitsgradepush\api\irequest; use local_sitsgradepush\assessment\assessment; use local_sitsgradepush\assessment\assessmentfactory; +use moodle_exception; defined('MOODLE_INTERNAL') || die(); global $CFG; @@ -1286,6 +1287,73 @@ public function test_get_mab_by_mapping_id(): void { $this->assertEquals($this->mab1->id, $mab->id); } + /** + * Test the remove mapping method. + * + * @covers \local_sitsgradepush\manager::remove_mapping + * @return void + * @throws \ReflectionException + * @throws \coding_exception + * @throws \dml_exception + * @throws \moodle_exception + */ + public function test_remove_mapping(): void { + global $DB; + // Set up the test environment. + $this->setup_testing_environment(assessmentfactory::get_assessment('mod', $this->assign1->cmid)); + $this->setUser($this->teacher1); + + try { + // Test removing mapping without capability. + $this->manager->remove_mapping($this->course1->id, $this->mappingid1); + } catch (\moodle_exception $e) { + $this->assertStringContainsString(get_string('error:remove_mapping', 'local_sitsgradepush'), $e->getMessage()); + } + + // Test mapping not exists. + try { + // Create role. + $roleid = $this->dg->create_role(['shortname' => 'canmapassessment']); + assign_capability( + 'local/sitsgradepush:mapassessment', + CAP_ALLOW, + $roleid, + context_course::instance($this->course1->id)->id + ); + $this->dg->enrol_user($this->teacher1->id, $this->course1->id, 'canmapassessment'); + $this->manager->remove_mapping($this->course1->id, 0); + } catch (\moodle_exception $e) { + $this->assertStringContainsString(get_string('error:assessmentmapping', 'local_sitsgradepush', 0), $e->getMessage()); + } + + // Test push records exist. + try { + // Add some push logs. + $transferlog = new \stdClass(); + $transferlog->type = 'pushgrade'; + $transferlog->userid = $this->student1->id; + $transferlog->assessmentmappingid = $this->mappingid1; + $transferlog->usermodified = $this->teacher1->id; + $transferlog->timecreated = time(); + $transferlogid = $DB->insert_record('local_sitsgradepush_tfr_log', $transferlog); + + // Test push logs exist. + $this->manager->remove_mapping($this->course1->id, $this->mappingid1); + } catch (\moodle_exception $e) { + $this->assertStringContainsString( + get_string('error:mab_has_push_records', 'local_sitsgradepush', 'Mapping ID: ' . $this->mappingid1), + $e->getMessage() + ); + } + + // Remove the push logs to allow mapping removal. + $DB->delete_records('local_sitsgradepush_tfr_log', ['id' => $transferlogid]); + + // Test successful removal of the mapping. + $this->manager->remove_mapping($this->course1->id, $this->mappingid1); + $this->assertFalse($DB->record_exists('local_sitsgradepush_mapping', ['id' => $this->mappingid1])); + } + /** * Set default configurations for the tests. *