Skip to content

Issue/13326 pluralisation of strings based on threats count#14021

Merged
ashiagr merged 11 commits intodevelopfrom
issue/13326-pluralisation-of-strings
Feb 11, 2021
Merged

Issue/13326 pluralisation of strings based on threats count#14021
ashiagr merged 11 commits intodevelopfrom
issue/13326-pluralisation-of-strings

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Feb 10, 2021 •
edited
Loading

Parent #13326

This PR

  1. Handles pluralisation of strings based on threats count:
Key Message Commit
threat_fix_all_warning_message Please confirm you want to fix %s active threats. aa0e8db
threat_fix_all_status_success_message Threats were successfully fixed. 4c12e1d
scan_fixing_threats_description We're hard at work in the background fixing these threats. In the meantime feel free to continue to use your site as normal, you can check the progress at anytime. f21b4ef
scan_fixing_threats_title Fixing Threats 34058f1
scan_idle_with_threats_description The scan found %1s potential threats with %2s. Please review them below and take action or tap the fix all button. We are %3$s if you need us. a3500cf
threats_found Threats found cb1a523

Does not make any changes to below messages:

Key Message Reason
threat_fix_all_warning_title Fix all threats Corresponds to Fix All button, no singular string found in Calypso
threat_fix_all_error_message Error fixing threats. Please contact our support. No singular string found in Calypso
  1. Updates messages based on editorial review:
Key Message Commit
scan_fixing_threats_description (Issue #14008) We're hard at work fixing these threats in the background. In the meantime feel free to continue to use your site as normal, you can check the progress at anytime. a52d9aa
threat_ignore_warning (p1612896373162300-slack-C0180B5PRJ4) You shouldn\’t ignore a security unless you are absolute sure it\’s harmless. If you choose to ignore this threat, it will remain on your site <b>%s</b>. cab95b2

Notes

  1. Due to translation issues with using quantity plurals, I just duplicated messages in the strings.xml. Let me know if you have a better suggestion.

  2. Should we create separate tests for singular/ plural message resources? I started adding here but wasn't sure if it's that important.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@ashiagr ashiagr added this to the 16.8 milestone Feb 10, 2021
@ashiagr ashiagr requested a review from malinajirka February 10, 2021 06:50
@ashiagr ashiagr self-assigned this Feb 10, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 10, 2021 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@ashiagr ashiagr mentioned this pull request Feb 10, 2021
Closed
67 tasks
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 10, 2021 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

@ashiagr ashiagr force-pushed the issue/13326-pluralisation-of-strings branch from b3dc786 to 5154eae Compare February 10, 2021 08:54
@malinajirka
Copy link
Copy Markdown
Contributor

malinajirka commented Feb 10, 2021

Thanks for working on this ;)! I haven't done a full review yet, but it looks great.

Due to translation issues with using quantity plurals, I just duplicated messages in the strings.xml. Let me know if you have a better suggestion.

I can't think of a better approach. One thing we might want to consider is using "_singular/_plural" suffixes instead of using eg. "threat vs threats". I noticed that we use these suffixes in some other strings. If the approach you used is used somewhere as well, feel free to ignore this comment.

Should we create separate tests for singular/ plural message resources? I started adding here but wasn't sure if it's that important.

I don't think that's necessary. Unless it takes just a few minutes, I wouldn't write them.

@ashiagr ashiagr marked this pull request as ready for review February 11, 2021 06:02
@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Feb 11, 2021

Thanks @malinajirka!

One thing we might want to consider is using "_singular/_plural" suffixes instead of using eg. "threat vs threats".

Done in 866f9bb.

Marked it ready for review now.

malinajirka
malinajirka approved these changes Feb 11, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM ;)! Feel free to merge it when the conflict is resolved.

@ashiagr
Merge branch 'develop' into issue/13326-pluralisation-of-strings
c55ffaa 
# Conflicts:
#	WordPress/src/test/java/org/wordpress/android/ui/jetpack/scan/builders/ScanStateListItemsBuilderTest.kt
@ashiagr
Fix merge
e16f940
@ashiagr ashiagr merged commit d257199 into develop Feb 11, 2021
@ashiagr ashiagr deleted the issue/13326-pluralisation-of-strings branch February 11, 2021 16:45
@JavonDavis JavonDavis mentioned this pull request Mar 3, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malinajirka malinajirka malinajirka approved these changes

Assignees

@ashiagr ashiagr

Labels

None yet

Projects

None yet

Milestone

16.8

Development

Successfully merging this pull request may close these issues.

2 participants

@ashiagr @malinajirka