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.

Sign up for GitHub

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

Jump to bottom

Fix #4002: Profiles cant have internationalised names #4081

Merged
merged 77 commits into from
Sep 9, 2022
Merged

Fix #4002: Profiles cant have internationalised names #4081

merged 77 commits into from
Sep 9, 2022

Conversation

JishnuGoyal
Copy link
Contributor

@JishnuGoyal JishnuGoyal commented Jan 6, 2022 •
edited by BenHenning
Loading

Explanation

Fixes #4002

This PR introduces a new injectable class into the codebase called ProfileNameValidator.

This class uses Kotlin functions to verify input, instead of Regex patterns which were used earlier.

In addition to what functionalities the regex patterns supported, this solution will now accept all International characters(English + non-English letters), and these characters: . , ', -

It will also reject the following:

Numbers (0, 1, 2, ... 9)
Symbols (!@#$%^&*... etc.)
Parentheses and Brackets ( (, [, { )

These built-in Kotlin methods function do not interfere with the RTL markers.

Hindi and Arabic profiles successfully created:

image

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

@JishnuGoyal
Copy link
Contributor Author

Note that this regex pattern will allow mixing of English and non English characters as some languages use of combination of those, for ex, Spanish, German, etc.

Kindly let me know if any further changes are required in the logic part @srushtirk

@JishnuGoyal
Copy link
Contributor Author

PTAL @BenHenning @bkaur-bkj Thanks!

BenHenning
BenHenning requested changes Jan 8, 2022
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @JishnuGoyal! Left some initial comments--PTAL.

domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt Outdated Show resolved Hide resolved
@oppiabot
Copy link

oppiabot bot commented Jan 8, 2022

Unassigning @BenHenning since the review is done.

@oppiabot
Copy link

oppiabot bot commented Jan 8, 2022

Hi @JishnuGoyal, it looks like some changes were requested on this pull request by @BenHenning. PTAL. Thanks!

@JishnuGoyal
Copy link
Contributor Author

Hello, @srushtirk , could you give some test cases/ examples of what names are acceptable and which ones are not acceptable?
What I want to make sure is that:
Although you have allowed ., - and ' in the name, can the name start/end with one of these symbols?
(For ex, is -jishnu.. allowed?)

This is one of the constraints I found, but I want to know if there are any more of such constraints that are needed to be taken care of.
PTAL @srushtirk Thanks!

@oppiabot oppiabot bot assigned srushtirk and unassigned JishnuGoyal Jan 10, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 10, 2022

Unassigning @JishnuGoyal since a re-review was requested. @JishnuGoyal, please make sure you have addressed all review comments. Thanks!

@srushtirk
Copy link

Hi @JishnuGoyal let's allow any order for the symbols . , ', -, English and non English characters. As it is difficult to predict where exactly in the user's name would these symbols be. This causes some edge cases where users can create profiles with random names starting or ending with these symbols, which is fine to trade off with.

We can put the following restrictions though:

  1. No two symbols next to each other. Eg: --, '-, .', ..
  2. Avoid spaces in profile names

@JishnuGoyal
Copy link
Contributor Author

PTAL @BenHenning , Thanks!

BenHenning
BenHenning requested changes Jan 19, 2022
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @JishnuGoyal! Added some more comments for ways I think this could be simplified, and made more future-proof.

domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned JishnuGoyal and unassigned BenHenning Jan 19, 2022
@oppiabot
Copy link

oppiabot bot commented Aug 4, 2022

Hi @JishnuGoyal, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 4, 2022
@oppiabot oppiabot bot closed this Aug 11, 2022
@BenHenning
Copy link
Member

I'm now taking a look at this (since it's functionality that we'd like to include in the upcoming beta release).

@BenHenning BenHenning reopened this Sep 8, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 8, 2022
This was referenced Sep 8, 2022
Merged
Merged
@BenHenning
Merge pull request #4 from JishnuGoyal/profiles-cant-have-internation…
e8f5d6c 
…alised-names

Profiles cant have internationalised names
@BenHenning BenHenning mentioned this pull request Sep 8, 2022
Merged
@BenHenning
Fix broken tests.
bf0282b
@BenHenning
Merge branch 'develop' into fix-tests-for-internationalized-profile-n…
e4f744d 
…ame-support

Conflicts:
	domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt
	scripts/assets/file_content_validation_checks.textproto
@BenHenning BenHenning assigned JishnuGoyal and unassigned BenHenning Sep 8, 2022
@BenHenning
Copy link
Member

BenHenning commented Sep 8, 2022 •
edited
Loading

Created JishnuGoyal#1 to address the conflicts & broken tests. PTAL @JishnuGoyal.

BenHenning
BenHenning approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Latest changes look good and CI is now passing. There are two comments that still need resolution before this can be fixed, though I think I'm going to go ahead and merge this and then address them in a separate PR (to avoid needing another day of iteration).

domain/src/test/java/org/oppia/android/domain/profile/ProfileManagementControllerTest.kt Outdated Show resolved Hide resolved
utility/src/test/java/org/oppia/android/util/profile/BUILD.bazel Outdated Show resolved Hide resolved
utility/src/test/java/org/oppia/android/util/profile/ProfileNameValidatorTest.kt Show resolved Hide resolved
@oppiabot
Copy link

oppiabot bot commented Sep 9, 2022

Assigning @rt4914, @vinitamurthi, @anandwana001 for code owner reviews. Thanks!

This was referenced Sep 9, 2022
Merged
Open
@BenHenning
Copy link
Member

Since this PR is part of broader urgent work for the Beta MR1 release, I'm force-merging this without @rt4914's review. I've self-reviewed it and found no major issues. Furthermore, #4567 is tracking ensuring that this does get reviewed by someone else later after it's been merged.

@BenHenning BenHenning merged commit f9e2f2c into oppia:develop Sep 9, 2022
BenHenning added a commit that referenced this pull request Sep 9, 2022
@BenHenning
Address comments from #4081.
f3a78d1
BenHenning added a commit that referenced this pull request Sep 9, 2022
@BenHenning
Fix #4348: Prepare for Beta MR1 by updating version codes & pulling i…
3f93526 
…n the latest pt-BR translations (#4565)

## Explanation
Fixes #4348

This PR performs the final preparation tasks necessary for the MR1 Beta release to be cut. Specifically:
- It updates the version codes & minor version.
- It pulls the latest pt-BR translations (for the ones that didn't make it in the automatic Thursday Translatewiki push).
- It addresses the final open comments from #4081 which included me needing to do the pt-BR translation for 2 strings related to profile names (I used Google Translate then made minor modifications).

There are no tests affected directly by these changes (though we could consider adding some in the future).

## Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
- [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation.
- [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
For the most part this PR is not UI-tied (though obviously the 1 English string change and all of the pt-BR string changes affect UIs). I'm deciding not to include screenshots for string-only changes since they aren't very interesting in this case (i.e. they don't seem like they would actually break the UI to warrant including screenshots).

Commits:

* Create dedicated alpha application component.

This simplifies application component management significantly and
allows individual build flavors to have their own unique module lists.

* Add beta & GA update notices.

This also introduces dedicated beta & GA build flavors which is a
necessary prerequisite.

It also introduces an extra beta, alpha, and dev mode labels for the splash
screen (the latter 2 were extra) with 2 second minimum wait timers for
beta and alpha to ensure they are seen. A 5-second safety timer was
added to ensure the splash screen can always be passed even if something
goes wrong at the domain level (since there are now quite a few moving
pieces to determine the user's current onboarding state).

* Add build tests for the new beta & GA flavors.

* Fix broken test per earlier changes.

* Fix general broken tests & builds.

Tests broken due to changes to the app startup experience haven't yet
been fixed.

* Lint fixes.

* First part of adding tests for GA notices.

There's a bunch left to do here, this is mainly needed so that I can
transfer changes to a different machine.

* Update TransformAndroidManifestTest.kt

Correct typos.

* Fix tests & static checks.

This also removes temporary debug code and TODOs, and finishes the tests
for SplashActivity.

* Post-merge fixes.

* Test fixes.

* Fix Gradle test.

* Add some string resource checks/tools.

Also, fixes major performance issue with all file-based CI checks.

* Ensure newline consistency in translated strings.

Also, fix reporting in new validation check script.

* Add tests & fix static checks.

* Update version codes & pt-BR strings.

The strings were manually pulled Translatewiki.

* Follow-up adjustments after self-review.

* Add latest translated pt-BR strings.

* Address comments from #4081.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning approved these changes

@seanlip seanlip seanlip left review comments

@anandwana001 anandwana001 Awaiting requested review from anandwana001

@rt4914 rt4914 Awaiting requested review from rt4914

@vinitamurthi vinitamurthi Awaiting requested review from vinitamurthi

Assignees

@BenHenning BenHenning

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Profiles can't have internationalized names
6 participants
@JishnuGoyal @srushtirk @BenHenning @seanlip @rt4914 @vinitamurthi