Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

refactor: Modify initial loading state for better control of subs section #356

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

julianajlk
Copy link
Contributor

@julianajlk julianajlk commented Dec 14, 2023

REV-3799.

There was a pre-existing bug that became apparent when subscriptions was introduced and sunset, since we would show the subscriptions section on a conditional - loading's initial state is false, and this caused the component to render before re-rendering in the loading state when that becomes true.

When isB2CSubsEnabled is true:
isLoading is initially false, which makes the page to go into renderOrdersandSubscriptions() (from isLoading ? renderLoading() : renderOrdersandSubscriptions()), which renders <Subscriptions/> and <OrderHistory/> components. Once the fetching of orders and subscriptions happen, isLoading changes to true, and the state change makes the component re-render with or without the subscriptions heading depending on whether the user has subs or not.

In this PR I've changed the loading initial state to start with true for both order-history and subscriptions.
I've also refactored the rendering logic to better accommodate the following scenarios:

  1. Subscriptions enabled and user has 0 subscriptions -> render Order History only
  2. Subscriptions enabled and user has subscriptions -> render main heading, Subscriptions and Order History.
  3. Subscriptions are disabled -> render Order History only

@julianajlk julianajlk requested a review from a team as a code owner December 14, 2023 20:57
@julianajlk julianajlk force-pushed the julianajlk/REV-3799/order-history-heading-subs-bug branch 2 times, most recently from 62d91a1 to 440e61a Compare December 15, 2023 03:35
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5cec05d) 61.53% compared to head (5f4e7f8) 63.58%.

Files Patch % Lines
...s-and-subscriptions/OrdersAndSubscriptionsPage.jsx 88.23% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #356      +/-   ##
==========================================
+ Coverage   61.53%   63.58%   +2.04%     
==========================================
  Files          34       34              
  Lines         325      335      +10     
  Branches       69       74       +5     
==========================================
+ Hits          200      213      +13     
+ Misses        118      115       -3     
  Partials        7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@julianajlk julianajlk force-pushed the julianajlk/REV-3799/order-history-heading-subs-bug branch 4 times, most recently from 555c5a5 to aa335aa Compare December 15, 2023 17:03
@julianajlk julianajlk force-pushed the julianajlk/REV-3799/order-history-heading-subs-bug branch from 0ff5a62 to fbceaa6 Compare December 15, 2023 17:38
Copy link
Contributor

@NawfalAhmed NawfalAhmed left a comment

Choose a reason for hiding this comment

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

LGTM

@julianajlk julianajlk merged commit 2a35e69 into master Dec 19, 2023
6 checks passed
@julianajlk julianajlk deleted the julianajlk/REV-3799/order-history-heading-subs-bug branch December 19, 2023 15:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants