OSAC-2452: add UI requirements to catalog items EP - #115
openshift-merge-bot[bot] merged 15 commits into
Conversation
|
@ElayAharoni: This pull request references OSAC-2452 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe catalog-items README updates its metadata, defines role-specific catalog item workflows, and adds a constraint preventing deletion of templates referenced by catalog items. ChangesCatalog Item Requirements
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks 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 |
AI Design Review: EP-115Score: 7/8 | Verdict: PASS
Verdict: A thorough and well-structured UI addition to the catalog items EP that covers role-gated management workflows comprehensively, held back from a perfect score by an unresolved search strategy and missing pagination design. Feedback: Resolve the search ambiguity: commit to either client-side filtering (acceptable if catalog item counts are bounded and small) or server-side filtering via the Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
ElayAharoni
assigned
rawagner,
AlonaKaplan and
batzionb and unassigned
rawagner,
AlonaKaplan and
batzionb
AI Design Review: EP-115Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured UI design section that comprehensively covers catalog item management for both admin roles, with clear API mappings, role-gated behavior, concrete test scenarios, and appropriate scope boundaries. Feedback: Two areas to tighten: (1) Decide on the search strategy — 'client-side on the loaded list, or server-side via the filter query parameter' leaves implementers guessing; pick one and document the pagination/data-loading approach (what happens with 500+ catalog items?). (2) The field definitions editor's 'Dynamic input' for default values ('Type depends on the field; free-form JSON value input') could benefit from specifying what widget types map to which field types, since this is the most complex UX in the form and implementers will need clearer guidance. Critical (0)None. Important (2)
Suggestions (4)
Review costModel: claude-opus-4-6 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@enhancements/catalog-items/README.md`:
- Around line 635-642: Extend the catalog management UI tests to cover
successful creation’s post-create redirect: verify the create operation returns
the new catalog item ID and that navigation reaches the corresponding detail
page, rather than only asserting creation capability or a toast.
- Around line 307-315: Update the catalog admin list flow described in the
README to use bounded pagination and server-side tenant/publication filtering
rather than loading and filtering all items client-side. Replace per-row
template Get calls with template display metadata returned by the API, or a
batched/cached lookup strategy, so rendering does not issue N+1 requests.
- Around line 412-415: Resolve the inconsistency around the catalog item detail
page by choosing one scope and applying it consistently: either make the detail
page mandatory in this section and all related requirements, including the
graduation criteria, or remove it from the graduation criteria and related
requirements while retaining its optional wording.
- Around line 460-463: Revise the proposal so useSession() only selects the
client-facing API route and is not treated as an authorization boundary. Require
the Fulfillment Service to authenticate requests and enforce Cloud Provider
Admin versus Tenant Admin permissions plus tenant resource scoping on every
operation, including direct or manipulated requests.
- Around line 336-348: Update the public API response contract used by the
Tenant Admin list page to expose an authoritative scope or ownership/capability
discriminator for distinguishing Global from Organization items, without
exposing the absent tenant field. Ensure the UI uses this metadata to label
scope and disable Edit/Delete for Global items, while retaining server-side
authorization as the final enforcement point.
- Around line 420-422: Add a documented CNA list/query API contract for the
detail page near the Related resources section, defining the catalog_item
filter, tenant and role scoping requirements, pagination parameters, and
response shape for CNAs (Clusters, ComputeInstances, and BareMetalInstances).
Ensure the API table references this contract so the UI has an explicit source
of truth for loading related resources.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f5c4ad0f-48d6-448d-b960-eab033789f65
📒 Files selected for processing (1)
enhancements/catalog-items/README.md
AI Design Review: EP-115Score: 8/8 | Verdict: PASS
Verdict: A well-structured UI specification that cleanly extends an existing EP with role-gated catalog management screens, leveraging established patterns and maintaining server-side authorization as the sole enforcement boundary. Feedback: This is a strong addition. Two minor improvements to consider: (1) the field definitions editor's JSON Schema code editor and drag-and-drop reorder are the most complex UI elements — consider noting which existing component library primitives you'll use (e.g., dnd-kit, Monaco) to confirm they're available in the project; (2) the test plan could add a scenario for the API hook selection logic — verifying that the correct API tier (private vs public) is called based on the user's role, since a mismatch would expose data or cause authorization errors. Critical (0)None. Important (1)
Suggestions (2)
Review costModel: claude-opus-4-6 |
AI EP Review: EP-115Score: 10/10 | Verdict: PASS
Verdict: A thorough, well-structured PRD with clear personas, concrete justification, and highly specific requirements — the strongest areas are the detailed functional requirements and clear scope boundaries, though the document would benefit from removing implementation-level prescriptiveness. Feedback: The PRD is strong and ready for implementation. Two actionable improvements: (1) Separate user-facing requirements from implementation guidance — move references to specific frameworks (React Query, Formik, Yup, PatternFly 6), internal functions (navRowsForRole(), useSession()), and implementation patterns (FieldMask, OPA/Authorino) into the EP's implementation details section, keeping the PRD focused on user-observable outcomes. (2) Fix the section numbering gap (2.1 Goals jumps to 2.3 Non-Goals, skipping 2.2) and consider adding brief user stories to complement the functional requirements for non-technical stakeholders. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-115Score: 5/8 | Verdict: PASS
Verdict: The design revision significantly strengthens persona coverage and validation constraint specificity, earning a narrow pass at 5/8, but is held back by underspecified implementation of constraint comparison ('tightening only') and thin test coverage for the complex validation logic. Feedback: The 'tightening only' enforcement for Tenant Admin constraints is the hardest part of this design and needs an explicit algorithm — define what 'tighter' means for each JSON Schema keyword (e.g., child minimum >= parent minimum, child enum ⊆ parent enum) and describe how the server compares schemas. Add unit test scenarios for validation edge cases (tightening comparison, resourceRef resolution failures, nested constraint validation) and at least one e2e scenario for the Tenant Admin constraint-inheritance flow. Consider whether resourceRef should be modeled as a first-class proto field annotation rather than a JSON Schema extension, and add owner reference annotations for the catalog item → template relationship per OSAC conventions. Critical (0)None. Important (4)
Suggestions (4)
Review costModel: claude-opus-4-6 |
Templates enumerate their parameter definitions and the catalog item's field list is pre-populated from them. The admin configures each field (editable, default, validation) but does not add or remove fields. The server rejects catalog items with fields not defined by the template. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
AI Design Review: EP-115Score: 5/8 | Verdict: PASS
Verdict: The design passes with a total of 5/8 — strong persona coverage and scope definition lift it, but flat object structure (missing spec/status split), thin test plan updates for new functionality, and underspecified edge cases in the hierarchical catalog model hold it back from a higher score. Feedback: Restructure the CatalogItem proto to follow the standard OSAC object shape with Critical (0)None. Important (5)
Suggestions (4)
Review costModel: claude-opus-4-6 |
The field set is fixed per resource type (ClusterSpec, ComputeInstanceSpec, etc.) and does not vary by template selection. The admin configures each field but the available fields are determined by the resource spec. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
AI Design Review: EP-115Score: 7/8 | Verdict: PASS
Verdict: A strong design revision that dramatically expands persona coverage and field definition detail; the main gap is a test plan that lacks unit test specifics and e2e scenarios. Feedback: The test plan should specify what unit tests cover — at minimum: JSON Schema validation logic (constraint application and rejection of invalid schemas), constraint-tightening validation for Tenant Admin creates (verifying that loosening constraints from the base item is rejected), dot-notation path parsing for nested and map fields, and default value injection for non-editable fields. Add at least one e2e scenario, e.g.: 'Cloud Provider Admin creates a ClusterCatalogItem with mixed editable/non-editable fields → Tenant User provisions a cluster providing only editable values → verify the resulting cluster has the correct merged configuration.' Consider whether the Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
- Remove implication that only specific JSON Schema keywords are accepted by the backend — validation_schema accepts any valid draft 2020-12 - Replace "no raw JSON Schema toggle" with Basic/Advanced mode description: Basic mode uses structured form controls for common constraints, Advanced mode provides a raw JSON Schema textarea for power users - Mode auto-detects based on existing schema content on load Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
AI Design Review: EP-115Score: 6/8 | Verdict: PASS
Verdict: The design is solid on scope and feasibility — comprehensive persona coverage and detailed validation constraint specification — but loses points on architecture (custom resourceRef keyword contradicts 'any valid JSON Schema' claim, tenant annotations not mapped to standard OSAC patterns) and testability (test plan additions don't cover the new validation, restriction, and deletion protection logic). Feedback: Two areas need attention before merge: (1) Resolve the resourceRef contradiction — either document it as a custom JSON Schema extension with its own validation path (not part of the standard JSON Schema validator), or redesign it as a separate field on FieldDefinition rather than embedding it in validation_schema. The current framing ('no keywords are restricted' + non-standard keyword) will confuse implementers and break standard validators. (2) Expand the test plan to cover the new functionality: validation schema enforcement (at least one test per constraint category), Tenant Admin restriction enforcement (cannot loosen constraints beyond base item), and template deletion protection (reject delete when referenced by catalog items). One test case for the scope of these changes is insufficient. Critical (0)None. Important (4)
Suggestions (3)
Review costModel: claude-opus-4-6 |
| - Fields that reference backend resources (such as `instance_type` referencing an InstanceType, or `image_type` referencing an ImageType) use a dedicated `"resourceRef"` constraint type rather than a static `enum`. The backend resolves available resources dynamically, and the UI presents them as a selectable list fetched from the corresponding API endpoint. The admin can optionally restrict the set to a subset of available resources. | ||
| Example: `instance_type` with `{"resourceRef": "InstanceType"}` — the UI fetches available InstanceType resources and presents them as a dropdown. The admin can further restrict by adding `{"resourceRef": "InstanceType", "enum": ["cx3.xlarge", "cx3.2xlarge"]}` to limit to specific instance types. | ||
| Example: `image_type` with `{"resourceRef": "ImageType"}` — the UI fetches available ImageType resources for selection. | ||
| - Resource references enable the backend to validate that the selected value is a valid, existing resource at provisioning time — not just a string that matches an enum. |
There was a problem hiding this comment.
This would require backend to add support for resourceRef, which doesnt exist now.
Why can't we keep using the enum ?
| ClusterCatalogItem | ||
| * references an existing ClusterTemplate by ID | ||
| * includes a list of field definitions, each of which specifies a field by dot-notation path, whether it is editable by the user, an optional default value, and an optional JSON Schema validation rule | ||
| * includes a list of field definitions covering all fields from the resource spec (e.g., ClusterSpec or ComputeInstanceSpec). Each field definition specifies a field by dot-notation path, whether it is editable by the user, an optional default value, and an optional JSON Schema validation rule. The field set is fixed per resource type — it does not vary by template. |
There was a problem hiding this comment.
This is a change from the current implementation, which does not require all fields to be in field definitions.
There was a problem hiding this comment.
Correct, not all fields have to be included. But the CatalogItem creator must list every field that the user is expected to provide, even fields they don't want to restrict; that's the EP design. If a field isn't listed, it's not available to the user.
| - `description` (string) - markdown-formatted long description. All consumers | ||
| that render this field (UI detail pages, catalog browsing, CLI output) must | ||
| use a sanitizing Markdown renderer that strips unsafe HTML tags, `javascript:` | ||
| URL schemes, and other XSS vectors. The server stores the raw Markdown as | ||
| provided; sanitization is a rendering-time responsibility. |
There was a problem hiding this comment.
I dont think we need this much detail into how the rendering is done. The original markdown-formatted long description is enough
1. Replace resourceRef with enum for resource reference constraints - the backend does not support resourceRef, so resource references use standard enum constraints populated from available resources. 2. Field definitions no longer require all fields from the resource spec - only the fields the admin wants to configure need to be included. This aligns with the current implementation. 3. Simplify the description field definition - remove the detailed Markdown sanitization requirements, keep it as "markdown-formatted long description" per reviewer feedback. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
AI Design Review: EP-115Score: 4/8 | Verdict: PASS
Verdict: The diff makes substantial improvements — detailed user stories per persona, rich JSON Schema constraint examples, template deletion protection, and a clearer partial-coverage field model — but introduces a significant gap between the new Tenant Admin user stories (create from catalog items, tightening only) and the implementation section, which still describes templates as the base reference with no proto support for catalog-item-based derivation. Feedback: The most impactful improvement would be aligning the Implementation Details section with the new Tenant Admin model: add a Critical (0)None. Important (4)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb, ElayAharoni, rawagner 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 |
Address rawagner's review feedback from PR osac-project#115: - Replace resourceRef custom keyword with standard enum constraints - Change field definitions from all-fields-required to admin-selected subset - Simplify markdown description (remove sanitization details) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Address rawagner's review feedback from PR osac-project#115: - Replace resourceRef custom keyword with standard enum constraints - Change field definitions from all-fields-required to admin-selected subset - Simplify markdown description (remove sanitization details) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Address rawagner's review feedback from PR osac-project#115: - Replace resourceRef custom keyword with standard enum constraints - Change field definitions from all-fields-required to admin-selected subset - Simplify markdown description (remove sanitization details) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Address rawagner's review feedback from PR osac-project#115: - Replace resourceRef custom keyword with standard enum constraints - Change field definitions from all-fields-required to admin-selected subset - Simplify markdown description (remove sanitization details) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
6 participants
Summary
What's added
The EP was written before UI work started and covered only API, database, and CLI. This PR adds the missing UI layer:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit