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

Spanish translation #1130

Merged
merged 21 commits into from
Jan 20, 2023
Merged

Spanish translation #1130

merged 21 commits into from
Jan 20, 2023

Conversation

emlys
Copy link
Member

@emlys emlys commented Dec 5, 2022

Description

This PR introduces our first translation of InVEST into Spanish:

expected tasks

  • Add the Spanish message catalogs
  • Remove the logic we used to hide the workbench language setting in production mode
  • Uncomment the code that builds message catalogs as part of the setup.py build process

bug fixes

  • Refresh the app when the language setting changes to ensure that all content displays in the new language. We decided that users will have to refresh the app for the language setting to take effect. It was more complicated than it's worth to make sure that every affected component re-renders in a new language. In a future PR, I'll add a feature to prompt the user to choose a language when first opening the app.
  • Fix a couple of module reloads in ui_server.py
  • I'm not sure exactly why, but the small change to module reloading in ui_server.py triggered a weird bug where the test suite would hang indefinitely, on unrelated tests, and only when running most of the tests all together (could not reproduce with an individual test suite). It was fixed by moving the unit registry definition to its own module so that u isn't redefined when we reload spec_utils.
  • While debugging that, I noticed that 3 test suites (CBC, CV, and translation) imported the module under test at the top level. That meant that any import errors would crash the whole pytest command, rather than record as a test failure. I moved those imports into each individual test like we do for other models.

small enhancement

We now get the list of supported languages and their localized names from the backend, rather than defining them in the workbench.

notes

I tried to test that the page reloads when the language is changed, following this example, but it didn't work and I didn't think it was that important.

There is also a PR in the works for the user's guide translation, but I don't think those need to be linked to this PR.

Checklist

  • Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
  • Updated the user's guide (if needed)
  • Tested the affected models' UIs (if relevant)

@emlys emlys self-assigned this Dec 8, 2022
@emlys emlys marked this pull request as ready for review December 15, 2022 22:52
@emlys emlys changed the base branch from main to feature/spanish December 16, 2022 16:45
Copy link
Member

@phargogh phargogh left a comment

Choose a reason for hiding this comment

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

👍 🎉

@@ -5,3 +5,4 @@ prune ci
prune exe
prune installer
prune workbench
include src/natcap/invest/internationalization/locales/*/LC_MESSAGES/messages.mo
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know we could include globs here! Very cool.

Comment on lines 11 to 12
#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:192
msgid ""
"Use user-defined local recharge data instead of calculating local "
"recharge from the other provided data."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:195
msgid "user-defined recharge layer (advanced)"
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:205
msgid ""
"Map of local recharge data. Required if User-Defined Local Recharge is "
"selected."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:208
msgid "local recharge"
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:212
msgid "Use user-defined climate zone data in lieu of a global rain events table."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:215
msgid "climate zones (advanced)"
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:222
msgid ""
"Climate zone ID numbers, corresponding to the values in the Climate Zones"
" map."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:228
msgid ""
"The number of rain events that occur in each month in this climate zone. "
"Replace [MONTH] with the month abbreviations: jan, feb, mar, apr, may, "
"jun, jul, aug, sep, oct, nov, dec, so that there is a column for each "
"month."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:236
msgid ""
"Table of monthly precipitation events for each climate zone. Required if "
"User-Defined Climate Zones is selected."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:239
msgid "climate zone table"
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:246
msgid ""
"Map of climate zones. All values in this raster must have corresponding "
"entries in the Climate Zone Table."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:249
msgid "climate zone map"
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:253
msgid "Use montly alpha values instead of a single value for the whole year."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:256
msgid "use monthly alpha table (advanced)"
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:264
msgid "Values are the numbers 1-12 corresponding to each month."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:271
msgid "The alpha value for that month."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:275
msgid ""
"Table of alpha values for each month. Required if Use Monthly Alpha Table"
" is selected."
msgstr ""

#: src/natcap/invest/seasonal_water_yield/seasonal_water_yield.py:278
msgid "monthly alpha table"
msgstr ""

# Spanish translations for InVEST.
# Copyright (C) 2022 Natural Capital Project
# This file is distributed under the same license as the InVEST project.
# FIRST AUTHOR <EMAIL@ADDRESS>, 2022.
#
msgid ""
msgstr ""
"Project-Id-Version: InVEST 3.12\n"
"Report-Msgid-Bugs-To: [email protected]\n"
"POT-Creation-Date: 2022-10-21 13:17-0700\n"
"PO-Revision-Date: 2022-11-19 17:42-0500\n"
"Last-Translator: \n"
Copy link
Member

Choose a reason for hiding this comment

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

Should the Last-Translator be Patricio's email?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I don't think any of these fields are required but I'll fill in his email

msgid ""
msgstr ""
"Project-Id-Version: InVEST 3.12\n"
"Report-Msgid-Bugs-To: [email protected]\n"
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall why my email is here ... @emlys would you like this to be your email? Or maybe we should have it email the software team email list, or else an alias at naturalcapitalproject.org?

Copy link
Contributor

@davemfish davemfish left a comment

Choose a reason for hiding this comment

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

Thanks @emlys , this is awesome. I reviewed the whole thing.

One thing I noticed when testing it out is that the Workbench strings are not translating, and I'm not sure how that is supposed to work exactly. And strangely, there is a test that tests for that, which somehow passes.

Other than that, I just a few minor comments.

src/natcap/invest/ui_server.py Show resolved Hide resolved
ipcRenderer.invoke(
ipcMainChannels.SET_LANGUAGE, settings.language
).then(() => saveSettingsStore(settings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Functionally it's the same, but for consistency across the codebase, how do you feel about using async/await here instead of chaining the .then calls?

I realize we're not 100% consistent on this -- we tend to use .then/.catch for fetch calls such as in server_requests.js (for no particular reason), but I'd prefer to migrate towards async/await.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

src/natcap/invest/ui_server.py Show resolved Hide resolved
@@ -876,6 +873,7 @@ describe('Translation', () => {

beforeAll(async () => {
getInvestModelNames.mockResolvedValue({});
getSupportedLanguages.mockResolvedValue({ en: 'english', es: 'spanish' });
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a comment about the test in this block, which didn't actually have any changes...

I think it would be best to mock window.location.reload() also. jsdom has a global window, but I see a console error saying Error: Not implemented: navigation (except hash changes) on the reload call.

Also in that test, it looks like we test that some workbench strings were translated (as opposed to invest strings) and those checks pass despite the browser refresh not happening, which I think makes sense? But if I use the workbench and change the language, I don't see those workbench strings translated. So I'm not exactly sure what's going on there.

Copy link
Member Author

@emlys emlys Jan 5, 2023

Choose a reason for hiding this comment

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

If you're not on the latest commit, you might also be missing the message catalog for the workbench strings, hence it's defaulting to English. The translation test mocks the message catalog, so I'd expect it to pass either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

@emlys I've confirmed that I have the latest changes, including the workbench message catalog and a fresh install, but I still can't get the workbench strings to translate. I also tried building a production version and don't get the translated strings there either. I confirmed this function is called with the correct & complete messageCatalogPath, so I'm not sure how to debug further.

function loadMessageCatalogs() {
  // load each language's message catalog PO file into an object
  // for easy access when we switch languages
  const languages = fs.readdirSync(`${__dirname}/../static/internationalization/locales`);
  if (languages) {
    languages.forEach((language) => {
      const messageCatalogPath = `${__dirname}/../static/internationalization/locales/${language}/LC_MESSAGES/messages.po`;
      console.log(messageCatalogPath)
      i18n.loadJSON(readMessageCatalog(messageCatalogPath), 'messages');
    });
  }
  logger.debug('loaded message catalogs');
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be fixed now - the language code in the PO file was wrong, resulting in the messages not being used.

@davemfish
Copy link
Contributor

I tried to test that the page reloads when the language is changed, following this example, but it didn't work and I didn't think it was that important.

I agree it's not important to test that. Though I think that basic approach would work if window.location.reload() is a mock function, which I suggested for a different reason in another comment.

@emlys
Copy link
Member Author

emlys commented Jan 6, 2023

Though I think that basic approach would work if window.location.reload() is a mock function

I've spent a few hours on this now and can't get it to work. The mock doesn't work, and the test doesn't pass. If it's important to mock this maybe we can debug it together, otherwise, I don't think it's worth the effort to test.

@emlys emlys requested a review from davemfish January 6, 2023 21:19
@davemfish
Copy link
Contributor

davemfish commented Jan 9, 2023

I've spent a few hours on this now and can't get it to work. The mock doesn't work, and the test doesn't pass.

@emlys I've been there.

If it's important to mock this maybe we can debug it together, otherwise, I don't think it's worth the effort to test.

I'd be happy to work through it together. I don't think it's important to test whether window.location.reload is called, but right now it is being called during the tests and it's raising a console.error because it's not implemented in jsdom. So I think it's worth mocking just to clean up our jest output. And from there we can decide whether to go further and actually assert anything in the test.

Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

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

Thanks @emlys, this looks awesome! And thanks Dave for giving this a thorough look and helping debug!

@dcdenu4
Copy link
Member

dcdenu4 commented Jan 10, 2023

Hey @emlys, I just loaded up the Workbench and tried switching to Spanish. I just had a few quick questions and sorry if they've already been addressed.

  • I noticed a capitalization difference in the options, "English" v "espanol". Should that be consistent?
  • Are we foregoing trying to translate the File | Edit | View | Window | About menus?
  • I noticed for a models input helper text (raster, csv, directory, number, ...) that some are translated and some are not. I was curious how that is determined. For instance, I see both tasa: un decimal de 0 - 1 and number. Just curious, thanks!

So cool to see it in action!

@emlys
Copy link
Member Author

emlys commented Jan 19, 2023

@davemfish I think I finally fixed the window reload mocking issue. I had to delete the object first, and also mock it in another test where the language setting changes.

@davemfish
Copy link
Contributor

@davemfish I think I finally fixed the window reload mocking issue. I had to delete the object first, and also mock it in another test where the language setting changes.

@emlys Thanks for figuring that out! I guess window.location is kind of special in jsdom jestjs/jest#5124

@davemfish davemfish merged commit 2745b5f into natcap:feature/spanish Jan 20, 2023
@emlys emlys deleted the task/spanish branch March 8, 2023 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants