[NAE-2047] Fulltext search and advanced search usability with multichoice caseRef #272
Conversation
…oice caseRef - fix the problem with selecting all things in multichoice caseref - move logic to components-core - add 100 item option to pagination
…oice caseRef - fix test
Kovy95
requested review from
Retoocs,
camperko,
SamuelPalaj,
MartinSiran and
mazarijuraj
…oice caseRef - fix on init
|
WalkthroughThe changes introduce enhancements to approval data field handling in header components, add synchronous access to cases in the case view service, and expand pagination options to include a page size of 100. The header component logic is refactored to delegate approval form control management to its abstract superclass. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HeaderComponent
participant AbstractHeaderComponent
participant CaseViewService
participant DataField (MultichoiceField/EnumerationField)
User->>HeaderComponent: Interacts with approval form control
HeaderComponent->>AbstractHeaderComponent: Delegates approval logic
AbstractHeaderComponent->>CaseViewService: Gets current cases
AbstractHeaderComponent->>DataField: Reads/writes approval value
AbstractHeaderComponent-->>HeaderComponent: Updates form control state
HeaderComponent-->>User: Reflects updated approval state
sequenceDiagram
participant Consumer
participant CaseViewService
Consumer->>CaseViewService: Accesses cases via cases getter
CaseViewService-->>Consumer: Returns current cases array
Note right of CaseViewService: _cases kept in sync with observable emissions
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
projects/netgrif-components-core/src/lib/panel/task-panel-list/task-panel-list-pagination/abstract-task-list-pagination.component.ts (1)
21-21: Style consistency: usenumber[]forpageSizeOptions.Align the type annotation with the case list paginator to reduce inconsistencies.
Apply this diff:
- public pageSizeOptions: Array<number> = [10, 20, 50, 100]; + public pageSizeOptions: number[] = [10, 20, 50, 100];projects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.ts (3)
117-118: Remove redundant type casting in tap operatorThe type casting
as Array<Case>is unnecessary since themapoperator on line 117 already returnsArray<Case>.-tap(cases => this._cases = cases as Array<Case>), +tap(cases => this._cases = cases),
140-142: Add documentation for the synchronous cases getterThe new synchronous getter should be documented to clarify its purpose and usage, especially since it exposes internal state that could become stale.
+/** + * Returns the current synchronous snapshot of cases. + * Note: This is a snapshot that gets updated via the observable stream. + * For reactive updates, prefer using cases$ observable instead. + */ public get cases(): Array<Case> { return this._cases; }
50-50: Consider memory implications for large case datasetsThe synchronous
_casesarray duplicates data from the observable stream. For applications handling large numbers of cases, this could impact memory usage. Consider if the synchronous access pattern is essential or if the components could be refactored to work with the observable stream directly.Also applies to: 71-71, 117-118, 140-142
projects/netgrif-components-core/src/lib/header/abstract-header.component.ts (2)
60-65: Consider documenting the_changeValueinitialization.The constructor properly injects the new optional dependencies. However, initializing
_changeValuetotruemight be counterintuitive. Consider adding a comment explaining why it starts astruerather thanfalse.constructor(protected _injector: Injector, protected _translate: TranslateService, @Optional() protected _overflowService: OverflowService, @Optional() protected _caseViewService: CaseViewService, @Optional() @Inject(DATA_FIELD_PORTAL_DATA) protected _dataFieldPortalData: DataFieldPortalData<MultichoiceField | EnumerationField>) { this.initializeFormControls(this._overflowService !== null); + // Initialize to true to allow the first value change to propagate this._changeValue = true; }
262-267: Consider optimizing the selection check for better performance.The current implementation uses
every()withincludes()which has O(n*m) complexity. For large datasets, this could be optimized using a Set.protected resolveApprovalValue() { if (this._caseViewService.cases.length === 0) { return false; } - return this._caseViewService.cases.every(value => this._dataFieldPortalData?.dataField.value.includes(value.stringId)); + const selectedIds = new Set(this._dataFieldPortalData?.dataField.value || []); + return this._caseViewService.cases.every(value => selectedIds.has(value.stringId)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
projects/netgrif-components-core/src/lib/data-fields/case-ref-field/model/abstract-case-ref-base-field-component.ts(1 hunks)projects/netgrif-components-core/src/lib/header/abstract-header.component.spec.ts(3 hunks)projects/netgrif-components-core/src/lib/header/abstract-header.component.ts(6 hunks)projects/netgrif-components-core/src/lib/panel/task-panel-list/task-panel-list-pagination/abstract-task-list-pagination.component.ts(1 hunks)projects/netgrif-components-core/src/lib/view/case-view/components/case-list-paginator/abstract-case-list-paginator.component.ts(1 hunks)projects/netgrif-components-core/src/lib/view/case-view/service/case-view-service.ts(4 hunks)projects/netgrif-components/src/lib/header/header.component.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
projects/netgrif-components-core/src/lib/header/abstract-header.component.ts (3)
projects/netgrif-components-core/src/lib/data-fields/models/data-field-portal-data-injection-token.ts (2)
DATA_FIELD_PORTAL_DATA(15-15)DataFieldPortalData(6-13)projects/netgrif-components-core/src/lib/data-fields/multichoice-field/models/multichoice-field.ts (1)
MultichoiceField(15-64)projects/netgrif-components-core/src/lib/data-fields/enumeration-field/models/enumeration-field.ts (1)
EnumerationField(22-88)
projects/netgrif-components-core/src/lib/header/abstract-header.component.spec.ts (3)
projects/netgrif-components-core/src/lib/data-fields/models/data-field-portal-data-injection-token.ts (2)
DATA_FIELD_PORTAL_DATA(15-15)DataFieldPortalData(6-13)projects/netgrif-components-core/src/lib/data-fields/multichoice-field/models/multichoice-field.ts (1)
MultichoiceField(15-64)projects/netgrif-components-core/src/lib/data-fields/enumeration-field/models/enumeration-field.ts (1)
EnumerationField(22-88)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (16)
- GitHub Check: Matrix Test (18)
- GitHub Check: Matrix Test (14)
🔇 Additional comments (8)
projects/netgrif-components-core/src/lib/view/case-view/components/case-list-paginator/abstract-case-list-paginator.component.ts (1)
20-20: Approve: Added support for 100 items per page.Including 100 in
pageSizeOptionsaligns this paginator with user expectations for bulk navigation.projects/netgrif-components-core/src/lib/panel/task-panel-list/task-panel-list-pagination/abstract-task-list-pagination.component.ts (1)
21-21: Approve: Added support for 100 items per page.Aligns task list pagination with the case list paginator for consistency in large lists.
projects/netgrif-components-core/src/lib/data-fields/case-ref-field/model/abstract-case-ref-base-field-component.ts (1)
35-35: Good defensive programming practice!Adding the optional chaining operator prevents potential runtime errors when
headersisundefinedornull.projects/netgrif-components/src/lib/header/header.component.ts (1)
25-33: Clean refactoring to delegate logic to parent class!The simplification appropriately moves the approval data field handling logic to
AbstractHeaderComponent, following the DRY principle and maintaining proper separation of concerns.projects/netgrif-components-core/src/lib/header/abstract-header.component.spec.ts (1)
7-7: Test setup properly updated for new dependencies.The test file correctly reflects the changes made to
AbstractHeaderComponentby adding the necessary imports and updating the constructor signature to include the new optional dependencies.Also applies to: 32-38, 99-102
projects/netgrif-components-core/src/lib/header/abstract-header.component.ts (3)
1-1: Well-structured imports and property declarations.The new imports are properly organized, and the added properties are appropriately typed. The
_changeValueflag for preventing feedback loops is a good practice for bidirectional data binding scenarios.Also applies to: 15-21, 55-56
108-108: Proper lifecycle management.Good implementation of lifecycle hooks with appropriate initialization in
ngOnInitand proper cleanup of subscriptions inngOnDestroyto prevent memory leaks.Also applies to: 117-119
219-227:❓ Verification inconclusive
Inconsistent logic between branches in
indeterminate()method.The method has fundamentally different logic depending on whether
_caseViewServiceis available:
- With service: checks if some (but not all) cases are selected
- Without service: compares value length with choices length
This could lead to unexpected behavior in different contexts. Consider unifying the logic or documenting why different approaches are needed.
Can you clarify the intended behavior when
_caseViewServiceis not available? The current implementation might not correctly represent an indeterminate state.
Clarify
indeterminate()logic when_caseViewServiceis unavailable.The two branches in
indeterminate()behave quite differently:
- With
_caseViewService: checks if any case IDs are selected and not all approvals are resolved.- Without
_caseViewService: simply compares the length ofvalueagainst the number ofchoices.This divergence may lead to inconsistent “indeterminate” states across contexts.
• File: projects/netgrif-components-core/src/lib/header/abstract-header.component.ts
• Lines: 219–227Can you confirm the intended behavior when
_caseViewServiceisn’t injected? Should both paths use the same criteria for “indeterminate,” or is there a specific reason for the discrepancy? Consider aligning the logic or documenting why they differ.
| protected resolveApprovalDatafields() { | ||
| if (this._dataFieldPortalData !== null && this._dataFieldPortalData.dataField instanceof MultichoiceField && this._caseViewService) { | ||
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | ||
| this.approvalFormControl.valueChanges.subscribe(value => { | ||
| if (this._changeValue) { | ||
| if (value) { | ||
| this._dataFieldPortalData.dataField.value = this._caseViewService.cases.map(caze => caze.stringId); | ||
| } else { | ||
| this._dataFieldPortalData.dataField.value = []; | ||
| } | ||
| } | ||
| this._changeValue = true; | ||
| }) | ||
| this._dataFieldPortalData.dataField.valueChanges().subscribe(() => { | ||
| this._changeValue = false; | ||
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | ||
| }) | ||
| this._subCases = this._caseViewService.cases$.subscribe(() => { | ||
| this._changeValue = false; | ||
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | ||
| }) | ||
| } | ||
| if (this._dataFieldPortalData !== null && this._dataFieldPortalData.dataField instanceof EnumerationField) { | ||
| this.approvalFormControl.valueChanges.subscribe(value => { | ||
| this._dataFieldPortalData.dataField.value = null; | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve null checks and clarify EnumerationField behavior.
- The null check should also handle
undefined:
-if (this._dataFieldPortalData !== null && this._dataFieldPortalData.dataField instanceof MultichoiceField && this._caseViewService) {
+if (this._dataFieldPortalData && this._dataFieldPortalData.dataField instanceof MultichoiceField && this._caseViewService) {- For
EnumerationField, the approval control always sets the field value tonull. Is this the intended behavior? It seems destructive and might confuse users.
Consider splitting this method into separate handlers for each field type to improve readability and maintainability.
📝 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.
| protected resolveApprovalDatafields() { | |
| if (this._dataFieldPortalData !== null && this._dataFieldPortalData.dataField instanceof MultichoiceField && this._caseViewService) { | |
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | |
| this.approvalFormControl.valueChanges.subscribe(value => { | |
| if (this._changeValue) { | |
| if (value) { | |
| this._dataFieldPortalData.dataField.value = this._caseViewService.cases.map(caze => caze.stringId); | |
| } else { | |
| this._dataFieldPortalData.dataField.value = []; | |
| } | |
| } | |
| this._changeValue = true; | |
| }) | |
| this._dataFieldPortalData.dataField.valueChanges().subscribe(() => { | |
| this._changeValue = false; | |
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | |
| }) | |
| this._subCases = this._caseViewService.cases$.subscribe(() => { | |
| this._changeValue = false; | |
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | |
| }) | |
| } | |
| if (this._dataFieldPortalData !== null && this._dataFieldPortalData.dataField instanceof EnumerationField) { | |
| this.approvalFormControl.valueChanges.subscribe(value => { | |
| this._dataFieldPortalData.dataField.value = null; | |
| }) | |
| } | |
| } | |
| protected resolveApprovalDatafields() { | |
| if (this._dataFieldPortalData && this._dataFieldPortalData.dataField instanceof MultichoiceField && this._caseViewService) { | |
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | |
| this.approvalFormControl.valueChanges.subscribe(value => { | |
| if (this._changeValue) { | |
| if (value) { | |
| this._dataFieldPortalData.dataField.value = this._caseViewService.cases.map(caze => caze.stringId); | |
| } else { | |
| this._dataFieldPortalData.dataField.value = []; | |
| } | |
| } | |
| this._changeValue = true; | |
| }) | |
| this._dataFieldPortalData.dataField.valueChanges().subscribe(() => { | |
| this._changeValue = false; | |
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | |
| }) | |
| this._subCases = this._caseViewService.cases$.subscribe(() => { | |
| this._changeValue = false; | |
| this.approvalFormControl.setValue(this.resolveApprovalValue()); | |
| }) | |
| } | |
| if (this._dataFieldPortalData !== null && this._dataFieldPortalData.dataField instanceof EnumerationField) { | |
| this.approvalFormControl.valueChanges.subscribe(value => { | |
| this._dataFieldPortalData.dataField.value = null; | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In projects/netgrif-components-core/src/lib/header/abstract-header.component.ts
around lines 233 to 260, update the null checks to also handle undefined values
for _dataFieldPortalData to make the checks more robust. Review the logic for
EnumerationField where approvalFormControl.valueChanges always sets the
dataField value to null, confirm if this is intended or if it should preserve or
update the value differently to avoid unintended data loss. Refactor the
resolveApprovalDatafields method by splitting the logic into separate handler
methods for MultichoiceField and EnumerationField to improve code readability
and maintainability.
|
Description
Implements NAE-2047
Dependencies
none
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
manually
Test Configuration
Checklist:
Summary by CodeRabbit