Skip to content
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

Studio: Translate Studio Assistant Welcome messages #483

Merged

Conversation

katinthehatsite
Copy link
Contributor

@katinthehatsite katinthehatsite commented Aug 20, 2024

Closes https://github.com/Automattic/dotcom-forge/issues/8279

Proposed Changes

This PR ensures that the welcome messages for which we have translated the strings in GlotPress are translated in Studio Assistant:

Zrzut ekranu 2024-08-20 o 10 41 39 AM

Testing Instructions

  • Apply the following diff to your sandbox: D158963-code
  • Sandbox the public API
  • Pull the changes from this branch
  • Switch the language to one of the supported languages e.g. Polish would be a good example as it has all the strings for welcome messages translated
  • Start the app with npm start
  • Navigate to the Assistant tab
  • Confirm that the welcome messages are translated

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

apiNamespace: 'wpcom/v2',
params: { locale },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with passing the parameter like this is that the request URL gets formatted like so:

wpcom/v2/studio-app/ai-assistant/welcome?_locale=pl with underscore which breaks the request to the API and the translation does not come through

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not adjust the code responsible for that in the custom request() function?

const localeParamName =

Copy link
Contributor Author

@katinthehatsite katinthehatsite Aug 21, 2024

Choose a reason for hiding this comment

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

Thanks for the review @wojtekn ! Interesting, looking at this setting, I see that when you switch to a language other than English, it appends the locale parameter to all requests going to different endpoints. If I remove the locale parameter from the /welcome endpoint, I see it also gets appended:

Captura de pantalla 2024-08-21 a las 9 33 49 a  m

Looking at the original PR where this was introduced - https://github.com/Automattic/local-environment/pull/91 - this seems to have been done so that the locale is set for the duration of the API call.

In regards to this, I have the following question:

  • Why do we need to set a specific locale for all of these API calls? From my understanding, we are not returning anything locale-specific in any of these endpoints beside the /welcome endpoint and even for the /welcome endpoint it does not seem to work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to set a specific locale for all of these API calls? From my understanding, we are not returning anything locale-specific in any of these endpoints beside the /welcome endpoint and even for the /welcome endpoint it does not seem to work?

I would think it's to let API endpoints return localized error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, that makes sense. So if I am testing this correctly, I am seeing the following:

  • When I pass locale into /welcome endpoint, I am getting the translations of the messages but I am not getting localized error messages
  • When I pass _locale, I am getting only the localized error messages and not the translated messages
  • The only way I found to make this work for both is to pass both locale and _locale - example of the URL:
Captura de pantalla 2024-08-21 a las 9 53 00 a  m

I am seeing two options for this case:

  • either we pass both as mentioned above if we want both error messages and the welcome messages
  • OR we pass only locale for this specific endpoint since we don't really display the localized error messages that come from the endpoint for the welcome messages.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: after Slack discussions, we will proceed with the fix of the API endpoint for the welcome messages. I will update here once it is done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wojtekn I prepared a fix here: D158963-code and it needs to be tested in combination with this PR (the testing steps in the PR are updated).

Let me know what you think!

@wojtekn
Copy link
Contributor

wojtekn commented Aug 21, 2024

Works as expected.

@katinthehatsite katinthehatsite merged commit 6e84021 into trunk Aug 21, 2024
12 checks passed
@katinthehatsite katinthehatsite deleted the fix/translation-of-welcome-messages-for-assistant branch August 21, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants