-
Notifications
You must be signed in to change notification settings - Fork 364
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-7380] - Use volumes/types
endpoint for pricing data
#10376
refactor: [M3-7380] - Use volumes/types
endpoint for pricing data
#10376
Conversation
packages/manager/src/features/Volumes/VolumeDrawer/LinodeVolumeCreateForm.tsx
Outdated
Show resolved
Hide resolved
<Notice | ||
text={ | ||
"You don't have permissions to create a new Volume. Please contact an account administrator for details." | ||
} | ||
important | ||
variant="error" |
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 cleaned up an unrelated inconsistency in our error notice. important
does not work without a variant
prop, and we shouldn't useimportant
here, anyway, to be consistent with how we displayed these errors elsewhere. I added the error
variant that was missing.
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 think we want to use the warning
variant for permissions banners, cause it's not technically an error
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 went back to check what we've done in other "Restricted User Access" PRs and it does look like we're using an error with this. I'm going to keep it an error here for consistency.
Billing access PR: #10201
Nodebalancer PR: #10095
If we want to discuss error vs warning for these restricted access notices, let's bring it up with UX in cafe, @abailly-akamai. (cc @jaalah-akamai)
@@ -32,7 +36,11 @@ export const PricePanel = ({ currentSize, regionId, value }: Props) => { | |||
|
|||
return ( | |||
<Box marginTop={4}> | |||
<DisplayPrice interval="mo" price={Number(price)} /> | |||
{isLoading ? ( |
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.
Added loading state for the display price.
Screen.Recording.2024-04-12.at.1.00.21.PM.mov
@@ -80,7 +85,12 @@ export const SizeField = (props: Props) => { | |||
</FormHelperText> | |||
); | |||
|
|||
const dynamicPricingHelperText = !resize && !isFromLinode && ( | |||
// The price display is only visible next to the size field on the Volumes Create page. |
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.
Did a little bit of renaming for clarity here and also added loading state to the price display for volumes create page to improve the slow network or error experience.
Screen.Recording.2024-04-12.at.12.58.39.PM.mov
@@ -81,6 +75,16 @@ describe('getDCSpecificPricingByType', () => { | |||
).toBe('14.00'); | |||
}); | |||
|
|||
it('calculates dynamic pricing for a volume based on size', () => { |
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.
We can remove getDynamicVolumePrice
entirely now that we have price data in the /types
endpoint; we just have to allow an optional size to be passed into getDCSpecificPriceByType
. Test coverage is updated here accordingly.
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.
Nice and clean!
Code is good and consistent ✅
UI Price display, error and loading handling look great ✅
Left a couple comments for consideration
packages/manager/.changeset/pr-10376-tech-stories-1712948758226.md
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Volumes/VolumeDrawer/LinodeVolumeCreateForm.tsx
Outdated
Show resolved
Hide resolved
<Notice | ||
text={ | ||
"You don't have permissions to create a new Volume. Please contact an account administrator for details." | ||
} | ||
important | ||
variant="error" |
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 think we want to use the warning
variant for permissions banners, cause it's not technically an error
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.
Awesome work! 🎉 Didn't spot any issues 👁️
Description 📝
API is now returning pricing data through the
/volumes/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 Volumes.
Changes 🔄
getDCSpecificPriceByType
to optionally accept a size param, if size is a factor of price and updated util unit test accordingly.create-volume-smoke.spec.ts
to test create volume error flows.Note: I did not update the resize or clone Cypress tests to test those error flows. These existing specs run slowly since they have to create a linode and I'm not sure it's worth it for an edge-case like this, so held off doing that and will talk to Joe when he returns.
Preview 📷
The only visual changes should be loading state for pricing and the disabled Create Volume, Resize Volume, and Clone Volume buttons if the price is invalid (e.g. undefined).
How to test 🧪
Prerequisites
(How to setup test environment)
yarn up
.types
endpoints are all in prod, so use the prod environment.Verification steps
(How to verify changes)
As an Author I have considered 🤔
Check all that apply