Fix i18n placeholder replacement and make tests robust (#5343)

* Fix story types
* Replace left-over placeholders
* Update i18n scripts
The placeholders containing single quote (`{'Openverse'})` were not converted to pot
Some incorrect placeholders were sent to GlotPress, and translation jsons contain {''} instead of ### ### placeholders
Fix vue-i18n link
* Make the Playwright tests more robust
* Add missing prop to audio collection
* Watch computeds instead of getters
* Remove stray log line
* Set the Playwright timeout to 5 seconds
* Fix plugin initialization to fix provider fetching
* Add pnpm lock in ci
* Pin @nuxtjs/i18n version
* Check if pnpm lock works in CI
* Make storybook smoke test more robust
* Update frontend/test/playwright/playwright.config.ts
* Revert stylistic change
* COPY the lock file from the frontend dir
* Log proxy requests if `verbose` is true

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

Author: obulat

.github/workflows/ci_cd.yml

@@ -914,6 +914,9 @@ jobs:
           install_recipe: node-install
           locales: "test"
 
+      - name: Copy pnpm-lock.yaml
+        run: cp pnpm-lock.yaml frontend/
+
       - name: Run Playwright tests
         run: just frontend/run ${{ matrix.script }} --workers=2
 

documentation/frontend/reference/i18n.md

@@ -69,8 +69,7 @@ line arguments. There are three main modes of operation:
     empty, written to `untranslated-locales.json`.
 
 - Pass the fallback locale mappings to the underlying Vue i18n plugin. This is
-  configured in the plugin configuration file,
-  `frontend/src/plugins/vue-i18n.ts`.
+  configured in the plugin configuration file, `frontend/src/i18n/vue-i18n.ts`.
 
 ## Test locales
 

frontend/Dockerfile.playwright

@@ -8,6 +8,7 @@ FROM mcr.microsoft.com/playwright:v${PLAYWRIGHT_VERSION}-jammy
 WORKDIR /frontend
 
 COPY package.json .
+COPY pnpm-lock.yaml .
 
 # Ensure the Playwright container's pnpm cache folder exists and is writable
 RUN mkdir -p /.cache/node/corepack/ && chmod -R 777 /.cache/node/corepack/

frontend/i18n/data/en.json5

@@ -17,7 +17,7 @@ _{link}_ part of "The translation for English locale is incomplete. Help us get
 */
   "notification.translation.link": "contributing a translation",
   "notification.translation.close": "Close the translation contribution help request banner",
-  "notification.analytics.text": "Openverse uses analytics to improve the quality of our service. Visit {link} to learn more or opt out.",
+  "notification.analytics.text": "{'Openverse'} uses analytics to improve the quality of our service. Visit {link} to learn more or opt out.",
   /**
   Interpolated into notification.analytics.text:
   _{link}_ part of "Visit _{link}_ to learn more or opt out."
@@ -348,7 +348,7 @@ _{link}_ part of "The translation for English locale is incomplete. Help us get
   /** generatedTags.content.a-d are parts of a single section explaining how generated tags work in Openverse. */
   "tags.generatedTags.content.b": "While generally reliable, automated systems can sometimes misinterpret or miss elements in an image.",
   /** generatedTags.content.a-d are parts of a single section explaining how generated tags work in Openverse. */
-  "tags.generatedTags.content.c": "Openverse makes efforts to exclude any generated tags that make inferences about the identities or affiliations of human subjects. ",
+  "tags.generatedTags.content.c": "{'Openverse'} makes efforts to exclude any generated tags that make inferences about the identities or affiliations of human subjects. ",
   /** generatedTags.content.a-d are parts of a single section explaining how generated tags work in Openverse. */
   "tags.generatedTags.content.d": 'If you encounter any images with generated tags making assumptions about, for example, gender, religion, or political affiliation, please report the images using the "Report" button on our single result pages.',
   "error.occurred": "An error occurred",

frontend/i18n/scripts/po/po-helpers.mjs

@@ -21,7 +21,7 @@ const PARSED_VUE_FILES = getParsedVueFiles()
 const escapeQuotes = (str) => str.replace(/"/g, '\\"')
 
 /** @param str {string} */
-const containsCurlyWord = (str) => /\{[a-zA-Z-]*}/.test(str)
+const containsCurlyWord = (str) => /\{(?:'[a-zA-Z]+'|[a-zA-Z-]+)}/.test(str)
 
 /** @param str {string} */
 const checkStringForVars = (str) =>
@@ -40,7 +40,7 @@ const replaceVarsPlaceholders = (str) => {
     return str
   }
 
-  const variable = /\{(?<variable>[a-zA-Z-]*)}/g
+  const variable = /\{(?<variable>'[a-zA-Z]+'|[a-zA-Z-]+)}/g
   return str.replace(variable, `###$<variable>###`)
 }
 

frontend/i18n/scripts/translations.mjs

@@ -182,7 +182,12 @@ const replacePlaceholders = (json, locale, deprecatedKeys, invalidKeys) => {
       .replace(/[{}]/g, "###") // Replace all { and } with ###
       .replace(/<\/?em>/g, "") // Remove <em> and </em> tags
 
-    let replaced = cleaned.replace(/###([a-zA-Z-]*?)###/g, replacer)
+    // Hotfix for bugs introduced in https://github.com/WordPress/openverse/pull/5261
+    // Not all Openverse instances were replaced with 'Openverse'
+    // Pot file generation wasn't updated, and {'Openverse'} wasn't replaced with
+    // ###'Openverse'### before being sent to GlotPress
+    let replaced = cleaned.replace(/{'Openverse'}/g, "Openverse")
+    replaced = replaced.replace(/###([a-zA-Z-]+?)###/g, replacer)
     // Irregular placeholders with more or fewer than 3 #s
     replaced = replaced.replace(/#{1,4}([a-zA-Z-]+?)#{1,4}/g, "{$1}")
 

frontend/i18n/scripts/utils.mjs

@@ -12,6 +12,12 @@ export const snakeToCamel = (str) =>
  * Convert a kebab-case string (`image-title`) to camel case (`imageTitle`).
  */
 export function kebabToCamel(input) {
+  if (!input || typeof input !== "string") {
+    return ""
+  }
+  if (input === "-") {
+    return input
+  }
   const split = input.split("-")
   if (split.length === 1) {
     return input

frontend/src/components/VContentLink/meta/VContentLink.stories.ts

@@ -8,8 +8,6 @@ import type { StoryObj } from "@storybook/vue3"
 
 const contentLinkArgTypes = {
   mediaType: { options: ["audio", "image"], control: { type: "radio" } },
-  searchTerm: { control: { type: "string" } },
-  resultsCount: { control: { type: "number" } },
   isSelected: { control: { type: "boolean" } },
   layout: { options: ["stacked", "horizontal"], control: { type: "radio" } },
 }
@@ -34,8 +32,11 @@ export const Default: Story = {
 
   args: {
     mediaType: "image",
-    searchTerm: "cat",
-    resultsCount: 5708,
+    labels: {
+      aria: `View 5708 image results for cat`,
+      visible: `View 5708 image`,
+    },
+    to: "/search/image/?q=cat",
   },
 }
 
@@ -44,13 +45,17 @@ export const Horizontal: Story = {
   render: (args) => ({
     components: { VContentLink },
     setup() {
-      return () => h("div", { class: "max-w-md" }, [h(VContentLink, args)])
+      const count = { image: 5708, audio: 4561 }[args.mediaType]
+      const labels = {
+        aria: `View ${count} ${args.mediaType} results for cat`,
+        visible: `View ${count} ${args.mediaType}`,
+      }
+      return () =>
+        h("div", { class: "max-w-md" }, [h(VContentLink, { ...args, labels })])
     },
   }),
   args: {
     mediaType: "audio",
-    searchTerm: "cat",
-    resultsCount: 4561,
     layout: "horizontal",
   } as typeof VContentLink.props,
 }
@@ -70,8 +75,11 @@ export const Mobile: Omit<Story, "args"> = {
           types.map(({ mediaType, resultsCount }, key) =>
             h(VContentLink, {
               mediaType: mediaType as SupportedMediaType,
-              resultsCount,
-              searchTerm: "cat",
+              labels: {
+                aria: `View ${resultsCount} ${mediaType} results for cat`,
+                visible: `View ${resultsCount} ${mediaType}`,
+              },
+              to: `/search/${mediaType}/?q=cat`,
               key,
             })
           )

frontend/src/components/VThemeSelect/VThemeSelect.vue

@@ -64,7 +64,7 @@ const updateRefs = () => {
 }
 
 onMounted(updateRefs)
-watch(() => [darkMode.colorMode.value, darkMode.osColorMode.value], updateRefs)
+watch([darkMode.colorMode, darkMode.osColorMode], updateRefs)
 </script>
 
 <template>

frontend/src/composables/use-analytics.ts

@@ -2,7 +2,7 @@ import { useNuxtApp } from "#imports"
 
 /**
  * This wrapper around the plugin, retained to reduce code churn.
- * @see Refer to frontend/src/plugins/analytics.ts for plugin implementation
+ * @see Refer to frontend/src/plugins/03.analytics.ts for plugin implementation
  *
  * @deprecated For new code, use `$sendCustomEvent` from Nuxt context
  */

frontend/src/pages/audio/collection.vue

@@ -28,6 +28,7 @@ const {
   fetchMedia,
   loadMore,
   pageTitle,
+  canLoadMore,
 } = useCollection({ mediaType: AUDIO })
 
 // Collection params are not nullable in the collections route, this is enforced by the middleware
@@ -66,6 +67,7 @@ await useAsyncData(
       :collection-label="collectionLabel"
       :collection-params="collectionParams"
       :creator-url="creatorUrl"
+      :can-load-more="canLoadMore"
       @load-more="loadMore"
     />
   </div>

frontend/src/plugins/02.init-stores.server.ts renamed to frontend/src/plugins/02.init.server.ts

@@ -3,7 +3,6 @@ import { defineNuxtPlugin, useCookie } from "#imports"
 import type { OpenverseCookieState } from "#shared/types/cookies"
 import { useFeatureFlagStore } from "~/stores/feature-flag"
 import { useUiStore } from "~/stores/ui"
-import { useProviderStore } from "~/stores/provider"
 
 /**
  * Initialize the feature flag, ui and provider stores.
@@ -24,10 +23,6 @@ export default defineNuxtPlugin(async () => {
     useCookie<OpenverseCookieState["sessionFeatures"]>("sessionFeatures")
   featureFlagStore.initFromCookies(sessionFeatures.value ?? {})
 
-  /* Provider store */
-  const providerStore = useProviderStore()
-  await providerStore.fetchProviders()
-
   /* UI store */
   const uiStore = useUiStore()
   const uiCookies = useCookie<OpenverseCookieState["ui"]>("ui")

frontend/src/plugins/analytics.ts renamed to frontend/src/plugins/03.analytics.ts

@@ -0,0 +1,13 @@
+import { defineNuxtPlugin } from "#imports"
+
+import { useProviderStore } from "~/stores/provider"
+
+/**
+ * Initialize the provider store on SSR request.
+ * This plugin should run after the error and analytics plugins were set up.
+ */
+export default defineNuxtPlugin(async () => {
+  /* Provider store */
+  const providerStore = useProviderStore()
+  await providerStore.fetchProviders()
+})

frontend/src/plugins/errors.ts renamed to frontend/src/plugins/04.errors.ts

@@ -213,7 +213,6 @@ export const useMediaStore = defineStore("media", {
     },
 
     showLoading(): boolean {
-      console.log("show loading", this.fetchState.status, this.currentPage)
       return (
         this.fetchState.status === "idle" ||
         (this.fetchState.status === "fetching" && this.currentPage < 2)

frontend/src/plugins/05.init-stores.server.ts

@@ -103,7 +103,7 @@ export const useProviderStore = defineStore("provider", {
       mediaType: SupportedMediaType,
       providers: MediaProvider[]
     ) {
-      if (!providers.length) {
+      if (!Array.isArray(providers) || !providers.length) {
         return
       }
       this.providers[mediaType] = sortProviders(providers)
@@ -112,6 +112,7 @@ export const useProviderStore = defineStore("provider", {
 
     async fetchProviders() {
       this._updateFetchState("start")
+      const { $processFetchingError } = useNuxtApp()
       try {
         const res =
           await $fetch<Record<SupportedMediaType, MediaProvider[]>>(
@@ -125,7 +126,6 @@ export const useProviderStore = defineStore("provider", {
         }
         this._updateFetchState("end")
       } catch (error: unknown) {
-        const { $processFetchingError } = useNuxtApp()
         const errorData = $processFetchingError(error, ALL_MEDIA, "provider")
         this._updateFetchState("end", errorData)
       }

frontend/src/stores/media/index.ts

@@ -54,7 +54,7 @@ const config: PlaywrightTestConfig = {
     baseURL,
     trace: "retain-on-failure",
   },
-  timeout: 60 * 1e3,
+  timeout: 5 * 1e3, // 5 seconds in enough to see if the test is stuck
   /**
    * When updating or recreating tapes, if we have more than one worker running
    * then Talkback is liable to see multiple requests at the same time that would

frontend/src/stores/provider.ts

@@ -39,7 +39,7 @@ const themeOption = (colorMode: EffectiveColorMode, dir: LanguageDirection) =>
 
 export const turnOnDarkMode = async (page: Page, dir: LanguageDirection) => {
   const themeSwitcher = getThemeSwitcher(page, dir)
-  await themeSwitcher.selectOption(themeOption("dark", dir))
+  await themeSwitcher.selectOption({ label: themeOption("dark", dir) })
   await expect(themeSwitcher).toHaveValue("dark")
 }
 
@@ -78,6 +78,7 @@ export const expectSnapshot: ExpectSnapshot = async (
   screenshotOptions = {
     ...(screenshotOptions ?? {}),
     style: `#storybook-theme-switcher { visibility: hidden; }`,
+    animations: "disabled",
   }
 
   expect

frontend/test/playwright/playwright.config.ts

@@ -227,6 +227,7 @@ export const preparePageForTests = async (
       isFilterDismissed: dismissFilter ?? false,
       breakpoint,
       isDarkModeSeen: true,
+      colorMode: "system",
     },
   }
   if (options.features) {

frontend/test/playwright/utils/expect-snapshot.ts

@@ -2,6 +2,7 @@ import { test } from "~~/test/playwright/utils/test"
 import {
   pathWithDir,
   preparePageForTests,
+  scrollToTop,
 } from "~~/test/playwright/utils/navigation"
 import breakpoints from "~~/test/playwright/utils/breakpoints"
 import audio from "~~/test/playwright/utils/audio"
@@ -28,6 +29,7 @@ for (const dir of languageDirections) {
         .locator(".global-track")
         .getByRole("button", { name: t("playPause.pause", dir) })
         .click()
+      await scrollToTop(page)
       // To make the tests consistent, set the played area to the same position
       await page.mouse.click(170, 650)
 

frontend/test/playwright/utils/navigation.ts

@@ -2,7 +2,6 @@ import { expect } from "@playwright/test"
 import { test } from "~~/test/playwright/utils/test"
 import breakpoints from "~~/test/playwright/utils/breakpoints"
 import {
-  isPageDesktop,
   pathWithDir,
   preparePageForTests,
 } from "~~/test/playwright/utils/navigation"
@@ -12,20 +11,20 @@ import {
   getHomepageSearchButton,
   getLanguageSelect,
   getLoadMoreButton,
-  getMenuButton,
+  getThemeSwitcher,
 } from "~~/test/playwright/utils/components"
 
 test.describe.configure({ mode: "parallel" })
 
-const contentPages = [
-  "about",
-  "privacy",
-  "search-help",
-  // "non-existent", TODO: re-add dir properties to error page
-  "sources",
-  "sensitive-content",
-]
-for (const contentPage of contentPages) {
+const contentPages = {
+  about: "about",
+  privacy: "privacy",
+  "search-help": "searchGuide",
+  sources: "sources",
+  "sensitive-content": "sensitive",
+}
+
+for (const [contentPage, title] of Object.entries(contentPages)) {
   for (const dir of languageDirections) {
     test.describe(`${contentPage} ${dir} page snapshots`, () => {
       breakpoints.describeEvery(({ breakpoint, expectSnapshot }) => {
@@ -34,14 +33,8 @@ for (const contentPage of contentPages) {
 
           await page.goto(pathWithDir(contentPage, dir))
           // Ensure the page is hydrated
-          // eslint-disable-next-line playwright/no-conditional-in-test
-          if (!isPageDesktop(page)) {
-            // eslint-disable-next-line playwright/no-conditional-expect
-            await expect(getMenuButton(page, dir)).toBeEnabled()
-          }
-          await expect(page.locator("#language")).toHaveValue(
-            dir === "ltr" ? "en" : "ar"
-          )
+          await expect(getH1(page, t(`${title}.title`, dir))).toBeVisible()
+          await expect(getThemeSwitcher(page, dir)).toHaveValue("system")
 
           // Make sure header is not hovered on
           await page.mouse.move(150, 150)

frontend/test/playwright/visual-regression/components/global-audio-player.spec.ts

@@ -70,6 +70,9 @@ const tapeNameGenerator = (tapeNumber, tape) => {
 const updatingTapes =
   process.argv.includes("--update-tapes") || process.env.UPDATE_TAPES === "true"
 
+const verbose =
+  process.argv.includes("--verbose") || process.env.VERBOSE === "true"
+
 /** @type {TalkbackOptions['record']} */
 const recordMode = updatingTapes
   ? talkback.Options.RecordMode.NEW
@@ -190,8 +193,10 @@ const opts = /** @type {Partial<TalkbackOptions>} */ ({
   tapeNameGenerator,
   tapeDecorator,
   responseDecorator: (tape, req, context) => {
-    // Log responses to make debugging easier
-    console.log(req.method, req.url, tape.res?.status, context.id)
+    // If `verbose`, log responses to make debugging easier
+    if (verbose) {
+      console.log(req.method, req.url, tape.res?.status, context.id)
+    }
     return tape
   },
 })

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

@@ -24,7 +24,7 @@ const ignoredProblems = [
   /Sentry DSN wasn't provided/,
   // Errors from the Storybook nuxt module
   // TypeError or Va
-  /\[nuxt] error caught during app initialization \w*: Cannot read properties of undefined \(reading 'cdnURL'\)/,
+  /\[nuxt] error caught during app initialization \S*: Cannot read properties of undefined \(reading 'cdnURL'\)/,
   /Failed to load resource: the server responded with a status of 404 \(Not Found\)/,
   /\[Vue Router warn]: Record with path "\/iframe\.html" is either missing a "component\(s\)" or "children" property/,
 ]

frontend/test/proxy.js

@@ -12,7 +12,7 @@ export default defineConfig({
     baseURL: "http://localhost:54000",
     trace: "retain-on-failure",
   },
-  timeout: 60 * 1e3, // 1 minute
+  timeout: 5 * 1e3, // 5 seconds in enough to see if the test is stuck
   expect: {
     toMatchSnapshot: {
       // To avoid flaky tests, we allow a small amount of pixel difference.