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

upcoming: [M3-7459] - Disable Contact / Billing Info for Restricted Users #10036

Merged
merged 11 commits into from
Jan 16, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Jan 5, 2024

Description 📝

Users with read-only access, should not have the ability to edit billing contacts or add payment methods. Currently, the "Edit" billing contact and "Add payment method" buttons are active for these types of users. While users receive denials via the API when trying to submit changes, this may creates confusion and frustration. To improve the user experience and align with intended permissions, it's necessary to disable these buttons.

Changes 🔄

  • Edit and Add Payment Method will be disabled for "Indirect Customers" and those with read_only account access.
  • A tooltip will appear saying To edit this section, please contact your {administrator || business partner}.
  • Fixed bug with disabled buttons not allowing us to tab out of them.

Preview 📷

Before After
prod-contact-info.mp4
Screenshot 2024-01-05 at 1 41 56 PM
Didn't want to update screenshot, but it's been updated to say business partner. Screenshot 2024-01-05 at 1 43 23 PM
Screenshot 2024-01-05 at 1 54 21 PM

How to test 🧪

Prerequisites

  • One account with full access
  • One account with restricted access

Reproduction steps

Verification steps

  1. Log into any restricted account with parent/child feature flag disabled, observe the Edit and Add Payment Method buttons are disabled.
  2. Turn parent/child feature flag on with Mock Service Workers

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@jaalah-akamai jaalah-akamai marked this pull request as ready for review January 8, 2024 20:52
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner January 8, 2024 20:52
@jaalah-akamai jaalah-akamai requested review from jdamore-linode, bnussman-akamai and mjac0bs and removed request for a team January 8, 2024 20:52
});

// TODO: When we figure out issue with Vitest circular dependencies
// vi.mock('src/queries/profile', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdamore-linode This is what we talked about earlier. This is not a critical test, so we'll revisit at Cafe. 👍

};
});

vi.mock('src/queries/profile', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdamore-linode To add to our investigation, oddly this causes no issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks again @jaalah-akamai! Still no explanation for why it doesn't hang here, but did confirm by upgrading Vitest that this Vitest PR fixed the issue.

Once this is merged I'll plan to open a PR that upgrades Vitest and re-enables the commented out test 👍

Copy link
Member

Choose a reason for hiding this comment

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

I see the benefit of this approach to avoid dealing with loading states, but I don't think it's better than the MSW approach. Would like to discuss in cafe!

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.

I think your test steps have a typo in Observe the Edit and Add Payment Method buttons are disabled (by default we're in a parent state), which should be enabled.

I was able to confirm the following:

Scenario Picture
✅ Restricted regular user with read_only account access, flag is off, mocks are off Screenshot 2024-01-10 at 1 50 52 PM
✅ Restricted child user (with read_write account access), flag is on, mocks are on Screenshot 2024-01-10 at 1 53 48 PM
parent user or restricted proxy user (with read_write account access), flag is on, mocks are on Screenshot 2024-01-10 at 1 55 12 PM

The disabled tooltips are read out via VO. See also the message I sent offline, but I may have not had the right key combination to be able to keyboard navigate away from the disabled Edit button - I can esc out of the open tooltip, but trying to Tab or Shift + Tab keeps me on that button.

Pending that the above is not an issue to resolve, approving with a couple of small things to address. In addition to my comments, this PR will need a changeset.

@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Jan 11, 2024

I think your test steps have a typo in Observe the Edit and Add Payment Method buttons are disabled (by default we're in a parent state), which should be enabled.

Ahh yes, it's a typo! 🔍

@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Jan 11, 2024

I can esc out of the open tooltip, but trying to Tab or Shift + Tab keeps me on that button.

@mjac0bs This should be fixed now while still preventing disabled buttons from submitting forms as outlined here: #10028 (review)

@jaalah-akamai jaalah-akamai added Add'tl Approval Needed Waiting on another approval! and removed Missing Changeset labels Jan 11, 2024
Comment on lines +80 to +82
const { data: user } = useAccountUser(profile?.username ?? '');
const isChildUser =
flags.parentChildAccountAccess && user?.user_type === 'child';
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to use useAccountUser? Is there no better way to get user_type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai I don't think so, user_type is a new field that's only returned from /account/users or /account/users/:user

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'll see if API could also return that field as part of /account data.

};
});

vi.mock('src/queries/profile', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I see the benefit of this approach to avoid dealing with loading states, but I don't think it's better than the MSW approach. Would like to discuss in cafe!

Copy link

Coverage Report:
Base Coverage: 79.87%
Current Coverage: 79.85%

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Disabled states look and new logic look good!

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 11, 2024
@jaalah-akamai jaalah-akamai merged commit f3c43a6 into linode:develop Jan 16, 2024
16 checks passed
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 Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants