Issue/1482 step2 magic link signin changes by anitaa1990 · Pull Request #1498 · woocommerce/woocommerce-android

anitaa1990 · 2019-10-24T14:05:14Z

Partially fixes #1482. This PR completes the second step of the Jetpack sign in with self credentials as defined in the master thread.

Changes

Modified UI in LoginMagicLinkRequestFragment and LoginMagicLinkSentFragment according to new design.
Created a new class: MagicLinkInterceptFragment that handles the magic link login process. This was previously handled by the MainActivity. This logic has been migrated to the new class.

Behaviour

(LEFT: Before . RIGHT: After)
.

Notes:

The are TODO's added to the PR to add events to the button clicks. This will take place in another PR.
Note that this PR is to be merged to a feature branch.
I've kept the option to enter the password as well since the user might not always have the email connected to their account on their phone. This is not part of the design flow though. Let me know if there are any objections and we can certainly hide it.

Testing:

Verify that the current flow with magic link is working correctly.
Verify the current flow to enter password instead of the magic flow link, is working correctly.

Update release notes:

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…magic link

peril-woocommerce · 2019-10-24T14:05:44Z

	Fails
🚫	Danger failed to run `/app/danger-0.reqr1rvieqe.ts`.
🚫	Danger failed to run `/app/danger-0.mrzlk8cp4e.ts`.

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

	Messages
📖	This PR contains changes in the subtree `libs/login/`. It is your responsibility to ensure these changes are merged back into `wordpress-mobile/WordPress-Login-Flow-Android`. Follow these handy steps! WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history! `cd woocommerce-android` `git checkout issue/1482-step2-magic-link-signin-changes` `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1498` Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/1498 and open a new PR.

Error TypeError

Cannot read property 'added' of null
TypeError: Cannot read property 'added' of null
    at Object.exports.default (/app/danger-0.reqr1rvieqe.ts:16:44)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

11|         warn("The PlayStoreStrings.po file must be updated any time changes are made to release notes");
12|     }
13| 
14|     // If changes were made to the strings, make sure they follow our guidelines
15|     const modifiedStrings = modifiedFiles.filter((path: string) => path.endsWith('values/strings.xml'))
----------------------------------------------^
16| 
17|     for (let file of modifiedStrings) {        
18|         const stringDiffs = await danger.git.diffForFile(file)
19|

Error TypeError

Cannot read property 'diff' of null
TypeError: Cannot read property 'diff' of null
    at checkCommitDiffs (/app/danger-0.mrzlk8cp4e.ts:43:49)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

38|         if (git === undefined) {
39|             console.log("About to crash due to an error")
40|             console.log("File:", thisFile)
41|             console.log("Danger Object: ", danger)
42| 
---------------------------------------------------^
43|             if (danger !== undefined) {
44|                 console.log("Danger is no longer defined")
45|             }
46|             else {

Generated by 🚫 dangerJS

…e/1482-step2-magic-link-signin-changes

peril-woocommerce · 2019-10-24T14:14:58Z

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

…github.com/woocommerce/woocommerce-android into issue/1482-step2-magic-link-signin-changes

AmandaRiu

@anitaa1990 Nice work on all of this! I've finished my first review and have left some comments in the code, plus the following:

Design Comments

Changes to the magic link view for the self-hosted flow has also changed the magic link view for the normal flow. Was this intentional? If I don’t use my self-hosted auth credentials and just do the normal flow of logging in with my WPcom creds, I see this:

...which feels wrong in this context since we're logging in and not just verifying an email address. cc: @kyleaparker for clarification.

Crash

Error while trying to logout:

AndroidRuntime  D  Shutting down VM
 E  FATAL EXCEPTION: main
 E  Process: com.woocommerce.android.dev, PID: 10933
 E  org.greenrobot.eventbus.EventBusException: Invoking subscriber failed
 E      at org.greenrobot.eventbus.EventBus.handleSubscriberException(EventBus.java:527)
 E      at org.greenrobot.eventbus.EventBus.invokeSubscriber(EventBus.java:509)
 E      at org.greenrobot.eventbus.EventBus.invokeSubscriber(EventBus.java:501)
 E      at org.greenrobot.eventbus.HandlerPoster.handleMessage(HandlerPoster.java:67)
 E      at android.os.Handler.dispatchMessage(Handler.java:106)
 E      at android.os.Looper.loop(Looper.java:193)
 E      at android.app.ActivityThread.main(ActivityThread.java:6669)
 E      at java.lang.reflect.Method.invoke(Native Method)
 E      at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
 E      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)
 E  Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String android.content.Context.getPackageName()' on a null object reference
 E      at android.content.ComponentName.<init>(ComponentName.java:130)
 E      at android.content.Intent.<init>(Intent.java:6082)
 E      at com.woocommerce.android.ui.login.MagicLinkInterceptFragment.showLoginScreen(MagicLinkInterceptFragment.kt:102)
 E      at com.woocommerce.android.ui.login.MagicLinkInterceptPresenter.onAuthenticationChanged(MagicLinkInterceptPresenter.kt:60)
 E      at java.lang.reflect.Method.invoke(Native Method)
 E      at org.greenrobot.eventbus.EventBus.invokeSubscriber(EventBus.java:507)
 E      ... 8 more

To reproduce:
1. Login with magic link (doesn’t matter which flow)
2. After successful login, log out.
3. Crash.

RELEASE-NOTES.txt

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/login/MagicLinkInterceptContract.kt

WooCommerce/src/main/res/values/strings.xml

...dPressLoginFlow/src/main/java/org/wordpress/android/login/LoginMagicLinkRequestFragment.java

WooCommerce/src/main/res/values/strings.xml

kyleaparker · 2019-10-25T14:34:17Z

...which feels wrong in this context since we're logging in and not just verifying an email address. cc: @kyleaparker for clarification.

Yes the copy should be different in the two different scenarios

…ials

anitaa1990 · 2019-10-28T04:36:54Z

Thanks for the thorough review @AmandaRiu

Changes to the magic link view for the self-hosted flow has also changed the magic link view for the normal flow. Was this intentional?

I've fixed this in a5321d2

Crash Error while trying to logout:

I haven't been able to reproduce this, for some reason. I tried logging in via the normal flow and the self hosted flow and both times it was successful 🤔 I've made some changes to the flow so perhaps it's fixed? Could I trouble you to let me know if you are able to reproduce this now?

PR ready for another round 👍

…e/1482-step2-magic-link-signin-changes

AmandaRiu

@anitaa1990 Looking great! The conversion to MVVM is so nice! I've only left a few code comments - once addressed this should be ready to merge 👍

Oh and Im no longer able to reproduce that crash either. Maybe the switch to MVVM fixed it.

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/login/MagicLinkInterceptRepository.kt

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/login/MagicLinkInterceptViewModel.kt

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/login/MagicLinkInterceptFragment.kt

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/login/MagicLinkInterceptRepository.kt

…info

anitaa1990 · 2019-10-30T05:19:57Z

Thanks for the review @AmandaRiu.

Just curious why you moved these out of the ActivityBindingModule.

Since we need the ViewModelModule and ThreadModule across multiple activities now (MainActivity and LoginActivity) and we might in the future use the same modules if we migrate from MVP to MVVM for the site picker or settings screen, I thought it made more sense to move these two to AppComponent

I've fixed the code changes requested and PR is ready for another round 👍

AmandaRiu

@anitaa1990

Since we need the ViewModelModule and ThreadModule across multiple activities now (MainActivity and LoginActivity) and we might in the future use the same modules if we migrate from MVP to MVVM for the site picker or settings screen, I thought it made more sense to move these two to AppComponent

Makes perfect sense to me! Changes look great...beautiful work!

anitaa1990 added 5 commits October 22, 2019 23:06

Design changes for request magic link screen

81429b1

Changed text of magic link sent screen

124e02b

Added new fragment, presenter, module, contract for opening/handling …

c60398c

…magic link

Added logic to open MagicLinkInterceptFragment when magic link is opened

cc82d25

Added release notes

f366aa1

anitaa1990 added type: enhancement A request for an enhancement. Login labels Oct 24, 2019

anitaa1990 added this to the 3.0 milestone Oct 24, 2019

anitaa1990 requested review from 0nko, AmandaRiu and nbradbury October 24, 2019 14:05

Merge branch 'feature/sign-in-with-self-hosted-credentials' into issu…

ac6411b

…e/1482-step2-magic-link-signin-changes

anitaa1990 added 2 commits October 24, 2019 20:06

Removed translated string since the base string has been updated

1a57344

Merge branch 'issue/1482-step2-magic-link-signin-changes' of https://…

21cb1ed

…github.com/woocommerce/woocommerce-android into issue/1482-step2-magic-link-signin-changes

AmandaRiu self-assigned this Oct 24, 2019

AmandaRiu suggested changes Oct 24, 2019

View reviewed changes

Migrating to MVVM from MVP

44ba65a

anitaa1990 added 3 commits October 28, 2019 09:30

Added new string labels for magic link sent label

f89abce

Modified release notes

8656163

Added option to verify email only if login is for self hosted credent…

a5321d2

…ials

anitaa1990 added type: enhancement A request for an enhancement. Login and removed type: enhancement A request for an enhancement. Login labels Oct 28, 2019

anitaa1990 requested a review from AmandaRiu October 29, 2019 00:07

anitaa1990 mentioned this pull request Oct 29, 2019

Jetpack sign-in with self-hosted credentials - Master checklist #1482

Closed

11 tasks

Merge branch 'feature/sign-in-with-self-hosted-credentials' into issu…

e8a00c3

…e/1482-step2-magic-link-signin-changes

AmandaRiu suggested changes Oct 29, 2019

View reviewed changes

Added logic to handle error scenarios when fetching account and site …

1a9050c

…info

anitaa1990 removed request for 0nko and nbradbury October 30, 2019 04:48

anitaa1990 requested a review from AmandaRiu October 30, 2019 05:21

AmandaRiu approved these changes Oct 30, 2019

View reviewed changes

AmandaRiu merged commit 4ee73e8 into feature/sign-in-with-self-hosted-credentials Oct 30, 2019

AmandaRiu deleted the issue/1482-step2-magic-link-signin-changes branch October 30, 2019 07:02

designsimply added feature: login Related to any part of the log in or sign in flow, or authentication. and removed Login labels May 6, 2021

Conversation

anitaa1990 commented Oct 24, 2019

Changes

Behaviour

Notes:

Testing:

Uh oh!

peril-woocommerce bot commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Error TypeError

Dangerfile

Error TypeError

Dangerfile

Uh oh!

peril-woocommerce bot commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmandaRiu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Design Comments

Crash

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kyleaparker commented Oct 25, 2019

Uh oh!

anitaa1990 commented Oct 28, 2019

Uh oh!

AmandaRiu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anitaa1990 commented Oct 30, 2019

Uh oh!

AmandaRiu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peril-woocommerce bot commented Oct 24, 2019 •

edited

Loading

peril-woocommerce bot commented Oct 24, 2019 •

edited

Loading

AmandaRiu left a comment •

edited

Loading