-
Notifications
You must be signed in to change notification settings - Fork 357
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-7508] - Add tests to check Parent and Child Close Account flows #10296
Conversation
Coverage Report: ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @cliu-akamai! I'd like to get either Mariah or Jaalah's approval before merging just to make sure all these scenarios are correct, but it's looking good to me! Thanks!
/** | ||
* Confirms that a proxy account cannot close the account | ||
*/ | ||
it('disables a proxy account to close the account', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it('disables a proxy account to close the account', () => { | |
it('disables "Close Account" button for proxy users', () => { |
/** | ||
* Confirms that a parent account with one or more active child accounts cannot close the account | ||
*/ | ||
it('disables a parent account with one or more active child accounts to close the account', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it('disables a parent account with one or more active child accounts to close the account', () => { | |
it('disables "Close Account" button for parent users', () => { |
|
||
// Tooltip message that appears when a parent account with one and more child accounts tries to close the account. | ||
const removeChildAccountTooltipsMessage = | ||
'Remove child accounts before closing the account.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We started created constants for things like this which would be nice to use especially when verbiage changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the other two if @jdamore-linode approves, but I understand if in a testing scenario we want to keep things silo'd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree @jaalah-akamai, I think using the constants from src
here would make sense.
(The only situations where I might insist that the strings be defined in the tests rather than imported from src
is when the language itself is really important, like with notices and warnings involving billing or potential data loss, and that's specifically so the tests break if the language changes: it basically forces us to double check the wording and gives us an extra opportunity to spot typos, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I agree with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good overall. Approved pending the suggestion comments are cleaned up, we make the final default user closing accounts test case accurate, and a changeset is added.
const contactParentUserTooltipsMessage = | ||
'Contact your parent user to close your account.'; | ||
|
||
// Tooltip message that appears when a child account tries to close the account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Tooltip message that appears when a child account tries to close the account. | |
// Tooltip message that appears when a proxy account tries to close the account. |
Let's clarify this by saying proxy.
const mockAccount = accountFactory.build(); | ||
const mockProfile = profileFactory.build({ | ||
username: 'default-user', | ||
restricted: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test shouldn't pass in real life because a restricted user should not be able to close an account, as we test in an earlier test case. Depending on what access they have for the account_access
grant, they may not be able to see the Settings page at all (they'll get a 403 unauthorized on /account
and see an error), or they may be able to see the page and click the Close Account button (with read_only
or read_write
access), but will see the final Close Account
button within the modal greyed out because they are still a restricted user.
So, we should mock an unrestricted default account here:
restricted: true, | |
restricted: false, |
If a restricted default user with read_only/read_write account_access
:
Description 📝
Add regression tests to check Parent/Child Close Account flows
Major Changes 🔄
How to test 🧪