Skip to content

Commit

Permalink
fix: 3 bugs in the provisioning tool
Browse files Browse the repository at this point in the history
1. We no longer double-multiply the subsidy balance by 100, leading to a higher subsidy starting balance than intended.
2. catalogUuid no longer is interpreted as invalid when a predefined query is selected.
3. On the edit view, the budget header now correctly reflects the catalog selection.

ENT-7922
  • Loading branch information
pwnage101 committed Dec 5, 2023
1 parent 8169d08 commit 5167c0c
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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}
/>
))}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
Expand Down
4 changes: 2 additions & 2 deletions src/Configuration/Provisioning/data/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions src/Configuration/Provisioning/data/tests/utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() }] }),
}));
Expand All @@ -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,
Expand All @@ -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,
Expand Down
10 changes: 6 additions & 4 deletions src/Configuration/Provisioning/data/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down
19 changes: 17 additions & 2 deletions src/data/services/SubsidyApiService.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
{
Expand All @@ -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,
},
Expand Down

0 comments on commit 5167c0c

Please sign in to comment.