feat: [M3-7063] - Provide dynamic price error handling for DC-specific pricing by cpathipa · Pull Request #9660 · linode/manager

cpathipa · 2023-09-11T16:26:19Z

Description 📝

Handles pricing error scenario
Renders Errored state TableCell for price column
Enhanced TableCell component to handle errored state cell data

Preview 📷

Add a Node Pool Drawer	Resize Pool

Add Node Pools	Migrate Linode

Linodes Plan panel	Resize Linode

Enable Backups Modal

How to test 🧪

To test this locally, we need to tweak the mock API response or modify the code accordingly for scenarios where regionId | type | price are not available. (Refer to the ticket for context.)
Validate all the components that render dynamic pricing, pricing column in plan selection table's (example: getDCSpecificPrice).
More specifically:

Here is a list of DC-specific pricing PRs. When testing this PR, first ensure that there are no regressions from prod in pricing behavior matches prod in all cases where there are NOT errors. (PRs that were copy changes, not price calculations, are already checked off.)
Then verify the error handling in the screenshots above is triggered when prices cannot be calculated correctly:

Verify e2e tests.

…not be determined

packages/manager/src/components/Currency/Currency.test.tsx

packages/manager/src/utilities/pricing/backups.ts

packages/manager/src/utilities/pricing/dynamicPricing.ts

packages/manager/src/utilities/pricing/kubernetes.ts

mjac0bs

For others who may be looking at this PR: there is an associated UX ticket (UX-427) discussion and we're going to incorporate those changes here.

That should result in some clean up in components like Currency and DisplayPrice, so the price doesn't have to accept a string. And in dynamicPricing.ts, we'd return undefined instead of unknown.

In places where we have display text with "unknown", (e.g. the resize pool component), we can display an error message like below, rather than rendering something like Resized pool: $unknown/month.

For table cells with pricing data, where we'd fall back on unknown if there was an error, we will instead display $--.-- with an tooltip error icon. This could be a new ErrorCell component that accepts the error text and tooltip text as props.

(Formatting subject to change so wrapping issue would be fixed.)

packages/manager/src/components/Currency/Currency.tsx

packages/manager/src/features/Kubernetes/KubeCheckoutBar/NodePoolSummary.tsx

...er/src/features/Kubernetes/KubernetesClusterDetail/NodePoolsDisplay/ResizeNodePoolDrawer.tsx

jdamore-linode · 2023-10-17T13:56:27Z

Because the functions are now checking if type exists and returning undefined if not, I had to add a type id to the mock DC pricing linode types.

Ah, nice catch @mjac0bs! I'll circle back and take another look soon

I also updated the total. I don't understand how we were passing that test with $7.74 before... the sum of those three values is $9.74. 😅

Haha I noticed that too -- I'm guessing I added it up before adding the $2.00 entry to the list... or maybe I'm just bad at basic math 😅

jdamore-linode · 2023-10-17T13:57:53Z

(@mjac0bs Should've mentioned above, but all of the failures can be disregarded!)

abailly-akamai

Thanks for all your work on this!

It's hard to test every scenario but I could confirm most of them except for volumes create but I think it's because I was using null values for monthly & hourly VS an undefined data set.

coliu-akamai · 2023-10-19T13:59:53Z

bnussman-akamai · 2023-10-19T14:12:52Z

packages/manager/src/features/NodeBalancers/NodeBalancerCreate.tsx

+    summaryItems.unshift({
+      title: `$${
+        renderMonthlyPriceToCorrectDecimalPlace(Number(price)) ?? UNKNOWN_PRICE
+      }/month`,
+    });


I understand the intention here, but I'm not a fan of the extra complexity. Because the NodeBalancer price is hard-coded I don't see why we'd have to handle an unknown price.

Also, I'm not sure if the fallback logic makes sense here.

Suggested change

summaryItems.unshift({

title: `$${

renderMonthlyPriceToCorrectDecimalPlace(Number(price)) ?? UNKNOWN_PRICE

}/month`,

});

summaryItems.unshift({

title: `$${

renderMonthlyPriceToCorrectDecimalPlace(Number(price))

}/month`,

});

Also, it looks like renderMonthlyPriceToCorrectDecimalPlace won't properly handle a service that is $0. (not that we're going to many free products)

Yeah, you're right that we really wouldn't end up needing to handle displaying an unknown price in this case because although price can be undefined, we're only displaying price in the summary once there is a valid region.
(And other than that, the fallback logic was just unnecessary there with the render function, and I probably just wasn't paying attention when doing some clean up, and missed it.)

As for:

looks like renderMonthlyPriceToCorrectDecimalPlace won't properly handle a service that is $0. (not that we're going to many free products)

In this case and in any other instances where we're using this in Cloud, I wouldn't expect these products to be free. Unless there's a likely situation that we'd encounter that, I think it's safer to leave as is so we don't risk inaccurately advertising free products.

packages/manager/src/utilities/pricing/linodes.ts

coliu-akamai · 2023-10-19T15:59:44Z

Awesome work! Tested the functionality, had the following questions:

couldn't get the region column in the Invoice label to show up -- thinking this might be because of the magic date constant thing though, as I can see the code for the region column is there
For Kubernetes node pool, just wanted to confirm that there shouldn't be any error icons (could also be an issue with my MSW which has happened before 😅 ) ? I see this after changing nanodeType in serverHandlers.ts (line 470) to include region_prices object with null monthly/hourly prices, and modifying the kubernetesAPIResponse and kubernetesClusterFactory factories and replaces region: 'us-central' with region: 'id-cgk'.
- Alternatively, are there additional steps I should follow?

bnussman-akamai

Didn't find any regressions on any of the flows I checked!

jaalah-akamai · 2023-10-19T16:49:26Z

Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>

mjac0bs · 2023-10-19T20:58:14Z

For Kubernetes node pool, just wanted to confirm that there shouldn't be any error icons (could also be an issue with my MSW which has happened before 😅 ) ? I see this after changing nanodeType in serverHandlers.ts (line 470) to include region_prices object with null monthly/hourly prices, and modifying the kubernetesAPIResponse and kubernetesClusterFactory factories and replaces region: 'us-central' with region: 'id-cgk'.

@coliu-akamai Thanks for raising this and including your repro steps -- super helpful and I appreciate it! You were right; this was missed, because I forgot that the Kube plan table is a separate component. It has been fixed now. I've also disabled the add button if a row has a pricing error (not visible in this screenshot; made that change afterwards).

Using the same mock/factory edits quoted above, just returning null for hourly:

Modifying price to return undefined in code:

mjac0bs

My review doesn't really count here, but @cpathipa - all feedback has been addressed and this should be good to merge once the e2es finish.

coliu-akamai · 2023-10-19T21:37:57Z

@mjac0bs awesome thanks! One other quick thing, the add button for the Shared CPU row on the Create Kubernetes flow doesn't get disabled (errors showing up now though! 🎉):
(To reproduce, same setup as above + then on the /kubernetes/create page, choose Jakarta)

mjac0bs · 2023-10-20T13:50:30Z

One other quick thing, the add button for the Shared CPU row on the Create Kubernetes flow doesn't get disabled (errors showing up now though! 🎉)

@coliu-akamai - Thanks, fixed now in 4646b77. Also made a minor change to only disable + and Add buttons if the monthly price is not available, since that's what we use to calculate the totals in our drawers and create flow.

mjac0bs · 2023-10-20T14:24:45Z

The last e2e run failed for unrelated reasons: a database-create spec and a pipeline error (one that prevents the entire runner from executing tests). I ran lke-update.spec.ts, lke-create.spec.ts, and smoke-create-nodebal.spec.ts locally, since those are the flows where changes have occurred since all (non-DB tests) ran and passed. Local tests passed. I'm going to merge. (cc @cpathipa)

feat: [M3-7063] - Provide error handling when DC-specific pricing can…

8449fb1

…not be determined

cpathipa added the Work in Progress label Sep 11, 2023

cpathipa self-assigned this Sep 11, 2023

cpathipa marked this pull request as draft September 11, 2023 16:26

cpathipa added 4 commits September 12, 2023 09:24

Code clean up

9549003

Merge remote-tracking branch 'origin/develop' into M3-7063

d9071f4

Adjust unit tests

042de12

Update Currency.test.tsx

1e005d2

cpathipa marked this pull request as ready for review September 12, 2023 17:03

mjac0bs self-requested a review September 12, 2023 18:20

mjac0bs added DC-Specific Pricing UX/UI Changes for UI/UX to review labels Sep 12, 2023

Update kubernetes.test.tsx

3f6cd69

cpathipa requested a review from a team as a code owner September 19, 2023 18:14

cpathipa requested review from cliu-akamai and jaalah-akamai and removed request for a team September 19, 2023 18:14

cpathipa commented Sep 19, 2023

View reviewed changes

cpathipa added 3 commits September 19, 2023 13:18

Merge remote-tracking branch 'origin/develop' into M3-7063

0fa33a6

Update linodes.test.ts

99d1297

Delete linodes.test.tsx

552eb9c

cpathipa added Ready for Review and removed Work in Progress labels Sep 19, 2023

mjac0bs reviewed Sep 20, 2023

View reviewed changes

cpathipa marked this pull request as draft September 21, 2023 14:28

cpathipa added Work in Progress and removed Ready for Review labels Sep 21, 2023

jaalah-akamai reviewed Sep 21, 2023

View reviewed changes

cpathipa added 2 commits September 21, 2023 11:55

Merge remote-tracking branch 'origin/develop' into M3-7063

241cd20

Code cleanup

047360b

mjac0bs mentioned this pull request Oct 17, 2023

test: [M3-7071] - Add Cypress integration tests for NodeBalancer Create DC-specific pricing #9765

Merged

abailly-akamai approved these changes Oct 17, 2023

View reviewed changes

cpathipa added the Add'tl Approval Needed Waiting on another approval! label Oct 17, 2023

cpathipa requested a review from mjac0bs October 19, 2023 13:53

Merge remote-tracking branch 'origin/develop' into M3-7063

df760f6

bnussman-akamai reviewed Oct 19, 2023

View reviewed changes

packages/manager/src/utilities/pricing/linodes.ts Show resolved Hide resolved

bnussman-akamai approved these changes Oct 19, 2023

View reviewed changes

cpathipa and others added 5 commits October 19, 2023 12:03

Update packages/manager/src/utilities/pricing/linodes.ts

b93982d

Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>

Merge branch 'develop' into M3-7063

ac0b29c

Update smoke-create-nodebal.spec.ts rendering of price

0338b09

Address feedback - remove unnecessary fallback

8f08a48

Address feedback - missing error icon

1102555

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 19, 2023

mjac0bs removed the Ready for Review label Oct 19, 2023

mjac0bs approved these changes Oct 19, 2023

View reviewed changes

Address feedback - disable Add button on pricing error

4646b77

mjac0bs merged commit 0fb7b36 into linode:develop Oct 20, 2023

This was referenced Oct 26, 2023

Release v1.106.0 - release → staging #9842

Merged

Release v1.106.0 - staging → master #9850

Merged

Conversation

cpathipa commented Sep 11, 2023 • edited by mjac0bs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Preview 📷

How to test 🧪

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjac0bs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jdamore-linode commented Oct 17, 2023

Uh oh!

jdamore-linode commented Oct 17, 2023

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

coliu-akamai commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnussman-akamai Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

mjac0bs Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coliu-akamai commented Oct 19, 2023

Uh oh!

bnussman-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

jaalah-akamai commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjac0bs commented Oct 19, 2023

Uh oh!

mjac0bs left a comment

Choose a reason for hiding this comment

Uh oh!

coliu-akamai commented Oct 19, 2023

Uh oh!

mjac0bs commented Oct 20, 2023

Uh oh!

mjac0bs commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cpathipa commented Sep 11, 2023 •

edited by mjac0bs

Loading

coliu-akamai commented Oct 19, 2023 •

edited

Loading

jaalah-akamai commented Oct 19, 2023 •

edited

Loading