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-7382] - Use object-storage/types endpoint for pricing data #10468

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented May 14, 2024

Description 📝

API is now returning pricing data through the /object-storage/types endpoint.

In this PR, prices can be dynamically displayed for base region pricing and DC-specific region pricing, using the prices returned in the new endpoint, for Object Storage.

Note

Outbound transfer overages will be done in a follow-up PR, since pricing for network transfer overages is the same across Cloud Manager and not currently being returned by the API. They are creating a new endpoint for this.

Changes 🔄

  • Created the necessary api-v4 endpoint, query, and interface to use the object-storage/types endpoint.
  • Modified getDCSpecificPriceByType to optionally accept interval (hourly or monthly) and decimal precision params and updated util unit test accordingly.
  • Created the necessary factories and endpoint in the MSW to support a mocked response to the API endpoint.
  • Added UX for error handling in the unlikely event that pricing cannot be calculated: disable primary action button and display tooltip error.
  • Added loading state for prices.
  • Updated unit tests.

Target release date 🗓️

5/28/24

Preview 📷

Before After
Screenshot 2024-05-17 at 12 50 05 AM Screenshot 2024-05-17 at 12 50 23 AM Screenshot 2024-05-17 at 12 50 33 AM Screenshot 2024-05-20 at 8 08 52 AM Screenshot 2024-05-17 at 12 47 38 AM Screenshot 2024-05-17 at 12 47 49 AM
N/A
Screen.Recording.2024-05-17.at.12.48.28.AM.mov
Screenshot 2024-05-17 at 12 50 46 AM Screenshot 2024-05-17 at 12 46 48 AM
Screenshot 2024-05-17 at 12 51 03 AM Screenshot 2024-05-17 at 12 47 10 AM

How to test 🧪

Prerequisites

(How to setup test environment)

Verification steps

(How to verify changes)

  • Go to http://localhost:3000/object-storage/buckets/create.
  • Select a region without DC specific pricing (e.g. us-east).
  • Open the network tab and filter on /types. Confirm the "additional storage overage" pricing is consistent with the overage pricing returned by the API endpoint AND that price is consistent with prod.
  • Repeat the same process for Sao Paulo (br-gru) and Jakarta (id-cgk).
  • Fill out the necessary fields, click Create Bucket, and confirm the Enable Object Storage modal pops up with no regressions. Cancel out of the modal - don't enable OBJ.
  • Use the Network tools to slow and block network requests - confirm that loading and error behavior is handled gracefully in the Create Bucket drawer if prices can't be retrieved from the object-storage/types endpoint.
  • Go to http://localhost:3000/object-storage/access-keys.
  • Fill out the necessary fields, click Create Access Key, and confirm the Enable Object Storage modal pops up with no regressions. Cancel out of the modal - don't enable OBJ.
  • Ensure object storage e2e tests pass.
  • Ensure updated unit tests pass:
yarn test OveragePricing dynamicPricing EnableObjectStorageModal

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-7382-add-dynamic-pricing-for-object-storage branch from 2d092a7 to c1d423a Compare May 15, 2024 19:32
@mjac0bs mjac0bs marked this pull request as ready for review May 20, 2024 15:31
@mjac0bs mjac0bs requested a review from a team as a code owner May 20, 2024 15:31
@mjac0bs mjac0bs requested review from jdamore-linode and bnussman-akamai and removed request for a team May 20, 2024 15:31
@mjac0bs mjac0bs requested a review from a team as a code owner May 20, 2024 15:35
@mjac0bs mjac0bs requested review from cliu-akamai and removed request for a team May 20, 2024 15:35
@mjac0bs mjac0bs added @linode/api-v4 Changes are made to the @linode/api-v4 package Ready for Review labels May 20, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of the other object-storage endpoint files seemed like a good fit for this, so I created a new file, prices.ts. I didn't name it types.ts, since that's another file. This seemed like the most straight-forward way to add the new endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these constants are old and can be cleaned up. They were leftovers from earlier stages of the project, pre-full GA.

? objectStoragePriceIncreaseMap[regionId].storage_overage
: OBJ_STORAGE_PRICE.storage_overage}{' '}
{storageOveragePrice && !isError
? parseFloat(storageOveragePrice)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using parseFloat here to truncate trailing zeros - e.g. we will display $0.02 and not $0.020, but otherwise display to 3 decimal places for the other overage prices.

Comment on lines +94 to +108
it('displays placeholder unknown pricing and disables the primary action button if pricing is not available', () => {
queryMocks.useObjectStorageTypesQuery.mockReturnValue({
data: undefined,
isError: true,
});

const { getByTestId, getByText } = render(
wrapWithTheme(<EnableObjectStorageModal regionId="us-east" {...props} />)
);

const primaryActionButton = getByTestId('enable-obj');

expect(getByText(`${UNKNOWN_PRICE}/month`, { exact: false })).toBeVisible();
expect(primaryActionButton).toBeDisabled();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting the UI in this state would require manually manipulating the code, since you can't Create Bucket if prices are invalid, and therefore would not be able to get to this modal. We can confirm error state for invalid pricing in the Enable Object Storage modal works with this test case passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates to this function were to account for the object storage storage overages, which are displayed by their hourly rate. Previously, this util was not flexible enough and calculated only monthly prices and only to a fixed 2 decimal places.

Copy link

github-actions bot commented May 20, 2024

Coverage Report:
Base Coverage: 81.62%
Current Coverage: 81.63%

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.

Confirmed pricing matches production ✅

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label May 21, 2024
@mjac0bs mjac0bs requested a review from carrillo-erik May 22, 2024 19:03
monthly: 5.0,
},
],
transfer: 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not specific to your work, just curious to know what transfer represents in the payload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the 1TB transfer quota (added to the global network transfer pool) that comes with enabling Object Storage. (Docs)

queryFn: getAllObjectStorageTypes,
queryKey: [queryKey, 'types'],
...queryPresets.oneTimeFetch,
enabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we determine whether enabled should be true or false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the query hook is called, we can pass in a boolean value to indicate whether the query should be enabled or not (see here and here). Otherwise, by default, it is enabled, so we don't need to pass anything here.

Copy link
Contributor

@carrillo-erik carrillo-erik left a comment

Choose a reason for hiding this comment

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

Verified the functionality works as expected and test passed locally. I left two questions/comments but those are for my own knowledge. ✅

@@ -19,13 +19,15 @@ import {
useCreateBucketMutation,
useObjectStorageBuckets,
useObjectStorageClusters,
useObjectStorageTypesQuery,
} from 'src/queries/objectStorage';
import { useProfile } from 'src/queries/profile';
import { useRegionsQuery } from 'src/queries/regions/regions';
import { isFeatureEnabled } from 'src/utilities/accountCapabilities';
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed this, isFeatureEnabled has been deprecated in favor of isFeatureEnabledV2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Out of scope of this PR, though.

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels May 23, 2024
@mjac0bs mjac0bs merged commit f99456f into linode:develop May 23, 2024
18 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! 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.

3 participants