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

FTUE - Msisdn (phone number) entry #6108

Merged
merged 7 commits into from
Jul 1, 2022
Merged

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented May 20, 2022

Type of change

  • WIP Feature
  • Bugfix
  • Technical
  • Other :

Content

Partly addresses #6043 - the confirmation screen is currently being designed and will be raised in a separate PR

Adds an updated styling version of the Phone number (Msisdn) entry

  • Extracts the phone number parsing to its own class with tests
  • Moves the msisdn 401 as success logic to the wizard delegate with test (matching the email flow)

Motivation and context

To update the phone number entry flow

Screenshots / GIFs

Screenshot_20220520_140735 Screenshot_20220520_140744 Screenshot_20220520_140812 Screenshot_20220520_140756

Tests

  • Setup a local synase with ThreePid msisdn's enabled
  • Create an account
  • See phone number entry screen

Tested devices

  • Physical
  • Emulator
  • OS version(s): 31

@github-actions
Copy link

github-actions bot commented May 20, 2022

Unit Test Results

130 files  130 suites   2m 27s ⏱️
221 tests 221 ✔️ 0 💤 0
754 runs  754 ✔️ 0 💤 0

Results for commit 8729b7f.

♻️ This comment has been updated with latest results.

@ouchadam ouchadam force-pushed the feature/adm/ftue-register-test-cases branch from 0a13e9a to 171056e Compare May 24, 2022 15:29
@ouchadam ouchadam force-pushed the feature/adm/ftue-register-test-cases branch from 171056e to 680b9a0 Compare June 1, 2022 09:37
@ouchadam ouchadam force-pushed the feature/adm/ftue-register-test-cases branch 2 times, most recently from 6d281ff to befcfe8 Compare June 9, 2022 07:52
@ouchadam ouchadam force-pushed the feature/adm/ftue-msisdn-entry branch 2 times, most recently from 6fccd1c to 666bf0e Compare June 9, 2022 16:41
Base automatically changed from feature/adm/ftue-register-test-cases to develop June 16, 2022 09:54
@ouchadam ouchadam force-pushed the feature/adm/ftue-msisdn-entry branch from ec6beae to ddbb64d Compare June 16, 2022 10:36
@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Jun 16, 2022
@ouchadam ouchadam marked this pull request as ready for review June 16, 2022 10:37
@ouchadam ouchadam requested review from a team, Florian14 and ericdecanini and removed request for a team June 16, 2022 10:37
@ericdecanini
Copy link
Contributor

Not reviewed yet but if it's a draft because it depends on another PR, wouldn't it be better to set this one's destination to the branch of that pr and add a do not merge tag so this can be reviewed in isolation?

@ouchadam
Copy link
Contributor Author

@ericdecanini my bad, I had forgot to update the PR description, I promoted this from a draft because the dependent branch has been merged 😄

Copy link
Contributor

@Florian14 Florian14 left a comment

Choose a reason for hiding this comment

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

Not tested, code looks fine

data class Success(val countryCode: String, val phoneNumber: String) : Result
}

private fun String.doesNotStartWith(input: String) = !startsWith(input)
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@ouchadam ouchadam force-pushed the feature/adm/ftue-msisdn-entry branch from ddbb64d to 5857387 Compare June 24, 2022 14:52

fun parseInternationalNumber(rawPhoneNumber: String): Result {
return when {
rawPhoneNumber.doesNotStartWith("+") -> Result.ErrorMissingInternationalCode
Copy link
Contributor

Choose a reason for hiding this comment

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

International phone numbers can also start with "00" instead of "+". Perhaps we should check that too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL

To avoid confusion especially in international context, a plus sign (+) is often used as a graphic symbol of the international access code; it informs the caller to replace it with the prefix code appropriate for their country.[3] Additionally, the GSM mobile telephony standard allows the use of the plus sign in place of the international call prefix; the mobile operator then automatically converts the plus sign to the correct international prefix, depending on the location where the phone is being used. This enables callers to use the same stored number when calling from any country.

https://en.wikipedia.org/wiki/International_call_prefix

2022-06-29T16:05:41,376511680+01:00

tl;dr mobile networks automatically covert from + to the correct IDD prefix

we would have to support multiple variants of 00, 011 etc which wouldn't scale too well and for the most part should be safe to replace with a +

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah...... let's stick with the + 😂

@ouchadam ouchadam force-pushed the feature/adm/ftue-msisdn-entry branch from 5857387 to bfa50f1 Compare July 1, 2022 10:15
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 1, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

36.8% 36.8% Coverage
0.0% 0.0% Duplication

@ouchadam ouchadam merged commit e326188 into develop Jul 1, 2022
@ouchadam ouchadam deleted the feature/adm/ftue-msisdn-entry branch July 1, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-FTUE Issue is relevant to the first time use project or experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants