Merge pull request #1584 from ArthurHoaro/feature/async-thumbnail-retrieval

Asynchronous retrieval of bookmark's thumbnails

application/bookmark/Bookmark.php

@@ -377,6 +377,24 @@ public function setThumbnail($thumbnail): 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.
      *

application/front/controller/admin/ManageShaareController.php

@@ -129,7 +129,8 @@ public function save(Request $request, Response $response): Response
         $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()));
         }

application/front/controller/visitor/BookmarkListController.php

@@ -169,14 +169,11 @@ public function permalink(Request $request, Response $response, array $args): Re
      */
     protected function updateThumbnail(Bookmark $bookmark, bool $writeDatastore = true): bool
     {
-        // Logged in, thumbnails enabled, not a note, is HTTP
+        // Logged in, not async retrieval, thumbnails enabled, and thumbnail should be updated
-        // and (never retrieved yet or no valid cache file)
         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->shouldUpdateThumbnail()
-            && !$bookmark->isNote()
-            && (null === $bookmark->getThumbnail() || !is_file($bookmark->getThumbnail()))
-            && startsWith(strtolower($bookmark->getUrl()), 'http')
         ) {
             $bookmark->setThumbnail($this->container->thumbnailer->get($bookmark->getUrl()));
             $this->container->bookmarkService->set($bookmark, $writeDatastore);
@@ -198,6 +195,7 @@ protected function initializeTemplateVars(): array
             'page_max' => '',
             'search_tags' => '',
             'result_count' => '',
+            'async_metadata' => $this->container->conf->get('general.enable_async_metadata', true)
         ];
     }
 

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 <div> 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) {
+  if (inputTitle != null) {
-    clearLoaders(loaders);
+    if (inputTitle.value.length > 0) {
-    return;
+      clearLoaders(loaders);
+      return;
+    }
+
+    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]);
+          }
+        }
+      });
+      clearLoaders(loaders);
+    };
+
+    xhr.send();
   }
 
-  const url = document.querySelector('input[name="lf_url"]').value;
+  /*
-  const basePath = document.querySelector('input[name="js_base_path"]').value;
+   * 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;
 
-  const xhr = new XMLHttpRequest();
+      updateThumb(basePath, divElement, id);
-  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);
+  }
-  };
-
-  xhr.send();
 })();

tests/bookmark/BookmarkTest.php

@@ -347,4 +347,48 @@ public function testDeleteTagNotExists()
         $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());
+    }
 }

tests/front/controller/admin/ManageShaareControllerTest/DisplayCreateFormTest.php

@@ -144,12 +144,14 @@ public function testDisplayCreateFormWithUrlAndWithoutMetadata(): void
 
         // 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);
+                if ('render_editlink' === $hook) {
-                static::assertSame('', $data['link']['title']);
+                    static::assertSame('', $data['link']['title']);
-                static::assertSame('', $data['link']['description']);
+                    static::assertSame('', $data['link']['description']);
+                }
 
                 return $data;
             })

tests/front/controller/admin/ManageShaareControllerTest/SaveBookmarkTest.php

@@ -209,7 +209,7 @@ public function testSaveExistingBookmark(): void
     /**
      * 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 @@ public function testSaveBookmarkWithThumbnail(): void
 
         $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 @@ public function testSaveBookmarkWithIdZero(): void
         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
      */

tests/front/controller/visitor/BookmarkListControllerTest.php

@@ -307,7 +307,13 @@ public function testThumbnailUpdateFromLinkList(): void
         $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 @@ public function testThumbnailUpdateFromPermalink(): void
         $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 @@ public function testThumbnailUpdateFromPermalink(): void
         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
      */

tpl/default/linklist.html

@@ -135,8 +135,12 @@
 
         <div class="linklist-item linklist-item{if="$value.class"} {$value.class}{/if}" data-id="{$value.id}">
           <div class="linklist-item-title">
-            {if="$thumbnails_enabled && !empty($value.thumbnail)"}
+            {if="$thumbnails_enabled && $value.thumbnail !== false"}
-              <div class="linklist-item-thumbnail" style="width:{$thumbnails_width}px;height:{$thumbnails_height}px;">
+              <div
+                class="linklist-item-thumbnail {if="$value.thumbnail === null"}hidden{/if}"
+                style="width:{$thumbnails_width}px;height:{$thumbnails_height}px;"
+                {if="$value.thumbnail === null"}data-async-thumbnail="1"{/if}
+              >
                 <div class="thumbnail">
                   {ignore}RainTPL hack: put the 2 src on two different line to avoid path replace bug{/ignore}
                   <a href="{$value.real_url}" aria-hidden="true" tabindex="-1">
@@ -158,7 +162,7 @@
             </div>
 
             <h2>
-              <a href="{$value.real_url}">
+              <a href="{$value.real_url}" class="linklist-real-url">
                 {if="strpos($value.url, $value.shorturl) === false"}
                   <i class="fa fa-external-link" aria-hidden="true"></i>
                 {else}
@@ -308,5 +312,6 @@ <h2>
 
 {include="page.footer"}
 <script src="{$asset_path}/js/thumbnails.min.js?v={$version_hash}#"></script>
+{if="$is_logged_in && $async_metadata"}<script src="{$asset_path}/js/metadata.min.js?v={$version_hash}#"></script>{/if}
 </body>
 </html>

tpl/vintage/linklist.html

@@ -77,10 +77,10 @@
     {/if}
     <ul>
         {loop="$links"}
-        <li{if="$value.class"} class="{$value.class}"{/if}>
+        <li{if="$value.class"} class="{$value.class}"{/if} data-id="{$value.id}">
             <a id="{$value.shorturl}"></a>
-            {if="$thumbnails_enabled && !empty($value.thumbnail)"}
+            {if="$thumbnails_enabled && $value.thumbnail !== false"}
-                <div class="thumbnail">
+                <div class="thumbnail" {if="$value.thumbnail === null"}data-async-thumbnail="1"{/if}>
                     <a href="{$value.real_url}">
                         {ignore}RainTPL hack: put the 2 src on two different line to avoid path replace bug{/ignore}
                         <img data-src="{$base_path}/{$value.thumbnail}#" class="b-lazy"
@@ -153,6 +153,7 @@
 
     {include="page.footer"}
 <script src="{$asset_path}/js/thumbnails.min.js#"></script>
+{if="$is_logged_in && $async_metadata"}<script src="{$asset_path}/js/metadata.min.js?v={$version_hash}#"></script>{/if}
 
 </body>
 </html>