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

test: [M3-7497] - Add tests for child user verification banner #10204

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

cliu-akamai
Copy link
Contributor

Description 📝

Add regression tests for checking child user verification banner

Major Changes 🔄

  • Add regression tests to check child user verification banner

How to test 🧪

yarn cy:run -s "cypress/e2e/core/account/user-verification-banner.spec.ts"

@cliu-akamai cliu-akamai requested a review from a team as a code owner February 16, 2024 21:03
@cliu-akamai cliu-akamai requested review from jdamore-linode and removed request for a team February 16, 2024 21:03
Copy link

github-actions bot commented Feb 16, 2024

Coverage Report:
Base Coverage: 81.28%
Current Coverage: 81.28%

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

@cliu-akamai I think we need one more test to ensure that the banner shows when we have security questions but not a phone number. Otherwise, this is looking good.

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Other than Jaalah's ask, we'll also need a changeset before this is good to go.

@cliu-akamai cliu-akamai requested a review from a team as a code owner February 26, 2024 19:33
@cliu-akamai cliu-akamai requested review from bnussman-akamai and carrillo-erik and removed request for a team February 26, 2024 19:33
@carrillo-erik
Copy link
Contributor

@cliu-akamai I was able to run this locally and after running the tests I did not see the test failing. I re-started the end-to-end test in the Jenkins UI and hopefully that resolves the issue. Everything else looks good.

Copy link
Contributor

@mjac0bs mjac0bs 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 the changes! For completeness, we could test if the banner appears with only questions defined.

Confirmed this spec runs locally and tests: banner with only phone number defined, banner with neither phone number or question defined, and no banner when both phone number and questions are defined.

Screenshot 2024-03-01 at 7 31 55 AM

Approving pending we clean up a few comments/test descriptions with typos or a slightly inaccurate description of the test. It would also be good to cover that final test.

I'm going to dismiss Jaalah's requested changes since his feedback has been addressed. Rerequesting your reviewers is another thing you can do in the future after addressing feedback - it's helpful for reviewers, especially when someone blocks changes on your PR.


describe('User verification banner', () => {
/*
* - Confirms that a banner is present the child users do not have a phone number or security questions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* - Confirms that a banner is present the child users do not have a phone number or security questions.
* - Confirms that a banner is present when child users do not have a phone number or security questions.

});

/*
* - Confirms that a banner is present the child users set up security questions but not have a phone number.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* - Confirms that a banner is present the child users set up security questions but not have a phone number.
* - Confirms that a banner is present when child users set up security questions but not a phone number.

/*
* - Confirms that a banner is not shown when the child user sets up security questions.
*/
it('does not show up when a child user adds a phone number or set up security questions', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('does not show up when a child user adds a phone number or set up security questions', () => {
it('does not show up when a child user adds a phone number and sets up security questions', () => {

Phone number and questions need to be set up in order for the banner to go away. We're testing this correctly! Description is just inaccurate.

});

/*
* - Confirms that a banner is not shown when the child user sets up security questions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* - Confirms that a banner is not shown when the child user sets up security questions.
* - Confirms that a banner is not shown when the child user sets up both phone number and security questions.

Same comment as above.

Comment on lines 50 to 55
mockAppendFeatureFlags({
parentChildAccountAccess: makeFeatureFlagData(true),
}).as('getFeatureFlags');
mockGetFeatureFlagClientstream().as('getClientStream');

mockGetUsers([mockRestrictedProxyUser]).as('getUsers');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if we're not using the aliases in a cy.wait call, we could remove them here. (Related: I don't know if not waiting for feature flags could potentially cause any flakiness, which is why we generally do it, or if we are confident it will happen fast enough that it's not very necessary in the first place.)

Suggested change
mockAppendFeatureFlags({
parentChildAccountAccess: makeFeatureFlagData(true),
}).as('getFeatureFlags');
mockGetFeatureFlagClientstream().as('getClientStream');
mockGetUsers([mockRestrictedProxyUser]).as('getUsers');
mockAppendFeatureFlags({
parentChildAccountAccess: makeFeatureFlagData(true),
});
mockGetFeatureFlagClientstream();
mockGetUsers([mockRestrictedProxyUser]);

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Mar 1, 2024
@mjac0bs mjac0bs dismissed jaalah-akamai’s stale review March 1, 2024 15:50

Feedback was addressed in 18c7881

@cliu-akamai cliu-akamai merged commit 661f074 into linode:develop Mar 4, 2024
17 of 18 checks passed
vrajesh73 added a commit to vrajesh73/manager that referenced this pull request Mar 12, 2024
…eature/namespace-create

* 'develop' of https://github.com/vrajesh73/manager: (89 commits)
  fix: [M3-7269] - Display parent email in user menu when no company name is available for restricted parent user (linode#10248)
  fix: [M3-7817] - Show correct status of Child Account Enabled column for parent users (linode#10233)
  upcoming: [M3-7616] - Add Placement Groups Events and Notifications (linode#10221)
  upcoming: [M3-7816-v2] - Adjust logic for when to show Switch Account button (linode#10266)
  fix: [M3-7831] - Persisting error messages in ACLB delete dialogs (linode#10254)
  upcoming: [M3-7842] - Update Assign Linode Drawer and improve query skipping (linode#10263)
  upcoming: [M3-7704] - Disable Cloning, Private IP, Backups for edge regions (linode#10222)
  test: Fix test flake for Images landing page test (linode#10267)
  fix: [M3-7824] - ACLB TCP Rule Creation and other fixes (linode#10264)
  refactor: [M3-7687] - Linodes Restricted User Experience 2/2 (linode#10227)
  test: Resolve OBJ create and delete E2E test flake (linode#10245)
  upcoming: [M3-7723] - Placement Group feature flag as object (linode#10256)
  chore(deps): Bump sanitize-html from 2.11.0 to 2.12.1 (linode#10247)
  change: [M3-7813] - Allow the disabling of the TypeToConfirm input (linode#10251)
  upcoming: [M3-7839] - Change Business Partner to Parent User (linode#10259)
  upcoming: [M3-7835] - Adjust user table column count (linode#10252)
  upcoming: [M3 7738] - Update Placement Group Create & Edit Drawers (linode#10205)
  refactor: [M3-7437] - Use `@lukemorales/query-key-factory` for Profile Queries (linode#10241)
  fix: React Query `updateInPaginatedStore` helper function not working as expected (linode#10249)
  test: [M3-7497] - Add tests for child user verification banner (linode#10204)
  ...

# Conflicts:
#	packages/manager/src/MainContent.tsx
#	packages/manager/src/dev-tools/FeatureFlagTool.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants