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

feat: [M3-7526] - Add setHeader to GET /profile endpoint #9987

Merged
merged 21 commits into from
Dec 17, 2023
Merged

feat: [M3-7526] - Add setHeader to GET /profile endpoint #9987

merged 21 commits into from
Dec 17, 2023

Conversation

tyler-akamai
Copy link
Contributor

@tyler-akamai tyler-akamai commented Dec 11, 2023

Description 📝

In order for the proxy account to access the username of the parent account, we need to use the auth token of the parent to collect the parent's profile info on behalf of the proxy account. We will have the parent account's token available to us for this use after storing it in local storage when the parent logs in as the proxy user via the "Switch Account" button.

Changes 🔄

List any change relevant to the reviewer.

  • Added setHeaders to the /profile endpoint
  • Added an empty object to all useProfile instances

How to test 🧪

  • Add the code from the commit named use this to test PR - make sure to remove the undefined param from the useProfile call
  • Make sure to add a PAT from another account to your .env file (this could be your personal account or a test account)
  • Ensure both usernames are logged similar to the screenshot below
    Screenshot 2023-12-13 at 5 35 25 PM

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

@tyler-akamai tyler-akamai marked this pull request as ready for review December 11, 2023 22:29
@tyler-akamai tyler-akamai requested a review from a team as a code owner December 11, 2023 22:29
@tyler-akamai tyler-akamai requested review from bnussman-akamai and coliu-akamai and removed request for a team December 11, 2023 22:29
Copy link

github-actions bot commented Dec 11, 2023

Coverage Report:
Base Coverage: 77.77%
Current Coverage: 77.77%

@mjac0bs
Copy link
Contributor

mjac0bs commented Dec 12, 2023

Cypress CI failures look relevant here and can be reproduced with a local run:

Screenshot 2023-12-12 at 9 12 10 AM

@jdamore-linode
Copy link
Contributor

Just a heads up that the E2E test failures all seem to stem from HTTP 500 responses from the API during resource clean up and aren't caused by these changes

@tyler-akamai tyler-akamai changed the title feat: [M3-7526] - Add setHeader to GET /account endpoint feat: [M3-7526] - Add setHeader to GET /profile endpoint Dec 13, 2023
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.

Nice, I confirmed that I see another request to /profile on behalf of the account we're passing a token in for, and their username is available:

Screenshot 2023-12-13 at 3 12 11 PM

and there's our RQ query key:
Screenshot 2023-12-13 at 3 19 12 PM

Thanks for your work on this! If you get a chance to update the internal ticket based on our discussion, that would be great.

Edit: The tests that failed in CI are billing/smoke-billing-activity.spec.ts (passes locally) and account/sms-verification.spec.ts, which seems to be failing consistently locally when looking for the phone number to display after it's verified, and it doesn't. This test passed in develop, so not quite sure what's happening here.
Screenshot 2023-12-13 at 7 47 18 PM

packages/manager/src/queries/profile.ts Outdated Show resolved Hide resolved
@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Dec 13, 2023
@tyler-akamai tyler-akamai requested a review from a team as a code owner December 14, 2023 16:38
@tyler-akamai tyler-akamai requested review from jdamore-linode and removed request for a team December 14, 2023 16:38
@jaalah-akamai
Copy link
Contributor

@tyler-akamai You may need to update this branch to avoid all the file noise we're seeing in Files Changed

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.

This looks good 👍

@tyler-akamai tyler-akamai removed the Add'tl Approval Needed Waiting on another approval! label Dec 17, 2023
@tyler-akamai tyler-akamai merged commit 1bde70b into linode:develop Dec 17, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants