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

fix(ecau): handle multiple a-tisket release suggestions
a-tisket can sometimes suggest multiple releases, e.g. https://atisket.pulsewidth.org.uk/?deez_id=507397
Up until now, we only generated one seed link for the first release. This can lead users to accidentally upload the same image twice, thinking they're adding it to different releases.

Instead, we'll now add multiple seed links if there are multiple release suggestions, and distinguish them by the first chunk of their MBID. The MBID is only included when there are multiple releases.

https://community.metabrainz.org/t/ropdebees-userscripts-support-thread/551947/94?u=ropdebee


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.`);
         }
 
diff --git a/src/mb_enhanced_cover_art_uploads/seeding/atisket/index.tsx b/src/mb_enhanced_cover_art_uploads/seeding/atisket/index.tsx
index e702600e..41d10548 100644
--- a/src/mb_enhanced_cover_art_uploads/seeding/atisket/index.tsx
+++ b/src/mb_enhanced_cover_art_uploads/seeding/atisket/index.tsx
@@ -17,18 +17,23 @@ export const AtisketSeeder: Seeder = {
     insertSeedLinks(): void {
         addDimensionsToCovers();
 
-        const alreadyInMB = qsMaybe('.already-in-mb-item');
-        if (alreadyInMB === null) {
+        const alreadyInMBItems = qsa('.already-in-mb-item');
+        // If a-tisket doesn't report a release as already existing, we can't
+        // know a potential MBID and we can't add a seed link.
+        if (alreadyInMBItems.length === 0) {
             return;
         }
-        const mbid = encodeURIComponent(qs<HTMLAnchorElement>('a.mb', alreadyInMB).textContent?.trim() ?? '');
+
+        const mbids = alreadyInMBItems
+            .map((alreadyInMB) => encodeURIComponent(qs<HTMLAnchorElement>('a.mb', alreadyInMB).textContent?.trim() ?? ''))
+            .filter(Boolean);
 
         // Try to use the cached link as the origin instead of the page URL itself,
         // so that we link to the state at the time the image was submitted.
         // If the cached link doesn't exist on the page, we're probably already
         // on a cached page, so fall back on the page URL instead.
         const cachedAnchor = qsMaybe<HTMLAnchorElement>('#submit-button + div > a');
-        addSeedLinkToCovers(mbid, cachedAnchor?.href ?? document.location.href);
+        addSeedLinkToCovers(mbids, cachedAnchor?.href ?? document.location.href);
     },
 };
 
@@ -51,14 +56,14 @@ export const AtasketSeeder: Seeder = {
         // For atasket links, we'll also use the cached URL as origin for the
         // same reasons as above.
         const cachedUrl = document.location.origin + '/?cached=' + selfId;
-        addSeedLinkToCovers(mbid, cachedUrl);
+        addSeedLinkToCovers([mbid], cachedUrl);
     },
 };
 
-function addSeedLinkToCovers(mbid: string, origin: string): void {
+function addSeedLinkToCovers(mbids: string[], origin: string): void {
     const covers = qsa<HTMLElement>('figure.cover');
     for (const fig of covers) {
-        addSeedLinkToCover(fig, mbid, origin);
+        addSeedLinkToCover(fig, mbids, origin);
     }
 }
 
@@ -83,7 +88,7 @@ function tryExtractReleaseUrl(fig: HTMLElement): string | undefined {
     return RELEASE_URL_CONSTRUCTORS[vendorCode](vendorId, countryCode);
 }
 
-function addSeedLinkToCover(fig: HTMLElement, mbid: string, origin: string): void {
+function addSeedLinkToCover(fig: HTMLElement, mbids: string[], origin: string): void {
     const imageUrl = qs<HTMLAnchorElement>('a.icon', fig).href;
 
     // On atj's mirror, we'll seed the release URLs, instead of the images
@@ -96,16 +101,20 @@ function addSeedLinkToCover(fig: HTMLElement, mbid: string, origin: string): voi
     const params = new SeedParameters([{
         url: new URL(realUrl),
     }], origin);
-    const seedUrl = params.createSeedURL(mbid);
 
-    const seedLink = <a href={seedUrl} style={{ display: 'block' }}>
-        Add to release
-    </a>;
+    for (const mbid of mbids) {
+        const seedUrl = params.createSeedURL(mbid);
+
+        // Include part of the release ID if there are multiple.
+        const seedLink = <a href={seedUrl} style={{ display: 'block' }}>
+            Add to release {mbids.length > 1 ? mbid.split('-')[0] : ''}
+        </a>;
 
-    // The way in which we're adding the seed link here and the dimensions span
-    // below should lead to a consistent ordering of elements.
-    qs<HTMLElement>('figcaption', fig)
-        .insertAdjacentElement('beforeend', seedLink);
+        // The way in which we're adding the seed link here and the dimensions span
+        // below should lead to a consistent ordering of elements.
+        qs<HTMLElement>('figcaption', fig)
+            .insertAdjacentElement('beforeend', seedLink);
+    }
 }
 
 async function addDimensions(fig: HTMLElement): Promise<void> {


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);