-
Notifications
You must be signed in to change notification settings - Fork 4
fix: restore selection plan edit route and page, remove popup #1069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
c5e2557
9dddaf5
475df1f
1ebd0de
26c1409
c3ea40d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| /** | ||
| * Copyright 2026 OpenStack Foundation | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * */ | ||
|
|
||
| import React from "react"; | ||
| import { screen, act, waitFor } from "@testing-library/react"; | ||
| import { Router, Route, Switch } from "react-router-dom"; | ||
| import { createMemoryHistory } from "history"; | ||
| import flushPromises from "flush-promises"; | ||
| import { renderWithRedux } from "../../utils/test-utils"; | ||
| import { getSelectionPlan } from "../../actions/selection-plan-actions"; | ||
| import { getMarketingSettingsBySelectionPlan } from "../../actions/marketing-actions"; | ||
| import SelectionPlanIdLayout from "../selection-plan-id-layout"; | ||
|
|
||
| jest.mock("i18n-react", () => ({ | ||
| __esModule: true, | ||
| default: { translate: (k) => k } | ||
| })); | ||
|
|
||
| // The page renders null until it's ready, so the breadcrumb's presence is our signal. | ||
| jest.mock("react-breadcrumbs", () => ({ | ||
| Breadcrumb: () => <div data-testid="breadcrumb" /> | ||
| })); | ||
|
|
||
| jest.mock("../../actions/selection-plan-actions", () => ({ | ||
| __esModule: true, | ||
| ...jest.requireActual("../../actions/selection-plan-actions"), | ||
| getSelectionPlan: jest.fn(), | ||
| resetSelectionPlanForm: jest.fn(() => ({ type: "RESET_SELECTION_PLAN_FORM" })) | ||
| })); | ||
|
|
||
| jest.mock("../../actions/marketing-actions", () => ({ | ||
| __esModule: true, | ||
| ...jest.requireActual("../../actions/marketing-actions"), | ||
| getMarketingSettingsBySelectionPlan: jest.fn() | ||
| })); | ||
|
|
||
| // Stub the page: the real form needs a fuller marketing-settings shape than set up here. | ||
| jest.mock("../../pages/selection-plans/edit-selection-plan-page", () => ({ | ||
| __esModule: true, | ||
| default: () => <div data-testid="edit-selection-plan-page" /> | ||
| })); | ||
|
|
||
| const renderAt = (path, currentSelectionPlan) => { | ||
| const history = createMemoryHistory({ initialEntries: [path] }); | ||
| const result = renderWithRedux( | ||
| <Router history={history}> | ||
| <Route | ||
| path="/app/summits/:summit_id/selection-plans/:selection_plan_id" | ||
| component={SelectionPlanIdLayout} | ||
| /> | ||
| </Router>, | ||
| { | ||
| initialState: { | ||
| currentSelectionPlanState: { entity: currentSelectionPlan }, | ||
| currentSummitState: { currentSummit: { id: 1 } } | ||
| } | ||
| } | ||
| ); | ||
| return { ...result, history }; | ||
| }; | ||
|
|
||
| const settle = () => act(async () => flushPromises()); | ||
|
|
||
| const isPageRendered = () => screen.queryByTestId("breadcrumb") !== null; | ||
|
|
||
| // Mirrors the sibling /new and /:id(\d+) routes in selection-plan-layout.js. | ||
| const NewOrEditHarness = ({ history }) => ( | ||
| <Router history={history}> | ||
| <Switch> | ||
| <Route | ||
| strict | ||
| exact | ||
| path="/app/summits/:summit_id/selection-plans/new" | ||
| component={SelectionPlanIdLayout} | ||
| /> | ||
| <Route | ||
| path="/app/summits/:summit_id/selection-plans/:selection_plan_id(\d+)" | ||
| component={SelectionPlanIdLayout} | ||
| /> | ||
| </Switch> | ||
| </Router> | ||
| ); | ||
|
|
||
| describe("SelectionPlanIdLayout load guard", () => { | ||
| beforeEach(() => { | ||
| getSelectionPlan.mockReset(); | ||
| getMarketingSettingsBySelectionPlan.mockReset(); | ||
| getSelectionPlan.mockImplementation(() => () => Promise.resolve()); | ||
| getMarketingSettingsBySelectionPlan.mockImplementation( | ||
| () => () => Promise.resolve() | ||
| ); | ||
| }); | ||
|
|
||
| it("does not render on direct load until the matching plan finishes fetching", async () => { | ||
| getSelectionPlan.mockImplementation(() => () => new Promise(() => {})); | ||
| renderAt("/app/summits/1/selection-plans/5", { id: 5 }); | ||
| expect(isPageRendered()).toBe(false); | ||
| }); | ||
|
|
||
| it("stops rendering when switching to a different plan id until the store catches up", async () => { | ||
| const history = createMemoryHistory({ | ||
| initialEntries: ["/app/summits/1/selection-plans/5"] | ||
| }); | ||
| renderWithRedux( | ||
| <Router history={history}> | ||
| <Route | ||
| path="/app/summits/:summit_id/selection-plans/:selection_plan_id" | ||
| component={SelectionPlanIdLayout} | ||
| /> | ||
| </Router>, | ||
| { | ||
| initialState: { | ||
| currentSelectionPlanState: { entity: { id: 5 } }, | ||
| currentSummitState: { currentSummit: { id: 1 } } | ||
| } | ||
| } | ||
| ); | ||
| await settle(); | ||
| expect(isPageRendered()).toBe(true); | ||
|
|
||
| act(() => { | ||
| history.push("/app/summits/1/selection-plans/8"); | ||
| }); | ||
| expect(isPageRendered()).toBe(false); | ||
| expect(getSelectionPlan).toHaveBeenCalledWith("8"); | ||
|
|
||
| // Fetch settles, but the store's entity.id is still "5" — must stay unrendered. | ||
| await settle(); | ||
| expect(isPageRendered()).toBe(false); | ||
| }); | ||
|
|
||
| it("stops rendering when navigating from an existing plan to /new until the store reflects the reset", async () => { | ||
| const history = createMemoryHistory({ | ||
| initialEntries: ["/app/summits/1/selection-plans/5"] | ||
| }); | ||
| renderWithRedux(<NewOrEditHarness history={history} />, { | ||
| initialState: { | ||
| currentSelectionPlanState: { entity: { id: 5 } }, | ||
| currentSummitState: { currentSummit: { id: 1 } } | ||
| } | ||
| }); | ||
| await settle(); | ||
| expect(isPageRendered()).toBe(true); | ||
|
|
||
| // Store still holds plan 5's entity (reset hasn't landed) — must not render. | ||
| act(() => { | ||
| history.push("/app/summits/1/selection-plans/new"); | ||
| }); | ||
| expect(isPageRendered()).toBe(false); | ||
| }); | ||
|
|
||
| it("renders on /new once the store reflects the reset (default) entity", async () => { | ||
| const history = createMemoryHistory({ | ||
| initialEntries: ["/app/summits/1/selection-plans/new"] | ||
| }); | ||
| renderWithRedux(<NewOrEditHarness history={history} />, { | ||
| initialState: { | ||
| currentSelectionPlanState: { entity: { id: 0 } }, | ||
| currentSummitState: { currentSummit: { id: 1 } } | ||
| } | ||
| }); | ||
| await settle(); | ||
| expect(isPageRendered()).toBe(true); | ||
| }); | ||
|
|
||
| it("redirects to the selection plans list when the fetch rejects", async () => { | ||
| getSelectionPlan.mockImplementation( | ||
| () => () => Promise.reject(new Error("fail")) | ||
| ); | ||
| const { history } = renderAt("/app/summits/1/selection-plans/5", { | ||
| id: 0 | ||
| }); | ||
| await waitFor(() => | ||
| expect(history.location.pathname).toBe("/app/summits/1/selection-plans") | ||
| ); | ||
| expect(isPageRendered()).toBe(false); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,20 @@ | ||
| import React, { Suspense, useEffect } from "react"; | ||
| import React, { Suspense, useEffect, useState } from "react"; | ||
| import { connect } from "react-redux"; | ||
| import { Redirect, Route, Switch } from "react-router-dom"; | ||
| import { Breadcrumb } from "react-breadcrumbs"; | ||
| import T from "i18n-react"; | ||
| import AjaxLoader from "openstack-uicore-foundation/lib/components/ajaxloader"; | ||
| import NoMatchPage from "../pages/no-match-page"; | ||
| import { | ||
| getSelectionPlan, | ||
| resetSelectionPlanForm | ||
| } from "../actions/selection-plan-actions"; | ||
| import { getMarketingSettingsBySelectionPlan } from "../actions/marketing-actions"; | ||
| import { MAX_PER_PAGE } from "../utils/constants"; | ||
|
|
||
| const EditSelectionPlanPage = React.lazy(() => | ||
| import("../pages/selection-plans/edit-selection-plan-page") | ||
| ); | ||
| const SelectionPlanExtraQuestionsLayout = React.lazy(() => | ||
| import("./selection-plan-extra-questions-layout") | ||
| ); | ||
|
|
@@ -26,31 +30,53 @@ const SelectionPlanIdLayout = ({ | |
| resetSelectionPlanForm, | ||
| getMarketingSettingsBySelectionPlan | ||
| }) => { | ||
| const [hasLoaded, setHasLoaded] = useState(false); | ||
| const [hasError, setHasError] = useState(false); | ||
| const selectionPlanId = match.params.selection_plan_id; | ||
| const breadcrumb = selectionPlanId | ||
| ? currentSelectionPlan.name | ||
| : T.translate("general.new"); | ||
|
|
||
| useEffect(() => { | ||
| setHasLoaded(false); | ||
| setHasError(false); | ||
| if (!selectionPlanId) { | ||
| resetSelectionPlanForm(); | ||
| setHasLoaded(true); | ||
| } else { | ||
| getSelectionPlan(selectionPlanId).then(() => | ||
| getMarketingSettingsBySelectionPlan( | ||
| selectionPlanId, | ||
| null, | ||
| 1, | ||
| MAX_PER_PAGE | ||
| getSelectionPlan(selectionPlanId) | ||
| .then(() => | ||
| getMarketingSettingsBySelectionPlan( | ||
| selectionPlanId, | ||
| null, | ||
| 1, | ||
| MAX_PER_PAGE | ||
| ) | ||
| ) | ||
| ); | ||
| .then(() => setHasLoaded(true)) | ||
| .catch(() => setHasError(true)); | ||
| } | ||
| }, [selectionPlanId]); | ||
|
|
||
| if (hasError) { | ||
| return <Redirect to={`/app/summits/${currentSummit.id}/selection-plans`} />; | ||
| } | ||
|
|
||
| if (!hasLoaded || currentSelectionPlan.id !== Number(selectionPlanId || 0)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift 🔎 Supported by static analysis🤖 get_repo_knowledge executed:
Length of output: 46525 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- file outline ---'
ast-grep outline src/layouts/selection-plan-id-layout.js --view expanded
printf '%s\n' '--- target file ---'
cat -n src/layouts/selection-plan-id-layout.js
printf '%s\n' '--- bound identifiers and related actions ---'
rg -n --glob '!node_modules' 'currentSelectionPlan|hasLoaded|selectionPlanId|load.*Selection|SelectionPlan|marketingSettings' src test tests 2>/dev/null | head -240Repository: fntechgit/summit-admin Length of output: 32276 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- selection-plan action implementation ---'
sed -n '1,175p' src/actions/selection-plan-actions.js
printf '%s\n' '--- selection-plan reducer files ---'
fd -i 'selection.*plan.*reducer|selection-plan-reducer' src/reducers
printf '%s\n' '--- reducer definition and action constants ---'
rg -n -A100 -B15 'export const selectionPlanReducer|function selectionPlanReducer|SELECTION_PLAN_(REQUEST|RECEIVED|RESET)|RECEIVE_SELECTION_PLAN|REQUEST_SELECTION_PLAN' src/reducers src/actions
printf '%s\n' '--- marketing action implementation ---'
sed -n '125,180p' src/actions/marketing-actions.js
printf '%s\n' '--- layout tests ---'
cat -n src/layouts/__tests__/selection-plan-id-layout.test.jsRepository: fntechgit/summit-admin Length of output: 50379 Prevent stale selection-plan responses from updating route state. When navigation changes from plan A to plan B, 🤖 Prompt for AI Agents🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Handle rejected loads before leaving the route blank.
🤖 Prompt for AI Agents |
||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <div> | ||
| <Breadcrumb data={{ title: breadcrumb, pathname: match.url }} /> | ||
| <Suspense fallback={<AjaxLoader show relative size={120} />}> | ||
| <Switch> | ||
| <Route | ||
| strict | ||
| exact | ||
| path={`${match.url}`} | ||
| component={EditSelectionPlanPage} | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| /> | ||
| <Route | ||
| path={`${match.url}/extra-questions`} | ||
| component={SelectionPlanExtraQuestionsLayout} | ||
|
|
@@ -59,7 +85,7 @@ const SelectionPlanIdLayout = ({ | |
| path={`${match.url}/rating-types`} | ||
| component={SelectionPlanRatingTypesLayout} | ||
| /> | ||
| <Redirect to={`/app/summits/${currentSummit.id}/selection-plans`} /> | ||
| <Route component={NoMatchPage} /> | ||
| </Switch> | ||
| </Suspense> | ||
| </div> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need any of these tests @tomrndom