Skip to content

Conversation

@AliSoftware
Copy link
Contributor

The value parameter of two NSLocalizedString calls (that were added in #19563) was not a string literal, which made those entries not being extracted properly by genstrings/generate_strings_file_for_glotpress into the Localizable.strings during code freeze, and thus led to the original / English copy exported to GlotPress to be the same as the semantic key, instead of being the expected English copy.

image

This was reported to us by a polyglot in the Making WordPress Slack here.

To test:

  • Ensure that the screen that uses that string still shows the proper copy to the end user in the UI.

Regression Notes

  1. Potential unintended areas of impact:

None

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

I haven't launched the app to test that those changes don't affect the copy presented to the end user (also because I didn't know how to reproduce the right context to navigate to that migration screen in the app and test it).
Feel free to test it yourself if you feel it's worth it — though the code diff is pretty straightforward I think.

  1. What automated tests I added (or what prevented me from doing so)

N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Because the `value` parameter of those two `NSLocalizedString` entries was not a literal, it wasn't extracted properly by `genstrings` into the `Localizable.strings` file, and led to the original / English copy exported to GlotPress to be the same as the semantic key instead of being the value.
@AliSoftware AliSoftware added Localization Tooling Build, Release, and Validation Tools Jetpack Focus labels Nov 16, 2022
@AliSoftware AliSoftware added this to the 21.2 ❄️ milestone Nov 16, 2022
@AliSoftware AliSoftware requested review from a team, Gio2018 and mokagio November 16, 2022 12:23
@AliSoftware AliSoftware self-assigned this Nov 16, 2022
@AliSoftware
Copy link
Contributor Author

AliSoftware commented Nov 16, 2022

@mokagio I'm guessing your PR #19553 would have helped catch this one too if it had landed in time? 😅

Though, when I tried to run fastlane generate_strings_file_for_glotpress on the releease/21.2 branch (so without this fix), I didn't see any warning appear, so I'm 🤔 😕 …


[EDIT] As you can see below, even running it manually, I don't see any warning… even while the NSLocalizedString(…, value:, …) was not using a string literal (in the code in release/21.2 at time of testing this) and led to that issue of migration.welcome.secondaryDescription.plural not containing the expected copy. So that means that even with #19553 we probably wouldn't have had a warning about that use case?

 13:39:30  📂 ~/Documents/Dev/apps/WordPress-iOS/  (release/21.2) 
✓ $ genstrings -o WordPress/Resources/en.lproj -s AppLocalizedString "WordPress/Jetpack/Classes/ViewRelated/WordPress-to-Jetpack Migration/Common/Views/Configuration/MigrationHeaderConfiguration.swift" 
                                                                                                                                                                                                                  
 13:39:38  📂 ~/Documents/Dev/apps/WordPress-iOS/  (release/21.2) 
✓ $ plutil -p WordPress/Resources/en.lproj/Localizable.strings                                                                                                                                           
{
  "migration.done.primaryDescription" => "We’ve transferred all your data and settings. Everything is right where you left it."
  "migration.done.title" => "Thanks for switching to Jetpack!"
  "migration.notifications.primaryDescription" => "You’ll get all the same notifications but now they’ll come from the Jetpack app."
  "migration.notifications.secondaryDescription" => "We’ve disabled notifications for the WordPress app."
  "migration.notifications.title" => "Allow notifications to keep up with your site"
  "migration.welcome.primaryDescription" => "It looks like you’re switching from the WordPress app."
  "migration.welcome.secondaryDescription.plural" => "migration.welcome.secondaryDescription.plural"
  "migration.welcome.secondaryDescription.singular" => "migration.welcome.secondaryDescription.singular"
  "migration.welcome.title" => "Welcome to Jetpack!"
}

@wpmobilebot
Copy link
Contributor

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19606-9249225 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19606-9249225 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Thanks @AliSoftware !

@mokagio mokagio merged commit ed3a360 into release/21.2 Nov 16, 2022
@mokagio mokagio deleted the fix-strings-non-literal-value-export branch November 16, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Localization Tooling Build, Release, and Validation Tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants