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

refactor: [M3-7831] - Use nodebalancers/types endpoint for pricing data #10265

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Mar 6, 2024

Description 📝

API is now returning pricing data through the /nodebalancers/types endpoint.

As developers, we want to update our code to rely on prices returned by the API so that we do not need to manually maintain our pricing maps as more DC-specific pricing regions are added.

Changes 🔄

  • Created the necessary api-v4 endpoint, query, and interface to use the nodebalancer/types endpoint.
  • Created a new util function, getDCSpecificPriceByType, to use data from the type object instead of from a constant pricing map.
  • Prices can be dynamically displayed for base region pricing and DC-specific region pricing, using the prices returned in the new endpoint, for this product.
  • The necessary factories and endpoints exist in the MSW to support a mocked response to the API endpoint
  • The NODEBALANCER_PRICE in utils/pricing/constants.ts for hardcoded prices is removed.

Preview 📷

The only visual change should be the disabled Create NodeBalancer button if a region is selected and the price is invalid (e.g. undefined).
Screenshot 2024-03-27 at 9 02 34 AM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Check out this PR and yarn up.
  • All new types endpoints are all in prod, so use the prod environment.

Verification steps

(How to verify changes)

  • Go to http://localhost:3000/nodebalancers/create.
  • Fill out the form.
  • Verify price calculation is correct by testing different options for the Region: base pricing ($10/month), Jakarta ($12/month), and Sao Paulo ($14/month).
    Confirm:
  • There are no regressions in decimal point value or calculations displayed in the UI.
  • $0 is a valid region price if returned from the API, the user sees $0 pricing, and a nodebalancer can be created. (Update the mock data to test this.)
  • A user can still create a NodeBalancer.
  • If price cannot be calculated (e.g. it is undefined), the Summary displays the unknown price ($--.--) string, the Create button is disabled, and a tooltip displays on hover. (Blocking network requests is one method.)
  • When a region is not yet selected, the Create button is still enabled. (Pricing can't be calculated yet; we don't know if the price is invalid.)

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

@mjac0bs mjac0bs force-pushed the M3-7831-use-nodebalancer-types-endpoint branch from 8eee1fa to 18393a4 Compare March 27, 2024 15:28
Comment on lines 40 to 46
export const typesQueries = createQueryKeys('types', {
nodebalancers: {
queryFn: getAllNodeBalancerTypes,
queryKey: null,
},
});

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 started transitioning this file to the query key factory with new types queries to keep the scope small, with the goal of adding additional keys with each /types PR. And I wasn't sure how to transition the linodes specific types query (queryClient ➡️ contextQueries?).

Query key changes don't have to be in this PR, and I could revert the query key factory change for now, to be refactored all at once in its own PR later.

@@ -15,6 +20,18 @@ export interface DataCenterPricingOptions {
regionId: Region['id'] | undefined;
}

export interface DataCenterPricingByTypeOptions {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually, we'll be able to remove the getDCSpecificPrice function and DataCenterPricingOptions interface and replace them with getDCSpecificPricingByType and DataCenterPricingByTypeOptions. We'll have both until all prices are being calculated using /types data, rather than constants we maintain.

@@ -115,3 +117,11 @@ export interface RequestHeaders {
'User-Agent'?: string;
'Content-Type'?: RequestContentType;
}

export interface PriceType {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Placed this new interface in this file because it's not specific to any one product... nodebalancers, volumes, obj, etc. /types endpoints will all use this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if those will be consistent with future price types for other entities? Otherwise they may just belong to NodeBalancersPriceTypes in
packages/api-v4/src/nodebalancers/nodebalancers.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They will be consistent.

Comment on lines 649 to 660
disabled={
(showGDPRCheckbox && !hasSignedAgreement) || isRestricted || !price
}
sx={{
flexShrink: 0,
mx: matchesSmDown ? theme.spacing(1) : null,
}}
buttonType="primary"
data-qa-deploy-nodebalancer
disabled={(showGDPRCheckbox && !hasSignedAgreement) || isRestricted}
loading={isLoading}
onClick={onCreate}
tooltipText={!price ? PRICE_ERROR_TOOLTIP_TEXT : ''}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only UI changes in this PR are to disable the Create button with a tooltip for context if, for any reason, the price is not available. (It will display as $--.-- in the summary.)

@mjac0bs mjac0bs marked this pull request as ready for review March 27, 2024 16:13
@mjac0bs mjac0bs requested a review from a team as a code owner March 27, 2024 16:13
@mjac0bs mjac0bs requested review from dwiley-akamai, abailly-akamai and bnussman-akamai and removed request for a team March 27, 2024 16:13
@mjac0bs mjac0bs added the @linode/api-v4 Changes are made to the @linode/api-v4 package label Mar 27, 2024
disabled={
(showGDPRCheckbox && !hasSignedAgreement) ||
isRestricted ||
isInvalidPrice
Copy link
Member

Choose a reason for hiding this comment

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

I kind of feel like we shouldn't disable the button if price isn't invalid, but because this will be a rare case, I don't think it's a huge deal either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feel like we shouldn't disable the button if price isn't invalid

I'm guessing you mean we shouldn't disable the button if the price is invalid, since that's the change here.

Going to add this as a topic at cafe today to get some UX feedback.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, my fault!

packages/manager/src/queries/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed UI, NB creation and new price types in creation flow ✅
Confirm error handling

Approving pending small code and coverage improvements
Screenshot 2024-03-28 at 11 08 17

packages/api-v4/src/nodebalancers/nodebalancers.ts Outdated Show resolved Hide resolved
packages/api-v4/src/types.ts Outdated Show resolved Hide resolved
@@ -115,3 +117,11 @@ export interface RequestHeaders {
'User-Agent'?: string;
'Content-Type'?: RequestContentType;
}

export interface PriceType {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if those will be consistent with future price types for other entities? Otherwise they may just belong to NodeBalancersPriceTypes in
packages/api-v4/src/nodebalancers/nodebalancers.ts?

packages/manager/src/queries/types.ts Outdated Show resolved Hide resolved
@mjac0bs mjac0bs added UX/UI Changes for UI/UX to review and removed Ready for Review labels Mar 28, 2024
@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Mar 28, 2024
Copy link

github-actions bot commented Mar 28, 2024

Coverage Report:
Base Coverage: 81.72%
Current Coverage: 81.74%

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Tested different options for the Region and observed the correct impacts on pricing -- base pricing ($10/month), Jakarta ($12/month), and Sao Paulo ($14/month) ✅

Observed no regressions in decimal point value or calculations displayed in the UI ✅

NodeBalancer flow functionality ✅

packages/api-v4/src/nodebalancers/nodebalancers.ts Outdated Show resolved Hide resolved
@mjac0bs
Copy link
Contributor Author

mjac0bs commented Apr 2, 2024

Merging this as is. Product is aware of this UX for the disabled create button and didn't request any changes to the behavior.

@mjac0bs mjac0bs merged commit 2a74280 into linode:develop Apr 2, 2024
18 checks passed
@mjac0bs mjac0bs removed the UX/UI Changes for UI/UX to review label Apr 2, 2024
bnussman-akamai pushed a commit to bnussman-akamai/manager that referenced this pull request Apr 4, 2024
…data (linode#10265)

* Replace nodebalancer constant pricing with /types endpoint pricing

* Add factory and MSW endpoint

* Remove price constant

* Clean up

* Disable Create button if pricing is unavailable

* Use query key factory for new types query

* Only disable Create button if region is valid but price is not

* Add changesets

* Fix typo

* Address feedback: types, unit test, move query

* Update the import statements I missed

* Gotta catch 'em all
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! DC-Specific Pricing @linode/api-v4 Changes are made to the @linode/api-v4 package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants