Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Add redirect url support to gotConnectedSiteInfo#23

Merged
malinajirka merged 2 commits intodevelopfrom
merge/woocommerce-android/1224-amanda-new
Jul 23, 2019
Merged

Add redirect url support to gotConnectedSiteInfo#23
malinajirka merged 2 commits intodevelopfrom
merge/woocommerce-android/1224-amanda-new

Conversation

@AmandaRiu
Copy link
Copy Markdown
Contributor

@AmandaRiu AmandaRiu commented Jul 17, 2019

This PR merges the changes in this WCAndroid PR: woocommerce/woocommerce-android#1224 to support passing the redirectUrl back to the LoginListener. See the original PR for more information.

To Test

I've created a new branch in WPAndroid that includes the changes in this branch, along with any tweaks necessary to support these changes: amanda/login-lib-redirect-support, and opened a draft PR for testing those changes: wordpress-mobile/WordPress-Android#10232

AmandaRiu added 2 commits July 8, 2019 19:09
Send redirectUrl to listener and strip protocol before saving
d4d717c 
If the url the user logs in with is redirected, we want to know the
redirect url and use that for login. We save this url (or the original
url) to app prefs with the protocol stripped since it's not needed
for the rest of the login process and can cause issues when looking
up the site by the url.
Merge commit '615ebab45e1bc3d0cdd3a9da060026fef704d3a8' into amanda/t…
3b1db07 
…est_git_subtree
@AmandaRiu AmandaRiu mentioned this pull request Jul 17, 2019
Merged
@malinajirka malinajirka self-assigned this Jul 22, 2019
malinajirka
malinajirka suggested changes Jul 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @AmandaRiu !

I tried to pull Login-lib develop into the branch in WPAndroid and it fails with the following message

iMac:WordPress-Android jirimalina$ git subtree pull --prefix=libs/login git@github.com:wordpress-mobile/WordPress-Login-Flow-Android.git develop --squash
From github.com:wordpress-mobile/WordPress-Login-Flow-Android
 * branch                  develop    -> FETCH_HEAD
fatal: ambiguous argument '3b1db07c3872c0935c721f1b5a8e5c4a9eaf9f97^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 3b1db07c3872c0935c721f1b5a8e5c4a9eaf9f97 from commit e3144c2c11b0c982eadb2478cf6926aa40c127e6
Can't squash-merge: 'libs/login' was never added.

If I recall correctly, this is an indication that the branch was created with a buggy version of git (2.20+). Can you please confirm it's the case @aforcier? Thank you!

I've added "request changes" label, just to be sure it's not merged until we clarify this possible issue:).

@AmandaRiu
Copy link
Copy Markdown
Contributor Author

AmandaRiu commented Jul 22, 2019

@malinajirka That's interesting. What branch were you using to pull in the develop branch of the login library into WPAndroid? I thought that step wouldn't be done until this PR had been approved and merged? I created a test branch in WP Android and a draft PR that I mentioned in the description of this PR - you can find it here.

@AmandaRiu
Copy link
Copy Markdown
Contributor Author

AmandaRiu commented Jul 22, 2019

FWIW I'm using git version 2.19.2 which should be okay.

Screen Shot 2019-07-22 at 11 21 32 AM

@aforcier
Copy link
Copy Markdown
Contributor

aforcier commented Jul 22, 2019

@malinajirka @AmandaRiu yes that is a bad sign 🤔 I too would like to know which branch of WPAndroid you tried @malinajirka - I had tested this branch before and it seemed okay, and I just did these tests with no issue:

# On WPAndroid
git checkout develop
git pull
git checkout -b test-login-subtree-pull
git subtree pull --prefix=libs/login git@github.com:wordpress-mobile/WordPress-Login-Flow-Android.git develop --squash

that works fine. Also tried:

# On WPAndroid
git checkout amanda/login-lib-redirect-support
git pull
git checkout -b test-login-subtree-pull-amandas-branch
git subtree pull --prefix=libs/login git@github.com:wordpress-mobile/WordPress-Login-Flow-Android.git develop --squash

I also tried pulling in the branch of this PR instead of develop and that worked as well (though I see in your case Jirka you were pulling the login subtree's develop).

Incidentally which version of git are you using @malinajirka ?

@malinajirka
Copy link
Copy Markdown
Contributor

malinajirka commented Jul 23, 2019

Thanks for looking into this. I'm still not sure what is going on 😕.

I run the second set of steps yesterday. The first set works as expected.

git-error

I tried to clone the WPAndroid repo from scratch and I'm still getting the same error :(.
I thought perhaps git has some kind of cache (which I don't think is true). However, I tried to run the same set of command on my old laptop (with git version 2.17.2) and I'm still getting the same error. I have never had the buggy version of git installed on the old laptop, so I'm positive it's not the issue.

It's really weird that it works for both of you and it doesn't work on neither of my laptops 😞.

I also tried the following

git checkout develop
git checkout -b create-new-branch-from-scratch
git subtree pull --prefix=libs/login git@github.com:wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1224-amanda-new --squash
git subtree pull --prefix=libs/login git@github.com:wordpress-mobile/WordPress-Login-Flow-Android.git develop --squash

And it worked without any issues 🤷‍♂ .

Any ideas?:)

@malinajirka
Copy link
Copy Markdown
Contributor

malinajirka commented Jul 23, 2019

Just for the record: I had to re-pull amanda's branch (merge/woocommerce-android/1224-amanda-new) from the subtree repo git subtree pull --prefix=libs/login git@github.com:wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1224-amanda-new --squash before pulling subtree's develop.

Here is the complete list of the commands I run in WPAndroid and it worked without any issues

git checkout amanda/login-lib-redirect-support
git pull
git checkout -b test-login-subtree-pull-amandas-branch

git subtree pull --prefix=libs/login git@github.com:wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1224-amanda-new --squash

git subtree pull --prefix=libs/login git@github.com:wordpress-mobile/WordPress-Login-Flow-Android.git develop --squash

So the mystery is resolved and this PR looks good to me. Sorry for the false alarm and thank you again for helping me out!

@malinajirka malinajirka merged commit 5a21797 into develop Jul 23, 2019
@malinajirka malinajirka deleted the merge/woocommerce-android/1224-amanda-new branch July 23, 2019 14:59
wzieba pushed a commit that referenced this pull request Oct 15, 2024
@malinajirka
Merge pull request #23 from wordpress-mobile/merge/woocommerce-androi…
1c6c2f3 
…d/1224-amanda-new

Add redirect url support to gotConnectedSiteInfo
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@aforcier aforcier Awaiting requested review from aforcier

1 more reviewer

@malinajirka malinajirka malinajirka approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@malinajirka malinajirka

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AmandaRiu @aforcier @malinajirka