feature/list blocks update with initialFilters option#409
Conversation
… feature/list-blocks-update-with-new-option
…l, service, and mappers
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughThe diff adds an optional Changes
Sequence DiagramsequenceDiagram
autonumber
participant CMS as CMS Block
participant Mapper as Block Mapper
participant Service as List Service
participant API as Backend API
CMS->>Mapper: block data + cms.initialFilters
Mapper->>Service: mappedBlock { ..., initialFilters }
Service->>Service: params = { ...(cms.initialFilters||{}), ...query, locale?, limit, offset }
Service->>API: getList(params)
API-->>Service: results
Service-->>Mapper: mapped results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts (1)
42-42: PlaceholderinitialFiltersis fine; remember to wire real CMS field laterSetting
initialFilters: undefinedkeeps the type in sync without changing behavior, which is appropriate while Strapi lacks this field. Once the CMS schema exposesinitialFilters, this mapper should be updated to map the actual value (likely via a dedicated mapper, mirroring other blocks) and the TODO can be removed.packages/blocks/notification-list/src/api-harmonization/notification-list.model.ts (1)
22-22:initialFilterstype is reasonable and consistent with domain modelUsing
Partial<Notifications.Model.Notification & { sort?: string }>forinitialFiltersaligns the default filter shape with the underlying notification domain plus the extrasortfield, and matches patterns used in other list blocks. Just ensure any future filter fields that aren’t part ofNotifications.Model.Notification(e.g. additional synthetic filter keys) are reflected here if they are meant to be configurable as initial filters.packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts (1)
43-43: ConsistentinitialFiltersplaceholder; update once Strapi exposes the fieldAs with the order-list mapper, explicitly setting
initialFilters: undefinedkeeps the CMS TicketListBlock shape aligned with the rest of the system without altering behavior. When Strapi adds this field, this mapper should be updated to map the real value and the TODO can be cleaned up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/blocks/invoice-list/src/api-harmonization/invoice-list.mapper.ts(1 hunks)packages/blocks/invoice-list/src/api-harmonization/invoice-list.model.ts(1 hunks)packages/blocks/invoice-list/src/api-harmonization/invoice-list.service.ts(1 hunks)packages/blocks/notification-list/src/api-harmonization/notification-list.mapper.ts(1 hunks)packages/blocks/notification-list/src/api-harmonization/notification-list.model.ts(1 hunks)packages/blocks/notification-list/src/api-harmonization/notification-list.service.ts(1 hunks)packages/blocks/order-list/src/api-harmonization/order-list.mapper.ts(1 hunks)packages/blocks/order-list/src/api-harmonization/order-list.model.ts(1 hunks)packages/blocks/order-list/src/api-harmonization/order-list.service.ts(1 hunks)packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts(1 hunks)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts(1 hunks)packages/blocks/ticket-list/src/api-harmonization/ticket-list.service.ts(1 hunks)packages/framework/src/modules/cms/models/blocks/invoice-list.model.ts(1 hunks)packages/framework/src/modules/cms/models/blocks/notification-list.model.ts(1 hunks)packages/framework/src/modules/cms/models/blocks/order-list.model.ts(1 hunks)packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts(1 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts(3 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.notification-list.mapper.ts(3 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts(3 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts(3 hunks)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts(1 hunks)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.notification-list.mapper.ts(1 hunks)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts(1 hunks)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T15:35:13.879Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 348
File: packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts:23-27
Timestamp: 2025-11-13T15:35:13.879Z
Learning: In the notification-summary block service (packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts), the hardcoded limit of 1000 notifications when calling notificationService.getNotificationList() is intentional, as there is no server-side aggregation API available for fetching notification counts by priority.
Applied to files:
packages/blocks/notification-list/src/api-harmonization/notification-list.service.tspackages/blocks/notification-list/src/api-harmonization/notification-list.model.tspackages/framework/src/modules/cms/models/blocks/notification-list.model.ts
🧬 Code graph analysis (4)
packages/framework/src/modules/cms/models/blocks/notification-list.model.ts (1)
packages/blocks/notification-list/src/api-harmonization/notification-list.model.ts (1)
Notification(25-43)
packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (1)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
Ticket(30-47)
packages/framework/src/modules/cms/models/blocks/invoice-list.model.ts (1)
packages/blocks/invoice-list/src/api-harmonization/invoice-list.model.ts (1)
Invoice(29-50)
packages/blocks/order-list/src/api-harmonization/order-list.model.ts (1)
packages/framework/src/modules/orders/orders.model.ts (1)
Orders(84-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (19)
packages/framework/src/modules/cms/models/blocks/notification-list.model.ts (1)
22-22: LGTM! Type definition aligns with similar blocks.The
initialFiltersproperty is properly typed as optional and usesPartial<Notification & { sort?: string }>, consistent with the filter patterns used in other list blocks (invoice, order, ticket).packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.notification-list.mapper.ts (1)
42-42: LGTM! Placeholder is appropriate.Setting
initialFilters: undefinedwith a TODO comment is a safe approach until the CMS schema is updated to support this field.packages/blocks/notification-list/src/api-harmonization/notification-list.service.ts (1)
28-33: LGTM! Parameter precedence is correct.The spread order ensures that:
initialFiltersprovides defaultsqueryparameters override defaults- Explicit
limit,offset, andlocaletake final precedenceThis correctly implements the intended behavior where user queries override CMS defaults.
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts (1)
41-41: LGTM! Placeholder consistent with other mappers.Setting
initialFilters: undefinedwith a TODO is appropriate until CMS schema support is added.packages/blocks/ticket-list/src/api-harmonization/ticket-list.service.ts (1)
28-33: LGTM! Implementation consistent with notification-list.The parameter merging and precedence logic matches the notification-list block, correctly prioritizing user queries over CMS defaults.
packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (1)
25-25: LGTM! Type definition consistent across blocks.The
initialFiltersproperty uses the same type pattern as other list blocks, maintaining consistency.packages/framework/src/modules/cms/models/blocks/order-list.model.ts (1)
24-24: LGTM! Type definition follows established pattern.The
initialFiltersproperty is properly typed and consistent with the other list blocks in this PR.packages/blocks/invoice-list/src/api-harmonization/invoice-list.model.ts (1)
26-26: Expose initialFilters on InvoiceListBlock
initialFilterstype is consistent with the invoice model and allows asortfield for query defaults; matches the pattern used across other list blocks.packages/blocks/order-list/src/api-harmonization/order-list.model.ts (1)
27-27: Expose initialFilters on OrderListBlockType and naming align with
filtersand the underlyingOrders.Model.Orderplus optionalsort, so this looks consistent and safe.packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
27-27: Expose initialFilters on TicketListBlockThis is a straightforward, consistent addition matching the other list blocks’
initialFiltersshape.packages/blocks/invoice-list/src/api-harmonization/invoice-list.mapper.ts (1)
30-30: Propagate CMS initialFilters in mapInvoiceListPassing through
cms.initialFilterscleanly exposes CMS-configured defaults on the block without changing existing behavior.packages/blocks/order-list/src/api-harmonization/order-list.mapper.ts (1)
29-29: Propagate CMS initialFilters in mapOrderList
initialFiltersis correctly forwarded from CMS to the block model, matching the other list mappers.packages/framework/src/modules/cms/models/blocks/invoice-list.model.ts (1)
23-23: Add initialFilters to CMS InvoiceListBlockCMS block now exposes
initialFilterswith a type compatible with the harmonization layer; this cleanly enables CMS-configured defaults.packages/blocks/order-list/src/api-harmonization/order-list.service.ts (1)
29-34: Correct precedence when merging CMS defaults and user queryMerging
{ ...(cms.initialFilters || {}), ...query }and then explicitly overridinglimit,offset, andlocalegives the right precedence: CMS defaults apply only when the user hasn’t specified a value, and pagination/locale stay authoritative from query/headers.packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts (1)
29-29: Propagate CMS initialFilters in mapTicketListThe mapper now correctly exposes
cms.initialFilterson the TicketListBlock, consistent with the other list mappers.packages/blocks/notification-list/src/api-harmonization/notification-list.mapper.ts (1)
27-27: Pass-through ofinitialFiltersfrom CMS looks correctPropagating
cms.initialFiltersinto the mappedNotificationListBlockkeeps the public API aligned with the CMS model and lets callers access configured defaults without changing existing behavior.packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts (1)
94-97: MockinitialFiltersvalues are consistent with defined filtersThe added
initialFiltersfor EN/DE/PL all use keys (status,sort) and values that exist in the corresponding filter option lists and domain enums, so they should work well as realistic defaults and good examples for consumers of the mocked CMS.Also applies to: 203-206, 312-315
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts (1)
154-158: Ticket mockinitialFiltersalign with field mappings and filter optionsThe initial filter presets for EN/DE/PL all use valid domain values for
status,topic,type, andsortthat are present in both the field mappings and the configured filter items, so they should behave correctly and give predictable default views per locale.Also applies to: 314-318, 475-480
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts (1)
114-117: Invoice mockinitialFiltersare consistent and useful defaultsThe added
initialFiltersfor EN/DE/PL correctly reference existingpaymentStatusandsortoptions, providing sensible default views (due, paid, past due) per locale. This keeps mocks aligned with the public model and should help exercise the new initial-filters behavior end-to-end.Also applies to: 242-245, 370-373
…cation list mapper
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/blocks/invoice-list/src/api-harmonization/invoice-list.service.ts (1)
28-33: Past review issue resolved: limit precedence now consistent.The limit logic has been updated to
query.limit || cms.pagination?.limit || 1, which addresses the previous review comment and aligns with the pattern used in other list blocks (Notification, Ticket, Order). The parameter precedence is now consistent:
cms.initialFiltersprovides CMS-configured defaultsqueryparams allow user overrides- Explicit
limit,offset, andlocaleensure required fields are setThe spreading order correctly prioritizes user query parameters over CMS defaults.
Based on learnings, if there's any concern about the initialFilters spreading introducing unexpected fields, consider adding type constraints in the CMS model definition to ensure only valid invoice list query parameters can be configured as initialFilters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/blocks/invoice-list/src/api-harmonization/invoice-list.service.ts(1 hunks)packages/framework/src/modules/invoices/invoices.request.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T15:35:13.879Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 348
File: packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts:23-27
Timestamp: 2025-11-13T15:35:13.879Z
Learning: In the notification-summary block service (packages/blocks/notification-summary/src/api-harmonization/notification-summary.service.ts), the hardcoded limit of 1000 notifications when calling notificationService.getNotificationList() is intentional, as there is no server-side aggregation API available for fetching notification counts by priority.
Applied to files:
packages/blocks/invoice-list/src/api-harmonization/invoice-list.service.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (1)
packages/framework/src/modules/invoices/invoices.request.ts (1)
15-15: LGTM! Locale support added to invoice list queries.The optional
localefield integrates cleanly with the existingGetInvoiceListQueryinterface and supports the PR's objective of enabling locale-specific invoice listings.
marcinkrasowski
left a comment
marcinkrasowski
left a comment
There was a problem hiding this comment.
please run npm run changeset and mark modified blocks and integrations as new minor versions
2 participants
What does this PR do?
Key Changes
Summary by CodeRabbit
New Features
Updates
Chores
✏️ Tip: You can customize this high-level summary in your review settings.