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

Fix: Cannot choose reader topics in some languages #16789

Merged
merged 4 commits into from
Jun 23, 2022

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Jun 22, 2022

Fixes #16237

This PR fixes a bug where a valid URL encoded tag with % symbols was sanitized to remove % symbols making it an invalid URL encoded tag and causing issues in following the tag.

See #16237 (comment) for more details.

/cc @thehenrybyrd

To test:

Test.1 - Follow/ unfollow a topic in a language like Chinese (Taiwan)

  1. Head to Reader > ⚙️ > Followed Topics and unfollow any topics you currently follow. (Alternatively, login to an account with no followed topics)
  2. Head to My Site > Me > App Settings and change Interface Language to a language reported in the linked issue such as Chinese (Taiwan).
  3. Back in Reader, choose the Discover tab (second tab from the left), and tap the "Get Started" button (the blue button in the middle of the tab when no topics are followed).
  4. Select a few topics and tap the button at the bottom.
  5. Note that the topics are followed.
  6. Unfollow a topic.
  7. Note that the topic is unfollowed.
device-2022-06-22-111733.mp4

Test.2 - Add a topic with invalid characters

  1. On the Reader tab, tap on the Setting menu (⚙️ icon) on the top to open "Manage Topics & Sites" screen.
  2. Add a tag with invalid characters on "Manage Topics & Sites" screen.
  3. Notice a toast is shown that the tag is invalid.
device-2022-06-22-112316.mp4

Test.3 - Add a topic with spaces

  1. On the Reader tab, tap on the Setting menu (⚙️ icon) on the top to open "Manage Topics & Sites" screen.
  2. Add a tag with spaces on "Manage Topics & Sites" screen.
  3. Notice that the tag is sanitized with dashes and followed.
device-2022-06-22-113057.mp4

Regression Notes

  1. Potential unintended areas of impact
    Strings that are not URL encoded should be sanitized with dashes.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Tested manually. See "To test" section.

  3. What automated tests I added (or what prevented me from doing so)
    Added string sanitization tests in ReaderUtilsTest.

PR submission checklist:

  • I have completed the Regression Notes.
  • 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 added 3 commits June 22, 2022 10:51
This fixes a bug where a valid URL encoded tag with % symbols was sanitized to remove % symbols making it an invalid URL encoded tag causing issues in following it.
@ashiagr ashiagr added the Reader label Jun 22, 2022
@ashiagr ashiagr added this to the 20.2 milestone Jun 22, 2022
@ashiagr ashiagr requested a review from a team June 22, 2022 06:35
@ashiagr ashiagr self-assigned this Jun 22, 2022
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 22, 2022

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

@wpmobilebot
Copy link
Contributor

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr16789-2a2eac3.apk), or scanning this QR code:

@wpmobilebot
Copy link
Contributor

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr16789-2a2eac3.apk), or scanning this QR code:

@ashiagr ashiagr requested a review from zwarm June 23, 2022 07:13
@zwarm zwarm self-assigned this Jun 23, 2022
Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

@ashiagr - Nice use of URI 👍 Works as expected in Jetpack & WordPress.

@zwarm zwarm merged commit 2c71867 into trunk Jun 23, 2022
@zwarm zwarm deleted the 16237/fix_reader_topics_not_selected_issue_il8n branch June 23, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot choose reader topics in some languages
3 participants