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

fix(aws-amplify-react-native): Update Hub listener for sign out events #5587

Merged
merged 8 commits into from Apr 30, 2020

Conversation

amhinson
Copy link
Contributor

@amhinson amhinson commented Apr 28, 2020

Issue #, if available:
Closes #4057

Description of changes:

  • Update Hub "auth" listener to handle calling Auth.signOut() directly instead of using the sign out button in Greeting. The onHubCapsule method was refactored based on the web UI components.
  • change handleStateChange to only block setting state if this._isMounted === false instead of the entire method. This also makes some changes to more closely resemble the web UI logic.
  • Also, this makes sure checkUser is only called once instead of every time an auth event occurs. That behavior is also to reflect the web UI.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@amhinson amhinson added the React Native label Apr 28, 2020
@amhinson amhinson requested a review from ashika01 Apr 28, 2020
@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Apr 28, 2020

This pull request fixes 2 alerts when merging b8fed61 into 14f1ef9 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@codecov
Copy link

@codecov codecov bot commented Apr 28, 2020

Codecov Report

Merging #5587 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5587   +/-   ##
=======================================
  Coverage   72.91%   72.91%           
=======================================
  Files         197      197           
  Lines       11536    11536           
  Branches     2178     2178           
=======================================
  Hits         8412     8412           
  Misses       2971     2971           
  Partials      153      153           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f03e22...06b408f. Read the comment docs.

@Amplifiyer
Copy link
Contributor

@Amplifiyer Amplifiyer commented Apr 28, 2020

Can we accompany this with unit tests and/or integration tests?

@amhinson
Copy link
Contributor Author

@amhinson amhinson commented Apr 28, 2020

Just added a test to the existing PR for React Native Auth integration tests: https://github.com/aws-amplify/amplify-js-samples-staging/pull/76

Co-Authored-By: Eric Clemmons <eric@smarterspam.com>
@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Apr 28, 2020

This pull request fixes 2 alerts when merging ea588fe into ebe0b69 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

Copy link
Contributor

@ashika01 ashika01 left a comment

Couple of nits. Looks good. Agree with Praveen, could we have a unit test added for this here?

@amhinson
Copy link
Contributor Author

@amhinson amhinson commented Apr 28, 2020

Couple of nits. Looks good. Agree with Praveen, could we have a unit test added for this here?

There are no existing unit tests for aws-amplify-react-native ... 😱 so that might need to be a separate PR to get all that setup. However, the integ test linked above covers this case now (and making sure this doesn't break anything else).

Co-Authored-By: Ashika <35131273+ashika01@users.noreply.github.com>
@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Apr 28, 2020

This pull request fixes 2 alerts when merging d437641 into e89ac00 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

let nextAuthState;
if (state === 'signedOut') {
state = 'signIn';
nextAuthState = this._initialAuthState;
} else {
nextAuthState = state;
}

let nextAuthData = this.state.authData;
if (data !== undefined) {
nextAuthData = data;
}
Copy link
Contributor

@ashika01 ashika01 Apr 30, 2020

Choose a reason for hiding this comment

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

I know the important bit here is the hub listener for signout and looks good.

We can clean nextAuthData and nextAuthState with ternary operator so this is readable and become easier later. :)

Copy link
Contributor Author

@amhinson amhinson Apr 30, 2020

Choose a reason for hiding this comment

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

Great call 👍 I just updated the PR

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Apr 30, 2020

This pull request fixes 2 alerts when merging f090258 into 41fb52c - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

Copy link
Contributor

@ashika01 ashika01 left a comment

Looks good 🎉 👍

Copy link
Contributor

@ericclemmons ericclemmons left a comment

Thanks for the updates @amhinson! Looking good!

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Apr 30, 2020

This pull request fixes 2 alerts when merging 06b408f into 8f03e22 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@amhinson amhinson merged commit b8d2c4d into aws-amplify:master Apr 30, 2020
5 checks passed
@amhinson amhinson deleted the rn-listen-for-signout branch Apr 30, 2020
@nihp
Copy link

@nihp nihp commented Jul 8, 2020

Can you able to look this?
#6257

This case is if the user is already in the app and the admin has disabled the user. So I called Auth.signOut. After this it changing the authState to signIn and then immediately changed to signedIn. So it not properly navigate to login screen.

Again reloaded the app will show the SignIn screen.

Steps:

  1. Login the app
  2. Admin disable the user
  3. Then it not shows the user is disabled state while i am in (i.e) it not show anything in UI (expected behaviour is not to allow this user to do anything in the app)
  4. So reload the app it will change the authState to signIn (It shows login screen for a second)
  5. Again it immediately change the authState to signedIn (So it shows a white screen)
  6. Because of this I need to reload again the app (Then it changes the authstate to signIn and shows the login screen as expected)

Here: I don't want to reload the app in the 6th step

Also, can you give any solution for the 3rd point

Your reply will be helpful

@github-actions
Copy link

@github-actions github-actions bot commented Jul 8, 2021

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
React Native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants