chore: PR feedback

openedx · Oct 3, 2024 · 10ab993 · 10ab993
1 parent bb53284
commit 10ab993
Show file tree

Hide file tree

Showing 10 changed files with 57 additions and 66 deletions.
diff --git a/src/components/course/CourseSidebarPrice.jsx b/src/components/course/CourseSidebarPrice.jsx
@@ -37,7 +37,7 @@ const CourseSidebarPrice = () => {
           description="Message to indicate that the price has been reduced."
         />
       </span>
-      ${originalPriceDisplay} {currency}
+      {originalPriceDisplay} {currency}
     </del>
   );
 
@@ -61,15 +61,13 @@ const CourseSidebarPrice = () => {
     );
   }
 
-  const hasDiscountedPrice = coursePrice.discounted
-    && sumOfArray(coursePrice.discounted) < sumOfArray(coursePrice.listRange);
+  const hasDiscountedPrice = coursePrice.discountedList
+    && sumOfArray(coursePrice.discountedList) < sumOfArray(coursePrice.listRange);
   // Case 2: No subsidies found but learner can request a subsidy
   if (!hasDiscountedPrice && canRequestSubsidy) {
     return (
       <span style={{ whiteSpace: 'pre-wrap' }} data-testid="browse-and-request-pricing">
-        <s>
-          ${originalPriceDisplay} {currency}
-        </s><br />
+        <s>{originalPriceDisplay} {currency}</s><br />
         <FormattedMessage
           id="enterprise.course.about.course.sidebar.price.free.when.approved"
           defaultMessage="Free to me{br}(when approved)"
@@ -84,7 +82,7 @@ const CourseSidebarPrice = () => {
   if (!hasDiscountedPrice) {
     return (
       <span className="d-block">
-        ${originalPriceDisplay} {currency}
+        {originalPriceDisplay} {currency}
       </span>
     );
   }
@@ -114,22 +112,22 @@ const CourseSidebarPrice = () => {
       });
     }
   }
-  const discountedPriceDisplay = `${getContentPriceDisplay(coursePrice.discounted)} ${currency}`;
+  const discountedPriceDisplay = `${getContentPriceDisplay(coursePrice.discountedList)} ${currency}`;
   return (
     <>
-      <div className={classNames({ 'mb-2': coursePrice.discounted > 0 || showOrigPrice })}>
-        {/* discounted > 0 means partial discount */}
+      <div className={classNames({ 'mb-2': coursePrice.discountedList > 0 || showOrigPrice })}>
+        {/* discountedList > 0 means partial discount */}
         {showOrigPrice && <>{crossedOutOriginalPrice}{' '}</>}
-        {sumOfArray(coursePrice.discounted) > 0 && (
+        {sumOfArray(coursePrice.discountedList) > 0 && (
           <>
             <span className="sr-only">
               <FormattedMessage
                 id="enterprise.course.about.price.discounted"
                 defaultMessage="Discounted price:"
-                description="Message to indicate that the price has been discounted."
+                description="Message to indicate that the price has been discountedList."
               />
             </span>
-            ${discountedPriceDisplay}
+            {discountedPriceDisplay}
           </>
         )}
       </div>

diff --git a/src/components/course/data/hooks.jsx b/src/components/course/data/hooks.jsx
@@ -173,8 +173,8 @@ export function useCoursePacingType(courseRun) {
 
 /**
  * @typedef {Object} CoursePrice
- * @property {number} list The list price.
- * @property {number} discounted The discounted price.
+ * @property {number} listRange The list price.
+ * @property {number} discountedList The discountedList price.
  */
 
 /**
@@ -208,29 +208,29 @@ export const useCoursePriceForUserSubsidy = ({
 
       if (userSubsidyApplicableToCourse) {
         const { discountType, discountValue } = userSubsidyApplicableToCourse;
-        let discountedPrice = [];
+        let discountedPriceList = [];
 
         if (discountType && discountType.toLowerCase() === SUBSIDY_DISCOUNT_TYPE_MAP.PERCENTAGE.toLowerCase()) {
-          discountedPrice = onlyListPrice.listRange.map(
+          discountedPriceList = onlyListPrice.listRange.map(
             (individualPrice) => individualPrice - (individualPrice * (discountValue / 100)),
           );
         }
 
         if (discountType && discountType.toLowerCase() === SUBSIDY_DISCOUNT_TYPE_MAP.ABSOLUTE.toLowerCase()) {
-          discountedPrice = onlyListPrice.listRange.map(
+          discountedPriceList = onlyListPrice.listRange.map(
             (individualPrice) => Math.max(individualPrice - discountValue, 0),
           );
         }
 
-        if (isDefinedAndNotNull(discountedPrice)) {
+        if (isDefinedAndNotNull(discountedPriceList)) {
           return {
             ...onlyListPrice,
-            discounted: discountedPrice,
+            discountedList: discountedPriceList,
           };
         }
         return {
           ...onlyListPrice,
-          discounted: onlyListPrice.listRange,
+          discountedList: onlyListPrice.listRange,
         };
       }
 

diff --git a/src/components/course/data/tests/hooks.test.jsx b/src/components/course/data/tests/hooks.test.jsx
@@ -801,7 +801,7 @@ describe('useCoursePriceForUserSubsidy', () => {
       userSubsidyApplicableToCourse,
     }));
     const { coursePrice } = result.current;
-    expect(coursePrice).toEqual({ listRange: [100], discounted: [90] });
+    expect(coursePrice).toEqual({ listRange: [100], discountedList: [90] });
   });
 
   it('should return the correct course price when a user subsidy is applicable with unknown discount type', () => {
@@ -818,7 +818,7 @@ describe('useCoursePriceForUserSubsidy', () => {
       userSubsidyApplicableToCourse,
     }));
     const { coursePrice } = result.current;
-    expect(coursePrice).toEqual({ listRange: [100], discounted: [] });
+    expect(coursePrice).toEqual({ listRange: [100], discountedList: [] });
   });
 
   it('should return the correct course price when a user subsidy is applicable with absolute discount', () => {
@@ -835,7 +835,7 @@ describe('useCoursePriceForUserSubsidy', () => {
       userSubsidyApplicableToCourse,
     }));
     const { coursePrice } = result.current;
-    expect(coursePrice).toEqual({ listRange: [150], discounted: [140] });
+    expect(coursePrice).toEqual({ listRange: [150], discountedList: [140] });
   });
 
   it('should return the correct course price when a user subsidy is not applicable', () => {
@@ -1685,7 +1685,7 @@ describe('useCourseListPrice', () => {
     );
     expect(result.current).toEqual(mockListPrice);
   });
-  it('should not return the list price if one doesnt, first fallback, fixed_price_usd', () => {
+  it('should not return the list price if one doesnt exist, first fallback price, fixed_price_usd', () => {
     const updatedListPrice = undefined;
     useCourseRedemptionEligibility.mockReturnValue({ data: { listPrice: updatedListPrice } });
     useCourseMetadata.mockReturnValue(updatedListPrice || getCoursePrice(baseCourseMetadataValue));
@@ -1695,7 +1695,7 @@ describe('useCourseListPrice', () => {
     );
     expect(result.current).toEqual([baseCourseMetadataValue.activeCourseRun.fixedPriceUsd]);
   });
-  it('should not return the list price if one doesnt, second fallback, firstEnrollablePaidSeatPrice', () => {
+  it('should not return the list price if one doesnt exist, second fallback price, firstEnrollablePaidSeatPrice', () => {
     const updatedListPrice = undefined;
     useCourseRedemptionEligibility.mockReturnValue({ data: { listPrice: updatedListPrice } });
     delete baseCourseMetadataValue.activeCourseRun.fixedPriceUsd;
@@ -1706,7 +1706,7 @@ describe('useCourseListPrice', () => {
     );
     expect(result.current).toEqual([baseCourseMetadataValue.activeCourseRun.firstEnrollablePaidSeatPrice]);
   });
-  it('should not return the list price if one doesnt, third fallback, entitlements', () => {
+  it('should not return the list price if one doesnt exit, third fallback price, entitlements', () => {
     const updatedListPrice = undefined;
     useCourseRedemptionEligibility.mockReturnValue({ data: { listPrice: updatedListPrice } });
     delete baseCourseMetadataValue.activeCourseRun.fixedPriceUsd;

diff --git a/src/components/course/data/utils.jsx b/src/components/course/data/utils.jsx
@@ -21,7 +21,7 @@ import XSeriesSvgIcon from '../../../assets/icons/xseries.svg';
 import CreditSvgIcon from '../../../assets/icons/credit.svg';
 import { PROGRAM_TYPE_MAP } from '../../program/data/constants';
 import { programIsMicroMasters, programIsProfessionalCertificate } from '../../program/data/utils';
-import { hasValidStartExpirationDates } from '../../../utils/common';
+import { formatPrice, hasValidStartExpirationDates } from '../../../utils/common';
 import { LICENSE_STATUS } from '../../enterprise-user-subsidy/data/constants';
 import {
   findHighestLevelEntitlementSku,
@@ -167,19 +167,9 @@ export const getContentPriceDisplay = (priceRange) => {
   const minPrice = Math.min(...priceRange);
   const maxPrice = Math.max(...priceRange);
   if (maxPrice !== minPrice) {
-    return `${numberWithPrecision(minPrice)} - ${numberWithPrecision(maxPrice)}`;
+    return `${formatPrice(minPrice)} - ${formatPrice(maxPrice)}`;
   }
-  return numberWithPrecision(priceRange[0]);
-};
-
-export const formatPrice = (price, options = {}) => {
-  const USDollar = new Intl.NumberFormat('en-US', {
-    style: 'currency',
-    currency: 'USD',
-    minimumFractionDigits: 0,
-    ...options,
-  });
-  return USDollar.format(Math.abs(price));
+  return formatPrice(priceRange[0]);
 };
 
 /**
@@ -831,7 +821,7 @@ export function getLinkToCourse(course, slug) {
  */
 export function getEntitlementPrice(entitlements) {
   if (entitlements?.length) {
-    return Number(entitlements[0].price);
+    return parseFloat(entitlements[0].price);
   }
   return undefined;
 }
@@ -846,10 +836,7 @@ export function getEntitlementPrice(entitlements) {
  */
 export function getCoursePrice(course) {
   if (course.activeCourseRun?.fixedPriceUsd) {
-    if (typeof course.activeCourseRun.fixedPriceUsd === 'string') {
-      return [Number(course.activeCourseRun.fixedPriceUsd)];
-    }
-    return [course.activeCourseRun?.fixedPriceUsd];
+    return [parseFloat(course.activeCourseRun.fixedPriceUsd)];
   }
   if (course.activeCourseRun?.firstEnrollablePaidSeatPrice) {
     return [course.activeCourseRun?.firstEnrollablePaidSeatPrice];

diff --git a/src/components/course/tests/CourseSidebarPrice.test.jsx b/src/components/course/tests/CourseSidebarPrice.test.jsx
@@ -62,7 +62,7 @@ describe('<CourseSidebarPrice/> ', () => {
     jest.clearAllMocks();
     useEnterpriseCustomer.mockReturnValue({ data: mockEnterpriseCustomer });
     useUserSubsidyApplicableToCourse.mockReturnValue({ userSubsidyApplicableToCourse: null });
-    useCoursePrice.mockReturnValue({ coursePrice: { listRange: [7.5], discounted: [7.5] }, currency: 'USD' });
+    useCoursePrice.mockReturnValue({ coursePrice: { listRange: [7.5], discountedList: [7.5] }, currency: 'USD' });
     useIsCourseAssigned.mockReturnValue({ isCourseAssigned: false });
     useCanUserRequestSubsidyForCourse.mockReturnValue(false);
   });
@@ -93,7 +93,7 @@ describe('<CourseSidebarPrice/> ', () => {
         subsidyType: ENTERPRISE_OFFER_SUBSIDY_TYPE,
       };
       useUserSubsidyApplicableToCourse.mockReturnValue({ userSubsidyApplicableToCourse: mockEnterpriseOfferSubsidy });
-      useCoursePrice.mockReturnValue({ coursePrice: { listRange: [7.5], discounted: [0] }, currency: 'USD' });
+      useCoursePrice.mockReturnValue({ coursePrice: { listRange: [7.5], discountedList: [0] }, currency: 'USD' });
       render(<CourseSidebarPriceWrapper />);
       expect(screen.getByText('Priced reduced from:')).toBeInTheDocument();
       expect(screen.getByText(/\$7.50 USD/)).toBeInTheDocument();
@@ -124,7 +124,7 @@ describe('<CourseSidebarPrice/> ', () => {
     });
 
     test('subscription license subsidy, shows no price, correct message', () => {
-      useCoursePrice.mockReturnValue({ coursePrice: { listRange: 7.5, discounted: 0 }, currency: 'USD' });
+      useCoursePrice.mockReturnValue({ coursePrice: { listRange: [7.5], discountedList: [0] }, currency: 'USD' });
       useUserSubsidyApplicableToCourse.mockReturnValue({
         userSubsidyApplicableToCourse: { subsidyType: LICENSE_SUBSIDY_TYPE },
       });
@@ -151,7 +151,7 @@ describe('<CourseSidebarPrice/> ', () => {
       expect(screen.queryByText('This course is assigned to you. The price of this course is already covered by your organization.')).not.toBeInTheDocument();
     });
 
-    test('coupon code non-full subsidy, shows discounted price only, correct message', () => {
+    test('coupon code non-full subsidy, shows discountedList price only, correct message', () => {
       useCoursePrice.mockReturnValue({ coursePrice: { listRange: [7.5], discounted: [3.75] }, currency: 'USD' });
       useUserSubsidyApplicableToCourse.mockReturnValue({
         userSubsidyApplicableToCourse: PARTIAL_COUPON_CODE_SUBSIDY,
@@ -213,7 +213,7 @@ describe('<CourseSidebarPrice/> ', () => {
       expect(screen.queryByText("This course can be purchased with your organization's learner credit")).not.toBeInTheDocument();
       expect(screen.queryByText('This course is assigned to you. The price of this course is already covered by your organization.')).not.toBeInTheDocument();
     });
-    test('coupon code non-full subsidy, shows orig and discounted price only, correct message', () => {
+    test('coupon code non-full subsidy, shows orig and discountedList price only, correct message', () => {
       useCoursePrice.mockReturnValue({ coursePrice: { listRange: [7.5], discounted: [3.75] }, currency: 'USD' });
       useUserSubsidyApplicableToCourse.mockReturnValue({
         userSubsidyApplicableToCourse: PARTIAL_COUPON_CODE_SUBSIDY,

diff --git a/src/components/executive-education-2u/components/CourseSummaryCard.jsx b/src/components/executive-education-2u/components/CourseSummaryCard.jsx
@@ -6,24 +6,21 @@ import {
 import { FormattedMessage } from '@edx/frontend-platform/i18n';
 
 import {
-  DATE_FORMAT,
-  getContentPriceDisplay,
-  numberWithPrecision,
-  useMinimalCourseMetadata,
-  ZERO_PRICE,
+  DATE_FORMAT, getContentPriceDisplay, useMinimalCourseMetadata, ZERO_PRICE,
 } from '../../course/data';
+import { formatPrice } from '../../../utils/common';
 
 const CourseSummaryCard = ({ enrollmentCompleted }) => {
   const { data: minimalCourseMetadata } = useMinimalCourseMetadata();
 
   let coursePrice = null;
-  const precisePrice = minimalCourseMetadata.priceDetails?.price ? `$${getContentPriceDisplay(
+  const precisePrice = minimalCourseMetadata.priceDetails?.price ? `${getContentPriceDisplay(
     minimalCourseMetadata.priceDetails.price,
   )} ${minimalCourseMetadata.priceDetails.currency}` : '-';
   if (enrollmentCompleted && minimalCourseMetadata.priceDetails?.price) {
     coursePrice = (
       <><del>{precisePrice}</del>
-        ${numberWithPrecision(ZERO_PRICE)} {minimalCourseMetadata.priceDetails.currency}
+        {formatPrice(ZERO_PRICE)} {minimalCourseMetadata.priceDetails.currency}
       </>
     );
   } else {

diff --git a/src/components/executive-education-2u/components/RegistrationSummaryCard.jsx b/src/components/executive-education-2u/components/RegistrationSummaryCard.jsx
@@ -2,8 +2,9 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import { Card, Col, Row } from '@openedx/paragon';
 import { FormattedMessage } from '@edx/frontend-platform/i18n';
-import { getContentPriceDisplay, numberWithPrecision } from '../../course/data/utils';
-import { CURRENCY_USD } from '../../course/data/constants';
+import { getContentPriceDisplay } from '../../course/data/utils';
+import { CURRENCY_USD, ZERO_PRICE } from '../../course/data/constants';
+import { formatPrice } from '../../../utils/common';
 
 const RegistrationSummaryCard = ({ priceDetails }) => (
   <Card
@@ -43,11 +44,11 @@ const RegistrationSummaryCard = ({ priceDetails }) => (
                 <Col xs={12} lg={{ span: 6, offset: 0 }} className="justify-content-end">
                   <div className="d-flex justify-content-end mr-2.5">
                     <del>
-                      {priceDetails?.price ? `$${getContentPriceDisplay(priceDetails.price)} ${priceDetails.currency}` : '-'}
+                      {priceDetails?.price ? `${getContentPriceDisplay(priceDetails.price)} ${priceDetails.currency}` : '-'}
                     </del>
                   </div>
                   <div className="d-flex justify-content-end mr-2.5">
-                    {priceDetails?.price ? `$${numberWithPrecision(0)} ${priceDetails?.currency ? priceDetails.currency : CURRENCY_USD}` : '-'}
+                    {priceDetails?.price ? `${formatPrice(ZERO_PRICE)} ${priceDetails?.currency ? priceDetails.currency : CURRENCY_USD}` : '-'}
                   </div>
                   <div className="d-flex justify-content-end small font-weight-light text-gray-500 mr-2.5">
                     <FormattedMessage

diff --git a/src/components/microlearning/VideoDetailPage.jsx b/src/components/microlearning/VideoDetailPage.jsx
@@ -200,7 +200,7 @@ const VideoDetailPage = () => {
                       description="Label for the original price of the course."
                     />
                   </strong>
-                  <s>${getContentPriceDisplay(coursePrice)} USD</s>
+                  <s>{getContentPriceDisplay(coursePrice)} USD</s>
                   <h4 className="text-danger ml-2 m-0">
                     <FormattedMessage
                       id="enterprise.courseAbout.courseSidebar.price.free"

diff --git a/src/components/microlearning/tests/VideoDetailPage.test.jsx b/src/components/microlearning/tests/VideoDetailPage.test.jsx
@@ -19,6 +19,7 @@ import {
 import { COURSE_PACING_MAP } from '../../course/data';
 import { LICENSE_STATUS } from '../../enterprise-user-subsidy/data/constants';
 import { features } from '../../../config';
+import { formatPrice } from '../../../utils/common';
 
 const APP_CONFIG = {
   USE_API_CACHE: true,
@@ -187,7 +188,7 @@ describe('VideoDetailPage', () => {
   it('renders the price', () => {
     useVideoCourseMetadata.mockReturnValue({ data: { ...mockCourseMetadata, activeCourseRun: { ...mockCourseRun, levelType: 'Unknown' } } });
     renderWithRouter(<VideoDetailPageWrapper />);
-    expect(screen.getByText(`$${mockCourseRun.firstEnrollablePaidSeatPrice}.00 USD`)).toBeInTheDocument();
+    expect(screen.getByText(`${formatPrice(mockCourseRun.firstEnrollablePaidSeatPrice)} USD`)).toBeInTheDocument();
   });
 
   it('renders a not found page when video data is not found', () => {

diff --git a/src/utils/common.js b/src/utils/common.js
@@ -25,9 +25,6 @@ export const isNull = (value) => {
 };
 
 export const isDefinedAndNotNull = (value) => {
-  if (Array.isArray(value)) {
-    return value.every(item => isDefined(item) && !isNull(item));
-  }
   const values = createArrayFromValue(value);
   return values.every(item => isDefined(item) && !isNull(item));
 };
@@ -183,3 +180,13 @@ export function i18nFormatTimestamp({ intl, timestamp, formatOpts = {} }) {
     ...formatOpts,
   });
 }
+
+export const formatPrice = (price, options = {}) => {
+  const USDollar = new Intl.NumberFormat('en-US', {
+    style: 'currency',
+    currency: 'USD',
+    minimumFractionDigits: 0,
+    ...options,
+  });
+  return USDollar.format(Math.abs(price));
+};