Skip to content

Fix i18n placeholder replacement and make tests robust #5343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/ci_cd.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 1 addition & 2 deletions documentation/frontend/reference/i18n.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions frontend/Dockerfile.playwright
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ WORKDIR /frontend

COPY package.json .

# Copy the pnpm-lock.yaml from the root of the monorepo
COPY ../pnpm-lock.yaml ./pnpm-lock.yaml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this copy pnpm-lock.yaml file from the parent directory when the ci_cd.yml file has copied it into frontend/? Also how is this Dockerfile able to access files from one level up? As per my understanding, it would not be part of the Docker context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to copy from frontend directory to docker.

# Ensure the Playwright container's pnpm cache folder exists and is writable
RUN mkdir -p /.cache/node/corepack/ && chmod -R 777 /.cache/node/corepack/
# Ensure that the dlx cache folder exists and is writable
Expand Down
4 changes: 2 additions & 2 deletions frontend/i18n/data/en.json5
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down Expand Up @@ -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",
Expand Down
4 changes: 2 additions & 2 deletions frontend/i18n/scripts/po/po-helpers.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand All @@ -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>###`)
}

Expand Down
7 changes: 6 additions & 1 deletion frontend/i18n/scripts/translations.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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}")

Expand Down
6 changes: 6 additions & 0 deletions frontend/i18n/scripts/utils.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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" } },
}
Expand All @@ -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",
},
}

Expand All @@ -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,
}
Expand All @@ -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,
})
)
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/components/VThemeSelect/VThemeSelect.vue
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const updateRefs = () => {
}

onMounted(updateRefs)
watch(() => [darkMode.colorMode.value, darkMode.osColorMode.value], updateRefs)
watch([darkMode.colorMode, darkMode.osColorMode], () => updateRefs())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is () => updateRefs() different from updateRefs? Does it fix a problem or is it a stylistic/conventional thing?

Copy link
Contributor Author

@obulat obulat Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain. I added this change trying to fix the cases where Playwright would select an option, but the selected option was still the same on the next line. I think the previous way has one more layer of indirection than what is here, and maybe the change happens in a different tick of the event loop so that Playwright sees the not-yet-changed version?

Update: sorry, all of the above refers to changing from ()=>[computed.value] to [computed]

With the function, the change should be reverted. I added it to add logs around it

</script>

<template>
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/composables/use-analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/pages/audio/collection.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -66,6 +67,7 @@ await useAsyncData(
:collection-label="collectionLabel"
:collection-params="collectionParams"
:creator-url="creatorUrl"
:can-load-more="canLoadMore"
@load-more="loadMore"
/>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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")
Expand Down
File renamed without changes.
13 changes: 13 additions & 0 deletions frontend/src/plugins/05.init-stores.server.ts
Original file line number Diff line number Diff line change
@@ -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()
})
1 change: 0 additions & 1 deletion frontend/src/stores/media/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/stores/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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[]>>(
Expand All @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion frontend/test/playwright/playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion frontend/test/playwright/utils/expect-snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down Expand Up @@ -78,6 +78,7 @@ export const expectSnapshot: ExpectSnapshot = async (
screenshotOptions = {
...(screenshotOptions ?? {}),
style: `#storybook-theme-switcher { visibility: hidden; }`,
animations: "disabled",
}

expect
Expand Down
1 change: 1 addition & 0 deletions frontend/test/playwright/utils/navigation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ export const preparePageForTests = async (
isFilterDismissed: dismissFilter ?? false,
breakpoint,
isDarkModeSeen: true,
colorMode: "system",
},
}
if (options.features) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)

Expand Down
31 changes: 12 additions & 19 deletions frontend/test/playwright/visual-regression/pages/pages.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 }) => {
Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions frontend/test/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,9 @@ const opts = /** @type {Partial<TalkbackOptions>} */ ({
summary: false,
tapeNameGenerator,
tapeDecorator,
responseDecorator: (tape, req, context) => {
responseDecorator: (tape) => {
// Log responses to make debugging easier
console.log(req.method, req.url, tape.res?.status, context.id)
// console.log(req.method, req.url, tape.res?.status, context.id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why not remove the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to only log if verbose is set to true. This could be very useful for debugging CI failures.

return tape
},
})
Expand Down
2 changes: 1 addition & 1 deletion frontend/test/storybook/functional/smoke-test.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/,
]
Expand Down
2 changes: 1 addition & 1 deletion frontend/test/storybook/playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading