[DO_NOT_MERGE][POC] feat: academic selection step#133
[DO_NOT_MERGE][POC] feat: academic selection step#133brobro10000 wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a proof-of-concept (POC) implementation of an "Essentials Academic Selection" step in the checkout flow, feature-flagged to allow controlled rollout. The implementation adds new routing, components, and a utility function for feature flag management.
Key Changes:
- Added a new
isFeatureEnabledutility function to centralize feature flag logic with support for both configuration-based and session-based overrides - Integrated the new Essentials step into the checkout stepper, conditionally rendered based on feature flags
- Refactored routing to support the essentials flow with new page routes, loaders, and type definitions
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/common.ts | Added isFeatureEnabled utility for feature flag checking with session storage override support |
| src/utils/test/common.test.ts | Added comprehensive test coverage for the new isFeatureEnabled function |
| src/types/types.d.ts | Extended type definitions to include Essentials step, AcademicSelection substep, and related form data types |
| src/constants/checkout.ts | Added Essentials-related enums, routes, schemas, and page details; reorganized route constants |
| src/routes.tsx | Commented out legacy essentials routing code in favor of new stepper-based approach |
| src/components/app/routes/loaders/rootLoader.ts | Refactored to use isFeatureEnabled utility; commented out checkout intent validation for essentials routes |
| src/components/app/routes/loaders/checkoutStepperLoader.ts | Added placeholder loader for EssentialsAcademicSelection page |
| src/components/app/routes/tests/rootLoader.test.ts | Updated test references from old to new essentials route naming convention |
| src/components/academic-selection-page/EssentialsAcademicSelectionPage.tsx | New page component implementing the essentials academic selection step with form validation |
| src/components/academic-selection-page/index.ts | Export file for the new academic selection page |
| src/components/Stepper/Steps/EssentialsAcademicSelection.tsx | Stepper step wrapper for the essentials academic selection page |
| src/components/Stepper/StepperContent/EssentialsAcademicSelectionContent.tsx | Content component for the essentials step (currently placeholder) |
| src/components/Stepper/CheckoutStepperContainer.tsx | Updated to conditionally render EssentialsAcademicSelection step based on feature flags; includes debug console.log |
| src/components/Stepper/Steps/index.ts | Added export for EssentialsAcademicSelection step |
| src/components/Stepper/StepperContent/index.ts | Added export for EssentialsAcademicSelectionContent |
| src/components/Stepper/Steps/hooks/useStepperContent.tsx | Registered EssentialsAcademicSelectionContent in the content mapping |
| src/components/account-details-page/AccountDetailsPage.tsx | Minor import reordering (no functional change) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { sendTrackEvent } from '@edx/frontend-platform/analytics'; | ||
| import { logError } from '@edx/frontend-platform/logging'; | ||
| import dayjs from 'dayjs'; | ||
| import {getConfig} from "@edx/frontend-platform/config"; |
There was a problem hiding this comment.
The import statement has inconsistent formatting. It should use double quotes and proper spacing to match the existing code style in this file (lines 1-3).
| import {getConfig} from "@edx/frontend-platform/config"; | |
| import { getConfig } from '@edx/frontend-platform/config'; |
| @@ -0,0 +1,8 @@ | |||
| import { EssentialsAcademicSelectionPage} from "@/components/academic-selection-page"; | |||
There was a problem hiding this comment.
The import statement uses inconsistent quote style (double quotes) and spacing. It should match the project's style guide with single quotes and proper spacing.
| import { EssentialsAcademicSelectionPage} from "@/components/academic-selection-page"; | |
| import { EssentialsAcademicSelectionPage } from '@/components/academic-selection-page'; |
| @@ -0,0 +1,16 @@ | |||
| import type { UseFormReturn } from "react-hook-form"; | |||
There was a problem hiding this comment.
The import statement uses double quotes instead of single quotes, which is inconsistent with the project's code style.
| import type { UseFormReturn } from "react-hook-form"; | |
| import type { UseFormReturn } from 'react-hook-form'; |
| import { zodResolver } from '@hookform/resolvers/zod'; | ||
| import { Stack, Stepper } from '@openedx/paragon'; | ||
| import { useMemo } from 'react'; | ||
| import { Helmet } from 'react-helmet'; | ||
| import { useForm } from 'react-hook-form'; | ||
|
|
||
| import { useFormValidationConstraints } from '@/components/app/data'; | ||
| import { useStepperContent } from '@/components/Stepper/Steps/hooks'; | ||
| import { CheckoutStepKey, DataStoreKey } from '@/constants/checkout'; | ||
| import { useCheckoutFormStore, useCurrentPageDetails } from '@/hooks/index'; | ||
|
|
||
| const EssentialsAcademicSelectionPage = () => { | ||
| const StepperContent = useStepperContent(); | ||
| const { data: formValidationConstraints } = useFormValidationConstraints(); | ||
| const eventKey = CheckoutStepKey.Essentials; | ||
| const { | ||
| formSchema, | ||
| } = useCurrentPageDetails(); | ||
| const essentialsFormData = useCheckoutFormStore((state) => state.formData[DataStoreKey.EssentialsAcademicSelection]); | ||
|
|
||
| const essentialsAcademicSelectionSchema = useMemo(() => ( | ||
| formSchema(formValidationConstraints) | ||
| ), [formSchema, formValidationConstraints]); | ||
|
|
||
| const form = useForm<EssentialAcademicSelectionData>({ | ||
| mode: 'onTouched', | ||
| resolver: zodResolver(essentialsAcademicSelectionSchema), | ||
| defaultValues: essentialsFormData, | ||
| }); | ||
|
|
||
| return ( | ||
| <> | ||
| <Helmet title="Academic Selection Page" /> | ||
| <Stack gap={4}> | ||
| <Stepper.Step eventKey={eventKey} title="Academic Selection Page"> | ||
| <Stack gap={4}> | ||
| <StepperContent form={form} /> | ||
| </Stack> | ||
| </Stepper.Step> | ||
| </Stack> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| export default EssentialsAcademicSelectionPage; |
There was a problem hiding this comment.
The new EssentialsAcademicSelectionPage component lacks test coverage. Similar page components in the codebase (e.g., AccountDetailsPage) have corresponding test files. Consider adding tests to cover the component's rendering, form validation, and integration with the stepper content.
| FEATURE_SELF_SERVICE_ESSENTIALS, | ||
| FEATURE_SELF_SERVICE_ESSENTIALS_KEY, | ||
| } = getConfig(); | ||
| console.log(isFeatureEnabled(FEATURE_SELF_SERVICE_ESSENTIALS, FEATURE_SELF_SERVICE_ESSENTIALS_KEY)); |
There was a problem hiding this comment.
A console.log statement is left in the production code. This should be removed or replaced with proper logging using the logging utility from '@edx/frontend-platform/logging'.
| @@ -0,0 +1 @@ | |||
| export { default as EssentialsAcademicSelectionPage } from './EssentialsAcademicSelectionPage' | |||
There was a problem hiding this comment.
Missing semicolon at the end of the export statement. This is inconsistent with the code style in other index files in the project.
| export { default as EssentialsAcademicSelectionPage } from './EssentialsAcademicSelectionPage' | |
| export { default as EssentialsAcademicSelectionPage } from './EssentialsAcademicSelectionPage'; |
| // const isCheckoutRoute = !Object.values(EssentialsPageRoute).some(route => isPathMatch(currentPath, route)); | ||
| // | ||
| // if (!isCheckoutRoute) { | ||
| // return null; | ||
| // } |
There was a problem hiding this comment.
The commented-out code block handling checkout intent validation should either be removed or have a clear explanation of why it's being commented out. Leaving large blocks of commented code in production reduces code maintainability. If this is intentional for the POC, consider adding a TODO comment explaining the reasoning.
| // { | ||
| // index: true, | ||
| // element: <Navigate to={EssentialsStepKey.AcademicSelection} replace />, | ||
| // }, | ||
| // { | ||
| // path: EssentialsStepKey.AcademicSelection, | ||
| // element: ( | ||
| // <PageWrap> | ||
| // <AcademicSelection /> | ||
| // </PageWrap> | ||
| // ), | ||
| // }, | ||
| // ], | ||
| // }, | ||
| // { | ||
| // path: 'essentials/*', | ||
| // element: <ErrorPage message="Page Not Found" />, | ||
| // }, |
There was a problem hiding this comment.
Large block of commented-out routing code should be removed rather than left in the codebase. This reduces maintainability and creates confusion about which routing approach is currently active. If this code needs to be preserved for reference, consider moving it to documentation or version control history.
| // { | |
| // index: true, | |
| // element: <Navigate to={EssentialsStepKey.AcademicSelection} replace />, | |
| // }, | |
| // { | |
| // path: EssentialsStepKey.AcademicSelection, | |
| // element: ( | |
| // <PageWrap> | |
| // <AcademicSelection /> | |
| // </PageWrap> | |
| // ), | |
| // }, | |
| // ], | |
| // }, | |
| // { | |
| // path: 'essentials/*', | |
| // element: <ErrorPage message="Page Not Found" />, | |
| // }, |
| import { checkoutFormStore } from '@/hooks/useCheckoutFormStore'; | ||
| import { extractPriceId, getCheckoutPageDetails, getStepFromParams } from '@/utils/checkout'; | ||
|
|
||
| async function essentialsAcademicSelectionLoader(): Promise<Response | null> { |
There was a problem hiding this comment.
The essentialsAcademicSelectionLoader function doesn't match the signature of other loader functions in this file. It should accept a QueryClient parameter for consistency with other loaders (e.g., accountDetailsLoader, billingDetailsLoader) even if it doesn't use it yet. This ensures a consistent interface for all page loaders.
| async function essentialsAcademicSelectionLoader(): Promise<Response | null> { | |
| async function essentialsAcademicSelectionLoader(_queryClient: QueryClient): Promise<Response | null> { |
brobro10000
force-pushed
the
hu/ent-academic-selection-poc
branch
from
b771812 to
4c33faa
Compare
brobro10000
force-pushed
the
hu/ent-academic-selection-poc
branch
from
4c33faa to
57c8065
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| formSchema(formValidationConstraints) | ||
| ), [formSchema, formValidationConstraints]); | ||
|
|
||
| const form = useForm<EssentialAcademicSelectionData>({ |
There was a problem hiding this comment.
Inconsistent naming: The type is "EssentialAcademicSelectionData" (singular) but should be "EssentialsAcademicSelectionData" (plural) to match the schema name "EssentialsAcademicSelectionSchema" and maintain naming consistency across the codebase.
| const form = useForm<EssentialAcademicSelectionData>({ | |
| const form = useForm<EssentialsAcademicSelectionData>({ |
| import type { UseFormReturn } from 'react-hook-form'; | ||
|
|
||
| interface EssentialsAcademicSelectionContentProps { | ||
| form: UseFormReturn<EssentialAcademicSelectionData>; |
There was a problem hiding this comment.
Inconsistent naming: The type is "EssentialAcademicSelectionData" (singular) but should be "EssentialsAcademicSelectionData" (plural) to match the schema name "EssentialsAcademicSelectionSchema" and maintain naming consistency across the codebase.
| form: UseFormReturn<EssentialAcademicSelectionData>; | |
| form: UseFormReturn<EssentialsAcademicSelectionData>; |
| FEATURE_SELF_SERVICE_ESSENTIALS, | ||
| FEATURE_SELF_SERVICE_ESSENTIALS_KEY, | ||
| } = getConfig(); | ||
| return ( |
There was a problem hiding this comment.
Remove console.log statement before merging. This debug output should not be committed to production code.
| isFeatureEnabled( | ||
| FEATURE_SELF_SERVICE_ESSENTIALS, | ||
| FEATURE_SELF_SERVICE_ESSENTIALS_KEY, | ||
| ) && <EssentialsAcademicSelection /> |
There was a problem hiding this comment.
This is very important
This pull request introduces a new "Essentials Academic Selection" step to the checkout flow, refactors related routing and feature flag logic, and updates supporting components and constants to enable the new flow. The changes ensure that the essentials step is properly gated by feature flags and integrated with the form handling and routing infrastructure.
Essentials Academic Selection Step Integration:
EssentialsAcademicSelectionstep to the checkout stepper, conditionally rendering it based on feature flags using the newisFeatureEnabledutility. (src/components/Stepper/CheckoutStepperContainer.tsx, src/components/Stepper/CheckoutStepperContainer.tsxR1-L20)EssentialsAcademicSelectionPagecomponent and supporting content component, integrating form validation and stepper content rendering for the essentials step. (src/components/academic-selection-page/EssentialsAcademicSelectionPage.tsx, [1];src/components/Stepper/StepperContent/EssentialsAcademicSelectionContent.tsx, [2]src/components/Stepper/Steps/index.ts, [1];src/components/Stepper/StepperContent/index.ts, [2];src/components/academic-selection-page/index.ts, [3]Routing and Loader Updates:
src/constants/checkout.ts, [1] [2] [3] [4] [5];src/components/app/routes/loaders/checkoutStepperLoader.ts, [6] [7]isFeatureEnabledutility for feature gating essentials routes and simplified checkout intent checks. (src/components/app/routes/loaders/rootLoader.ts, [1] [2] [3]Test and Miscellaneous Updates:
src/components/app/routes/tests/rootLoader.test.ts, [1] [2] [3] [4] [5]src/routes.tsxin favor of the new approach.These changes collectively enable a feature-flagged essentials step in the checkout flow, improve maintainability of routing and form logic, and ensure proper test coverage for the new flow.