From be7143381f12938741b8da28284f9d67badc36a4 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Thu, 25 Feb 2021 15:02:23 +0100 Subject: [PATCH] MDL-69331 core_h5p: Display error when main library is disabled H5P contents with the main library disabled won't be deployed; a message error will be displayed instead of. --- .../displayh5p/tests/behat/h5p_filter.feature | 50 ++++++ h5p/classes/api.php | 55 ++++++- h5p/classes/helper.php | 7 + h5p/tests/api_test.php | 144 ++++++++++++++++++ lang/en/h5p.php | 2 + 5 files changed, 252 insertions(+), 6 deletions(-) diff --git a/filter/displayh5p/tests/behat/h5p_filter.feature b/filter/displayh5p/tests/behat/h5p_filter.feature index df8f2207ed745..2663b44347a90 100644 --- a/filter/displayh5p/tests/behat/h5p_filter.feature +++ b/filter/displayh5p/tests/behat/h5p_filter.feature @@ -181,3 +181,53 @@ Feature: Render H5P content using filters And I switch to "h5p-iframe" class iframe Then I should not see "missing-required-library" And I should see "Lorum ipsum" + + @javascript + Scenario: Render local H5P file with a disabled main library + Given I log in as "admin" +# Upload H5P file to private files. + And I follow "Private files" + And I upload "h5p/tests/fixtures/ipsums.h5p" file to "Files" filemanager + And I click on "Save changes" "button" +# Upload manually the H5P content-type library and disable it. + And I navigate to "H5P > Manage H5P content types" in site administration + And I upload "h5p/tests/fixtures/ipsums.h5p" file to "H5P content type" filemanager + And I click on "Upload H5P content types" "button" in the "#fitem_id_uploadlibraries" "css_element" + And I click on "Disable" "link" in the "Accordion" "table_row" +# Add H5P content to the page. + And I am on "Course 1" course homepage with editing mode on + And I follow "PageName1" + And I navigate to "Edit settings" in current page administration + When I click on "Insert H5P" "button" in the "#fitem_id_page" "css_element" + And I click on "Browse repositories..." "button" in the "Insert H5P" "dialogue" + And I click on "Private files" "link" in the ".fp-repo-area" "css_element" + And I click on "ipsums.h5p" "link" + And I click on "Select this file" "button" + And I click on "Insert H5P" "button" in the "Insert H5P" "dialogue" + And I click on "Save and display" "button" + And I switch to "h5p-iframe" class iframe +# Library is disabled, so an error should be displayed. + Then I should see "This file can't be displayed because its content-type is disabled." + And I should not see "Lorum ipsum" + And I switch to the main frame + And I navigate to "H5P > Manage H5P content types" in site administration + And I click on "Enable" "link" in the "Accordion" "table_row" + And I am on "Course 1" course homepage +# Content should be deployed now that main library is enabled. + And I follow "PageName1" +# Switch to iframe created by filter. + And I switch to "h5p-iframe" class iframe +# Switch to iframe created by embed.php page. + And I switch to "h5p-iframe" class iframe + And I should see "Lorum ipsum" + And I should not see "This file can't be displayed because its content-type is disabled." + And I switch to the main frame + And I navigate to "H5P > Manage H5P content types" in site administration + And I click on "Disable" "link" in the "Accordion" "table_row" + And I am on "Course 1" course homepage +# Library is disabled again, so an error should be displayed. + And I follow "PageName1" + And I switch to "h5p-iframe" class iframe + And I should see "This file can't be displayed because its content-type is disabled." + And I should not see "Lorum ipsum" + And I switch to the main frame diff --git a/h5p/classes/api.php b/h5p/classes/api.php index 850904c95d0b0..f2d4a9ac083f5 100644 --- a/h5p/classes/api.php +++ b/h5p/classes/api.php @@ -251,13 +251,21 @@ public static function create_content_from_pluginfile_url(string $url, \stdClass $context = \context::instance_by_id($file->get_contextid()); if ($h5p) { // The H5P content has been deployed previously. - $displayoptions = helper::get_display_options($core, $config); - // Check if the user can set the displayoptions. - if ($displayoptions != $h5p->displayoptions && has_capability('moodle/h5p:setdisplayoptions', $context)) { - // If the displayoptions has changed and the user has permission to modify it, update this information in the DB. - $core->h5pF->updateContentFields($h5p->id, ['displayoptions' => $displayoptions]); + + // If the main library for this H5P content is disabled, the content won't be displayed. + $mainlibrary = (object) ['id' => $h5p->mainlibraryid]; + if (!self::is_library_enabled($mainlibrary)) { + $core->h5pF->setErrorMessage(get_string('mainlibrarydisabled', 'core_h5p')); + return [$file, false]; + } else { + $displayoptions = helper::get_display_options($core, $config); + // Check if the user can set the displayoptions. + if ($displayoptions != $h5p->displayoptions && has_capability('moodle/h5p:setdisplayoptions', $context)) { + // If displayoptions has changed and user has permission to modify it, update this information in DB. + $core->h5pF->updateContentFields($h5p->id, ['displayoptions' => $displayoptions]); + } + return [$file, $h5p->id]; } - return [$file, $h5p->id]; } else { // The H5P content hasn't been deployed previously. @@ -614,4 +622,39 @@ public static function set_library_enabled(int $libraryid, bool $isenabled): voi } } + /** + * Check whether a library is enabled or not. When machinename is passed, it will return false if any of the versions + * for this machinename is disabled. + * If the library doesn't exist, it will return true. + * + * @param \stdClass $librarydata Supported fields for library: 'id' and 'machichename'. + * @return bool + * @throws \moodle_exception + */ + public static function is_library_enabled(\stdClass $librarydata): bool { + global $DB; + + $params = []; + if (property_exists($librarydata, 'machinename')) { + $params['machinename'] = $librarydata->machinename; + } + if (property_exists($librarydata, 'id')) { + $params['id'] = $librarydata->id; + } + + if (empty($params)) { + throw new \moodle_exception("Missing 'machinename' or 'id' in librarydata parameter"); + } + + $libraries = $DB->get_records('h5p_libraries', $params); + + // If any of the libraries with these values have been disabled, return false. + foreach ($libraries as $id => $library) { + if (!$library->enabled) { + return false; + } + } + + return true; + } } diff --git a/h5p/classes/helper.php b/h5p/classes/helper.php index 2cf6598c0d9c7..004757dab614b 100644 --- a/h5p/classes/helper.php +++ b/h5p/classes/helper.php @@ -67,6 +67,13 @@ public static function save_h5p(factory $factory, \stored_file $file, \stdClass // Check if the h5p file is valid before saving it. $h5pvalidator = $factory->get_validator(); if ($h5pvalidator->isValidPackage($skipcontent, $onlyupdatelibs)) { + // If the main library of the package is disabled, the H5P content won't be saved. + $mainlibrary = (object) ['machinename' => $h5pvalidator->h5pC->mainJsonData['mainLibrary']]; + if (!api::is_library_enabled($mainlibrary)) { + $core->h5pF->setErrorMessage(get_string('mainlibrarydisabled', 'core_h5p')); + return false; + } + $h5pstorage = $factory->get_storage(); $content = [ diff --git a/h5p/tests/api_test.php b/h5p/tests/api_test.php index 24ea653b1e36e..6872fde2f4406 100644 --- a/h5p/tests/api_test.php +++ b/h5p/tests/api_test.php @@ -598,4 +598,148 @@ public function set_library_enabled_provider(): array { ], ]; } + + /** + * Test the behaviour of is_library_enabled(). + * + * @covers ::is_library_enabled + * @dataProvider is_library_enabled_provider + * + * @param string $libraryname Library name to check. + * @param bool $expected Expected result after calling the method. + * @param bool $exception Exception expected or not. + * @param bool $useid Whether to use id for calling is_library_enabled method. + * @param bool $uselibraryname Whether to use libraryname for calling is_library_enabled method. + */ + public function test_is_library_enabled(string $libraryname, bool $expected, bool $exception = false, + bool $useid = false, bool $uselibraryname = true): void { + global $DB; + + $this->resetAfterTest(); + + // Create the following libraries: + // - H5P.Lib1: 1 version enabled, 1 version disabled. + // - H5P.Lib2: 2 versions enabled. + // - H5P.Lib3: 2 versions disabled. + // - H5P.Lib4: 1 version disabled. + // - H5P.Lib5: 1 version enabled. + $generator = $this->getDataGenerator()->get_plugin_generator('core_h5p'); + $libraries = [ + 'H5P.Lib1.1' => $generator->create_library_record('H5P.Lib1', 'Lib1', 1, 1, 0, '', null, null, null, false), + 'H5P.Lib1.2' => $generator->create_library_record('H5P.Lib1', 'Lib1', 1, 2), + 'H5P.Lib2.1' => $generator->create_library_record('H5P.Lib2', 'Lib2', 2, 1), + 'H5P.Lib2.2' => $generator->create_library_record('H5P.Lib2', 'Lib2', 2, 2), + 'H5P.Lib3.1' => $generator->create_library_record('H5P.Lib3', 'Lib3', 3, 1, 0, '', null, null, null, false), + 'H5P.Lib3.2' => $generator->create_library_record('H5P.Lib3', 'Lib3', 3, 2, 0, '', null, null, null, false), + 'H5P.Lib4.1' => $generator->create_library_record('H5P.Lib4', 'Lib4', 4, 1, 0, '', null, null, null, false), + 'H5P.Lib5.1' => $generator->create_library_record('H5P.Lib5', 'Lib5', 5, 1), + ]; + + $countenabledlibraries = $DB->count_records('h5p_libraries', ['enabled' => 1]); + $this->assertEquals(4, $countenabledlibraries); + + if ($useid) { + $librarydata = ['id' => $libraries[$libraryname]->id]; + } else if ($uselibraryname) { + $librarydata = ['machinename' => $libraryname]; + } else { + $librarydata = ['invalid' => true]; + } + + if ($exception) { + $this->expectException(\moodle_exception::class); + } + + $result = api::is_library_enabled((object) $librarydata); + $this->assertEquals($expected, $result); + } + + /** + * Data provider for test_is_library_enabled(). + * + * @return array + */ + public function is_library_enabled_provider(): array { + return [ + 'Library with 2 versions, one of them disabled' => [ + 'libraryname' => 'H5P.Lib1', + 'expected' => false, + ], + 'Library with 2 versions, all enabled' => [ + 'libraryname' => 'H5P.Lib2', + 'expected' => true, + ], + 'Library with 2 versions, all disabled' => [ + 'libraryname' => 'H5P.Lib3', + 'expected' => false, + ], + 'Library with only one version, disabled' => [ + 'libraryname' => 'H5P.Lib4', + 'expected' => false, + ], + 'Library with only one version, enabled' => [ + 'libraryname' => 'H5P.Lib5', + 'expected' => true, + ], + 'Library with 2 versions, one of them disabled (using id) - 1.1 (disabled)' => [ + 'libraryname' => 'H5P.Lib1.1', + 'expected' => false, + 'exception' => false, + 'useid' => true, + ], + 'Library with 2 versions, one of them disabled (using id) - 1.2 (enabled)' => [ + 'libraryname' => 'H5P.Lib1.2', + 'expected' => true, + 'exception' => false, + 'useid' => true, + ], + 'Library with 2 versions, all enabled (using id) - 2.1' => [ + 'libraryname' => 'H5P.Lib2.1', + 'expected' => true, + 'exception' => false, + 'useid' => true, + ], + 'Library with 2 versions, all enabled (using id) - 2.2' => [ + 'libraryname' => 'H5P.Lib2.2', + 'expected' => true, + 'exception' => false, + 'useid' => true, + ], + 'Library with 2 versions, all disabled (using id) - 3.1' => [ + 'libraryname' => 'H5P.Lib3.1', + 'expected' => false, + 'exception' => false, + 'useid' => true, + ], + 'Library with 2 versions, all disabled (using id) - 3.2' => [ + 'libraryname' => 'H5P.Lib3.2', + 'expected' => false, + 'exception' => false, + 'useid' => true, + ], + 'Library with only one version, disabled (using id)' => [ + 'libraryname' => 'H5P.Lib4.1', + 'expected' => false, + 'exception' => false, + 'useid' => true, + ], + 'Library with only one version, enabled (using id)' => [ + 'libraryname' => 'H5P.Lib5.1', + 'expected' => true, + 'exception' => false, + 'useid' => true, + ], + 'Unexisting library' => [ + 'libraryname' => 'H5P.Unexisting', + 'expected' => true, + ], + 'Missing required parameters' => [ + 'libraryname' => 'H5P.Unexisting', + 'expected' => false, + 'exception' => true, + 'useid' => false, + 'uselibraryname' => false, + ], + ]; + } } diff --git a/lang/en/h5p.php b/lang/en/h5p.php index f4b6a8f282235..bfdecb0c3a44a 100644 --- a/lang/en/h5p.php +++ b/lang/en/h5p.php @@ -138,6 +138,8 @@ $string['licenseextras'] = 'Licence extras'; $string['licenseversion'] = 'Licence version'; $string['lockh5pdeploy'] = 'This H5P content cannot be accessed because it is being deployed. Please try again later.'; +$string['mainlibrarydisabled'] = 'This file can\'t be displayed because its content-type is disabled. Please contact your administrator to ask for the content type to +be enabled.'; $string['missingcontentfolder'] = 'A valid content folder is missing'; $string['missingcoreversion'] = 'The system was unable to install the {$a->%component} component from the package, as it requires a newer version of the H5P plugin. This site is currently running version {$a->%current}, whereas the required version is {$a->%required} or higher. Please upgrade and then try again.'; $string['missingdependency'] = 'Missing dependency {$a->@dep} required by {$a->@lib}.';