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

refresh-user #224

Merged
merged 6 commits into from
Mar 15, 2018
Merged

refresh-user #224

merged 6 commits into from
Mar 15, 2018

Conversation

eoji
Copy link
Contributor

@eoji eoji commented Mar 13, 2018

what

Refreshing the user on app resumes.

why

We are running into a bug where creators won't see the button to the dashboard because their user object is stale. memberProjectsCount on the User object was previously unused so for all users it's 0 until they are refreshed. Previously, we only refreshed when they logged in or out or they viewed settings, messages or profile.

how

This happens in the onActivityResumed(when the app is foregrounded) method of ApplicationLifecycleUtil where we are currently tracking app opens and refreshing the config. We have the access token (a nullable string) and the user observable. I make the token an observable by calling .just. Then, I use zipWith to combine them in a pair/tuple (You don’t want to subscribe to the current user observable because it would emit every time the user is refreshed and then you'd kick off never ending network requests to refresh the user ). Then I call refresh using the current user.

Observable.just(accessToken)
.filter(ObjectUtils::isNotNull)
.zipWith(user, Pair::create)
.subscribe(tokenUserPair -> this.currentUser.refresh(tokenUserPair.second));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey as far as I can tell (but I might be missing something in the Java) is that this.currentUser.refresh() will just refresh with whatever is passed to it. So I think this will just take the currentUser and "refresh" it again. At which point do we make a new call to the API to fetch a fresh user?

Also, in this particular scenario it might not even be necessary to use RxJava because we're just imperatively lifting the token into FRP land in order to subscribe to it (with the user) and then to refresh the user with the user 🤔

I think I might need a little more insight into how the lifecycle stuff works in Android but maybe we should pair on this 💪

Copy link
Contributor Author

@eoji eoji Mar 14, 2018

Choose a reason for hiding this comment

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

😅 look for my calendar invite!

I changed this line! Which is where we get a fresh user 🙏 https://github.com/kickstarter/android-oss/pull/224/files#diff-40963cc3fde7a9c5d143820bc2075139R65

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah got it! I missed the fetch part of that 😄

Then actually this is probably fine but couldn't it just be:

this.client.fetchCurrentUser()
  .filter(accessToken != null) //pseudeocode
  .subscribe(freshUser -> this.currentUser.refresh(freshUser))

Think that just removes the need to lift the accessToken into RxJava and to zip it with the new user.

Added a util method to check if the user has "changed", meaning they have different IDs. This is to stop the discovery screen (both activity and fragment) from reloading every time we refresh the user. Tests!
Also made videoStats null in ProjectStats because I saw a crash.
Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Looks great!

@eoji eoji merged commit de0fde9 into master Mar 15, 2018
@eoji eoji deleted the refresh-user branch March 15, 2018 19:04
Rcureton pushed a commit that referenced this pull request Jul 31, 2018
* Refreshing the user on app resumes.

* Lots of changes!
Added a util method to check if the user has "changed", meaning they have different IDs. This is to stop the discovery screen (both activity and fragment) from reloading every time we refresh the user. Tests!
Also made videoStats nullable in ProjectStats because I saw a crash.

* Adding a comment to help clarify what `userHasChanged` does.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants