fix: resolves refetching issue resulting in premature FullscreenModal…

… closure (#1147)

* fix: example possible fix, dont pass isFetchingBudgets in EnterpriseSubsidiesContext

* fix: removes budgets from context in favor of useQuery

* feat: ADR on intentional usage frontend state and react-query cache

* chore: ADR update

* fix: Update ADR

---------

Co-authored-by: Hamzah Ullah <[email protected]>

docs/decisions/0008-application_state_differenciation.rst

@@ -0,0 +1,86 @@
+8. Intentional separation between local application state and API data state using ``@tanstack/react-query``
+============================================================================================================
+
+Status
+******
+
+Accepted (January 2024)
+
+Context
+*******
+
+The ``frontend-app-admin-portal`` currently implements ``@tanstack/react-query`` predominantly within the ``learner-credit-management`` feature. ``@tanstack/react-query`` is a tool used to manage server state in react applications.
+
+See `0006-tanstack-react-query ADR <https://github.com/openedx/frontend-app-admin-portal/blob/master/docs/decisions/0006-tanstack-react-query.rst>`_ for more information
+
+As part of its implementation, a pattern has emerged of API data from the ``react-query`` output from ``useQuery`` is being populated within context providers. This has resulted in the unintended consequence of component re-renders when the top-level component context data is updated with the latest data from the ``useQuery`` output.
+
+The re-fetch behavior of ``react-query`` results in components that aren't dependent on the updated context data to re-render.
+
+The re-fetch behavior was occurring in the background of a component while the user is interacting with the page or if they had returned focus back onto the browser while interacting with other tabs or applications.
+
+An example where populating context data from ``react-query`` output data data resulted in unintentional behavior is the ``EnterpriseSubsidiesContext`` containing API data from the query key 'learner-credit-management.budgets', ``isFetchingBudgets`` attribute which has since been fixed.
+
+While the user was interacting with a ``FullscreenModal`` component and had not completed their intended activity within the modal before the global ``staleTime`` had expired, the re-fetch behavior would trigger resulting in a re-render of all components at the same component tree lvel and below, thus closing the modal without saving the users state information (form data etc.).
+
+Decisions
+*********
+
+We will take a more intentional approach when implementing API data fetching using the ``react-query`` in the following ways:
+
+#. Avoid adding API data fetched using ``react-query`` to a context layer along with related lifecycle state data related to ``useQuery``. See the `useQuery documentation <https://tanstack.com/query/v4/docs/react/reference/useQuery>`_ for more information
+ * If a context layer does require API data to exist in the context layer, it is not recommended to use ``react-query`` to retrieve the data
+#. When using ``useQuery`` implement its usage within the component that uses the returned response data
+ * Because the output data is already cached, there is no need to add to the context makes it redundant
+#. Leverage the `query key factory <https://github.com/openedx/frontend-app-admin-portal/blob/c67c5e4d8a0328fe75cb9d46791a8b733fad8257/src/components/learner-credit-management/data/constants.js#L67-L77>`_ pattern to simplify boilerplate code when making the `useQuery` calls and `invalidateQueries`
+#. Implement a hook when calling the ``useQuery`` method to avoid duplicate code and easy maintainability. See this `hook <https://github.com/openedx/frontend-app-admin-portal/blob/c67c5e4d8a0328fe75cb9d46791a8b733fad8257/src/components/EnterpriseSubsidiesContext/data/hooks.js#L108-L120>`_ as boilerplate as an example, there are `others to reference in the repo <https://github.com/search?q=repo%3Aopenedx%2Ffrontend-app-admin-portal+%2FuseQuery%5C%28%2F&type=code>`_ as additional guides
+#. Invalidate queries or update the the query state when user interaction results in an API data change to avoid stale data
+ * If the query is not invalidated, the re-fetching of the data is entirely dependent on the length of time set for the ``staleTime``
+ * We can also leverage optimistic updates to set the query state within a ``useMutation`` hook. See `here <https://tanstack.com/query/v4/docs/react/guides/optimistic-updates>`_ for more information
+
+Consequences
+************
+
+Based on the call you're making, it may be challenging to coalesce all the required metadata required to make a ```useQuery``.
+
+When writing tests for a hook that leverages useQuery, certain boilerplate code is required. The important fact to highlight is the necessity of wrapping your test component in a ``QueryClientProvider`` component to adequately test the component.
+Below is an example of how writing tests with a useQuery directly in the component in the form of a hook, where the ``props`` of the component come from the ``portalConfiguration``.
+
+Note that the ``queryClient`` is a test utility function that populates the ``QueryClientProvider`` client prop. See its implementation `here <https://github.com/openedx/frontend-app-admin-portal/blob/c67c5e4d8a0328fe75cb9d46791a8b733fad8257/src/components/test/testUtils.jsx#L47-L56>`_
+
+.. code-block:: javascript
+
+ import { QueryClientProvider } from '@tanstack/react-query';
+ import { queryClient } from '../../test/testUtils';
+ import { Provider } from 'react-redux';
+ import configureMockStore from 'redux-mock-store';
+ import thunk from 'redux-thunk';
+
+ // Configure the portal configuration used in the Provider
+ const mockStore = configureMockStore([thunk]);
+ const getMockStore = store => mockStore(store);
+ const initialStore = {
+ portalConfiguration: {
+ enterpriseId: 'test-id',
+ enterpriseFeatures: {
+ topDownAssignmentRealTimeLcm: true,
+ },
+ enablePortalLearnerCreditManagementScreen: true,
+ },
+ };
+
+ // Create your component wrapper
+ const TestComponentWrapper = () => (
+ <QueryClientProvider client={queryClient()}>
+ <Provider store={store}>
+ <TestComponent />
+ </Provider>
+ </QueryClientProvider>
+ );
+
+
+Alternatives Considered
+***********************
+
+Other alternatives to were to selectively disable the re-fetch behavior on API calls, or increase the global ``staleTime`` to a longer interval to avoid the re-render disrupting the user experience.
+Another solution would be to use context selectors to reduce the effect of components unintentionally re-rendering by specifically re-rendering only the related context we were referencing.

src/components/EnterpriseSubsidiesContext/index.jsx

@@ -11,7 +11,6 @@ export const useEnterpriseSubsidiesContext = ({
 }) => {
  const {
  isLoading: isLoadingBudgets,
- isFetching: isFetchingBudgets,
  data: budgetsOverview,
  } = useEnterpriseBudgets({
  enablePortalLearnerCreditManagementScreen,
@@ -54,21 +53,17 @@ export const useEnterpriseSubsidiesContext = ({
  const isLoading = isLoadingBudgets || isLoadingCustomerAgreement || isLoadingCoupons;
 
  const context = useMemo(() => ({
- budgets,
  customerAgreement,
  coupons,
  canManageLearnerCredit,
  enterpriseSubsidyTypes,
  isLoading,
- isFetchingBudgets,
  }), [
- budgets,
  customerAgreement,
  coupons,
  canManageLearnerCredit,
  enterpriseSubsidyTypes,
  isLoading,
- isFetchingBudgets,
  ]);
 
  return context;

src/components/learner-credit-management/MultipleBudgetsPage.jsx

@@ -17,15 +17,26 @@ import MultipleBudgetsPicker from './MultipleBudgetsPicker';
 import { EnterpriseSubsidiesContext } from '../EnterpriseSubsidiesContext';
 
 import { configuration } from '../../config';
+import { useEnterpriseBudgets } from '../EnterpriseSubsidiesContext/data/hooks';
 
 const PAGE_TITLE = 'Learner Credit Management';
 
 const MultipleBudgetsPage = ({
  enterpriseUUID,
  enterpriseSlug,
  enableLearnerPortal,
+ enterpriseFeatures,
+ enablePortalLearnerCreditManagementScreen,
 }) => {
- const { budgets, isLoading } = useContext(EnterpriseSubsidiesContext);
+ const { isLoading } = useContext(EnterpriseSubsidiesContext);
+ const { data: budgetsOverview } = useEnterpriseBudgets({
+ enterpriseId: enterpriseUUID,
+ enablePortalLearnerCreditManagementScreen,
+ isTopDownAssignmentEnabled: enterpriseFeatures.topDownAssignmentRealTimeLcm,
+ });
+ const {
+ budgets = [],
+ } = budgetsOverview || {};
 
  if (isLoading) {
  return (
@@ -86,12 +97,18 @@ const mapStateToProps = state => ({
  enterpriseUUID: state.portalConfiguration.enterpriseId,
  enterpriseSlug: state.portalConfiguration.enterpriseSlug,
  enableLearnerPortal: state.portalConfiguration.enableLearnerPortal,
+ enterpriseFeatures: state.portalConfiguration.enterpriseFeatures,
+ enablePortalLearnerCreditManagementScreen: state.portalConfiguration.enablePortalLearnerCreditManagementScreen,
 });
 
 MultipleBudgetsPage.propTypes = {
  enterpriseUUID: PropTypes.string.isRequired,
  enterpriseSlug: PropTypes.string.isRequired,
  enableLearnerPortal: PropTypes.bool.isRequired,
+ enterpriseFeatures: PropTypes.shape({
+ topDownAssignmentRealTimeLcm: PropTypes.bool,
+ }).isRequired,
+ enablePortalLearnerCreditManagementScreen: PropTypes.bool.isRequired,
 };
 
 export default connect(mapStateToProps)(MultipleBudgetsPage);

src/components/learner-credit-management/SubBudgetCard.jsx

@@ -1,6 +1,6 @@
-import { useContext } from 'react';
 import { Link } from 'react-router-dom';
 import PropTypes from 'prop-types';
+import { connect } from 'react-redux';
 import dayjs from 'dayjs';
 import classNames from 'classnames';
 import {
@@ -15,14 +15,43 @@ import {
 
 import { BUDGET_STATUSES, ROUTE_NAMES } from '../EnterpriseApp/data/constants';
 import { formatPrice, getBudgetStatus } from './data/utils';
-import { EnterpriseSubsidiesContext } from '../EnterpriseSubsidiesContext';
+import { useEnterpriseBudgets } from '../EnterpriseSubsidiesContext/data/hooks';
 
-const BackgroundFetchingWrapper = ({ children }) => {
- const { isFetchingBudgets } = useContext(EnterpriseSubsidiesContext);
+const BaseBackgroundFetchingWrapper = ({
+ enterpriseId,
+ isTopDownAssignmentEnabled,
+ enablePortalLearnerCreditManagementScreen,
+ children,
+}) => {
+ const { isFetching: isFetchingBudgets } = useEnterpriseBudgets({
+ enablePortalLearnerCreditManagementScreen,
+ enterpriseId,
+ isTopDownAssignmentEnabled,
+ });
  return <span style={{ opacity: isFetchingBudgets ? 0.5 : 1 }}>{children}</span>;
 };
 
-const SubBudgetCard = ({
+BaseBackgroundFetchingWrapper.propTypes = {
+ children: PropTypes.node.isRequired,
+ enterpriseId: PropTypes.string.isRequired,
+ isTopDownAssignmentEnabled: PropTypes.bool,
+ enablePortalLearnerCreditManagementScreen: PropTypes.bool.isRequired,
+};
+
+BaseBackgroundFetchingWrapper.defaultProps = {
+ isTopDownAssignmentEnabled: false,
+};
+
+const mapStateToProps = state => ({
+ enterpriseId: state.portalConfiguration.enterpriseId,
+ enablePortalLearnerCreditManagementScreen: state.portalConfiguration.enablePortalLearnerCreditManagementScreen,
+ isTopDownAssignmentEnabled: state.portalConfiguration.isTopDownAssignmentEnabled,
+ enterpriseFeatures: state.features,
+});
+
+const BackgroundFetchingWrapper = connect(mapStateToProps)(BaseBackgroundFetchingWrapper);
+
+const BaseSubBudgetCard = ({
  id,
  start,
  end,
@@ -31,10 +60,17 @@ const SubBudgetCard = ({
  spent,
  displayName,
  enterpriseSlug,
+ enterpriseId,
+ isTopDownAssignmentEnabled,
+ enablePortalLearnerCreditManagementScreen,
  isLoading,
  isAssignable,
 }) => {
- const { isFetchingBudgets } = useContext(EnterpriseSubsidiesContext);
+ const { isFetching: isFetchingBudgets } = useEnterpriseBudgets({
+ enablePortalLearnerCreditManagementScreen,
+ enterpriseId,
+ isTopDownAssignmentEnabled,
+ });
  const budgetLabel = getBudgetStatus(start, end);
  const formattedDate = dayjs(budgetLabel?.date).format('MMMM D, YYYY');
 
@@ -120,12 +156,11 @@ const SubBudgetCard = ({
  );
 };
 
-BackgroundFetchingWrapper.propTypes = {
- children: PropTypes.node.isRequired,
-};
-
-SubBudgetCard.propTypes = {
+BaseSubBudgetCard.propTypes = {
  enterpriseSlug: PropTypes.string.isRequired,
+ enterpriseId: PropTypes.string.isRequired,
+ isTopDownAssignmentEnabled: PropTypes.bool,
+ enablePortalLearnerCreditManagementScreen: PropTypes.bool.isRequired,
  id: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
  start: PropTypes.string,
  end: PropTypes.string,
@@ -137,4 +172,8 @@ SubBudgetCard.propTypes = {
  isAssignable: PropTypes.bool,
 };
 
-export default SubBudgetCard;
+BaseSubBudgetCard.defaultProps = {
+ isTopDownAssignmentEnabled: false,
+};
+
+export default connect(mapStateToProps)(BaseSubBudgetCard);

src/components/learner-credit-management/tests/BudgetCard.test.jsx

@@ -11,11 +11,19 @@ import {
 import '@testing-library/jest-dom/extend-expect';
 
 import { IntlProvider } from '@edx/frontend-platform/i18n';
+import { QueryClientProvider } from '@tanstack/react-query';
 import BudgetCard from '../BudgetCard';
 import { formatPrice, useSubsidySummaryAnalyticsApi, useBudgetRedemptions } from '../data';
 import { BUDGET_TYPES } from '../../EnterpriseApp/data/constants';
 import { EnterpriseSubsidiesContext } from '../../EnterpriseSubsidiesContext';
+import { queryClient } from '../../test/testUtils';
 
+jest.mock('../../EnterpriseSubsidiesContext/data/hooks', () => ({
+ ...jest.requireActual('../../EnterpriseSubsidiesContext/data/hooks'),
+ useEnterpriseBudgets: jest.fn().mockReturnValue({
+ isFetchingBudgets: false,
+ }),
+}));
 jest.mock('../data', () => ({
  ...jest.requireActual('../data'),
  useSubsidySummaryAnalyticsApi: jest.fn(),
@@ -43,6 +51,10 @@ const initialStore = {
  portalConfiguration: {
  enterpriseId: enterpriseUUID,
  enterpriseSlug,
+ enterpriseFeatures: {
+ topDownAssignmentRealTimeLcm: true,
+ },
+ enablePortalLearnerCreditManagementScreen: true,
  },
 };
 const store = getMockStore({ ...initialStore });
@@ -55,20 +67,21 @@ const mockBudgetDisplayName = 'Test Enterprise Budget Display Name';
 const defaultEnterpriseSubsidiesContextValue = {
  isFetchingBudgets: false,
 };
-
 const BudgetCardWrapper = ({
  enterpriseSubsidiesContextValue = defaultEnterpriseSubsidiesContextValue,
  ...rest
 }) => (
- <MemoryRouter initialEntries={['/test-enterprise/admin/learner-credit']}>
- <Provider store={store}>
- <IntlProvider locale="en">
- <EnterpriseSubsidiesContext.Provider value={enterpriseSubsidiesContextValue}>
- <BudgetCard {...rest} />
- </EnterpriseSubsidiesContext.Provider>
- </IntlProvider>
- </Provider>
- </MemoryRouter>
+ <QueryClientProvider client={queryClient()}>
+ <MemoryRouter initialEntries={['/test-enterprise/admin/learner-credit']}>
+ <Provider store={store}>
+ <IntlProvider locale="en">
+ <EnterpriseSubsidiesContext.Provider value={enterpriseSubsidiesContextValue}>
+ <BudgetCard {...rest} />
+ </EnterpriseSubsidiesContext.Provider>
+ </IntlProvider>
+ </Provider>
+ </MemoryRouter>
+ </QueryClientProvider>
 );
 
 describe('<BudgetCard />', () => {