LNK-4651: Add Adhoc Type for AdhocReportGeneration - #1345
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces ad-hoc report type tracking by adding an Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Generate Report Form
participant Listener as GenerateReportListener
participant Factory as ScheduledReportFactory
participant Display as View Report Component
User->>UI: Select "Enter Patients" (manual) or "Use Census (Automatic)"
UI->>Listener: Create ad-hoc report request with patient selection
rect rgb(200, 220, 255)
Note over Listener: Determine AdHocType
Listener->>Listener: Check: Regenerate=false AND no PatientIds?
alt Automatic pathway
Listener->>Listener: isAutomatic = true<br/>AdHocType = Automatic
else Manual pathway
Listener->>Listener: isAutomatic = false<br/>AdHocType = Manual
end
end
Listener->>Factory: Create ScheduledReportListSummary<br/>with AdHocType
Factory->>Factory: Map reportScheduleModel.AdHocType<br/>to result.AdHocType
Factory-->>Display: Return mapped summary
Display->>Display: Render AdHocType field
Display-->>User: Display report with<br/>AdHocType (Manual/Automatic)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.html (1)
98-109: Misleading section heading.The heading displays "Census Patients" but this section is shown when
manualPatientsis selected, which is for manual patient entry. Consider renaming the heading to "Manual Patient Entry" or "Enter Patients" to match the radio button label and avoid confusion.🔎 Proposed fix
@if (selectedFormControl.value === 'manualPatients') { <div> - <h3>Census Patients</h3> + <h3>Enter Patients</h3> <div> <mat-form-field appearance="outline"> <input matInput formControlName="patients" placeholder="Enter comma-delimited string of patients"/>
🧹 Nitpick comments (5)
Web/Admin.UI/src/app/components/tenant/facility-view/report-view.interface.ts (1)
12-12: Consider makingadhocTypeoptional and using a union type for type safety.Since the backend has
AdHocType?as nullable and older reports may not have this property, consider:
- Making the property optional:
adhocType?: string;- Using a union type for better type safety:
adhocType?: 'Manual' | 'Automatic';This prevents runtime errors when displaying reports created before this feature and provides compile-time validation of valid values.
🔎 Proposed refactor
- adhocType: string; + adhocType?: 'Manual' | 'Automatic';Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.ts (1)
150-158: Consider displaying facility name in the input field for better UX.Line 155 sets
facilityInputControltofacility.facilityId, but users typically expect to see the human-readable facility name in the autocomplete input, not the ID. This could be confusing.🔎 Proposed change
onFacilitySelected(selectedValue: { facilityId: string; facilityName: string }) { const facility = this.facilities.find(f => f.facilityId === selectedValue.facilityId); if (facility) { this.facilityIdControl.setValue(facility.facilityId); - this.facilityInputControl.setValue(facility.facilityId); + this.facilityInputControl.setValue(facility.facilityName); this.facilityIdControl.setErrors(null); } }Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.spec.ts (3)
4-4: Remove unused import 'async'.The
asyncfunction from RxJS is imported but never used (replaced by Angular'swaitForAsyncon line 23). Additionally, importingasyncfrom RxJS is misleading as it's not related to Angular's async testing helper.🔎 Proposed fix
-import {async, of, throwError} from 'rxjs'; +import {of, throwError} from 'rxjs';
137-152: Verify test expectations match component behavior.The test expects a
{ notFound: true }error (line 150) when an invalid facility is entered, but theonFacilityInputBlurmethod doesn't directly set this error. The error would come from the asyncfacilityExistsValidator.Ensure:
- The mock for
checkFacilityis properly configured to work with the async validator- The timing allows the async validation to complete before assertions
- Consider adding
tick()orflushMicrotasks()if needed
180-213: Test validates core submission logic but has timing concerns.The test correctly validates that:
- The service is called with the right parameters
- The
lastGeneratedReportis updated on successHowever, line 187 has a commented
tick(), which might indicate timing issues. For async tests, consider properly managing the asynchronous flow withfakeAsyncandtick()orflush()to avoid flaky tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
DotNet/Report/Application/Factory/ScheduledReportFactory.csDotNet/Report/Entities/ReportSchedule.csDotNet/Report/Listeners/GenerateReportListener.csWeb/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.htmlWeb/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.spec.tsWeb/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.tsWeb/Admin.UI/src/app/components/tenant/facility-view/report-view.interface.tsWeb/Admin.UI/src/app/components/tenant/facility-view/view-report/view-report.component.html
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
⚙️ CodeRabbit configuration file
**/*.cs: TheHtmlInputSanitizerclass'sSanitize()andSanitizeAndRemove()methods should be used when dealing withstringquery parameters from REST requests.
Files:
DotNet/Report/Entities/ReportSchedule.csDotNet/Report/Application/Factory/ScheduledReportFactory.csDotNet/Report/Listeners/GenerateReportListener.cs
**
⚙️ CodeRabbit configuration file
**: Pull requests that have "TECH_DEBT" in the title should only contain changes related to typos, unused code, linter/IDE suggestions, swagger specification updates,
and logging improvements. These TECH_DEBT PRs must not affect core functionality. All PRs that are not considered technical debt must include information on what
testing was performed in the description of the PR. If it does not, ask the author to provide details on what testing was performed.
When reviewing code, suggest unit tests using XUnit in the following scenarios:
- If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test.
- Logic that depends on service or interface configuration — suggest tests to validate different implementations are correctly resolved.
- No network activity (HTTP calls, sockets, etc.) should appear in unit tests. Recommend using mocks (via Moq) for any external communication.
Large unit tests should be avoided; keeping unit tests small and focused on targeted business logic (i.e. string sanitization)
**: Pull requests that have DOCS in the title should only contain changes related to documentation within the /docs folder or in .md files through-out the code-base. The description
of the PR should specify what documentation was updated. Documentation updates should use EventCatalog.dev structure, where service-specific functionality should be described
in the service's index.mdx (i.e. /services/XXX/index.mdx or /domains/XXX/services/YYY/index.mdx). Configurations that are shared by multiple services should be
reflected in the /docs/docs/config files.
Files:
DotNet/Report/Entities/ReportSchedule.csWeb/Admin.UI/src/app/components/tenant/facility-view/view-report/view-report.component.htmlDotNet/Report/Application/Factory/ScheduledReportFactory.csWeb/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.htmlWeb/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.spec.tsDotNet/Report/Listeners/GenerateReportListener.csWeb/Admin.UI/src/app/components/tenant/facility-view/report-view.interface.tsWeb/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.ts
🧠 Learnings (2)
📚 Learning: 2025-09-11T20:18:30.425Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1070
File: DotNet/Report/KafkaProducers/SubmitPayLoadProducer.cs:51-54
Timestamp: 2025-09-11T20:18:30.425Z
Learning: ReportScheduleModel.ReportTypes is declared as List<string> (non-nullable) with a default value of new List<string>(), so it should never be null in the Report service codebase.
Applied to files:
DotNet/Report/Entities/ReportSchedule.csDotNet/Report/Application/Factory/ScheduledReportFactory.cs
📚 Learning: 2025-03-20T22:11:00.226Z
Learnt from: amphillipsLGC
Repo: lantanagroup/link-cloud PR: 737
File: DotNet/Admin.BFF/Presentation/Endpoints/Aggregation/Handlers/Report/GetReportSummaries.cs:23-23
Timestamp: 2025-03-20T22:11:00.226Z
Learning: The facilityId validation in GetReportSummaries.Handle method in DotNet/Admin.BFF/Presentation/Endpoints/Aggregation/Handlers/Report/GetReportSummaries.cs will be implemented in a future phase of work by amphillipsLGC.
Applied to files:
Web/Admin.UI/src/app/components/tenant/facility-view/view-report/view-report.component.htmlWeb/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.ts
🧬 Code graph analysis (2)
Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.spec.ts (1)
Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.ts (3)
facilityIdControl(169-171)startDateControl(177-179)endDateControl(181-183)
Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.ts (2)
Web/Admin.UI/src/app/components/testing/integration-test/integration-test.component.ts (1)
facilityIdControl(161-163)Web/Admin.UI/src/app/interfaces/entity-created-response.model.ts (1)
IReportGenerationResponse(7-9)
🪛 GitHub Check: Integration Tests
DotNet/Report/Entities/ReportSchedule.cs
[failure] 26-26:
The type or namespace name 'AdHocType' could not be found (are you missing a using directive or an assembly reference?)
[failure] 26-26:
The type or namespace name 'AdHocType' could not be found (are you missing a using directive or an assembly reference?)
⏰ 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). (5)
- GitHub Check: Smoke Test with Docker Compose
- GitHub Check: Unit Tests for DotNet
- GitHub Check: Build Documentation
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (8)
Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.html (1)
79-80: Verify UI behavior aligns with option labels.The two new radio options are:
- "Enter Patients" (value:
manualPatients) - for manual patient entry- "Use Census (Automatic)" (value:
censusPatients) - for automatic census-based reportsEnsure that the component logic correctly handles both options and that the form behavior matches user expectations for each selection.
DotNet/Report/Application/Factory/ScheduledReportFactory.cs (1)
22-22: LGTM!The mapping of
AdHocTypefrom the domain model to the summary model is straightforward and consistent with the existing property mappings.Web/Admin.UI/src/app/components/tenant/facility-view/view-report/view-report.component.html (1)
57-62: LGTM!The display of the new
AdHocTypefield follows the established pattern for other summary fields and uses appropriate optional chaining to handle null values.DotNet/Report/Listeners/GenerateReportListener.cs (1)
201-201: LGTM!The logic for determining
isAutomaticcorrectly distinguishes between automatic (census-based) and manual (provided patient list) report generation:
- Automatic: when not regenerating AND no patient IDs provided (will fetch from census)
- Manual: when regenerating OR patient IDs are provided
Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.ts (3)
108-108: LGTM! Default value aligns with manual entry focus.Changing the default from 'fileUpload' to 'manualPatients' aligns with the PR objective of distinguishing manual versus automatic report generation, making the manual entry path the primary workflow.
229-249: Excellent improvement adding proper error handling.The addition of structured error handling with user feedback via snackbar significantly improves the user experience. Users will now be informed when report generation fails instead of seeing silent failures.
326-332: LGTM! Return statement fix ensures correct function behavior.The explicit return statement ensures the parsed patient array is properly returned, fixing what was likely a bug where the function would return undefined.
Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.spec.ts (1)
154-163: Test correctly validates current implementation.This test properly validates the
onFacilitySelectedmethod's behavior, including the explicit error clearing (line 156 of component). The test expectations align with the current implementation.Note: This test will need updating if the facilityInputControl is changed to display facilityName instead of facilityId (as suggested in the component review).
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @arianamihailescu. * #1345 (comment) The following files were modified: * `DotNet/Report/Application/Factory/ScheduledReportFactory.cs` * `DotNet/Report/Listeners/GenerateReportListener.cs`
… LNK-4651-AddHic-Type-for-AdhocReportGeneration
…portGeneration' into LNK-4651-AddHic-Type-for-AdhocReportGeneration
…portGeneration' into LNK-4651-AddHic-Type-for-AdhocReportGeneration
… the option selected
2 participants
🛠️ Description of Changes
Add AdHoc type for Adhoc reports to track Manual versus Automatic generation
🧪 Testing Performed
Tested locally
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.