Fix the sources table crash (#4725)

* Fix the sources table crash

Signed-off-by: Olga Bulat <obulat@gmail.com>

* Update frontend/test/playwright/e2e/sources.spec.ts

Co-authored-by: Dhruv Bhanushali <hi@dhruvkb.dev>

* Update the unit tests

Signed-off-by: Olga Bulat <obulat@gmail.com>

* Update source sorting

Signed-off-by: Olga Bulat <obulat@gmail.com>

* Add a space between sentences

Signed-off-by: Olga Bulat <obulat@gmail.com>

* Snapshots

Signed-off-by: Olga Bulat <obulat@gmail.com>

---------

Signed-off-by: Olga Bulat <obulat@gmail.com>
Co-authored-by: Dhruv Bhanushali <hi@dhruvkb.dev>

Author: obulat

frontend/src/components/VSourcesTable.vue

@@ -40,7 +40,7 @@
         </tr>
       </thead>
       <tbody>
-        <tr v-for="provider in sortedProviders" :key="provider.display_name">
+        <tr v-for="provider in providers" :key="provider.display_name">
           <td>
             <VLink :href="providerViewUrl(provider)">{{
               provider.display_name
@@ -60,7 +60,7 @@
 
     <section role="region" class="mobile-source-table md:hidden">
       <article
-        v-for="provider in sortedProviders"
+        v-for="provider in providers"
         :key="provider.display_name"
         :title="provider.display_name"
       >
@@ -87,7 +87,7 @@
 </template>
 
 <script lang="ts">
-import { computed, defineComponent, type PropType, reactive } from "vue"
+import { defineComponent, type PropType, reactive, ref } from "vue"
 
 import { useProviderStore } from "~/stores/provider"
 import { useGetLocaleFormattedNumber } from "~/composables/use-get-locale-formatted-number"
@@ -113,19 +113,32 @@ export default defineComponent({
     },
   },
   setup(props) {
+    const providerStore = useProviderStore()
+    const searchStore = useSearchStore()
+
     const sorting = reactive({
       direction: "asc",
       field: "display_name" as keyof Omit<MediaProvider, "logo_url">,
     })
 
+    // The providers in store are sorted by `source_name`, here we sort them by `display_name`.
+    const providers = ref<MediaProvider[]>(
+      providerStore.providers[props.media].sort(compareProviders)
+    )
+
     function sortTable(field: keyof Omit<MediaProvider, "logo_url">) {
-      let direction = "asc"
+      let direction = field === "media_count" ? "desc" : "asc"
       if (field === sorting.field) {
         direction = sorting.direction === "asc" ? "desc" : "asc"
       }
 
       sorting.direction = direction
       sorting.field = field
+
+      const sortedProviders = providers.value.sort(compareProviders)
+
+      providers.value =
+        direction === "asc" ? sortedProviders : sortedProviders.reverse()
     }
 
     function cleanSourceUrlForPresentation(url: string) {
@@ -138,7 +151,6 @@ export default defineComponent({
     }
 
     const getLocaleFormattedNumber = useGetLocaleFormattedNumber()
-    const providerStore = useProviderStore()
 
     function compareProviders(prov1: MediaProvider, prov2: MediaProvider) {
       let field1 = prov1[sorting.field]
@@ -161,13 +173,6 @@ export default defineComponent({
       return 0
     }
 
-    const sortedProviders = computed<MediaProvider[]>(() => {
-      const providers = providerStore.providers[props.media]
-      providers.sort(compareProviders)
-      return sorting.direction === "asc" ? providers : providers.reverse()
-    })
-
-    const searchStore = useSearchStore()
     const providerViewUrl = (provider: MediaProvider) => {
       return searchStore.getCollectionPath({
         type: props.media,
@@ -179,7 +184,7 @@ export default defineComponent({
     }
     return {
       getLocaleFormattedNumber,
-      sortedProviders,
+      providers,
       sorting,
       sortTable,
       cleanSourceUrlForPresentation,

frontend/src/pages/sources.vue

@@ -44,6 +44,7 @@ useHead({ title: `${t("sources.title")} | Openverse` })
           <VLink href="https://www.si.edu/">Smithsonian Institute</VLink>
         </template>
       </i18n-t>
+      {{ " " }}
       <i18n-t
         scope="global"
         keypath="sources.ccContent.provider.b"

frontend/test/playwright/e2e/sources.spec.ts

@@ -16,3 +16,17 @@ test("sources table has links to source pages", async ({ page }) => {
 
   await expect(getH1(page, "Flickr")).toBeVisible()
 })
+
+// Tests the fix for https://github.com/WordPress/openverse/issues/4724
+test("sources table can be sorted multiple times", async ({ page }) => {
+  await preparePageForTests(page, "xl")
+  await page.goto("/sources")
+
+  const totalItems = page.getByRole("cell", { name: /total items/i }).first()
+  await totalItems.click()
+  await totalItems.click()
+  await totalItems.click()
+
+  const firstRow = page.getByRole("row", { name: "Flickr" })
+  await expect(firstRow).toBeVisible()
+})

frontend/test/playwright/visual-regression/pages/pages.spec.ts

@@ -39,7 +39,7 @@ for (const contentPage of contentPages) {
             {
               fullPage: true,
             },
-            { maxDiffPixelRatio: 0.01 }
+            { maxDiffPixelRatio: 0.005 }
           )
         })
       })

frontend/test/playwright/visual-regression/pages/pages.spec.ts-snapshots/about-ltr-lg-linux.png

@@ -26,6 +26,12 @@ describe("VSourcesTable", () => {
     providerStore.$patch({
       providers: {
         image: [
+          {
+            source_name: "Provider_A",
+            display_name: "Provider A",
+            source_url: "https://zzz.com",
+            media_count: 3333,
+          },
           {
             source_name: "Provider_B",
             display_name: "Provider B",
@@ -38,12 +44,6 @@ describe("VSourcesTable", () => {
             source_url: "www.xxx.com",
             media_count: 2222,
           },
-          {
-            source_name: "Provider_A",
-            display_name: "Provider A",
-            source_url: "https://zzz.com",
-            media_count: 3333,
-          },
         ],
       },
     })
@@ -55,7 +55,7 @@ describe("VSourcesTable", () => {
     }
   })
 
-  it('should be sorted by display_name ("Source") by default', async () => {
+  it('should use the provider store default sorting by display_name ("Source") by default', async () => {
     await render(VSourcesTable, options)
 
     const table = getTableData(screen.getAllByRole("row"))
@@ -90,9 +90,9 @@ describe("VSourcesTable", () => {
     await userEvent.click(domainCell)
     const table = getTableData(screen.getAllByRole("row"))
     const expectedTable = [
-      ["Provider B", "yyy.com", "1,111"],
-      ["Provider C", "xxx.com", "2,222"],
       ["Provider A", "zzz.com", "3,333"],
+      ["Provider C", "xxx.com", "2,222"],
+      ["Provider B", "yyy.com", "1,111"],
     ]
     expect(table).toEqual(expectedTable)
   })