search

wordpress-mobile / WordPress-Login-Flow-Android

Pluggable WordPress login flow for Android
GNU General Public License v2.0
14 stars 3 forks source link

Merge Woo Signin M1 login flow changes #22

Closed AmandaRiu closed 4 years ago

AmandaRiu commented 4 years ago

This PR merges the woocommerce-android changes to the login library from this PR - which includes detailed documentation on what changed in the Woo login flow, including screenshots.

Every effort was made to not effect WPAndroid's use of the login library during this project, but there were a handful of new interface methods and strings added. I've opened a draft PR on WordPress-Android that merges the changes from this branch, as well as implements the interface methods so WPAndroid can be fully tested before this branch is merged, and includes recommended testing scenarios.

malinajirka commented 4 years ago

Thanks @AmandaRiu! I've tested the changes in WPAndroid and it works as expected.

@aforcier I'm not sure what to look out for in these "subtree" pull/push PRs. Any tips? Eg. Can I somehow double-check it wasn't pulled/pushed using the buggy version of git?

aforcier commented 4 years ago

Thanks @AmandaRiu & @malinajirka!

@malinajirka generally the big thing is to look for client-specific things that maybe don't need to be exposed to the library. For instance, I'm not sure the flow needs to be called WOO_LOGIN_MODE, a more generic name would probably be suitable. But, it's pretty minor + this is just a step in a series of iterations. But maybe something to note for the next iteration.

Other than that, making sure it builds independently (we have CI for that 👍), and checking the integration with the rest of the client apps (you've both done that) are the main things.

Can I somehow double-check it wasn't pulled/pushed using the buggy version of git? Absolutely.

First, note that the buggy version of git fails when pushing, but not when pulling. So if the changes have been pushed to the login library, they're probably okay. We should check anyway, but just an FYI of what behavior to expect while this is still a problem with git 😔

Regardless, you can test it by then pulling these changes from a client app:

  1. First, make sure you're not running a buggy version of git.
  2. a. subtree pull the changes in this branch to a test branch of WPAndroid b. Or, if a branch already exists (in this case there's already https://github.com/wordpress-mobile/WordPress-Android/pull/10150), branch off that one and subtree pull again
  3. In either 2a or 2b, if you have a safe git version and the changes were pushed/pulled using a broken version, the subtree pull will completely fail:
git subtree pull --prefix=libs/login loginlib develop --squash
remote: Enumerating objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 1
Unpacking objects: 100% (1/1), done.
From github.com:wordpress-mobile/WordPress-Login-Flow-Android
 * branch                  develop    -> FETCH_HEAD
   78c5307f7a..6d486bbf6b  develop    -> loginlib/develop
fatal: ambiguous argument '979ddf4aec664cf9afafa1cdf7d99599488f1eee^0': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
could not rev-parse split hash 979ddf4aec664cf9afafa1cdf7d99599488f1eee from commit 43aef9534664821893f4a65ac7fb52b2e8c01eee
Can't squash-merge: 'libs/login' was never added.

Otherwise, you should be okay.

(You can see where I did that to test an earlier PR that did use a bad git version: https://github.com/wordpress-mobile/WordPress-Android/pull/10149#issuecomment-508286175)