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

hotfix: DC-specific pricing -- monthly price decimals and invoice region column #9759

Merged
merged 11 commits into from
Oct 5, 2023

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Oct 5, 2023

Description 📝

There are two issues with DC-specific pricing:

  1. on past invoices, the Region column for invoice items is not backfilled due to how this is data is collected and populated on the back-end, which may cause confusion for users who have a region column with no data
  2. monthly prices (previously all integers, displayed as $n/mo) are now displaying inconsistently -- most often with one decimal place, but in at least one instance (resize node pool drawer, 2GB linode) with way too many decimal places

Major Changes 🔄

  • Fixes issue 1 by hiding the Region column on invoices before DC-specific pricing was implemented. This month's invoice and invoices going forward should include region data for invoice items.
  • Fixes issue 2 by displaying monthly prices returned from the API as decimals (at this point, only Jakarta and Sao Paulo) with two decimal places. Monthly prices that are integers (e.g. a 2GB linode at $10/month) should remain integers (not $10.00). We want to avoid adding extra zeroes on the screen (making the price look higher than it is) if it is not necessary. For regions with decimals in their monthly prices, we display to two decimal place rather than rounding or truncating to an integer because prices should be consistent with the prices listed in the linode.com docs.
  • Updates unit and cypress tests accordingly for both these fixes.

Preview 📷

Before After
Screenshot 2023-10-04 at 7 46 52 PM Screenshot 2023-10-04 at 7 46 25 PM
Screenshot 2023-10-04 at 7 47 13 PM Screenshot 2023-10-04 at 7 48 21 PM
Screenshot 2023-10-04 at 9 55 38 PM Screenshot 2023-10-04 at 7 48 32 PM
Screenshot 2023-10-04 at 10 00 23 PM Screenshot 2023-10-04 at 7 49 21 PM
Screenshot 2023-10-04 at 9 59 56 PM Screenshot 2023-10-04 at 7 49 41 PM
Screenshot 2023-10-04 at 7 55 45 PM Screenshot 2023-10-04 at 7 55 29 PM
Screenshot 2023-10-04 at 10 05 09 PM Screenshot 2023-10-04 at 10 04 48 PM

How to test 🧪

  1. How to setup test environment?
  • Check out this PR.
  • Have another tab open to compare to prod.
  1. How to reproduce the issue (if applicable)?
  • In prod, through the linode create flow, kube create flow, and kube node pools add/resize pool flows.
  • Observe you see monthly prices in the plans tables, summaries, and drawers with a weird number of decimal points.
  • Go to your latest account invoice on the billing page.
  • Observe you see a regions column but no regions listed.
  1. How to verify changes?
  • On localhost, go through the linode create flow, kube create flow, and kube node pools add/resize pool flows.
  • Observe you see monthly prices in the plans tables, summaries, and drawers are unchanged (no decimals) for resources outside of Jakarta or Sao Paulo.
  • Observe you see monthly prices in the plans tables, summaries, and drawers with two decimals for resources in Jakarta or Sao Paulo, which have DC-specific pricing.
  • Go to your latest account invoice on the billing page.
  • Observe you do not see a regions column any of the following: details page (invoice table), csv download, pdf download.
  • Edit the regions mock data in serverHandler.ts or the factory to include an invoice with a future date.
  • Observe that you do see the regions column in all of the following: details page (invoice table), csv download, pdf
  • If you want to confirm regions will show on a future invoice (like the end of this month) update the existing mock request to include a date of 2023-10-31T18:04:01 and the go to http://localhost:3000/account/billing/invoices/1234:
  rest.get('*/account/invoices/:invoiceId', (req, res, ctx) => {
    const linodeInvoice = invoiceFactory.build({
      date: '2022-12-01T18:04:01',
      id: 1234,
      label: 'LinodeInvoice',
    });
    return res(ctx.json(linodeInvoice));
  }),
  1. How to run Unit or E2E tests?
yarn test

e2es should pass, but I've been seeing issues with the LKE smoke tests locally due to some odd behavior with the region select scrolling out of view and returning no results after a selection is made -- that doesn't replicate in the UI, so not sure what is going on here.

@mjac0bs mjac0bs requested a review from a team as a code owner October 5, 2023 05:03
@mjac0bs mjac0bs self-assigned this Oct 5, 2023
@mjac0bs mjac0bs requested review from jaalah-akamai and abailly-akamai and removed request for a team October 5, 2023 05:03
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.

Looking good so far!

Comment on lines +92 to +93
export const MAGIC_DATE_THAT_DC_SPECIFIC_PRICING_WAS_IMPLEMENTED =
'2023-10-05 00:00:00Z';
Copy link
Member

@bnussman-akamai bnussman-akamai Oct 5, 2023

Choose a reason for hiding this comment

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

Does this have to exist in 2 places?

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 think so, unless you know of a way to import from src/utilities/pricing/constants? We do have a M3 ticket in the backlog to take a look at de-duplicating constants in cypress tests, which we've run into often in this file, as discussed in another ticket. It looks like I could import from @src/constants in a test file, but couldn't do the same with the pricing file without an error.

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.

Confirming all updates look good - but left a comment regarding the overall implementation of MAGIC_DATE_THAT_DC_SPECIFIC_PRICING_WAS_IMPLEMENTED

@mjac0bs mjac0bs changed the base branch from develop to staging October 5, 2023 15:29
@mjac0bs mjac0bs changed the title fix: DC-specific pricing -- monthly price decimals and invoice region column hotfix: DC-specific pricing -- monthly price decimals and invoice region column Oct 5, 2023
abailly-akamai
abailly-akamai previously approved these changes Oct 5, 2023
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.

Looks good!

  • pricing are showing properly in plan selection and summaries ✅
  • region is hidden ✅

abailly-akamai
abailly-akamai previously approved these changes Oct 5, 2023
abailly-akamai
abailly-akamai previously approved these changes Oct 5, 2023
cpathipa
cpathipa previously approved these changes Oct 5, 2023
Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

LGTM!

  • Validate Monthly Pricing columns rendering data as expect.
  • Validated Region column hidden.
  • Validated summary sections.

image

@bnussman-akamai
Copy link
Member

Looking good! I started the e2es locally so I'll let you know if I see an issues there

@bnussman-akamai
Copy link
Member

Looks like we're expecting 12.2 but we render 12.20 in create-linode.spec.ts

create-linode.spec.ts.mp4

@mjac0bs mjac0bs dismissed stale reviews from cpathipa and abailly-akamai via 4c4c0ef October 5, 2023 17:03
@mjac0bs
Copy link
Contributor Author

mjac0bs commented Oct 5, 2023

Fixed create-linode.spec.ts:
image

@mjac0bs mjac0bs merged commit d1d726c into linode:staging Oct 5, 2023
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.

4 participants