From: ArthurHoaro Date: Thu, 15 Oct 2020 09:46:24 +0000 (+0200) Subject: Asynchronous retrieval of bookmark's thumbnails X-Git-Tag: v0.12.1^2~31^2 X-Git-Url: https://git.immae.eu/?p=github%2Fshaarli%2FShaarli.git;a=commitdiff_plain;h=21e72da9ee34cec56b10c83ae0c75b4bf320dfcb Asynchronous retrieval of bookmark's thumbnails This feature is based general.enable_async_metadata setting and works with existing metadata.js file. The script is compatible with any template: - the thumbnail div bloc must have attribute - the bookmark bloc must have attribute with the bookmark ID as value Fixes #1564 --- diff --git a/application/bookmark/Bookmark.php b/application/bookmark/Bookmark.php index ea565d1f..4810c5e6 100644 --- a/application/bookmark/Bookmark.php +++ b/application/bookmark/Bookmark.php @@ -377,6 +377,24 @@ class Bookmark return $this; } + /** + * Return true if: + * - the bookmark's thumbnail is not already set to false (= not found) + * - it's not a note + * - it's an HTTP(S) link + * - the thumbnail has not yet be retrieved (null) or its associated cache file doesn't exist anymore + * + * @return bool True if the bookmark's thumbnail needs to be retrieved. + */ + public function shouldUpdateThumbnail(): bool + { + return $this->thumbnail !== false + && !$this->isNote() + && startsWith(strtolower($this->url), 'http') + && (null === $this->thumbnail || !is_file($this->thumbnail)) + ; + } + /** * Get the Sticky. * diff --git a/application/front/controller/admin/ManageShaareController.php b/application/front/controller/admin/ManageShaareController.php index df2f1631..908ebae3 100644 --- a/application/front/controller/admin/ManageShaareController.php +++ b/application/front/controller/admin/ManageShaareController.php @@ -129,7 +129,8 @@ class ManageShaareController extends ShaarliAdminController $bookmark->setTagsString($request->getParam('lf_tags')); if ($this->container->conf->get('thumbnails.mode', Thumbnailer::MODE_NONE) !== Thumbnailer::MODE_NONE - && false === $bookmark->isNote() + && true !== $this->container->conf->get('general.enable_async_metadata', true) + && $bookmark->shouldUpdateThumbnail() ) { $bookmark->setThumbnail($this->container->thumbnailer->get($bookmark->getUrl())); } diff --git a/application/front/controller/visitor/BookmarkListController.php b/application/front/controller/visitor/BookmarkListController.php index 18368751..a8019ead 100644 --- a/application/front/controller/visitor/BookmarkListController.php +++ b/application/front/controller/visitor/BookmarkListController.php @@ -169,14 +169,11 @@ class BookmarkListController extends ShaarliVisitorController */ protected function updateThumbnail(Bookmark $bookmark, bool $writeDatastore = true): bool { - // Logged in, thumbnails enabled, not a note, is HTTP - // and (never retrieved yet or no valid cache file) + // Logged in, not async retrieval, thumbnails enabled, and thumbnail should be updated if ($this->container->loginManager->isLoggedIn() + && true !== $this->container->conf->get('general.enable_async_metadata', true) && $this->container->conf->get('thumbnails.mode', Thumbnailer::MODE_NONE) !== Thumbnailer::MODE_NONE - && false !== $bookmark->getThumbnail() - && !$bookmark->isNote() - && (null === $bookmark->getThumbnail() || !is_file($bookmark->getThumbnail())) - && startsWith(strtolower($bookmark->getUrl()), 'http') + && $bookmark->shouldUpdateThumbnail() ) { $bookmark->setThumbnail($this->container->thumbnailer->get($bookmark->getUrl())); $this->container->bookmarkService->set($bookmark, $writeDatastore); @@ -198,6 +195,7 @@ class BookmarkListController extends ShaarliVisitorController 'page_max' => '', 'search_tags' => '', 'result_count' => '', + 'async_metadata' => $this->container->conf->get('general.enable_async_metadata', true) ]; } diff --git a/assets/common/js/metadata.js b/assets/common/js/metadata.js index 5200b481..2b013364 100644 --- a/assets/common/js/metadata.js +++ b/assets/common/js/metadata.js @@ -1,5 +1,19 @@ import he from 'he'; +/** + * This script is used to retrieve bookmarks metadata asynchronously: + * - title, description and keywords while creating a new bookmark + * - thumbnails while visiting the bookmark list + * + * Note: it should only be included if the user is logged in + * and the setting general.enable_async_metadata is enabled. + */ + +/** + * Removes given input loaders - used in edit link template. + * + * @param {object} loaders List of input DOM element that need to be cleared + */ function clearLoaders(loaders) { if (loaders != null && loaders.length > 0) { [...loaders].forEach((loader) => { @@ -8,32 +22,82 @@ function clearLoaders(loaders) { } } +/** + * AJAX request to update the thumbnail of a bookmark with the provided ID. + * If a thumbnail is retrieved, it updates the divElement with the image src, and displays it. + * + * @param {string} basePath Shaarli subfolder for XHR requests + * @param {object} divElement Main
DOM element containing the thumbnail placeholder + * @param {int} id Bookmark ID to update + */ +function updateThumb(basePath, divElement, id) { + const xhr = new XMLHttpRequest(); + xhr.open('PATCH', `${basePath}/admin/shaare/${id}/update-thumbnail`); + xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded'); + xhr.responseType = 'json'; + xhr.onload = () => { + if (xhr.status !== 200) { + alert(`An error occurred. Return code: ${xhr.status}`); + } else { + const { response } = xhr; + + if (response.thumbnail !== false) { + const imgElement = divElement.querySelector('img'); + + imgElement.src = response.thumbnail; + imgElement.dataset.src = response.thumbnail; + imgElement.style.opacity = '1'; + divElement.classList.remove('hidden'); + } + } + }; + xhr.send(); +} + (() => { + const basePath = document.querySelector('input[name="js_base_path"]').value; const loaders = document.querySelectorAll('.loading-input'); + + /* + * METADATA FOR EDIT BOOKMARK PAGE + */ const inputTitle = document.querySelector('input[name="lf_title"]'); - if (inputTitle != null && inputTitle.value.length > 0) { - clearLoaders(loaders); - return; - } + if (inputTitle != null) { + if (inputTitle.value.length > 0) { + clearLoaders(loaders); + return; + } - const url = document.querySelector('input[name="lf_url"]').value; - const basePath = document.querySelector('input[name="js_base_path"]').value; + const url = document.querySelector('input[name="lf_url"]').value; - const xhr = new XMLHttpRequest(); - xhr.open('GET', `${basePath}/admin/metadata?url=${encodeURI(url)}`, true); - xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded'); - xhr.onload = () => { - const result = JSON.parse(xhr.response); - Object.keys(result).forEach((key) => { - if (result[key] !== null && result[key].length) { - const element = document.querySelector(`input[name="lf_${key}"], textarea[name="lf_${key}"]`); - if (element != null && element.value.length === 0) { - element.value = he.decode(result[key]); + const xhr = new XMLHttpRequest(); + xhr.open('GET', `${basePath}/admin/metadata?url=${encodeURI(url)}`, true); + xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded'); + xhr.onload = () => { + const result = JSON.parse(xhr.response); + Object.keys(result).forEach((key) => { + if (result[key] !== null && result[key].length) { + const element = document.querySelector(`input[name="lf_${key}"], textarea[name="lf_${key}"]`); + if (element != null && element.value.length === 0) { + element.value = he.decode(result[key]); + } } - } - }); - clearLoaders(loaders); - }; + }); + clearLoaders(loaders); + }; - xhr.send(); + xhr.send(); + } + + /* + * METADATA FOR THUMBNAIL RETRIEVAL + */ + const thumbsToLoad = document.querySelectorAll('div[data-async-thumbnail]'); + if (thumbsToLoad != null) { + [...thumbsToLoad].forEach((divElement) => { + const { id } = divElement.closest('[data-id]').dataset; + + updateThumb(basePath, divElement, id); + }); + } })(); diff --git a/tests/bookmark/BookmarkTest.php b/tests/bookmark/BookmarkTest.php index 4c7ae4c0..4c1ae25d 100644 --- a/tests/bookmark/BookmarkTest.php +++ b/tests/bookmark/BookmarkTest.php @@ -347,4 +347,48 @@ class BookmarkTest extends TestCase $bookmark->deleteTag('nope'); $this->assertEquals(['tag1', 'tag2', 'chair'], $bookmark->getTags()); } + + /** + * Test shouldUpdateThumbnail() with bookmarks needing an update. + */ + public function testShouldUpdateThumbnail(): void + { + $bookmark = (new Bookmark())->setUrl('http://domain.tld/with-image'); + + static::assertTrue($bookmark->shouldUpdateThumbnail()); + + $bookmark = (new Bookmark()) + ->setUrl('http://domain.tld/with-image') + ->setThumbnail('unknown file') + ; + + static::assertTrue($bookmark->shouldUpdateThumbnail()); + } + + /** + * Test shouldUpdateThumbnail() with bookmarks that should not update. + */ + public function testShouldNotUpdateThumbnail(): void + { + $bookmark = (new Bookmark()); + + static::assertFalse($bookmark->shouldUpdateThumbnail()); + + $bookmark = (new Bookmark()) + ->setUrl('ftp://domain.tld/other-protocol', ['ftp']) + ; + + static::assertFalse($bookmark->shouldUpdateThumbnail()); + + $bookmark = (new Bookmark()) + ->setUrl('http://domain.tld/with-image') + ->setThumbnail(__FILE__) + ; + + static::assertFalse($bookmark->shouldUpdateThumbnail()); + + $bookmark = (new Bookmark())->setUrl('/shaare/abcdef'); + + static::assertFalse($bookmark->shouldUpdateThumbnail()); + } } diff --git a/tests/front/controller/admin/ManageShaareControllerTest/DisplayCreateFormTest.php b/tests/front/controller/admin/ManageShaareControllerTest/DisplayCreateFormTest.php index 4fd88480..eafa54eb 100644 --- a/tests/front/controller/admin/ManageShaareControllerTest/DisplayCreateFormTest.php +++ b/tests/front/controller/admin/ManageShaareControllerTest/DisplayCreateFormTest.php @@ -144,12 +144,14 @@ class DisplayCreateFormTest extends TestCase // Make sure that PluginManager hook is triggered $this->container->pluginManager - ->expects(static::at(0)) + ->expects(static::atLeastOnce()) ->method('executeHooks') + ->withConsecutive(['render_editlink'], ['render_includes']) ->willReturnCallback(function (string $hook, array $data): array { - static::assertSame('render_editlink', $hook); - static::assertSame('', $data['link']['title']); - static::assertSame('', $data['link']['description']); + if ('render_editlink' === $hook) { + static::assertSame('', $data['link']['title']); + static::assertSame('', $data['link']['description']); + } return $data; }) diff --git a/tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php b/tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php index 37542c26..1adeef5a 100644 --- a/tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php +++ b/tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php @@ -209,7 +209,7 @@ class SaveBookmarkTest extends TestCase /** * Test save a bookmark - try to retrieve the thumbnail */ - public function testSaveBookmarkWithThumbnail(): void + public function testSaveBookmarkWithThumbnailSync(): void { $parameters = ['lf_url' => 'http://url.tld/other?part=3#hash']; @@ -224,7 +224,13 @@ class SaveBookmarkTest extends TestCase $this->container->conf = $this->createMock(ConfigManager::class); $this->container->conf->method('get')->willReturnCallback(function (string $key, $default) { - return $key === 'thumbnails.mode' ? Thumbnailer::MODE_ALL : $default; + if ($key === 'thumbnails.mode') { + return Thumbnailer::MODE_ALL; + } elseif ($key === 'general.enable_async_metadata') { + return false; + } + + return $default; }); $this->container->thumbnailer = $this->createMock(Thumbnailer::class); @@ -274,6 +280,51 @@ class SaveBookmarkTest extends TestCase static::assertSame(302, $result->getStatusCode()); } + /** + * Test save a bookmark - do not attempt to retrieve thumbnails if async mode is enabled. + */ + public function testSaveBookmarkWithThumbnailAsync(): void + { + $parameters = ['lf_url' => 'http://url.tld/other?part=3#hash']; + + $request = $this->createMock(Request::class); + $request + ->method('getParam') + ->willReturnCallback(function (string $key) use ($parameters): ?string { + return $parameters[$key] ?? null; + }) + ; + $response = new Response(); + + $this->container->conf = $this->createMock(ConfigManager::class); + $this->container->conf->method('get')->willReturnCallback(function (string $key, $default) { + if ($key === 'thumbnails.mode') { + return Thumbnailer::MODE_ALL; + } elseif ($key === 'general.enable_async_metadata') { + return true; + } + + return $default; + }); + + $this->container->thumbnailer = $this->createMock(Thumbnailer::class); + $this->container->thumbnailer->expects(static::never())->method('get'); + + $this->container->bookmarkService + ->expects(static::once()) + ->method('addOrSet') + ->willReturnCallback(function (Bookmark $bookmark): Bookmark { + static::assertNull($bookmark->getThumbnail()); + + return $bookmark; + }) + ; + + $result = $this->controller->save($request, $response); + + static::assertSame(302, $result->getStatusCode()); + } + /** * Change the password with a wrong existing password */ diff --git a/tests/front/controller/visitor/BookmarkListControllerTest.php b/tests/front/controller/visitor/BookmarkListControllerTest.php index 0c95df97..5ca92507 100644 --- a/tests/front/controller/visitor/BookmarkListControllerTest.php +++ b/tests/front/controller/visitor/BookmarkListControllerTest.php @@ -307,7 +307,13 @@ class BookmarkListControllerTest extends TestCase $this->container->conf ->method('get') ->willReturnCallback(function (string $key, $default) { - return $key === 'thumbnails.mode' ? Thumbnailer::MODE_ALL : $default; + if ($key === 'thumbnails.mode') { + return Thumbnailer::MODE_ALL; + } elseif ($key === 'general.enable_async_metadata') { + return false; + } + + return $default; }) ; @@ -357,7 +363,13 @@ class BookmarkListControllerTest extends TestCase $this->container->conf ->method('get') ->willReturnCallback(function (string $key, $default) { - return $key === 'thumbnails.mode' ? Thumbnailer::MODE_ALL : $default; + if ($key === 'thumbnails.mode') { + return Thumbnailer::MODE_ALL; + } elseif ($key === 'general.enable_async_metadata') { + return false; + } + + return $default; }) ; @@ -378,6 +390,47 @@ class BookmarkListControllerTest extends TestCase static::assertSame('linklist', (string) $result->getBody()); } + /** + * Test getting a permalink with thumbnail update with async setting: no update should run. + */ + public function testThumbnailUpdateFromPermalinkAsync(): void + { + $request = $this->createMock(Request::class); + $response = new Response(); + + $this->container->loginManager = $this->createMock(LoginManager::class); + $this->container->loginManager->method('isLoggedIn')->willReturn(true); + + $this->container->conf = $this->createMock(ConfigManager::class); + $this->container->conf + ->method('get') + ->willReturnCallback(function (string $key, $default) { + if ($key === 'thumbnails.mode') { + return Thumbnailer::MODE_ALL; + } elseif ($key === 'general.enable_async_metadata') { + return true; + } + + return $default; + }) + ; + + $this->container->thumbnailer = $this->createMock(Thumbnailer::class); + $this->container->thumbnailer->expects(static::never())->method('get'); + + $this->container->bookmarkService + ->expects(static::once()) + ->method('findByHash') + ->willReturn((new Bookmark())->setId(2)->setUrl('https://url.tld')->setTitle('Title 1')) + ; + $this->container->bookmarkService->expects(static::never())->method('set'); + $this->container->bookmarkService->expects(static::never())->method('save'); + + $result = $this->controller->permalink($request, $response, ['hash' => 'abc']); + + static::assertSame(200, $result->getStatusCode()); + } + /** * Trigger legacy controller in link list controller: permalink */ diff --git a/tpl/default/linklist.html b/tpl/default/linklist.html index beab0eac..48cd9aad 100644 --- a/tpl/default/linklist.html +++ b/tpl/default/linklist.html @@ -135,8 +135,12 @@
- {if="$thumbnails_enabled && !empty($value.thumbnail)"} -
+ {if="$thumbnails_enabled && $value.thumbnail !== false"} +
{ignore}RainTPL hack: put the 2 src on two different line to avoid path replace bug{/ignore}

+ {if="strpos($value.url, $value.shorturl) === false"} {else} @@ -308,5 +312,6 @@ {include="page.footer"} +{if="$is_logged_in && $async_metadata"}{/if} diff --git a/tpl/vintage/linklist.html b/tpl/vintage/linklist.html index 00896eb5..90f5cf8f 100644 --- a/tpl/vintage/linklist.html +++ b/tpl/vintage/linklist.html @@ -77,10 +77,10 @@ {/if}