From 2b8d97326dc692f4eee94692e1aa4b36cf704dcc Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Sat, 1 Feb 2020 14:51:27 +0100 Subject: [PATCH 1/2] MDL-67062 core_h5p: delete cache assets when removing libraries The cache assets linked to a library have to be removed also when a library is deleted, in order to rebuild them again next time they are required. --- h5p/classes/framework.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/h5p/classes/framework.php b/h5p/classes/framework.php index 4818d861224..3a2dc33574e 100644 --- a/h5p/classes/framework.php +++ b/h5p/classes/framework.php @@ -1138,6 +1138,8 @@ class framework implements \H5PFrameworkInterface { $fs = new \core_h5p\file_storage(); // Delete the library from the file system. $fs->delete_library(array('libraryId' => $library->id)); + // Delete also the cache assets to rebuild them next time. + $this->deleteCachedAssets($library->id); // Remove library data from database. $DB->delete_records('h5p_library_dependencies', array('libraryid' => $library->id)); @@ -1451,6 +1453,10 @@ class framework implements \H5PFrameworkInterface { list($sql, $params) = $DB->get_in_or_equal($hashes, SQL_PARAMS_NAMED); // Remove all invalid keys. $DB->delete_records_select('h5p_libraries_cachedassets', 'hash ' . $sql, $params); + + // Remove also the cachedassets files. + $fs = new file_storage(); + $fs->deleteCachedAssets($hashes); } return $hashes; From 64a2bd19b06124cf192827e5d85916ba504cbbf3 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Mon, 3 Feb 2020 12:37:09 +0100 Subject: [PATCH 2/2] MDL-67062 core_h5p: delete libraries New feature to let admins to remove H5P libraries/content types. Thanks Ferran Recio for your contribution with the renderer! AMOS BEGIN CPY [actions,core],[actions,core_h5p] AMOS END --- h5p/classes/api.php | 106 ++++++++++ h5p/classes/framework.php | 13 +- h5p/classes/output/libraries.php | 99 +++++++++ h5p/libraries.php | 47 +++-- h5p/templates/h5plibraries.mustache | 12 ++ h5p/tests/api_test.php | 276 ++++++++++++++++++++++++++ h5p/tests/behat/h5p_libraries.feature | 17 ++ lang/en/h5p.php | 4 + 8 files changed, 546 insertions(+), 28 deletions(-) create mode 100644 h5p/classes/api.php create mode 100644 h5p/classes/output/libraries.php create mode 100644 h5p/tests/api_test.php diff --git a/h5p/classes/api.php b/h5p/classes/api.php new file mode 100644 index 00000000000..c1087d17c79 --- /dev/null +++ b/h5p/classes/api.php @@ -0,0 +1,106 @@ +. + +/** + * Contains API class for the H5P area. + * + * @package core_h5p + * @copyright 2020 Sara Arjona + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace core_h5p; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Contains API class for the H5P area. + * + * @copyright 2020 Sara Arjona + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class api { + + /** + * Delete a library and also all the libraries depending on it and the H5P contents using it. For the H5P content, only the + * database entries in {h5p} are removed (the .h5p files are not removed in order to let users to deploy them again). + * + * @param factory $factory The H5P factory. + * @param \stdClass $library The library to delete. + */ + public static function delete_library(factory $factory, \stdClass $library): void { + global $DB; + + // Get the H5P contents using this library, to remove them from DB. The .h5p files won't be removed + // so they will be displayed by the player next time a user with the proper permissions accesses it. + $sql = 'SELECT DISTINCT hcl.h5pid + FROM {h5p_contents_libraries} hcl + WHERE hcl.libraryid = :libraryid'; + $params = ['libraryid' => $library->id]; + $h5pcontents = $DB->get_records_sql($sql, $params); + foreach ($h5pcontents as $h5pcontent) { + $factory->get_framework()->deleteContentData($h5pcontent->h5pid); + } + + $fs = $factory->get_core()->fs; + $framework = $factory->get_framework(); + // Delete the library from the file system. + $fs->delete_library(array('libraryId' => $library->id)); + // Delete also the cache assets to rebuild them next time. + $framework->deleteCachedAssets($library->id); + + // Remove library data from database. + $DB->delete_records('h5p_library_dependencies', array('libraryid' => $library->id)); + $DB->delete_records('h5p_libraries', array('id' => $library->id)); + + // Remove the libraries using this library. + $requiredlibraries = self::get_dependent_libraries($library->id); + foreach ($requiredlibraries as $requiredlibrary) { + self::delete_library($factory, $requiredlibrary); + } + } + + /** + * Get all the libraries using a defined library. + * + * @param int $libraryid The library to get its dependencies. + * @return array List of libraryid with all the libraries required by a defined library. + */ + public static function get_dependent_libraries(int $libraryid): array { + global $DB; + + $sql = 'SELECT DISTINCT hl.* + FROM {h5p_library_dependencies} hld + JOIN {h5p_libraries} hl ON hl.id = hld.libraryid + WHERE hld.requiredlibraryid = :libraryid'; + $params = ['libraryid' => $libraryid]; + + return $DB->get_records_sql($sql, $params); + } + + /** + * Get a library from an identifier. + * + * @param int $libraryid The library identifier. + * @return \stdClass The library object having the library identifier defined. + * @throws dml_exception A DML specific exception is thrown if the libraryid doesn't exist. + */ + public static function get_library(int $libraryid): \stdClass { + global $DB; + + return $DB->get_record('h5p_libraries', ['id' => $libraryid], '*', MUST_EXIST); + } +} diff --git a/h5p/classes/framework.php b/h5p/classes/framework.php index 3a2dc33574e..b30c69d10db 100644 --- a/h5p/classes/framework.php +++ b/h5p/classes/framework.php @@ -1133,17 +1133,8 @@ class framework implements \H5PFrameworkInterface { * @param stdClass $library Library object with id, name, major version and minor version */ public function deleteLibrary($library) { - global $DB; - - $fs = new \core_h5p\file_storage(); - // Delete the library from the file system. - $fs->delete_library(array('libraryId' => $library->id)); - // Delete also the cache assets to rebuild them next time. - $this->deleteCachedAssets($library->id); - - // Remove library data from database. - $DB->delete_records('h5p_library_dependencies', array('libraryid' => $library->id)); - $DB->delete_records('h5p_libraries', array('id' => $library->id)); + $factory = new \core_h5p\factory(); + \core_h5p\api::delete_library($factory, $library); } /** diff --git a/h5p/classes/output/libraries.php b/h5p/classes/output/libraries.php new file mode 100644 index 00000000000..07a8fe97c8c --- /dev/null +++ b/h5p/classes/output/libraries.php @@ -0,0 +1,99 @@ +. + +/** + * Contains class core_h5p\output\libraries + * + * @package core_h5p + * @copyright 2020 Ferran Recio + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace core_h5p\output; + +defined('MOODLE_INTERNAL') || die(); + +use renderable; +use templatable; +use renderer_base; +use stdClass; +use moodle_url; +use action_menu; +use action_menu_link; +use pix_icon; + +/** + * Class to help display H5P library management table. + * + * @copyright 2020 Ferran Recio + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class libraries implements renderable, templatable { + + /** @var H5P factory */ + protected $factory; + + /** @var H5P library list */ + protected $libraries; + + /** + * Constructor. + * + * @param factory $factory The H5P factory + * @param array $libraries array of h5p libraries records + */ + public function __construct(\core_h5p\factory $factory, array $libraries) { + $this->factory = $factory; + $this->libraries = $libraries; + } + + /** + * Export this data so it can be used as the context for a mustache template. + * + * @param renderer_base $output + * @return stdClass + */ + public function export_for_template(renderer_base $output) { + $installed = []; + $filestorage = $this->factory->get_core()->fs; + foreach ($this->libraries as $libraryname => $versions) { + foreach ($versions as $version) { + // Get the icon URL. + $version->icon = $filestorage->get_icon_url( + $version->id, + $version->machine_name, + $version->major_version, + $version->minor_version + ); + // Get the action menu options. + $actionmenu = new action_menu(); + $actionmenu->set_menu_trigger(get_string('actions', 'core_h5p')); + $actionmenu->set_alignment(action_menu::TL, action_menu::BL); + $actionmenu->prioritise = true; + $actionmenu->add_primary_action(new action_menu_link( + new moodle_url('/h5p/libraries.php', ['deletelibrary' => $version->id]), + new pix_icon('t/delete', get_string('deletelibraryversion', 'core_h5p')), + get_string('deletelibraryversion', 'core_h5p') + )); + $version->actionmenu = $actionmenu->export_for_template($output); + $installed[] = $version; + } + } + $r = new stdClass(); + $r->contenttypes = $installed; + return $r; + } +} diff --git a/h5p/libraries.php b/h5p/libraries.php index bffce142d14..80ad08937de 100644 --- a/h5p/libraries.php +++ b/h5p/libraries.php @@ -26,6 +26,9 @@ require_once(__DIR__ . '/../config.php'); require_login(null, false); +$deletelibrary = optional_param('deletelibrary', null, PARAM_INT); +$confirm = optional_param('confirm', false, PARAM_BOOL); + $context = context_system::instance(); require_capability('moodle/h5p:updatelibraries', $context); @@ -38,12 +41,34 @@ $PAGE->set_title($pagetitle); $PAGE->set_pagelayout('admin'); $PAGE->set_heading($pagetitle); +$h5pfactory = new \core_h5p\factory(); +if ($deletelibrary) { + $library = \core_h5p\api::get_library($deletelibrary); + if ($confirm) { + require_sesskey(); + \core_h5p\api::delete_library($h5pfactory, $library); + redirect(new moodle_url('/h5p/libraries.php')); + } + + echo $OUTPUT->header(); + echo $OUTPUT->heading(get_string('deleting', 'core_h5p')); + echo $OUTPUT->confirm( + get_string('deletelibraryconfirm', 'core_h5p', [ + 'name' => format_string($library->title), + 'version' => format_string($library->majorversion . '.' . $library->minorversion . '.' . $library->patchversion), + ]), + new moodle_url($PAGE->url, ['deletelibrary' => $deletelibrary, 'confirm' => 1]), + new moodle_url('/h5p/libraries.php') + ); + echo $OUTPUT->footer(); + die(); +} + echo $OUTPUT->header(); echo $OUTPUT->heading($pagetitle); echo $OUTPUT->box(get_string('librariesmanagerdescription', 'core_h5p')); $form = new \core_h5p\form\uploadlibraries_form(); -$h5pfactory = new \core_h5p\factory(); if ($data = $form->get_data()) { require_sesskey(); @@ -55,7 +80,7 @@ if ($data = $form->get_data()) { $file = reset($files); // Validate and save the H5P package. - // Because we are passing skipcontent = true to save_h5p function, the returning value is false in an error + // Because we are passing skipcontent = true to save_h5p function, the returning value is false if an error // is encountered, null when successfully saving the package without creating the content. if (\core_h5p\helper::save_h5p($h5pfactory, $file, new stdClass(), false, true) === false) { echo $OUTPUT->notification(get_string('invalidpackage', 'core_h5p'), 'error'); @@ -67,23 +92,11 @@ $form->display(); // Load installed Libraries. $framework = $h5pfactory->get_framework(); -$filestorage = $h5pfactory->get_core()->fs; $libraries = $framework->loadLibraries(); -$installed = []; -foreach ($libraries as $libraryname => $versions) { - foreach ($versions as $version) { - $version->icon = $filestorage->get_icon_url( - $version->id, - $version->machine_name, - $version->major_version, - $version->minor_version - ); - $installed[] = $version; - } -} -if (count($installed)) { - echo $OUTPUT->render_from_template('core_h5p/h5plibraries', (object)['contenttypes' => $installed]); +if (!empty($libraries)) { + $libs = new \core_h5p\output\libraries($h5pfactory, $libraries); + echo $OUTPUT->render_from_template('core_h5p/h5plibraries', $libs->export_for_template($OUTPUT)); } echo $OUTPUT->footer(); diff --git a/h5p/templates/h5plibraries.mustache b/h5p/templates/h5plibraries.mustache index a6125fb0eee..cb6e7b06051 100644 --- a/h5p/templates/h5plibraries.mustache +++ b/h5p/templates/h5plibraries.mustache @@ -69,6 +69,7 @@ {{#str}}description, core{{/str}} {{#str}}version, core{{/str}} + @@ -87,6 +88,11 @@ {{{ title }}} {{{ major_version }}}.{{{ minor_version }}}.{{{ patch_version }}} + + {{#actionmenu}} + {{>core/action_menu}} + {{/actionmenu}} + {{/runnable}} {{/contenttypes}} @@ -101,6 +107,7 @@ {{#str}}description, core{{/str}} {{#str}}version, core{{/str}} + @@ -111,6 +118,11 @@ {{{ title }}} {{{ major_version }}}.{{{ minor_version }}}.{{{ patch_version }}} + + {{#actionmenu}} + {{>core/action_menu}} + {{/actionmenu}} + {{/runnable}} {{/contenttypes}} diff --git a/h5p/tests/api_test.php b/h5p/tests/api_test.php new file mode 100644 index 00000000000..d64e1eb3775 --- /dev/null +++ b/h5p/tests/api_test.php @@ -0,0 +1,276 @@ +. + +/** + * Testing the H5P API. + * + * @package core_h5p + * @category test + * @copyright 2020 Sara Arjona + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types = 1); + +namespace core_h5p; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Test class covering the H5P API. + * + * @package core_h5p + * @copyright 2020 Sara Arjona + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class api_testcase extends \advanced_testcase { + + /** + * Test the behaviour of delete_library(). + * + * @dataProvider delete_library_provider + * @param string $libraryname Machine name of the library to delete. + * @param int $expectedh5p Total of H5P contents expected after deleting the library. + * @param int $expectedlibraries Total of H5P libraries expected after deleting the library. + * @param int $expectedcontents Total of H5P content_libraries expected after deleting the library. + * @param int $expecteddependencies Total of H5P library dependencies expected after deleting the library. + */ + public function test_delete_library(string $libraryname, int $expectedh5p, int $expectedlibraries, + int $expectedcontents, int $expecteddependencies): void { + global $DB; + + $this->setRunTestInSeparateProcess(true); + $this->resetAfterTest(); + + // Generate h5p related data. + $generator = $this->getDataGenerator()->get_plugin_generator('core_h5p'); + $generator->generate_h5p_data(); + $generator->create_library_record('H5P.TestingLibrary', 'TestingLibrary', 1, 0); + + // Check the current content in H5P tables is the expected. + $counth5p = $DB->count_records('h5p'); + $counth5plibraries = $DB->count_records('h5p_libraries'); + $counth5pcontents = $DB->count_records('h5p_contents_libraries'); + $counth5pdependencies = $DB->count_records('h5p_library_dependencies'); + + $this->assertSame(1, $counth5p); + $this->assertSame(7, $counth5plibraries); + $this->assertSame(5, $counth5pcontents); + $this->assertSame(7, $counth5pdependencies); + + // Delete this library. + $factory = new factory(); + $library = $DB->get_record('h5p_libraries', ['machinename' => $libraryname]); + if ($library) { + api::delete_library($factory, $library); + } + + // Check the expected libraries and content have been removed. + $counth5p = $DB->count_records('h5p'); + $counth5plibraries = $DB->count_records('h5p_libraries'); + $counth5pcontents = $DB->count_records('h5p_contents_libraries'); + $counth5pdependencies = $DB->count_records('h5p_library_dependencies'); + + $this->assertSame($expectedh5p, $counth5p); + $this->assertSame($expectedlibraries, $counth5plibraries); + $this->assertSame($expectedcontents, $counth5pcontents); + $this->assertSame($expecteddependencies, $counth5pdependencies); + } + + /** + * Data provider for test_delete_library(). + * + * @return array + */ + public function delete_library_provider(): array { + return [ + 'Delete MainLibrary' => [ + 'MainLibrary', + 0, + 6, + 0, + 4, + ], + 'Delete Library1' => [ + 'Library1', + 0, + 5, + 0, + 1, + ], + 'Delete Library2' => [ + 'Library2', + 0, + 4, + 0, + 1, + ], + 'Delete Library3' => [ + 'Library3', + 0, + 4, + 0, + 0, + ], + 'Delete Library4' => [ + 'Library4', + 0, + 4, + 0, + 1, + ], + 'Delete Library5' => [ + 'Library5', + 0, + 3, + 0, + 0, + ], + 'Delete a library without dependencies' => [ + 'H5P.TestingLibrary', + 1, + 6, + 5, + 7, + ], + 'Delete unexisting library' => [ + 'LibraryX', + 1, + 7, + 5, + 7, + ], + ]; + } + + /** + * Test the behaviour of get_dependent_libraries(). + * + * @dataProvider get_dependent_libraries_provider + * @param string $libraryname Machine name of the library to delete. + * @param int $expectedvalue Total of H5P required libraries expected. + */ + public function test_get_dependent_libraries(string $libraryname, int $expectedvalue): void { + global $DB; + + $this->resetAfterTest(); + + // Generate h5p related data. + $generator = $this->getDataGenerator()->get_plugin_generator('core_h5p'); + $generator->generate_h5p_data(); + $generator->create_library_record('H5P.TestingLibrary', 'TestingLibrary', 1, 0); + + // Get required libraries. + $library = $DB->get_record('h5p_libraries', ['machinename' => $libraryname], 'id'); + if ($library) { + $libraries = api::get_dependent_libraries((int)$library->id); + } else { + $libraries = []; + } + + $this->assertCount($expectedvalue, $libraries); + } + + /** + * Data provider for test_get_dependent_libraries(). + * + * @return array + */ + public function get_dependent_libraries_provider(): array { + return [ + 'Main library of a content' => [ + 'MainLibrary', + 0, + ], + 'Library1' => [ + 'Library1', + 1, + ], + 'Library2' => [ + 'Library2', + 2, + ], + 'Library without dependencies' => [ + 'H5P.TestingLibrary', + 0, + ], + 'Unexisting library' => [ + 'LibraryX', + 0, + ], + ]; + } + + /** + * Test the behaviour of get_library(). + * + * @dataProvider get_library_provider + * @param string $libraryname Machine name of the library to delete. + * @param bool $emptyexpected Wether the expected result is empty or not. + */ + public function test_get_library(string $libraryname, bool $emptyexpected): void { + global $DB; + + $this->resetAfterTest(); + + // Generate h5p related data. + $generator = $this->getDataGenerator()->get_plugin_generator('core_h5p'); + $generator->generate_h5p_data(); + $generator->create_library_record('H5P.TestingLibrary', 'TestingLibrary', 1, 0); + + // Get the library identifier. + $library = $DB->get_record('h5p_libraries', ['machinename' => $libraryname], 'id'); + if ($library) { + $result = api::get_library((int)$library->id); + } else { + $result = null; + } + + if ($emptyexpected) { + $this->assertEmpty($result); + } else { + $this->assertEquals($library->id, $result->id); + $this->assertEquals($libraryname, $result->machinename); + } + + } + + /** + * Data provider for test_get_library(). + * + * @return array + */ + public function get_library_provider(): array { + return [ + 'Main library of a content' => [ + 'MainLibrary', + false, + ], + 'Library1' => [ + 'Library1', + false, + ], + 'Library without dependencies' => [ + 'H5P.TestingLibrary', + false, + ], + 'Unexisting library' => [ + 'LibraryX', + true, + ], + ]; + } +} diff --git a/h5p/tests/behat/h5p_libraries.feature b/h5p/tests/behat/h5p_libraries.feature index 103c057612c..66b5323ec36 100644 --- a/h5p/tests/behat/h5p_libraries.feature +++ b/h5p/tests/behat/h5p_libraries.feature @@ -44,3 +44,20 @@ Feature: Upload and list H5P libraries and content types installed And I click on "Installed H5P libraries" "link" And I should see "1.3" in the "Question" "table_row" And I should see "1.4" + + @javascript + Scenario: Delete H5P library. + Given I log in as "admin" + And I navigate to "H5P > Manage H5P content types" in site administration + And I upload "h5p/tests/fixtures/filltheblanks.h5p" file to "H5P content type" filemanager + And I click on "Upload H5P content types" "button" in the "#fitem_id_submitbutton" "css_element" + And I wait until the page is ready + And I click on "Installed H5P libraries" "link" + When I click on "Delete version" "link" in the "H5P.FontIcons" "table_row" + And I press "Continue" + And I click on "Installed H5P content types" "link" + Then I should not see "Fill in the Blanks" + And I click on "Installed H5P libraries" "link" + And I should not see "H5P.FontIcons" + And I should not see "Joubel UI" + And I should see "Transition" diff --git a/lang/en/h5p.php b/lang/en/h5p.php index febe040477f..355f523d967 100644 --- a/lang/en/h5p.php +++ b/lang/en/h5p.php @@ -23,6 +23,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +$string['actions'] = 'Actions'; $string['addedandupdatedpp'] = 'Added {$a->%new} new H5P libraries and updated {$a->%old} old ones.'; $string['addedandupdatedps'] = 'Added {$a->%new} new H5P libraries and updated {$a->%old} old one.'; $string['addedandupdatedsp'] = 'Added {$a->%new} new H5P library and updated {$a->%old} old ones.'; @@ -67,6 +68,9 @@ $string['couldNotParseJSONFromZip'] = 'Unable to parse JSON from the package: {$ $string['couldNotReadFileFromZip'] = 'Unable to read file from the package: {$a->%fileName}'; $string['creativecommons'] = 'Creative Commons'; $string['date'] = 'Date'; +$string['deletelibraryconfirm'] = '

Are you sure you want to delete version \'{$a->version}\' from library \'{$a->name}\'? It will remove the library and all its uses.

This operation can not be undone.

'; +$string['deletelibraryversion'] = 'Delete version'; +$string['deleting'] = 'Deleting a library'; $string['description'] = 'Description'; $string['disablefullscreen'] = 'Disable fullscreen'; $string['download'] = 'Download';