[Gel] Add PeriodicReport queries | refactor service/repo layers
#3247
Conversation
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
2 times, most recently
from
1d3efe9 to
07d8f1b
Compare
src/components/periodic-report/periodic-report.edgedb.repository.ts
Outdated
Show resolved
Hide resolved
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
3 times, most recently
from
5b844a6 to
ec52210
Compare
src/components/periodic-report/periodic-report.edgedb.repository.ts
Outdated
Show resolved
Hide resolved
bryanjnelson
changed the title
PeriodicReport queries | refactor service/repo layers appropriatelyPeriodicReport queries | refactor service/repo layers
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
6 times, most recently
from
22a97e2 to
8962f0d
Compare
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
from
8962f0d to
3e3db8d
Compare
📝 WalkthroughWalkthroughThese changes introduce a new Gel-specific repository for periodic reports with advanced query methods and hydration logic, refactor service and handler classes to use repositories instead of services, add type and utility enhancements for report DTOs, and improve repository methods with error handling, logging, and input validation. Configuration-based logic is added to syncing handlers. Changes
Possibly related issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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: 3
♻️ Duplicate comments (1)
src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts (1)
47-49: Consistent gel database bypass pattern.This handler implements the same early return logic for gel databases as the project sync handler. The consistency is good, and this aligns with the verification script from the previous handler review.
🧹 Nitpick comments (4)
src/components/periodic-report/dto/periodic-report.dto.ts (1)
54-54: Track the TODO comment for future implementation.The TODO suggests changing the type to
Secured<LinkTo<'File'> | null>. This would make the property nullable and potentially change how file references are handled.Would you like me to create an issue to track this TODO for future implementation?
src/components/periodic-report/periodic-report.gel.repository.ts (2)
44-44: Address the TODO comments for ProgressReport hydration.Multiple methods have the same TODO comment indicating that ProgressReport hydration needs work. This suggests incomplete implementation that should be addressed.
Would you like me to help implement the proper hydration logic for ProgressReport or create an issue to track this work?
Also applies to: 93-93, 112-112, 132-132, 152-152
55-55: Consider using an injected date provider for better testability.Using
Date.now()directly in queries makes unit testing difficult and could cause timezone-related issues. Consider injecting a date provider or accepting the current date as a parameter.-e.op(report.end, '<', Date.now()), +e.op(report.end, '<', this.dateProvider.now()),Also applies to: 124-124
src/components/periodic-report/periodic-report.repository.ts (1)
71-196: Consider breaking down the complex merge method.The merge method is quite long and handles multiple responsibilities. Consider extracting helper methods for better readability and maintainability.
Consider extracting:
- Interval preparation logic (lines 81-88)
- Query construction logic
- Result processing and logging
This would make the method easier to test and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/periodic-report/dto/periodic-report.dto.ts(4 hunks)src/components/periodic-report/handlers/abstract-periodic-report-sync.ts(3 hunks)src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts(2 hunks)src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts(3 hunks)src/components/periodic-report/periodic-report.gel.repository.ts(1 hunks)src/components/periodic-report/periodic-report.module.ts(3 hunks)src/components/periodic-report/periodic-report.repository.ts(5 hunks)src/components/periodic-report/periodic-report.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/periodic-report/dto/periodic-report.dto.ts (1)
src/components/file/dto/file.dto.ts (1)
DefinedFile(165-165)
🔇 Additional comments (11)
src/components/periodic-report/periodic-report.module.ts (1)
2-2: LGTM: Clean dual repository pattern implementation.The introduction of
splitDbto provide bothPeriodicReportRepositoryandPeriodicReportGelRepositoryimplements a clean dual repository pattern. This allows conditional database access based on configuration while maintaining the same interface.Also applies to: 11-11, 30-30
src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts (2)
3-9: LGTM: Clean service-to-repository refactoring.The refactoring correctly replaces
PeriodicReportServicewithPeriodicReportRepositoryand addsConfigServicedependency injection. The constructor and super call are properly updated.Also applies to: 13-13, 27-31
35-37:✅ Verification successful
Verify the gel database bypass logic.
The early return when
databaseEngine === 'gel'completely bypasses periodic report syncing for gel databases. This seems intentional given the newPeriodicReportGelRepository, but it's worth confirming that this handler should indeed be completely skipped for gel databases rather than using the gel-specific repository.
🏁 Script executed:
#!/bin/bash # Description: Check if other handlers have similar gel database bypass logic and verify gel repository usage # Search for similar gel database bypasses in other handlers echo "=== Searching for gel database bypass patterns ===" rg -A 2 -B 2 "databaseEngine.*gel" # Search for gel repository usage patterns echo -e "\n=== Searching for PeriodicReportGelRepository usage ===" rg -A 5 -B 2 "PeriodicReportGelRepository" # Check for any gel-specific handling in the repository echo -e "\n=== Checking for gel-specific methods or logic ===" fd "periodic-report.*gel" --type fLength of output: 7614
Bypass logic for GEL database is consistent and intentional
I’ve verified that all other sync and setup handlers early-return on
databaseEngine === 'gel', and thatPeriodicReportGelRepositoryis correctly wired in viasplitDbinperiodic-report.module.ts. No changes needed here.src/components/periodic-report/handlers/abstract-periodic-report-sync.ts (1)
4-4: LGTM: Consistent service-to-repository refactoring.The abstract base class correctly refactors from
PeriodicReportServicetoPeriodicReportRepository. All method calls are consistently updated to use the newperiodicReportsRepoproperty, maintaining the same logic while using the repository pattern.Also applies to: 12-14, 28-28, 30-30, 39-39
src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts (1)
2-8: LGTM: Consistent refactoring pattern across handlers.This handler follows the same clean refactoring pattern as
sync-periodic-report-to-project.handler.ts, replacingPeriodicReportServicewithPeriodicReportRepositoryand addingConfigServicedependency. The constructor and super call are correctly updated.Also applies to: 17-17, 38-43
src/components/periodic-report/dto/periodic-report.dto.ts (2)
22-25: LGTM! Well-designed mutually exclusive union type.The use of
MergeExclusiveensures type safety by preventing an instance from being multiple report types simultaneously. This is a good pattern for discriminated unions.
93-97: Good addition for type-safe report class references.The
ReportConcretesmapping provides a clean way to dynamically reference report classes, which is useful for factory patterns and type resolution.src/components/periodic-report/periodic-report.service.ts (2)
50-57: LGTM! Clean refactoring of the update method.The update method now properly constructs a single object with all necessary properties for the repository update. This aligns well with the repository's simplified interface.
61-61: Good security practice!Using the secured version of the report file ensures proper authorization checks are applied.
src/components/periodic-report/periodic-report.repository.ts (2)
198-218: Well-implemented final report merge logic.The method cleanly handles both updating existing final reports and creating new ones, with an appropriate early return for no-change scenarios.
433-433: Good addition of deletion logging.Adding logging for delete operations provides a valuable audit trail and helps with debugging.
| getCurrentDue( | ||
| _parentId: ID, | ||
| _reportType: ReportType, | ||
| ): Promise< | ||
| | UnsecuredDto< | ||
| | (Without< | ||
| | (Without<FinancialReport, NarrativeReport> & NarrativeReport) | ||
| | (Without<NarrativeReport, FinancialReport> & FinancialReport), | ||
| ProgressReport | ||
| > & | ||
| ProgressReport) | ||
| | (Without< | ||
| ProgressReport, | ||
| | (Without<FinancialReport, NarrativeReport> & NarrativeReport) | ||
| | (Without<NarrativeReport, FinancialReport> & FinancialReport) | ||
| > & | ||
| ( | ||
| | (Without<FinancialReport, NarrativeReport> & NarrativeReport) | ||
| | (Without<NarrativeReport, FinancialReport> & FinancialReport) | ||
| )) | ||
| > | ||
| | undefined | ||
| > { | ||
| //TODO - is this needed? How does this differ from matchCurrentDue? | ||
| throw new Error('Method not implemented.'); | ||
| } |
There was a problem hiding this comment.
Remove or implement the getCurrentDue method.
This method throws a "Method not implemented" error and has a TODO questioning if it's needed. Either implement it properly or remove it from the interface if it's not required.
Consider either:
- Implementing the method if it's required by the interface
- Removing it if the TODO comment is correct and it's not needed
- Clarifying how it differs from
matchCurrentDueas mentioned in the comment
🤖 Prompt for AI Agents
In src/components/periodic-report/periodic-report.gel.repository.ts between
lines 66 and 91, the getCurrentDue method is currently unimplemented and throws
an error with a TODO questioning its necessity. You should either fully
implement this method according to its intended functionality if it is required
by the interface, or remove it entirely if it is redundant or unnecessary.
Additionally, clarify its purpose and how it differs from the existing
matchCurrentDue method to avoid confusion.
| @@ -0,0 +1,171 @@ | |||
| import { Injectable } from '@nestjs/common'; | |||
| import { type Without } from 'type-fest/source/merge-exclusive'; | |||
There was a problem hiding this comment.
Avoid importing from internal paths.
Importing from type-fest/source/merge-exclusive uses an internal path that could break in future versions. Import from the public API instead.
-import { type Without } from 'type-fest/source/merge-exclusive';
+import { type Without } from 'type-fest';📝 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.
| import { type Without } from 'type-fest/source/merge-exclusive'; | |
| -import { type Without } from 'type-fest/source/merge-exclusive'; | |
| +import { type Without } from 'type-fest'; |
🤖 Prompt for AI Agents
In src/components/periodic-report/periodic-report.gel.repository.ts at line 2,
the import statement uses an internal path 'type-fest/source/merge-exclusive'
which is unstable. Change the import to use the public API of 'type-fest' by
importing 'Without' directly from 'type-fest' without referencing internal
source paths.
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
from
3e3db8d to
1125d52
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/periodic-report/periodic-report.gel.repository.ts (2)
2-2: Avoid importing from internal paths.This is the same issue flagged in previous reviews - importing from internal type-fest paths is unstable and could break in future versions.
66-91: Remove or implement thegetCurrentDuemethod.This method is unimplemented and has been flagged in previous reviews. The complex union type signature also suggests potential design issues.
🧹 Nitpick comments (2)
src/components/periodic-report/periodic-report.gel.repository.ts (2)
44-44: Address the repeated TODO comments about ProgressReport hydration.Multiple methods have identical TODO comments indicating that ProgressReport hydration needs work. This suggests a systemic issue that should be addressed rather than leaving multiple TODO markers.
Would you like me to help analyze what specific issues exist with ProgressReport hydration and propose a solution, or should this be tracked as a separate issue?
Also applies to: 93-93, 112-112, 132-132, 152-152
45-170: Consider refactoring to reduce code duplication.The query methods share significant boilerplate code for setting up the enhanced resource, casting the resource, and building the base query structure. This duplication makes the code harder to maintain.
Consider extracting a common query builder method:
+ private buildBaseQuery(parentId: ID, reportType: ReportType) { + const enhancedResource = EnhancedResource.of( + resolveReportType({ type: reportType }), + ); + const resource = e.cast(enhancedResource.db, e.uuid(parentId)); + return { enhancedResource, resource }; + } async matchCurrentDue(parentId: ID, reportType: ReportType) { - const enhancedResource = EnhancedResource.of( - resolveReportType({ type: reportType }), - ); - const resource = e.cast(enhancedResource.db, e.uuid(parentId)); + const { resource } = this.buildBaseQuery(parentId, reportType); const report = e.select(resource, (report) => ({ ...this.hydrate(report), filter: e.all( e.set( e.op(resource.id, '=', report.container.id), e.op(report.end, '<', Date.now()), ), ), order_by: [ { expression: report.end, direction: e.DESC }, { expression: report.start, direction: e.ASC }, ], })); return await this.db.run(report); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/periodic-report/dto/periodic-report.dto.ts(4 hunks)src/components/periodic-report/handlers/abstract-periodic-report-sync.ts(3 hunks)src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts(2 hunks)src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts(3 hunks)src/components/periodic-report/periodic-report.gel.repository.ts(1 hunks)src/components/periodic-report/periodic-report.module.ts(3 hunks)src/components/periodic-report/periodic-report.repository.ts(5 hunks)src/components/periodic-report/periodic-report.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/periodic-report/periodic-report.module.ts
- src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts
- src/components/periodic-report/handlers/abstract-periodic-report-sync.ts
- src/components/periodic-report/dto/periodic-report.dto.ts
- src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts
- src/components/periodic-report/periodic-report.service.ts
- src/components/periodic-report/periodic-report.repository.ts
| hydrate: (periodicReport) => ({ | ||
| ...periodicReport['*'], | ||
| type: castToEnum(periodicReport.__type__.name.slice(9, -7), ReportType), | ||
| reportFile: true, | ||
| sensitivity: periodicReport.container.is(e.Project.ContextAware) | ||
| .sensitivity, | ||
| scope: false, | ||
| parent: e.select({ | ||
| identity: periodicReport.id, | ||
| labels: e.array_agg(e.set(periodicReport.__type__.name.slice(9, null))), | ||
| properties: e.select({ | ||
| id: periodicReport.id, | ||
| createdAt: periodicReport.createdAt, | ||
| }), | ||
| }), | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the hardcoded hydration values.
The hydration logic has some concerning hardcoded values:
reportFile: trueis always set to true, which seems incorrect for all reportsscope: falseis hardcoded, which may not reflect actual data
These hardcoded values could lead to inaccurate data representation.
Consider making these values dynamic based on actual report state:
hydrate: (periodicReport) => ({
...periodicReport['*'],
type: castToEnum(periodicReport.__type__.name.slice(9, -7), ReportType),
- reportFile: true,
+ reportFile: e.op('exists', periodicReport.reportFile),
sensitivity: periodicReport.container.is(e.Project.ContextAware)
.sensitivity,
- scope: false,
+ scope: periodicReport.scope ?? false,
parent: e.select({
identity: periodicReport.id,
labels: e.array_agg(e.set(periodicReport.__type__.name.slice(9, null))),
properties: e.select({
id: periodicReport.id,
createdAt: periodicReport.createdAt,
}),
}),
}),📝 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.
| hydrate: (periodicReport) => ({ | |
| ...periodicReport['*'], | |
| type: castToEnum(periodicReport.__type__.name.slice(9, -7), ReportType), | |
| reportFile: true, | |
| sensitivity: periodicReport.container.is(e.Project.ContextAware) | |
| .sensitivity, | |
| scope: false, | |
| parent: e.select({ | |
| identity: periodicReport.id, | |
| labels: e.array_agg(e.set(periodicReport.__type__.name.slice(9, null))), | |
| properties: e.select({ | |
| id: periodicReport.id, | |
| createdAt: periodicReport.createdAt, | |
| }), | |
| }), | |
| }), | |
| hydrate: (periodicReport) => ({ | |
| ...periodicReport['*'], | |
| type: castToEnum(periodicReport.__type__.name.slice(9, -7), ReportType), | |
| reportFile: e.op('exists', periodicReport.reportFile), | |
| sensitivity: periodicReport.container.is(e.Project.ContextAware) | |
| .sensitivity, | |
| scope: periodicReport.scope ?? false, | |
| parent: e.select({ | |
| identity: periodicReport.id, | |
| labels: e.array_agg(e.set(periodicReport.__type__.name.slice(9, null))), | |
| properties: e.select({ | |
| id: periodicReport.id, | |
| createdAt: periodicReport.createdAt, | |
| }), | |
| }), | |
| }), |
🤖 Prompt for AI Agents
In src/components/periodic-report/periodic-report.gel.repository.ts between
lines 24 and 39, the hydrate function sets reportFile to true and scope to false
as hardcoded values, which may not accurately represent the actual report data.
Update these fields to be dynamically assigned based on the properties or state
of the periodicReport object or related data, ensuring they reflect the true
values instead of fixed literals.
src/components/periodic-report/periodic-report.gel.repository.ts
Outdated
Show resolved
Hide resolved
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
from
1125d52 to
9420c44
Compare
No description provided.