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 #1416 - Wrap flow params into a Bundle to avoid a ClassNotFoundEx… #1453

Merged
merged 2 commits into from
Sep 20, 2018
Merged

Fix #1416 - Wrap flow params into a Bundle to avoid a ClassNotFoundEx… #1453

merged 2 commits into from
Sep 20, 2018

Conversation

dimipaun
Copy link
Contributor

…ception during unmarshaling.

See ArthurHub/Android-Image-Cropper#332

Hey there! So you want to contribute to FirebaseUI? Before you file this pull request, follow these steps:

  • Read the contribution guidelines.
  • Run ./gradlew check to ensure the Travis build passes.
  • If this has been discussed in an issue, make sure to mention the issue number here. If not, go file an issue about this to make sure this is a desirable change.
  • If this is a new feature please co-ordinate with someone on FirebaseUI-iOS to make sure that we can implement this on both platforms and maintain feature parity.

@dimipaun dimipaun requested a review from samtstern as a code owner September 20, 2018 20:25
@samtstern samtstern mentioned this pull request Sep 20, 2018
Closed
@samtstern
Copy link
Contributor

Looks like you have some competition from @SUPERCILEX in #1452

I will let you two decide which of these PRs should move forward.

@SUPERCILEX
Copy link
Collaborator

Whoops, lol. 😂 @dimipaun I'd be happy to have yours go though. 👍 If you want to go that route, please target the v2.2.1 branch, remove the unused toIntent method, and remove the null check in fromIntent since returning null is invalid and we'll crash later anyway.

@dimipaun
Copy link
Contributor Author

Ok will do!

@dimipaun
Copy link
Contributor Author

@SUPERCILEX @samtstern -- Both fixes are basically the same. Either way it's good -- if @SUPERCILEX is ready go with it! I think this one is also ready, pick one and don't feel bad :)

SUPERCILEX
SUPERCILEX reviewed Sep 20, 2018
View reviewed changes
Copy link
Collaborator

@SUPERCILEX SUPERCILEX left a comment

Choose a reason for hiding this comment

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

The main issue is that you need to target the dev branch, dunno how comfortable you are with rebasing.

auth/src/main/java/com/firebase/ui/auth/data/model/FlowParameters.java Outdated
Bundle bundle = new Bundle();
bundle.putParcelable(ExtraConstants.FLOW_PARAMS, this);
return bundle;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could you move this to right below the the fromIntent method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course.

@dimipaun
Copy link
Contributor Author

Let me try to rebase, I've never done it before, but happy to try.

@samtstern
Copy link
Contributor

@dimipaun the branch is version-2.1.0-dev

@dimipaun dimipaun force-pushed the fix-for-flow-params-class-not-found-2 branch from f5d754a to 628717a Compare September 20, 2018 21:54
@dimipaun
Copy link
Contributor Author

@samtstern I'm confused, why version-2.1.0-dev, I thought we're working on version-4.2.1-dev. And I see only version-2.1.0. Sorry for the back-and-forth, it's a new workflow for me.

@SUPERCILEX
Copy link
Collaborator

@dimipaun yeah, I believe that was a typo. If you edit the PR title, you'll see a button to change the base branch.

@dimipaun dimipaun changed the base branch from master to version-4.2.1-dev September 20, 2018 22:02
SUPERCILEX
SUPERCILEX approved these changes Sep 20, 2018
View reviewed changes
Copy link
Collaborator

@SUPERCILEX SUPERCILEX left a comment

Choose a reason for hiding this comment

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

LGTM! 👏

@samtstern
Copy link
Contributor

Hah yeah wow that was a typo!

@samtstern samtstern merged commit 1f3b18e into firebase:version-4.2.1-dev Sep 20, 2018
@samtstern samtstern added this to the 4.2.1 milestone Sep 20, 2018
@dimipaun dimipaun mentioned this pull request Sep 20, 2018
Closed
@dimipaun
Copy link
Contributor Author

Hi guys, has this made it into 4.2.1-SNAPSHOT so I can test it?
I was looking at https://oss.jfrog.org/webapp/#/artifacts/browse/tree/General/oss-snapshot-local/com/firebaseui/firebase-ui-auth/4.2.1-SNAPSHOT but I can't seem to be able to tell from the information displayed there.

@samtstern
Copy link
Contributor

@dimipaun it's a little tricky to tell but the last version I see is 4.2.1-20180920.235105-4 which means that it was updated late yesterday, so if you clear your cache the latest SNAPSHOT version should have what you want!

In general a SNAPSHOT version is pushed after every PR to a dev branch is merged, the only reason that wouldn't happen is errors in our pusher.

samtstern added a commit that referenced this pull request Oct 12, 2018
@samtstern
Revert "Fix #1416 - Wrap flow params into a Bundle to avoid a ClassNo…
ef4f5fe 
…tFoundEx… (#1453)"

This reverts commit 1f3b18e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SUPERCILEX SUPERCILEX SUPERCILEX approved these changes

@samtstern samtstern Awaiting requested review from samtstern

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.2.1
Development

Successfully merging this pull request may close these issues.
3 participants
@dimipaun @samtstern @SUPERCILEX