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(Angular): Fixed bug with Setup TOTP and added Jest #2539

Merged
merged 24 commits into from
Sep 1, 2022
Merged

fix(Angular): Fixed bug with Setup TOTP and added Jest #2539

merged 24 commits into from
Sep 1, 2022

Conversation

ErikCH
Copy link
Contributor

@ErikCH ErikCH commented Aug 30, 2022 •
edited
Loading

Description of changes

Fixed issue with Angular Setup TOTP not seeing the totpIssuer and username. Included jest tests for Setup TOTP page.

Issue #, if available

#2475

PASS projects/ui-angular/src/lib/components/authenticator/components/setup-totp/setup-totp.component.spec.ts (8.425 s)
PASS projects/ui-angular/src/lib/primitives/button/button.component.spec.ts
PASS projects/ui-angular/src/lib/primitives/checkbox/checkbox.component.spec.ts

Test Suites: 3 passed, 3 total
Tests:       5 passed, 5 total
Snapshots:   0 total
Time:        10.455 s
Ran all test suites.
Done in 12.03s.

Description of how you validated changes

Added jest test

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.

SantoshLatpate and others added 19 commits July 28, 2022 11:25
@SantoshLatpate
Commit for jest setup
5fde7ad
@SantoshLatpate
Remove jasmin
80e7ea6
@SantoshLatpate
Jest working
49d2b32
@SantoshLatpate
Added 2 test classes for checkbox and button.
ffa8dd4
@SantoshLatpate
commit to remove changes in example and canery
a077769
@SantoshLatpate
Merge branch 'main' into poc/jest
addf881
@SantoshLatpate
revert the unrelated changes
8747fda
@SantoshLatpate
Revert karma file changes
38894ad
@SantoshLatpate
remove karma.config.js and revert changes
da88228
@SantoshLatpate
Remove the unwanted ng-package.json file
7d18605
angular fix first commit
c727188
@wlee221
Consolidate devDependencies in parent folder
aa5b40c
@wlee221
Merge remote-tracking branch 'origin/main' into santosh-jest-setup
87a6c19
@wlee221
Consolidate tsconfig and jest config
9fdde93
@wlee221
Fix linting issue
83b4512
Merge branch 'main' into setup-totp-angular-fix
3140c66
Merge remote-tracking branch 'origin/santosh-jest-setup' into setup-t…
f860420 
…otp-angular-fix-with-jest
Fixed jest tests, added in testbed tests
92f3b04
Added changeset
3251b6b
@ErikCH ErikCH requested a review from a team as a code owner August 30, 2022 17:55
@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 955ac0b

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-angular Patch
@aws-amplify/ui-react Patch
@aws-amplify/ui Patch
@aws-amplify/ui-vue 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

ErikCH
ErikCH commented Aug 30, 2022
View reviewed changes
packages/angular/angular.json
Comment on lines -24 to -31
"test": {
"builder": "@angular-devkit/build-angular:karma",
"options": {
"main": "projects/ui-angular/src/test.ts",
"tsConfig": "projects/ui-angular/tsconfig.spec.json",
"karmaConfig": "projects/ui-angular/karma.conf.js"
}
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer needed

ErikCH
ErikCH commented Aug 30, 2022
View reviewed changes
...-angular/src/lib/components/authenticator/components/setup-totp/setup-totp.component.spec.ts

setupTOTPSpy.mockResolvedValue(SECRET_KEY);

const mockAuthenticatorService = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New mocked service

ErikCH
ErikCH commented Aug 30, 2022
View reviewed changes
...ts/ui-angular/src/lib/components/authenticator/components/setup-totp/setup-totp.component.ts

constructor(public authenticator: AuthenticatorService) {}

ngOnInit(): void {
this.generateQRCode();
async ngOnInit(): Promise<void> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be async

ErikCH
ErikCH commented Aug 30, 2022
View reviewed changes
packages/angular/projects/ui-angular/src/lib/services/authenticator.service.ts Outdated
@@ -35,8 +35,10 @@ export class AuthenticatorService implements OnDestroy {
const authService = interpret(machine).start();

this._machineSubscription = authService.subscribe((state) => {
this._authState = state;
this._facade = getServiceContextFacade(state);
this._authState = state as unknown as AuthMachineState;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was throwing a TS error, so I fixed the type

Copy link
Member

Choose a reason for hiding this comment

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

Rather than cast twice (here and on line 40), can we just type the state argument on line 37?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can clean it up a little, by setting state to unknown then only typecasing to AuthMachineState. That helps clean it up a bit. But I'm not sure how to do that all in one in line 37.


    this._machineSubscription = authService.subscribe((state: unknown) => {
      this._authState = state as AuthMachineState;
      this._facade = getServiceContextFacade(state as AuthMachineState);
    });

ErikCH
ErikCH commented Aug 30, 2022
View reviewed changes
packages/react/src/components/Authenticator/SetupTOTP/SetupTOTP.tsx
@@ -7,6 +7,7 @@ import {
getActorState,
SignInState,
translate,
getTotpCode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this into it's own utility function in ui

ErikCH
ErikCH commented Aug 30, 2022
View reviewed changes
packages/vue/package.json
@@ -53,7 +53,6 @@
"@vitejs/plugin-vue": "^2.3.3",
"@vitejs/plugin-vue-jsx": "^1.3.10",
"@vue/babel-plugin-jsx": "^1.1.1",
"@vue/cli-plugin-unit-jest": "5.0.0-beta.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a really old import that I never used, but was installing an old version of jest. So I removed it.

@lgtm-com
Copy link

lgtm-com bot commented Aug 30, 2022

This pull request introduces 1 alert when merging d6421f3 into c12e9f5 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@ErikCH ErikCH temporarily deployed to ci August 30, 2022 18:13 Inactive
calebpollman
calebpollman reviewed Aug 30, 2022
View reviewed changes
Copy link
Member

@calebpollman calebpollman left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Left some feedback and questions.

@ErikCH ErikCH dismissed stale reviews from joebuono and 0618 via b226db3 August 31, 2022 21:18
calebpollman
calebpollman previously approved these changes Aug 31, 2022
View reviewed changes
Copy link
Member

@calebpollman calebpollman left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

packages/angular/package.json
@@ -7,50 +7,47 @@
"dev": "ng build --watch",
"start": "ng serve",
"build": "ng build --prod",
"test": "echo \"Skipped: ng test\"",
"test": "jest",
Copy link
Member

Choose a reason for hiding this comment

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

✨ Definitely can be done in a separate PR but we should add yarn angular test in the top level yarn test script

@ErikCH ErikCH temporarily deployed to ci August 31, 2022 22:24 Inactive
@ErikCH ErikCH temporarily deployed to ci August 31, 2022 22:24 Inactive
@ErikCH ErikCH temporarily deployed to ci August 31, 2022 22:24 Inactive
@ErikCH ErikCH temporarily deployed to ci August 31, 2022 22:24 Inactive
wlee221
wlee221 approved these changes Aug 31, 2022
View reviewed changes
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.

:shipit:

@ErikCH ErikCH merged commit ba9818f into main Sep 1, 2022
@ErikCH ErikCH deleted the setup-totp-angular-fix-with-jest branch September 1, 2022 16:41
@github-actions github-actions bot mentioned this pull request Sep 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wlee221 wlee221 wlee221 approved these changes

@calebpollman calebpollman calebpollman approved these changes

@joebuono joebuono joebuono left review comments

@0618 0618 0618 left review comments

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

Successfully merging this pull request may close these issues.
6 participants
@ErikCH @0618 @calebpollman @wlee221 @joebuono @SantoshLatpate