Merge pull request #369 from openedx/pwnage101/ENT-7922-2

fix: 3 bugs in the provisioning tool

src/Configuration/Provisioning/ProvisioningForm/PolicyForm/ProvisioningFormPolicyType.jsx

@@ -2,29 +2,46 @@ import {
   Form,
 } from '@edx/paragon';
 import { v4 as uuidv4 } from 'uuid';
+import { useContextSelector } from 'use-context-selector';
 import React, { useState } from 'react';
 import PROVISIONING_PAGE_TEXT from '../../data/constants';
 import useProvisioningContext from '../../data/hooks';
-import { indexOnlyPropType, selectProvisioningContext } from '../../data/utils';
 import ProvisioningFormHelpText from '../ProvisioningFormHelpText';
+import { indexOnlyPropType } from '../../data/utils';
+import { ProvisioningContext } from '../../ProvisioningContext';
 
 const ProvisioningFormPolicyType = ({ index }) => {
-  const { perLearnerCap, setPolicyType, setInvalidPolicyFields } = useProvisioningContext();
+  const {
+    perLearnerCap,
+    setPolicyType,
+    setInvalidPolicyFields,
+  } = useProvisioningContext();
   const { POLICY_TYPE } = PROVISIONING_PAGE_TEXT.FORM;
-  const [formData, showInvalidField] = selectProvisioningContext('formData', 'showInvalidField');
-  const { policies } = showInvalidField;
-  const isPolicyTypeDefinedAndFalse = policies[index]?.policyType === false;
+  const contextData = useContextSelector(ProvisioningContext, v => v[0]);
+  const {
+    isEditMode,
+    formData,
+    showInvalidField: { policies },
+  } = contextData;
+  const isFormFieldInvalid = policies[index]?.policyType === false;
 
-  const [value, setValue] = useState(null);
+  let submittedFormPolicyType;
+  if (isEditMode) {
+    submittedFormPolicyType = formData.policies[index].policyType;
+  }
+  const [value, setValue] = useState(submittedFormPolicyType || null);
 
   const handleChange = (e) => {
     const policyTypeValue = e.target.value;
-    if (policyTypeValue === POLICY_TYPE.OPTIONS.LEARNER_SELECTS.DESCRIPTION) {
+    if (isEditMode) {
+      return; // Editing policy type is not supported.
+    }
+    if (policyTypeValue === POLICY_TYPE.OPTIONS.LEARNER_SELECTS.VALUE) {
       setPolicyType({
         policyType: POLICY_TYPE.OPTIONS.LEARNER_SELECTS.VALUE,
         accessMethod: POLICY_TYPE.OPTIONS.LEARNER_SELECTS.ACCESS_METHOD,
       }, index);
-    } else if (policyTypeValue === POLICY_TYPE.OPTIONS.ADMIN_SELECTS.DESCRIPTION) {
+    } else if (policyTypeValue === POLICY_TYPE.OPTIONS.ADMIN_SELECTS.VALUE) {
       setPolicyType({
         policyType: POLICY_TYPE.OPTIONS.ADMIN_SELECTS.VALUE,
         accessMethod: POLICY_TYPE.OPTIONS.ADMIN_SELECTS.ACCESS_METHOD,
@@ -51,24 +68,26 @@ const ProvisioningFormPolicyType = ({ index }) => {
         <Form.RadioSet
           name={`display-policy-type-${index}`}
           onChange={handleChange}
-          value={value || formData.policies[index]?.policyType}
+          value={value}
           className="mt-2.5"
         >
           {
-            Object.values(POLICY_TYPE.OPTIONS).map(({ DESCRIPTION }) => (
+            Object.values(POLICY_TYPE.OPTIONS).map(({ DESCRIPTION, VALUE }) => (
               <Form.Radio
-                value={DESCRIPTION}
+                value={VALUE}
                 type="radio"
                 key={uuidv4()}
                 data-testid={DESCRIPTION}
-                isInvalid={isPolicyTypeDefinedAndFalse}
+                data-description={DESCRIPTION}
+                isInvalid={isFormFieldInvalid}
+                disabled={isEditMode}
               >
                 {DESCRIPTION}
               </Form.Radio>
             ))
           }
         </Form.RadioSet>
-        {isPolicyTypeDefinedAndFalse && (
+        {isFormFieldInvalid && (
           <Form.Control.Feedback
             type="invalid"
           >

src/Configuration/Provisioning/ProvisioningForm/PolicyForm/tests/ProvisioningFormPolicyContainer.test.jsx

@@ -83,10 +83,9 @@ describe('ProvisioningFormPolicyContainer', () => {
     );
     expect(screen.getByText(POLICY_TYPE.TITLE)).toBeTruthy();
     expect(screen.getByText(POLICY_TYPE.LABEL)).toBeTruthy();
-    const learnerOption = screen.getByTestId(POLICY_TYPE.OPTIONS.ADMIN_SELECTS.DESCRIPTION);
-    userEvent.click(learnerOption);
+    userEvent.click(screen.getByTestId(POLICY_TYPE.OPTIONS.ADMIN_SELECTS.DESCRIPTION));
     await waitFor(() => {
-      expect(learnerOption.checked).toBeTruthy();
+      expect(screen.getByTestId(POLICY_TYPE.OPTIONS.ADMIN_SELECTS.DESCRIPTION).checked).toBeTruthy();
     });
   });
 });

src/Configuration/Provisioning/ProvisioningForm/PolicyForm/tests/ProvisioningFormPolicyType.test.jsx

@@ -40,16 +40,13 @@ describe('ProvisioningFormPolicyType', () => {
     expect(screen.getByText(POLICY_TYPE.LABEL)).toBeTruthy();
 
     const policyTypeOptions = Object.keys(POLICY_TYPE.OPTIONS);
-    const policyTypeButtons = [];
-    // Retrieves a list of input elements based on test ids
-    for (let i = 0; i < policyTypeOptions.length; i++) {
-      policyTypeButtons.push(screen.getByTestId(POLICY_TYPE.OPTIONS[policyTypeOptions[i]].DESCRIPTION));
-    }
 
     // Clicks on each input element and checks if it is checked
-    for (let i = 0; i < policyTypeButtons.length; i++) {
-      fireEvent.click(policyTypeButtons[i]);
-      expect(policyTypeButtons[i].checked).toBeTruthy();
+    for (let i = 0; i < policyTypeOptions.length; i++) {
+      const buttonBeforeClick = screen.getByTestId(POLICY_TYPE.OPTIONS[policyTypeOptions[i]].DESCRIPTION);
+      fireEvent.click(buttonBeforeClick);
+      const buttonAfterClick = screen.getByTestId(POLICY_TYPE.OPTIONS[policyTypeOptions[i]].DESCRIPTION);
+      expect(buttonAfterClick.checked).toBeTruthy();
     }
     expect(screen.getByText('Not editable')).toBeTruthy();
   });

src/Configuration/Provisioning/SubsidyEditView/SubsidyEditView.jsx

@@ -4,6 +4,7 @@ import { useContextSelector } from 'use-context-selector';
 
 import { logError } from '@edx/frontend-platform/logging';
 
+import { generateBudgetDisplayName } from '../data/utils';
 import useProvisioningContext from '../data/hooks';
 import PROVISIONING_PAGE_TEXT from '../data/constants';
 import { ProvisioningContext } from '../ProvisioningContext';
@@ -87,13 +88,10 @@ const SubsidyEditView = () => {
           <ProvisioningFormInternalOnly />
           <ProvisioningFormSubsidy />
           <AccountTypeDetail isMultipleFunds={multipleFunds} />
-          {(multipleFunds !== undefined) && formData.policies?.map(({
-            uuid,
-            policyFormTitle,
-          }, index) => (
+          {(multipleFunds !== undefined) && formData.policies?.map((policy, index) => (
             <ProvisioningFormPolicyContainer
-              key={uuid}
-              title={policyFormTitle}
+              key={policy.uuid}
+              title={generateBudgetDisplayName(policy)}
               index={index}
             />
           ))}

src/Configuration/Provisioning/SubsidyEditView/tests/SubsidyEditView.test.jsx

@@ -147,6 +147,7 @@ describe('SubsidyEditView', () => {
     expect(screen.getByTestId('internal-only-checkbox').checked).toBeTruthy();
     expect(screen.getByTestId('partner-no-rev-prepay').checked).toBeTruthy();
     expect(screen.getByText('No, create one Learner Credit budget')).toBeInTheDocument();
+    expect(screen.getByText('Open Courses budget')).toBeInTheDocument();
     expect(screen.getByTestId('account-name').value).toBe('Paper company --- Open Courses');
     expect(screen.getByText(
       'The maximum USD value a single learner may redeem from the budget. This value should be less than the budget starting balance.',

src/Configuration/Provisioning/data/hooks.js

@@ -243,11 +243,11 @@ export default function useProvisioningContext() {
             // Old values should remain static and will help us later decide whether to skip catalog creation.
             oldPredefinedQueryType: predefinedQueryType,
             oldCustomCatalog: !predefinedQueryType,
-            oldCatalogUuid: catalog.uuid,
+            oldCatalogUuid: !predefinedQueryType ? catalog.uuid : undefined,
             // New values will change over time as different options are selected.
             predefinedQueryType,
             customCatalog: !predefinedQueryType,
-            catalogUuid: catalog.uuid,
+            catalogUuid: !predefinedQueryType ? catalog.uuid : undefined,
             // We ostensibly don't rely on the catalog title for anything critical, but in case it is a custom catalog
             // we can cache the title here so that we have something to display in the detail view header.
             catalogTitle: catalog.title,

src/Configuration/Provisioning/data/tests/utils.test.js

@@ -342,7 +342,7 @@ const emptyDataSet = {
   subsidyRevReq: '',
 };
 describe('determineInvalidFields', () => {
-  it('returns false for all subsidy fields', async () => {
+  it('returns false (invalid)for all subsidy fields', async () => {
     getAuthenticatedHttpClient.mockImplementation(() => ({
       get: jest.fn().mockResolvedValue({ data: [{ id: uuidv4() }] }),
     }));
@@ -358,7 +358,7 @@ describe('determineInvalidFields', () => {
     const output = await determineInvalidFields(emptyDataSet);
     expect(output).toEqual([expectedFailedSubsidyOutput]);
   });
-  it('returns false for all policy fields', async () => {
+  it('returns false (invalid)for all policy fields', async () => {
     const expectedFailedPolicyOutput = [{
       subsidyTitle: false,
       enterpriseUUID: false,
@@ -370,7 +370,7 @@ describe('determineInvalidFields', () => {
     }, [{
       accountName: false,
       accountValue: false,
-      catalogUuid: false,
+      catalogUuid: true, // This is true (i.e. valid) because catalogUuid is not required when customCatalog != true.
       predefinedQueryType: false,
       perLearnerCap: false,
       perLearnerCapAmount: false,
@@ -410,7 +410,7 @@ describe('transformPatchPolicyData', () => {
   it('returns the correct data', async () => {
     const mockFormData = {
       policies: [{
-        policy_type: 'PerLearnerSpendCreditAccessPolicy',
+        policyType: 'PerLearnerSpendCreditAccessPolicy',
         accountDescription: 'awesome policy description',
         customCatalog: true,
         catalogTitle: 'awesome custom catalog',
@@ -428,7 +428,6 @@ describe('transformPatchPolicyData', () => {
       catalogUuid: '2afb0a7f-103d-43c3-8b1a-db8c5b3ba1f4',
       subsidyUuid: '205f11a4-0303-4407-a2e7-80261ef8fb8f',
       perLearnerSpendLimit: 200,
-      spendLimit: 12000,
       uuid: '12324232',
     }]);
   });

src/Configuration/Provisioning/data/utils.js

@@ -88,9 +88,11 @@ export async function determineInvalidFields(formData) {
     const policyData = {
       accountName: !!accountName,
       accountValue: !!accountValue,
-      // Either a predefined query type must be selected, or a custom catalog is selected.
-      predefinedQueryType: !!predefinedQueryType && !customCatalog,
-      catalogUuid: !!catalogUuid && customCatalog,
+      // Either a predefined query type must be selected, or a catalog UUID is selected, depending on customCatalog.
+      // When customCatalog is false, make sure predefinedQueryType is selected:
+      predefinedQueryType: !customCatalog ? !!predefinedQueryType : true,
+      // When customCatalog is true, make sure predefinedQueryType is selected:
+      catalogUuid: customCatalog ? !!catalogUuid : true,
       perLearnerCap: perLearnerCap !== undefined || perLearnerCap === false,
       perLearnerCapAmount: !!perLearnerCapAmount || perLearnerCap === false,
       policyType: !!policyType,
@@ -130,7 +132,7 @@ export function hasValidPolicyAndSubsidy(formData) {
     const isAccountNameValid = !!policy.accountName;
     const isAccountValueValid = !!policy.accountValue;
 
-    const isCatalogDefined = policy.customCatalog === true ? !!policy.catalogUuid : !!policy.predefinedQueryType;
+    const isCatalogDefined = policy.customCatalog ? !!policy.catalogUuid : !!policy.predefinedQueryType;
 
     // Requires learner cap to pass conditionals to be true
     const { perLearnerCap, perLearnerCapAmount } = policy;
@@ -460,26 +462,28 @@ export async function createPolicy({
  * subsidy and catalog uuid.
  *
  * @param {{
-* description: String,
-* catalogUuid: String,
-* subsidyUuid: String,
-* perLearnerSpendLimit: Number,
-* spendLimit: Number
-* }}
-* @returns {Promise<Object>} - Returns a promise that resolves to the response data from the API
-*/
+ * description: String,
+ * catalogUuid: String,
+ * subsidyUuid: String,
+ * perLearnerSpendLimit: Number,
+ * accessMethod: String,
+ * }}
+ * @returns {Promise<Object>} - Returns a promise that resolves to the response data from the API
+ */
 export async function patchPolicy({
   uuid,
   description,
   catalogUuid,
   perLearnerSpendLimit,
+  accessMethod,
 }) {
-  const data = LmsApiService.patchSubsidyAccessPolicy(
+  const data = LmsApiService.patchSubsidyAccessPolicy({
     uuid,
     description,
     catalogUuid,
     perLearnerSpendLimit,
-  );
+    accessMethod,
+  });
   return data;
 }
 
@@ -564,7 +568,13 @@ export function transformPatchPolicyPayload(formData, catalogCreationResponses)
     catalogUuid: catalogCreationResponses[index]?.uuid || policy.catalogUuid,
     subsidyUuid,
     perLearnerSpendLimit: policy.perLearnerCap ? policy.perLearnerCapAmount : null,
-    spendLimit: policy.accountValue,
+
+    // The spendLimit is currently NOT EDITABLE so do not include it in the PATCH payload.
+    // spendLimit: policy.accountValue,
+
+    // The policyType and accessMethod is currently NOT EDITABLE so do not include it in the PATCH payload.
+    // policyType: policy.policyType,
+    // accessMethod: policy.accessMethod,
   }));
   return payloads;
 }

src/data/services/EnterpriseApiService.js

@@ -89,14 +89,14 @@ class LmsApiService {
     },
   );
 
-  static patchSubsidyAccessPolicy = (
+  static patchSubsidyAccessPolicy = ({
     uuid,
     description,
     catalogUuid,
     perLearnerSpendLimit,
-    accessMethod = 'direct',
+    accessMethod,
     active = true,
-  ) => LmsApiService.apiClient().patch(
+  }) => LmsApiService.apiClient().patch(
     `${getConfig().ENTERPRISE_ACCESS_BASE_URL}/api/v1/subsidy-access-policies/${uuid}/`,
     {
       description,

src/data/services/SubsidyApiService.js

@@ -25,6 +25,22 @@ class SubsidyApiService {
     return SubsidyApiService.apiClient().get(`${subsidiesURL}?uuid=${uuid}`);
   };
 
+  /**
+   * postSubsidy gets or creates a learner credit Subsidy (and corresponding ledger).
+   *
+   * @param {String} financialIdentifier - A reference to the object responsible for originating this subsidy, and the
+   * key on which existing subsidies are retrieved.
+   * @param {String} title
+   * @param {String} enterpriseUUID
+   * @param {String} startDate
+   * @param {String} endDate
+   * @param {Number} startingBalance - The initial balance of the new subsidy in USD Cents (integer).
+   * @param {String} revenueCategory
+   * @param {Boolean} internalOnly
+   * @param {String} unit = 'usd_cents'
+   *
+   * @returns {Object} - The subsidy create endpoint response, containing a serialized subsidy.
+   */
   static postSubsidy = (
     financialIdentifier,
     title,
@@ -37,7 +53,6 @@ class SubsidyApiService {
     unit = 'usd_cents',
   ) => {
     const subsidiesURL = `${getConfig().SUBSIDY_BASE_URL}/api/v1/subsidies/`;
-    const wholeDollarStartingBalance = startingBalance * 100;
     return SubsidyApiService.apiClient().post(
       subsidiesURL,
       {
@@ -47,7 +62,7 @@ class SubsidyApiService {
         default_active_datetime: startDate,
         default_expiration_datetime: endDate,
         default_unit: unit,
-        default_starting_balance: wholeDollarStartingBalance,
+        default_starting_balance: startingBalance,
         default_revenue_category: revenueCategory,
         default_internal_only: internalOnly,
       },