WIP ACM-30854 Remove PlacementRule from UI#5965
WIP ACM-30854 Remove PlacementRule from UI#5965oksanabaza wants to merge 2 commits intostolostron:mainfrom
Conversation
Signed-off-by: Oksana Bazylieva <obazylie@redhat.com>
Signed-off-by: Oksana Bazylieva <obazylie@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oksanabaza The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR consolidates "Placement Rules" to "Placements" by renaming control identifiers, state atoms, function exports, and type references across the codebase. Deprecated PlacementRule kind handling is removed from template logic, and the PlacementRule items list/table is removed from AdvancedConfiguration. Support for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)frontend/src/routes/Applications/CreateSubscriptionApplication/transformers/transform-resources-to-controls.test.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@oksanabaza: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/routes/Applications/CreateSubscriptionApplication/templates/templatePlacement.hbs (1)
7-19:⚠️ Potential issue | 🟠 MajorLegacy
PlacementRuleedits are now serialized asPlacement.This template no longer branches on the deprecated controls, but the edit pipeline still populates
isDeprecatedPR/deprecated-rulefor existing subscriptions that referencePlacementRule. Saving one of those apps will now emitplacementRef.kind: PlacementandselfLinks.Placement, which changes the referenced resource instead of preserving the legacy object.Also applies to: 36-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Applications/CreateSubscriptionApplication/templates/templatePlacement.hbs` around lines 7 - 19, The template currently always emits placementRef.kind: Placement causing legacy PlacementRule references to be converted; update templatePlacement.hbs to detect the legacy flags (isDeprecatedPR and/or deprecated-rule) and, when present, emit placementRef.kind as PlacementRule and preserve the original name/selfLink instead of forcing Placement: adjust the branching around selectedPlacementName and existing-placement-checkbox (and where `@root.name` / uniqueGroupID are used) so that legacy subscriptions keep kind: PlacementRule and their selfLinks.Placement handling rather than being rewritten to kind: Placement.
🧹 Nitpick comments (1)
frontend/src/routes/Applications/CreateSubscriptionApplication/SubscriptionApplication.test.tsx (1)
636-679: Keep one regression test for legacyPlacementRuleedit/save.These updates remove the only changed-path coverage around deprecated
PlacementRulesubscriptions, buttransform-resources-to-controls.jsstill has explicit handling forspec.placement.placementRef.kind === PlacementRule. A round-trip break there would now go uncaught. Please keep one edit/save test for a legacy subscription until that compatibility path is removed end-to-end.As per coding guidelines, "Verify test coverage is meaningful, not just for metrics".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Applications/CreateSubscriptionApplication/SubscriptionApplication.test.tsx` around lines 636 - 679, Restore a regression test that covers editing and saving a legacy PlacementRule subscription: add or keep one test (e.g., alongside or as a variant of the existing "edit a git subscription application" test in SubscriptionApplication.test.tsx) that mounts a subscription whose spec.placement.placementRef.kind is "PlacementRule", exercises the edit/save flow (clicking the git card, modifying fields, saving), and asserts round-trip behavior so transform-resources-to-controls.js handling for spec.placement.placementRef.kind === PlacementRule remains covered; reference the existing test harness (RecoilRoot state setup, MemoryRouter route to NavigationPath.editApplicationSubscription, and waitForNocks helpers) to implement this single preserved regression test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/routes/Applications/CreateSubscriptionApplication/controlData/ControlDataPlacement.js`:
- Around line 29-40: loadExistingPlacements currently only returns Placement
objects which drops support for legacy PlacementRule refs; change the query
returned by loadExistingPlacements to fetch and merge both Placements and
PlacementRules (or fetch PlacementRules as a fallback) using nsControl.active so
setAvailableRules receives a combined list and legacy PlacementRule names remain
in available; keep variables() assigning nsControl as-is and ensure
reverseExistingPlacement still reads
Subscription.spec.placement.placementRef.name (and/or
transform-resources-to-controls.js continues seeding PlacementRule state) so
legacy rule names are preserved or explicitly migrated during initialization.
In `@frontend/src/routes/Governance/policies/Policies.tsx`:
- Line 1113: The title prop is interpolating a raw literal 'Placements' into
t('policy.modal.message.reused', { kind: 'Placements' }) — replace the
hard-coded label with a localized string before interpolation (e.g., create/use
a translation key like t('policy.kind.placements') or
t('policy.kind.placementLabel') and pass that value as the kind parameter) so
the call becomes t('policy.modal.message.reused', { kind:
t('policy.kind.placements') }); update Policies.tsx where title is set to use
the localized kind.
---
Outside diff comments:
In
`@frontend/src/routes/Applications/CreateSubscriptionApplication/templates/templatePlacement.hbs`:
- Around line 7-19: The template currently always emits placementRef.kind:
Placement causing legacy PlacementRule references to be converted; update
templatePlacement.hbs to detect the legacy flags (isDeprecatedPR and/or
deprecated-rule) and, when present, emit placementRef.kind as PlacementRule and
preserve the original name/selfLink instead of forcing Placement: adjust the
branching around selectedPlacementName and existing-placement-checkbox (and
where `@root.name` / uniqueGroupID are used) so that legacy subscriptions keep
kind: PlacementRule and their selfLinks.Placement handling rather than being
rewritten to kind: Placement.
---
Nitpick comments:
In
`@frontend/src/routes/Applications/CreateSubscriptionApplication/SubscriptionApplication.test.tsx`:
- Around line 636-679: Restore a regression test that covers editing and saving
a legacy PlacementRule subscription: add or keep one test (e.g., alongside or as
a variant of the existing "edit a git subscription application" test in
SubscriptionApplication.test.tsx) that mounts a subscription whose
spec.placement.placementRef.kind is "PlacementRule", exercises the edit/save
flow (clicking the git card, modifying fields, saving), and asserts round-trip
behavior so transform-resources-to-controls.js handling for
spec.placement.placementRef.kind === PlacementRule remains covered; reference
the existing test harness (RecoilRoot state setup, MemoryRouter route to
NavigationPath.editApplicationSubscription, and waitForNocks helpers) to
implement this single preserved regression test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ac1871a-7791-403a-b77a-108388216bb6
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (15)
frontend/src/components/TemplateEditor/TemplateEditor.test.jsfrontend/src/components/TemplateEditor/controls/ControlPanelComboBox.test.jsfrontend/src/components/TemplateEditor/utils/initialize-control-data.test.tsxfrontend/src/routes/Applications/AdvancedConfiguration.test.tsxfrontend/src/routes/Applications/AdvancedConfiguration.tsxfrontend/src/routes/Applications/CreateSubscriptionApplication/SubscriptionApplication.test.tsxfrontend/src/routes/Applications/CreateSubscriptionApplication/controlData/ControlData.jsfrontend/src/routes/Applications/CreateSubscriptionApplication/controlData/ControlDataPlacement.jsfrontend/src/routes/Applications/CreateSubscriptionApplication/controlData/utils.jsfrontend/src/routes/Applications/CreateSubscriptionApplication/templates/template.hbsfrontend/src/routes/Applications/CreateSubscriptionApplication/templates/templatePlacement.hbsfrontend/src/routes/Applications/CreateSubscriptionApplication/transformers/transform-resources-to-controls.test.jsfrontend/src/routes/Applications/components/ToggleSelector.tsxfrontend/src/routes/Governance/policies/Policies.test.tsxfrontend/src/routes/Governance/policies/Policies.tsx
| export const loadExistingPlacements = (t) => { | ||
| let nsControl = undefined | ||
|
|
||
| return { | ||
| query: () => { | ||
| return Promise.all([listPlacementRules(nsControl.active).promise, listPlacements(nsControl.active).promise]) | ||
| return listPlacements(nsControl.active).promise | ||
| }, | ||
| variables: (control, globalControl) => { | ||
| nsControl = globalControl.find(({ id: idCtrl }) => idCtrl === 'namespace') | ||
| }, | ||
| loadingDesc: t('creation.app.loading.rules'), | ||
| loadingDesc: t('Loading placements...'), | ||
| setAvailable: setAvailableRules.bind(null), |
There was a problem hiding this comment.
This drops round-trip support for existing PlacementRule subscriptions.
transform-resources-to-controls.js still seeds deprecated PlacementRule state for edit flows, and reverseExistingPlacement() still loads the old ref name from Subscription.spec.placement.placementRef.name. After this change, the combo can only load Placement objects, so a legacy rule name can never exist in available and gets cleared during initialization instead of being preserved or explicitly migrated.
Also applies to: 66-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/routes/Applications/CreateSubscriptionApplication/controlData/ControlDataPlacement.js`
around lines 29 - 40, loadExistingPlacements currently only returns Placement
objects which drops support for legacy PlacementRule refs; change the query
returned by loadExistingPlacements to fetch and merge both Placements and
PlacementRules (or fetch PlacementRules as a fallback) using nsControl.active so
setAvailableRules receives a combined list and legacy PlacementRule names remain
in available; keep variables() assigning nsControl as-is and ensure
reverseExistingPlacement still reads
Subscription.spec.placement.placementRef.name (and/or
transform-resources-to-controls.js continues seeding PlacementRule state) so
legacy rule names are preserved or explicitly migrated during initialization.
| <AcmAlert | ||
| variant="warning" | ||
| title={t('policy.modal.message.reused', { kind: 'Placements/PlacementRules' })} | ||
| title={t('policy.modal.message.reused', { kind: 'Placements' })} |
There was a problem hiding this comment.
Localize interpolated kind label in warning title
'Placements' is user-facing text passed as a raw literal. Please localize it before interpolation.
Suggested fix
- title={t('policy.modal.message.reused', { kind: 'Placements' })}
+ title={t('policy.modal.message.reused', { kind: t('Placements') })}As per coding guidelines, "Ensure all user-facing strings use useTranslation() or from src/lib/acm-i18next."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| title={t('policy.modal.message.reused', { kind: 'Placements' })} | |
| title={t('policy.modal.message.reused', { kind: t('Placements') })} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/routes/Governance/policies/Policies.tsx` at line 1113, The title
prop is interpolating a raw literal 'Placements' into
t('policy.modal.message.reused', { kind: 'Placements' }) — replace the
hard-coded label with a localized string before interpolation (e.g., create/use
a translation key like t('policy.kind.placements') or
t('policy.kind.placementLabel') and pass that value as the kind parameter) so
the call becomes t('policy.modal.message.reused', { kind:
t('policy.kind.placements') }); update Policies.tsx where title is set to use
the localized kind.
1 participant
📝 Summary
Ticket Summary (Title):
ACM-30854 Remove PlacementRule from UI
Ticket Link:
https://redhat.atlassian.net/browse/ACM-30854
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
Release Notes