Repository: ROpdebee/mb-userscripts
Pull request #529 (2022-07-28 11:25:32)

fix(ecau): allow retrying provider URLs when some images failed
https://community.metabrainz.org/t/ropdebees-userscripts-support-thread/551947/94?u=ropdebee

We were marking a provider as done even if some (or even all!) of the images failed to fetch. This change will allow retrying when one or more images failed to fetch. Only the failed images will be retried, the ones that are already queued will be skipped via pre-existing logic.

codecov[bot]: # [Codecov](https://codecov.io/gh/ROpdebee/mb-userscripts/pull/529?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck) Report
> Merging [#529](https://codecov.io/gh/ROpdebee/mb-userscripts/pull/529?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck) (3870cfb) into [main](https://codecov.io/gh/ROpdebee/mb-userscripts/commit/a5bfdb439ed2bcc631af35b2b03a7f5a3cce2696?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck) (a5bfdb4) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##             main     #529   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files          57       57           
  Lines        1308     1309    +1     
  Branches      204      205    +1     
=======================================
+ Hits         1305     1306    +1     
  Misses          3        3           
```


| [Impacted Files](https://codecov.io/gh/ROpdebee/mb-userscripts/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck) | Coverage Δ | |
|---|---|---|
| [src/mb\_enhanced\_cover\_art\_uploads/fetch.ts](https://codecov.io/gh/ROpdebee/mb-userscripts/pull/529/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck#diff-c3JjL21iX2VuaGFuY2VkX2NvdmVyX2FydF91cGxvYWRzL2ZldGNoLnRz) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ROpdebee/mb-userscripts/pull/529?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/ROpdebee/mb-userscripts/pull/529?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck). Last update [a5bfdb4...3870cfb](https://codecov.io/gh/ROpdebee/mb-userscripts/pull/529?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ruben+Opdebeeck).


Patch:
diff --git a/src/mb_enhanced_cover_art_uploads/fetch.ts b/src/mb_enhanced_cover_art_uploads/fetch.ts
index 48adf336..aa7a7020 100644
--- a/src/mb_enhanced_cover_art_uploads/fetch.ts
+++ b/src/mb_enhanced_cover_art_uploads/fetch.ts
@@ -172,11 +172,11 @@ export class ImageFetcher {
             }
         }
 
-        if (!hasMoreImages) {
+        if (!hasMoreImages && queuedResults.length === finalImages.length) {
             // Don't mark the whole provider URL as done if we haven't grabbed
-            // all images.
+            // all images or some images failed.
             this.doneImages.add(url.href);
-        } else {
+        } else if (hasMoreImages) {
             LOGGER.warn(`Not all images were fetched: ${images.length - finalImages.length} covers were skipped.`);
         }
 


Test patch:
diff --git a/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts b/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts
index 32f34e22..6e8e3d77 100644
--- a/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts
+++ b/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts
@@ -81,6 +81,8 @@ function disableDummyFetch(mock: FetchImageContentsSpy): void {
 
 beforeEach(() => {
     mockEnqueueImage.mockClear();
+    mockFindImages.mockReset();
+    mockGetProvider.mockReset();
     hooks.onFetchFinished.mockClear();
     hooks.onFetchProgress.mockClear();
     hooks.onFetchStarted.mockClear();
@@ -753,6 +755,46 @@ describe('fetching images', () => {
             .resolves.toHaveProperty('images', []);
     });
 
+    it('does not fetch provider URL which was already fully fetched', async () => {
+        mockGetProvider.mockImplementation(() => fakeProvider);
+        mockFindImages.mockResolvedValue([
+            createCoverArt('https://example.com/1'),
+            createCoverArt('https://example.com/2'),
+        ]);
+
+        await expect(fetcher.fetchImages({ url: new URL('https://example.com/1') }, false))
+            .resolves.toHaveProperty('images', expect.toBeArrayOfSize(2));
+        // Second fetch should be blocked, since all previous images are done.
+        await expect(fetcher.fetchImages({ url: new URL('https://example.com/1') }, false))
+            .resolves.toHaveProperty('images', []);
+        expect(mockFindImages).toHaveBeenCalledOnce();
+    });
+
+    it('allows re-fetching provider URL for which some images failed previously', async () => {
+        mockGetProvider.mockImplementation(() => fakeProvider);
+        mockFindImages.mockResolvedValue([
+            createCoverArt('https://example.com/1'),
+            createCoverArt('https://example.com/2'),
+        ]);
+        mockFetchImageContents.mockRejectedValueOnce(new Error('test'));
+
+        // First fetch will fail to fetch the first of the two images.
+        await expect(fetcher.fetchImages({ url: new URL('https://example.com/1') }, false))
+            .resolves.toHaveProperty('images', [
+                expect.objectContaining({
+                    fetchedUrl: new URL('https://example.com/2'),
+                }),
+            ]);
+        // Second fetch should now only fetch the first image, second one already fetched previously.
+        await expect(fetcher.fetchImages({ url: new URL('https://example.com/1') }, false))
+            .resolves.toHaveProperty('images', [
+                expect.objectContaining({
+                    fetchedUrl: new URL('https://example.com/1'),
+                }),
+            ]);
+        expect(mockFindImages).toHaveBeenCalledTimes(2);
+    });
+
     it('does not fetch maximised URL which was already fetched previously', async () => {
         await fetcher.fetchImages({ url: new URL('https://example.com/1') }, false);