From 296ae788852e0c20a28ed264c64a6c6659154ade Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20D=C3=A9niz=20Falc=C3=B3n?= Date: Fri, 1 May 2020 18:09:44 +0100 Subject: [PATCH 1/4] MDL-68636 core_contentbank: added method to get H5P id from pathnamehash --- h5p/classes/api.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/h5p/classes/api.php b/h5p/classes/api.php index b86acad5676..81c76256512 100644 --- a/h5p/classes/api.php +++ b/h5p/classes/api.php @@ -487,4 +487,19 @@ class api { $pathnamehash = $fs->get_pathname_hash($contextid, $component, $filearea, $itemid, $filepath, $filename); return $pathnamehash; } + + /** + * Returns the H5P content object corresponding to an H5P content file. + * + * @param string $pathnamehash The pathnamehash of the file associated to an H5P content. + * + * @return null|\stdClass H5P content object or null if not found. + */ + public static function get_content_from_pathnamehash(string $pathnamehash): ?\stdClass { + global $DB; + + $h5p = $DB->get_record('h5p', ['pathnamehash' => $pathnamehash]); + + return ($h5p) ? $h5p : null; + } } From f9830e456ad6ce5a71cfdc633dcdbf51b01076b9 Mon Sep 17 00:00:00 2001 From: Amaia Anabitarte Date: Tue, 5 May 2020 16:16:23 +0200 Subject: [PATCH 2/4] MDL-68636 core_contentbank: Changing parameter classes to be consistent --- contentbank/classes/contenttype.php | 13 +++++-------- contentbank/classes/output/bankcontent.php | 2 +- contentbank/contenttype/h5p/classes/contenttype.php | 8 +++----- contentbank/tests/fixtures/testable_contenttype.php | 6 +++--- contentbank/view.php | 2 +- 5 files changed, 13 insertions(+), 18 deletions(-) diff --git a/contentbank/classes/contenttype.php b/contentbank/classes/contenttype.php index 7f7fbf59694..3a0aa315fd4 100644 --- a/contentbank/classes/contenttype.php +++ b/contentbank/classes/contenttype.php @@ -159,26 +159,23 @@ abstract class contenttype { /** * Returns the URL where the content will be visualized. * - * @param stdClass $record The content to be displayed. + * @param content $content The content to be displayed. * @return string URL where to visualize the given content. */ - public function get_view_url(\stdClass $record): string { - return new moodle_url('/contentbank/view.php', ['id' => $record->id]); + public function get_view_url(content $content): string { + return new moodle_url('/contentbank/view.php', ['id' => $content->get_id()]); } /** * Returns the HTML content to add to view.php visualizer. * - * @param stdClass $record The content to be displayed. + * @param content $content The content to be displayed. * @return string HTML code to include in view.php. */ - public function get_view_content(\stdClass $record): string { + public function get_view_content(content $content): string { // Trigger an event for viewing this content. $event = contentbank_content_viewed::create_from_record($record); $event->trigger(); - - // Main contenttype class can visualize the content, but plugins could overwrite visualization. - return ''; } /** diff --git a/contentbank/classes/output/bankcontent.php b/contentbank/classes/output/bankcontent.php index 471266e5ae8..773f2d0e411 100644 --- a/contentbank/classes/output/bankcontent.php +++ b/contentbank/classes/output/bankcontent.php @@ -85,7 +85,7 @@ class bankcontent implements renderable, templatable { $name = $content->get_name(); $contentdata[] = array( 'name' => $name, - 'link' => $contenttype->get_view_url($record), + 'link' => $contenttype->get_view_url($content), 'icon' => $contenttype->get_icon($name) ); } diff --git a/contentbank/contenttype/h5p/classes/contenttype.php b/contentbank/contenttype/h5p/classes/contenttype.php index 806205d4f63..e6a0adf9ca2 100644 --- a/contentbank/contenttype/h5p/classes/contenttype.php +++ b/contentbank/contenttype/h5p/classes/contenttype.php @@ -25,7 +25,6 @@ namespace contenttype_h5p; use core\event\contentbank_content_viewed; -use stdClass; use html_writer; /** @@ -57,15 +56,14 @@ class contenttype extends \core_contentbank\contenttype { /** * Returns the HTML content to add to view.php visualizer. * - * @param stdClass $record Th content to be displayed. + * @param content $content The content to be displayed. * @return string HTML code to include in view.php. */ - public function get_view_content(\stdClass $record): string { + public function get_view_content(\core_contentbank\content $content): string { // Trigger an event for viewing this content. - $event = contentbank_content_viewed::create_from_record($record); + $event = contentbank_content_viewed::create_from_record($content->get_content()); $event->trigger(); - $content = new content($record); $fileurl = $content->get_file_url(); $html = html_writer::tag('h2', $content->get_name()); $html .= \core_h5p\player::display($fileurl, new \stdClass(), true); diff --git a/contentbank/tests/fixtures/testable_contenttype.php b/contentbank/tests/fixtures/testable_contenttype.php index 850d4efc7ec..7cbe10a4808 100644 --- a/contentbank/tests/fixtures/testable_contenttype.php +++ b/contentbank/tests/fixtures/testable_contenttype.php @@ -40,11 +40,11 @@ class contenttype extends \core_contentbank\contenttype { /** * Returns the URL where the content will be visualized. * - * @param stdClass $record Th content to be displayed. + * @param content $content The content to delete. * @return string URL where to visualize the given content. */ - public function get_view_url(\stdClass $record): string { - $fileurl = $this->get_file_url($record->id); + public function get_view_url(content $content): string { + $fileurl = $this->get_file_url($content->get_id()); $url = $fileurl."?forcedownload=1"; return $url; diff --git a/contentbank/view.php b/contentbank/view.php index d34205cda4e..c95d7fd72f3 100644 --- a/contentbank/view.php +++ b/contentbank/view.php @@ -118,7 +118,7 @@ if ($errormsg !== '') { echo $OUTPUT->notification($statusmsg, 'notifysuccess'); } if ($contenttype->can_access()) { - echo $contenttype->get_view_content($record); + echo $contenttype->get_view_content($content); } echo $OUTPUT->box_end(); From 6fc3477cc50e818f14a5d83cd770d429c2c1dec6 Mon Sep 17 00:00:00 2001 From: Amaia Anabitarte Date: Wed, 6 May 2020 10:11:35 +0200 Subject: [PATCH 3/4] MDL-68636 core_contentbank: Changing get_icon function --- contentbank/classes/contenttype.php | 6 +-- contentbank/classes/output/bankcontent.php | 2 +- .../contenttype/h5p/classes/contenttype.php | 28 +++++++++++-- .../h5p/tests/contenttype_h5p_test.php | 42 +++++++++++++++++++ contentbank/templates/bankcontent.mustache | 6 +-- contentbank/tests/contenttype_test.php | 5 ++- .../tests/fixtures/testable_contenttype.php | 8 ++-- 7 files changed, 81 insertions(+), 16 deletions(-) diff --git a/contentbank/classes/contenttype.php b/contentbank/classes/contenttype.php index 3a0aa315fd4..79d658b3612 100644 --- a/contentbank/classes/contenttype.php +++ b/contentbank/classes/contenttype.php @@ -181,12 +181,12 @@ abstract class contenttype { /** * Returns the HTML code to render the icon for content bank contents. * - * @param string $contentname The contentname to add as alt value to the icon. + * @param content $content The content to be displayed. * @return string HTML code to render the icon */ - public function get_icon(string $contentname): string { + public function get_icon(content $content): string { global $OUTPUT; - return $OUTPUT->pix_icon('f/unknown-64', $contentname, 'moodle', ['class' => 'iconsize-big']); + return $OUTPUT->image_url('f/unknown-64', 'moodle')->out(false); } /** diff --git a/contentbank/classes/output/bankcontent.php b/contentbank/classes/output/bankcontent.php index 773f2d0e411..2ea5c4d8391 100644 --- a/contentbank/classes/output/bankcontent.php +++ b/contentbank/classes/output/bankcontent.php @@ -86,7 +86,7 @@ class bankcontent implements renderable, templatable { $contentdata[] = array( 'name' => $name, 'link' => $contenttype->get_view_url($content), - 'icon' => $contenttype->get_icon($name) + 'icon' => $contenttype->get_icon($content) ); } $data->contents = $contentdata; diff --git a/contentbank/contenttype/h5p/classes/contenttype.php b/contentbank/contenttype/h5p/classes/contenttype.php index e6a0adf9ca2..d48941dfc70 100644 --- a/contentbank/contenttype/h5p/classes/contenttype.php +++ b/contentbank/contenttype/h5p/classes/contenttype.php @@ -73,12 +73,32 @@ class contenttype extends \core_contentbank\contenttype { /** * Returns the HTML code to render the icon for H5P content types. * - * @param string $contentname The contentname to add as alt value to the icon. + * @param content $content The content to be displayed. * @return string HTML code to render the icon */ - public function get_icon(string $contentname): string { - global $OUTPUT; - return $OUTPUT->pix_icon('f/h5p-64', $contentname, 'moodle', ['class' => 'iconsize-big']); + public function get_icon(\core_contentbank\content $content): string { + global $OUTPUT, $DB; + + $iconurl = $OUTPUT->image_url('f/h5p-64', 'moodle')->out(false); + $file = $content->get_file(); + if (!empty($file)) { + $h5p = \core_h5p\api::get_content_from_pathnamehash($file->get_pathnamehash()); + if (!empty($h5p)) { + \core_h5p\local\library\autoloader::register(); + if ($h5plib = $DB->get_record('h5p_libraries', ['id' => $h5p->mainlibraryid])) { + $h5pfilestorage = new \core_h5p\file_storage(); + $h5picon = $h5pfilestorage->get_icon_url( + $h5plib->id, + $h5plib->machinename, + $h5plib->majorversion, + $h5plib->minorversion); + if (!empty($h5picon)) { + $iconurl = $h5picon; + } + } + } + } + return $iconurl; } /** diff --git a/contentbank/contenttype/h5p/tests/contenttype_h5p_test.php b/contentbank/contenttype/h5p/tests/contenttype_h5p_test.php index 3b9e3e3941c..c708516718a 100644 --- a/contentbank/contenttype/h5p/tests/contenttype_h5p_test.php +++ b/contentbank/contenttype/h5p/tests/contenttype_h5p_test.php @@ -105,4 +105,46 @@ class contenttype_h5p_contenttype_plugin_testcase extends advanced_testcase { $this->assertFalse($coursetype->can_upload()); $this->assertFalse($systemtype->can_upload()); } + + /** + * Tests get_icon result. + * + * @covers ::get_icon + */ + public function test_get_icon() { + global $CFG; + + $this->resetAfterTest(); + $systemcontext = context_system::instance(); + $this->setAdminUser(); + $contenttype = new contenttype_h5p\contenttype($systemcontext); + + // Add an H5P fill the blanks file to the content bank. + $filepath = $CFG->dirroot . '/h5p/tests/fixtures/filltheblanks.h5p'; + $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank'); + $contents = $generator->generate_contentbank_data('contenttype_h5p', 1, 0, $systemcontext, true, $filepath); + $filltheblanks = array_shift($contents); + + // Add an H5P find the words file to the content bank. + $filepath = $CFG->dirroot . '/h5p/tests/fixtures/find-the-words.h5p'; + $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank'); + $contents = $generator->generate_contentbank_data('contenttype_h5p', 1, 0, $systemcontext, true, $filepath); + $findethewords = array_shift($contents); + + // Check before deploying the icon for both contents is the same: default one. + // Because we don't know specific H5P content type yet. + $defaulticon = $contenttype->get_icon($filltheblanks); + $this->assertEquals($defaulticon, $contenttype->get_icon($findethewords)); + $this->assertContains('h5p', $defaulticon); + + // Deploy one of the contents though the player to create the H5P DB entries and know specific content type. + $h5pplayer = new \core_h5p\player($findethewords->get_file_url(), new \stdClass(), true); + $h5pplayer->add_assets_to_page(); + $h5pplayer->output(); + + // Once the H5P has been deployed, we know the specific H5P content type, so the icon returned is not default one. + $findicon = $contenttype->get_icon($findethewords); + $this->assertNotEquals($defaulticon, $findicon); + $this->assertContains('find', $findicon, '', true); + } } diff --git a/contentbank/templates/bankcontent.mustache b/contentbank/templates/bankcontent.mustache index 396c2978526..8c4362683bc 100644 --- a/contentbank/templates/bankcontent.mustache +++ b/contentbank/templates/bankcontent.mustache @@ -23,11 +23,11 @@ { "name": "accordion.h5p", "link": "http://something/contentbank/contenttype/h5p/view.php?url=http://something/pluginfile.php/1/contentbank/public/accordion.h5p", - "icon" : "" + "icon" : "http://something/theme/image.php/boost/core/1581597850/f/h5p-64" }, { "name": "resume.pdf", - "icon": "" + "icon": "http://something/theme/image.php/boost/core/1584597850/f/pdf-64" } ], "tools": [ @@ -65,7 +65,7 @@
- {{{ icon }}} + {{{name}}}
{{#link}} diff --git a/contentbank/tests/contenttype_test.php b/contentbank/tests/contenttype_test.php index d5cf377b449..982d2f1bf30 100644 --- a/contentbank/tests/contenttype_test.php +++ b/contentbank/tests/contenttype_test.php @@ -106,7 +106,10 @@ class core_contenttype_contenttype_testcase extends \advanced_testcase { $systemcontext = \context_system::instance(); $testable = new contenttype($systemcontext); - $icon = $testable->get_icon('new content'); + $record = new stdClass(); + $record->name = 'New content'; + $content = $testable->create_content($record); + $icon = $testable->get_icon($content); $this->assertContains('archive', $icon); } diff --git a/contentbank/tests/fixtures/testable_contenttype.php b/contentbank/tests/fixtures/testable_contenttype.php index 7cbe10a4808..f279481fe3d 100644 --- a/contentbank/tests/fixtures/testable_contenttype.php +++ b/contentbank/tests/fixtures/testable_contenttype.php @@ -43,7 +43,7 @@ class contenttype extends \core_contentbank\contenttype { * @param content $content The content to delete. * @return string URL where to visualize the given content. */ - public function get_view_url(content $content): string { + public function get_view_url(\core_contentbank\content $content): string { $fileurl = $this->get_file_url($content->get_id()); $url = $fileurl."?forcedownload=1"; @@ -53,13 +53,13 @@ class contenttype extends \core_contentbank\contenttype { /** * Returns the HTML code to render the icon for content bank contents. * - * @param string $contentname The contentname to add as alt value to the icon. + * @param content $content The content to delete. * @return string HTML code to render the icon */ - public function get_icon(string $contentname): string { + public function get_icon(\core_contentbank\content $content): string { global $OUTPUT; - return $OUTPUT->pix_icon('f/archive-64', $contentname, 'moodle', ['class' => 'iconsize-big']); + return $OUTPUT->image_url('f/archive-64', 'moodle')->out(false); } /** From a09129b09c166cad5d6b805a3e7b80393ccb9e27 Mon Sep 17 00:00:00 2001 From: Amaia Anabitarte Date: Thu, 7 May 2020 13:10:46 +0200 Subject: [PATCH 4/4] MDL-68636 core_contentbank: Redirect to view page when uploading a file --- .../h5p/tests/behat/admin_upload_content.feature | 1 - contentbank/tests/behat/delete_content.feature | 13 ++++--------- contentbank/upload.php | 2 ++ 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/contentbank/contenttype/h5p/tests/behat/admin_upload_content.feature b/contentbank/contenttype/h5p/tests/behat/admin_upload_content.feature index ad9c5c56a9e..8639d35e937 100644 --- a/contentbank/contenttype/h5p/tests/behat/admin_upload_content.feature +++ b/contentbank/contenttype/h5p/tests/behat/admin_upload_content.feature @@ -34,7 +34,6 @@ Feature: H5P file upload to content bank for admins And I click on "Select this file" "button" And I click on "Save changes" "button" And I wait until the page is ready - And I click on "filltheblanks.h5p" "link" And I switch to "h5p-player" class iframe And I switch to "h5p-iframe" class iframe Then I should see "Of which countries" diff --git a/contentbank/tests/behat/delete_content.feature b/contentbank/tests/behat/delete_content.feature index b49a79c76a1..9b142b7b3e6 100644 --- a/contentbank/tests/behat/delete_content.feature +++ b/contentbank/tests/behat/delete_content.feature @@ -26,14 +26,12 @@ Feature: Delete H5P file from the content bank And I click on "Save changes" "button" Scenario: Admins can delete content from the content bank - Given I should see "filltheblanks.h5p" - And I follow "filltheblanks.h5p" - When I open the action menu in "region-main-settings-menu" "region" - Then I should see "Delete" - And I choose "Delete" in the open action menu + Given I open the action menu in "region-main-settings-menu" "region" + And I should see "Delete" + When I choose "Delete" in the open action menu And I should see "Are you sure you want to delete the content 'filltheblanks.h5p'" And I click on "Cancel" "button" in the "Delete content" "dialogue" - And I should see "filltheblanks.h5p" + Then I should see "filltheblanks.h5p" And I open the action menu in "region-main-settings-menu" "region" And I choose "Delete" in the open action menu And I click on "Delete" "button" in the "Delete content" "dialogue" @@ -68,8 +66,5 @@ Feature: Delete H5P file from the content bank And I click on "find-the-words.h5p" "link" And I click on "Select this file" "button" And I click on "Save changes" "button" - And I should see "filltheblanks.h5p" - And I should see "find-the-words.h5p" - And I follow "find-the-words.h5p" And I open the action menu in "region-main-settings-menu" "region" And I should see "Delete" diff --git a/contentbank/upload.php b/contentbank/upload.php index 0a88a6928ee..371d27f9843 100644 --- a/contentbank/upload.php +++ b/contentbank/upload.php @@ -80,6 +80,8 @@ if ($mform->is_cancelled()) { $file = reset($files); $content = $cb->create_content_from_file($context, $USER->id, $file); file_save_draft_area_files($formdata->file, $contextid, 'contentbank', 'public', $content->get_id()); + $viewurl = new \moodle_url('/contentbank/view.php', ['id' => $content->get_id(), 'contextid' => $contextid]); + redirect($viewurl); } redirect($returnurl); }