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

chore(rwa): update user lookup in getServiceFacade, authenticator util typing and useAuthenticator updates #2538

Merged
merged 14 commits into from
Aug 31, 2022

Conversation

calebpollman
Copy link
Member

@calebpollman calebpollman commented Aug 30, 2022

Description of changes

  • add return types to Authenticator state machine utils in facade.ts
  • update user lookup in getServiceContextFacade to initially check actorContext prior to falling back on state.context to allow removal of direct lookup of user from actorContext in the SetupTOTP UI components
  • minor changes to useAuthenticator to clean up duplication

Issue #, if available

NA

Description of how you validated changes

e2e tests

Checklist

  • PR description included
  • yarn test passes
  • Tests are updated
  • No side effects or sideEffects field updated
  • Relevant documentation is changed or added (and PR referenced)

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

@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2022

🦋 Changeset detected

Latest commit: 41ec1ff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@aws-amplify/ui-react Patch
@aws-amplify/ui Patch
@aws-amplify/ui-vue Patch
@aws-amplify/ui-angular Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@calebpollman calebpollman added the run-tests Adding this label will trigger tests to run label Aug 30, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Aug 30, 2022
@calebpollman calebpollman temporarily deployed to ci August 30, 2022 17:27 Inactive
@calebpollman calebpollman temporarily deployed to ci August 30, 2022 17:27 Inactive
@calebpollman calebpollman temporarily deployed to ci August 30, 2022 17:27 Inactive
@calebpollman calebpollman temporarily deployed to ci August 30, 2022 17:27 Inactive
@calebpollman calebpollman added the run-tests Adding this label will trigger tests to run label Aug 30, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Aug 30, 2022
@lgtm-com
Copy link

lgtm-com bot commented Aug 30, 2022

This pull request introduces 2 alerts when merging 8aa0b09 into c12e9f5 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@calebpollman calebpollman added the run-tests Adding this label will trigger tests to run label Aug 30, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Aug 30, 2022
@lgtm-com
Copy link

lgtm-com bot commented Aug 30, 2022

This pull request introduces 2 alerts when merging 331c42f into c12e9f5 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@calebpollman calebpollman temporarily deployed to ci August 30, 2022 19:29 Inactive
@calebpollman calebpollman temporarily deployed to ci August 30, 2022 19:29 Inactive
@calebpollman calebpollman temporarily deployed to ci August 30, 2022 19:29 Inactive
@calebpollman calebpollman temporarily deployed to ci August 30, 2022 19:29 Inactive
@calebpollman calebpollman changed the title [POC - DO NOT MERGE] chore(rwa): improve machine typing [POC - DO NOT MERGE] chore(rwa): authenticator util typing and useAuthenticator updates Aug 30, 2022
@calebpollman calebpollman added the run-tests Adding this label will trigger tests to run label Aug 30, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Aug 30, 2022
@calebpollman calebpollman added the run-tests Adding this label will trigger tests to run label Aug 31, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Aug 31, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Aug 31, 2022
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 02:48 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 02:48 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 02:48 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 02:48 Inactive
@calebpollman calebpollman added the run-tests Adding this label will trigger tests to run label Aug 31, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Aug 31, 2022
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 04:56 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 04:56 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 04:56 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 04:56 Inactive
@calebpollman calebpollman changed the title [POC - DO NOT MERGE] chore(rwa): authenticator util typing and useAuthenticator updates chore(rwa): update user lookup in getServiceFacade, authenticator util typing and useAuthenticator updates Aug 31, 2022
@calebpollman calebpollman marked this pull request as ready for review August 31, 2022 21:32
@calebpollman calebpollman requested a review from a team as a code owner August 31, 2022 21:32
@calebpollman calebpollman added the run-tests Adding this label will trigger tests to run label Aug 31, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Aug 31, 2022
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 22:22 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 22:22 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 22:22 Inactive
@calebpollman calebpollman temporarily deployed to ci August 31, 2022 22:22 Inactive

// check for user in actorContext prior to state context. actorContext is more "up to date",
// but is not available on all states
const user = actorContext?.user ?? state.context?.user;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@wlee221 wlee221 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ErikCH ErikCH left a comment

Choose a reason for hiding this comment

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

This looks like a good start. From looking at the Angular and Vue implementations, I noticed that we have some discrepancies in a few place, that can be looked at in future PRs. Angular still uses getServiceContextFacade in the authenticator.service while React and Vue are using composable/hooks with getServiceFacade. 👍

@calebpollman calebpollman merged commit 4a4b5c9 into aws-amplify:main Aug 31, 2022
@calebpollman calebpollman deleted the rwa/poc branch August 31, 2022 23:01
@calebpollman
Copy link
Member Author

This looks like a good start. From looking at the Angular and Vue implementations, I noticed that we have some discrepancies in a few place, that can be looked at in future PRs. Angular still uses getServiceContextFacade in the authenticator.service while React and Vue are using composable/hooks with getServiceFacade. 👍

Good call out. Will make sure that they get addressed 👍

@github-actions github-actions bot mentioned this pull request Aug 31, 2022
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.

3 participants