[Gel] - Add triggers to create/update periodic reports upon project creation #3279
Conversation
There was a problem hiding this comment.
Wow love the initiative!
For this to be fully out of app code, we also need to handle:
- Date range expanding (on update), which would add new periods.
Jan-March -> Jan-July - Date range shrinking (on update), which would delete empty periods
Jan-May -> Jan-March
But we only want to delete reports that don't have a file uploaded or a progress report started - On financial reporting frequency change create new and delete old empty periods.
Monthly (Jan, Feb, March) -> Quarterly (Jan-March)
Honestly "on insert" will never be hit by users since they don't define the date ranges when they create the project.
May be useful for migration though?
Should the create_periodic_report_ranges function be made more generic to handle other use cases? (Probably not.)
Probably not
Is the
create_periodic_report_rangesfunction located in the best place? It could be moved to thecommon.esdl, but if this is the only place it will be used then no. It could also potentially be moved to theperiodic-report.esdl?
Probably their own files for each concrete report. They all have distinct use-cases / logic.
It's currently problematic that the
mouStart,mouEnd, andfinancialReportPeriodfields are not required on aProject; this will obviously prevent any reports from being generated correctly. We'll need to figure out the best way to handle that. Make them required? Update the trigger to also run on update when all 3 are finally populated?
That's what we do in app code. If all 3 are given, synchronize.
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
cf4bbc4 to
c6d7120
Compare
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
436412b to
f594e65
Compare
📝 WalkthroughWalkthroughTriggers and supporting functions were introduced in the project schema to automate the creation, updating, and deletion of periodic financial, narrative, and progress reports based on changes to project MOU dates and financial report periods. Additionally, a constraint was added to ensure periodic report end dates are not before start dates. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)dbschema/project.gel (2)⏰ 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). (12)
🔇 Additional comments (12)
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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbschema/project.gel(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: Generate (head)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Generate (base)
- GitHub Check: Clean
- GitHub Check: Unit
- GitHub Check: lint
- GitHub Check: Analyze (javascript)
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
f594e65 to
e0b8c5d
Compare
bryanjnelson
changed the title
bryanjnelson
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
dbschema/project.gel (2)
125-157:⚠️ Potential issueMissing guard for NULL MOU / period values
The trigger could fail at runtime if any of the required fields (
mouStart,mouEnd,financialReportPeriod) are NULL, as the functioncreate_periodic_report_rangeswill attempt to cast them tocal::local_date.Apply this diff to add a guard clause:
-trigger createPeriodicReports after insert for each do ( +trigger createPeriodicReports after insert for each + when ( + exists __new__.mouStart + and exists __new__.mouEnd + and exists __new__.financialReportPeriod + ) + do (This will make the insert path safe and allow you to decide later if you want an additional trigger to create reports once these fields are populated.
307-323:⚠️ Potential issueOff-by-one error: upper bound of range is exclusive
In EdgeDB,
range(lower, upper)is inclusive on the lower bound and exclusive on the upper bound[lower, upper). The current implementation would exclude the last day of each period.Apply this diff to correct the range calculation:
- firstDayOfNextMonth := (select firstDayOfMonth + <cal::relative_duration>(monthInterval ++ ' month')), - lastDayOfMonth := firstDayOfNextMonth - <cal::relative_duration>'1 day' - select range(<cal::local_date>firstDayOfMonth, <cal::local_date>lastDayOfMonth) + firstDayOfNextPeriod := ( + select firstDayOfMonth + <cal::relative_duration>(monthInterval ++ ' month') + ) + select range( + <cal::local_date>firstDayOfMonth, + <cal::local_date>firstDayOfNextPeriod + )This will create a range from the first day of the period to the first day of the next period, which correctly includes all days in the period.
🧹 Nitpick comments (1)
dbschema/project.gel (1)
187-206: Be consistent with parentheses for clarityThe code logic is complex with nested conditionals. Consistent parentheses would make it easier to understand the structure.
Add parentheses around conditional expressions:
- else if __old__.financialReportPeriod ?!= __new__.financialReportPeriod - or (__new__.mouStart > __old__.mouStart) ?? false - or (__new__.mouEnd < __old__.mouEnd) ?? false then ( + else if ( + __old__.financialReportPeriod ?!= __new__.financialReportPeriod + or (__new__.mouStart > __old__.mouStart) ?? false + or (__new__.mouEnd < __old__.mouEnd) ?? false + ) then (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbschema/project.gel(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: Generate (head)
- GitHub Check: Unit
- GitHub Check: Generate (base)
- GitHub Check: Clean
- GitHub Check: lint
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
dbschema/project.gel (3)
325-366: Function reuses requestedReportPeriodsForInsertion logic properlyGood job implementing the insertion logic in a separate function that correctly filters out periods that already have reports, addressing potential duplication issues.
159-218: Comprehensive trigger for handling report lifecycleThe trigger properly handles:
- Removing reports when MOU dates are removed
- Removing and adding reports when financial report period changes
- Adding new reports when MOU dates change
The code uses the correct filter
.container.id = __old__.idto prevent deleting reports from other projects.
202-204:✅ Verification successful
Ensure deletion is scoped to current project
The
delete reportoperation appears properly scoped to the current project through the initial filter onexistingReports, but it's good practice to double-check at deletion time too.
🏁 Script executed:
#!/bin/bash # Verify that delete operation properly scopes to the current project # Looking for any similar trigger that has an explicit container check at deletion time rg -A 5 "delete.*filter.*container"Length of output: 37
🏁 Script executed:
#!/bin/bash # Show context around the delete report operation to verify project scoping rg -n "delete report" -A10 -B10 dbschema/project.gelLength of output: 1352
🏁 Script executed:
#!/bin/bash # Locate the definition of existingReports to confirm project scoping rg -n "existingReports" -A10 -B10 dbschema/project.gelLength of output: 3693
Deletion correctly scoped to current project
TheexistingReportsselection (lines 171–174) already filters by.container.id = __old__.id, so all entries inreportsForDeletion(and the subsequentdelete reportcalls) are inherently limited to the current project. No additional container filter is needed.
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
e0b8c5d to
5be235c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dbschema/project.gel (2)
125-157:⚠️ Potential issueMissing null-checks may cause runtime failures in
createPeriodicReportstriggerThe trigger attempts to use
mouStart,mouEnd, andfinancialReportPeriodwithout checking if these optional fields are populated. This could cause runtime errors when inserting projects with incomplete data.As mentioned in a previous review comment, add a
whencondition to prevent runtime errors:-trigger createPeriodicReports after insert for each do ( +trigger createPeriodicReports after insert for each + when ( + exists __new__.mouStart + and exists __new__.mouEnd + and exists __new__.financialReportPeriod + ) + do (
319-321:⚠️ Potential issueOff-by-one error: upper bound of the range is exclusive
The current implementation builds
[firstDayOfMonth, lastDayOfMonth)which omits the last day of the period.As suggested in a previous review, use the next period's first day as the exclusive upper bound:
- firstDayOfNextMonth := (select firstDayOfMonth + <cal::relative_duration>(monthInterval ++ ' month')), - lastDayOfMonth := firstDayOfNextMonth - <cal::relative_duration>'1 day' - select range(<cal::local_date>firstDayOfMonth, <cal::local_date>lastDayOfMonth) + firstDayOfNextPeriod := ( + select firstDayOfMonth + <cal::relative_duration>(monthInterval ++ ' month') + ) + select range( + <cal::local_date>firstDayOfMonth, + <cal::local_date>firstDayOfNextPeriod + )
🧹 Nitpick comments (4)
dbschema/project.gel (4)
165-218: Trigger branch handling could be improved with early returnsThe current implementation uses a sequence of
if/else ifbranches with nested CTEs that makes the code difficult to follow. Consider restructuring the logic for better readability.- select if not exists __new__.mouStart or not exists __new__.mouEnd then ( + # Handle MOU date removal first + select if not exists __new__.mouStart or not exists __new__.mouEnd then ( for report in reportsForDeletion union ( delete report ) ) - else if __old__.financialReportPeriod ?!= __new__.financialReportPeriod - or (__new__.mouStart > __old__.mouStart) ?? false - or (__new__.mouEnd < __old__.mouEnd) ?? false then ( + # Handle period type change or MOU period shrinking + else if __old__.financialReportPeriod ?!= __new__.financialReportPeriod or + (__new__.mouStart > __old__.mouStart) ?? false or + (__new__.mouEnd < __old__.mouEnd) ?? false then (
339-351: Consider refactoring duplicate report insertion logicThere's significant code duplication between financial and narrative report insertion. While EdgeQL has limitations for dynamic typing, the code could still be more maintainable.
While the previous attempt to combine loops didn't work due to EdgeQL limitations (as noted in past review comments), you could at least extract common properties into a CTE:
+ commonReportProps := { + createdAt := datetime_of_statement(), + modifiedAt := datetime_of_statement(), + createdBy := assert_exists(global default::currentActor), + modifiedBy := assert_exists(global default::currentActor), + project := newProject, + projectContext := newProject.projectContext, + container := newProject, + }, financialReports := (for reportPeriod in requestedReportPeriodsForInsertion union ( insert default::FinancialReport { - createdAt := datetime_of_statement(), - modifiedAt := datetime_of_statement(), - createdBy := assert_exists(global default::currentActor), - modifiedBy := assert_exists(global default::currentActor), - project := newProject, - projectContext := newProject.projectContext, - container := newProject, + commonReportProps, period := reportPeriod, }Also applies to: 352-364
309-311: Document function usage and expected behaviorThe function lacks documentation that explains expected behavior and valid input parameters.
# creates the ranges for the given start and end dates based upon the given month interval + # Parameters: + # startDate: The start date of the MOU period + # endDate: The end date of the MOU period + # monthInterval: Either '1' for monthly or '3' for quarterly reporting + # Returns: + # A set of date ranges covering the MOU period, each representing a reporting period function create_periodic_report_ranges(startDate: cal::local_date, endDate: cal::local_date, monthInterval: str) -> set of range<cal::local_date>
126-129: Hardcoded interval mapping may be better expressed in a functionThe mapping of
ReportPeriod.Monthlyto '1' and other values to '3' is hardcoded in multiple places and could be extracted to a utility function.+ # Helper function to get month interval string from ReportPeriod + function report_period_to_month_interval(period: default::ReportPeriod) -> str + using ( + if period = default::ReportPeriod.Monthly then '1' else '3' + ); + trigger createPeriodicReports after insert for each do ( with - interval := (select - if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' else '3'), + interval := Project::report_period_to_month_interval(__new__.financialReportPeriod),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbschema/project.gel(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: Generate (head)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Analyze (javascript)
- GitHub Check: Generate (base)
- GitHub Check: lint
- GitHub Check: Clean
- GitHub Check: Unit
🔇 Additional comments (1)
dbschema/project.gel (1)
130-133:❓ Verification inconclusive
Add error handling for production concerns
While the current implementation handles core functionality, it doesn't account for potential edge cases that could occur in production.
Please consider these cases:
- What happens if a project is created without MOU dates or financial report period, and these are added later?
- What happens if a project's MOU dates or financial report period are updated frequently?
- How will the system handle extremely long MOU periods that would generate hundreds of reports?
Also applies to: 341-350, 354-363
🏁 Script executed:
#!/bin/bash # Check for existing reports to understand the typical number of periods rg -A 3 "FinancialReport|NarrativeReport" --type edgeql | grep "period" # Look for existing date range handling elsewhere in the codebase rg "range<cal::local_date>" --type edgeqlLength of output: 177
Add production-grade error handling for MOU and reporting date ranges
The existing code around generating reports (e.g. mouStart, mouEnd, interval) doesn’t guard against several real-world scenarios. I wasn’t able to locate any existing date-range validation in
.gelfiles (therg --type edgeqlscan failed on unrecognized file type), so please manually verify or extend the logic to cover:
- Missing dates
• What ifmouStart,mouEndor report period aren’t set at creation and are added later?- Frequent updates
• How does the system behave if those dates are changed often? Are old reports cleaned up/re-generated?- Very long periods
• A multi-year MOU could spawn hundreds of report intervals—do you cap, batch or page those?Also review the same patterns at lines 341–350 and 354–363 to ensure consistent error checking and performance guards.
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
5be235c to
faef758
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dbschema/project.gel (1)
303-320:⚠️ Potential issueAddress the off-by-one error and add input validation
Two issues need attention:
Off-by-one error still present: The range calculation on lines 314-316 uses
lastDayOfMonth := firstDayOfNextMonth - <cal::relative_duration>'1 day'which creates[firstDay, lastDay)that excludes the last day of the period.Missing input validation: The function lacks validation for input parameters.
Apply this diff to fix the off-by-one error:
- firstDayOfNextMonth := (select firstDayOfMonth + <cal::relative_duration>(monthInterval ++ ' month')), - lastDayOfMonth := firstDayOfNextMonth - <cal::relative_duration>'1 day' - select range(<cal::local_date>firstDayOfMonth, <cal::local_date>lastDayOfMonth) + firstDayOfNextPeriod := ( + select firstDayOfMonth + <cal::relative_duration>(monthInterval ++ ' month') + ) + select range( + <cal::local_date>firstDayOfMonth, + <cal::local_date>firstDayOfNextPeriod + )And add input validation at the beginning:
function create_periodic_report_ranges(startDate: cal::local_date, endDate: cal::local_date, monthInterval: str) -> set of range<cal::local_date> using ( + # Validate input parameters + assert(startDate <= endDate, + message := "Start date must be before or equal to end date"), + assert(monthInterval = '1' or monthInterval = '3', + message := "Month interval must be '1' or '3'"), with
🧹 Nitpick comments (1)
dbschema/project.gel (1)
145-214: LGTM! Comprehensive update logic with proper scopingThe trigger correctly handles multiple update scenarios and properly scopes operations to the specific project (line 159:
filter .container.id = __old__.id). The conditional logic appropriately handles:
- Field deletions → cleanup reports without files
- Period/range changes → selective deletion and insertion
- Date-only changes → insertion of missing reports
The complexity is justified by the comprehensive business logic requirements.
Consider adding inline comments to document the different scenarios handled by each conditional branch for future maintainability:
select + # Scenario 1: Required fields were deleted - cleanup empty reports if not exists __new__.mouStart or not exists __new__.mouEnd or not exists __new__.financialReportPeriod then ( ... + # Scenario 2: Period type changed or date range shrunk - selective update else if __old__.financialReportPeriod ?!= __new__.financialReportPeriod ... + # Scenario 3: Date range expanded or shifted - add missing reports else if newMouStart ?!= oldMouStart ... + # Scenario 4: No relevant changes - no action needed ) else (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbschema/project.gel(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Generate (head)
- GitHub Check: Analyze (javascript)
- GitHub Check: Clean
- GitHub Check: lint
- GitHub Check: Unit
- GitHub Check: Generate (base)
🔇 Additional comments (3)
dbschema/project.gel (3)
126-143: LGTM! Proper NULL guard implementationThe trigger correctly implements the
whenclause to guard against NULL values for required fields, addressing the previous concern about runtime failures. The logic flow is clean and uses the helper functions appropriately.
322-335: LGTM! Proper duplicate prevention logicThe function correctly implements duplicate prevention by filtering out requested periods that already have existing reports. The use of
distinctandarray_unpackproperly handles the array operations.
337-368: LGTM! Clean report insertion implementationThe function properly creates both financial and narrative reports with correct metadata and relationships. The use of
datetime_of_statement()ensures consistency, and the union pattern cleanly combines both report types.
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
faef758 to
68d60d7
Compare
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
68d60d7 to
ac6823b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
dbschema/project.gel (2)
204-208: Fix inconsistent function naming (duplicate occurrence)Same naming inconsistency appears again in this branch.
304-306: 🛠️ Refactor suggestionAdd input validation for create_periodic_report_ranges function
The function should validate that
monthIntervalis a supported value and that the date parameters are valid.function create_periodic_report_ranges(startDate: cal::local_date, endDate: cal::local_date, monthInterval: str) -> set of range<cal::local_date> using ( + assert(startDate <= endDate, message := "Start date must be before or equal to end date"), + assert(monthInterval in {'1', '3'}, message := "Month interval must be '1' (monthly) or '3' (quarterly)"), with
🧹 Nitpick comments (2)
dbschema/project.gel (2)
343-368: Consider reducing code duplication in report creationThe financial and narrative report creation loops are nearly identical. While EdgeQL limitations may prevent full unification, consider extracting common metadata into a shared expression.
with + commonMetadata := ( + createdAt := datetime_of_statement(), + modifiedAt := datetime_of_statement(), + createdBy := assert_exists(global default::currentActor), + modifiedBy := assert_exists(global default::currentActor), + project := newProject, + projectContext := newProject.projectContext, + container := newProject, + ), financialReports := (for reportPeriod in array_unpack(requestedReportPeriodsForInsertion) union ( insert default::FinancialReport { - createdAt := datetime_of_statement(), - modifiedAt := datetime_of_statement(), - createdBy := assert_exists(global default::currentActor), - modifiedBy := assert_exists(global default::currentActor), - project := newProject, - projectContext := newProject.projectContext, - container := newProject, + commonMetadata, period := reportPeriod, } )), narrativeReports := (for reportPeriod in array_unpack(requestedReportPeriodsForInsertion) union ( insert default::NarrativeReport { - createdAt := datetime_of_statement(), - modifiedAt := datetime_of_statement(), - createdBy := assert_exists(global default::currentActor), - modifiedBy := assert_exists(global default::currentActor), - project := newProject, - projectContext := newProject.projectContext, - container := newProject, + commonMetadata, period := reportPeriod, } ))
193-197: Optimize deletion filter for better performanceThe deletion logic filters reports by checking if their period is not in
requestedReportPeriods. For large sets, this could be inefficient. Consider using a more direct approach.- deletedReports := (for report in reportsForDeletion - union ( - delete report - filter report.period not in requestedReportPeriods - )) + deletedReports := ( + delete reportsForDeletion + filter reportsForDeletion.period not in requestedReportPeriods + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dbschema/periodic-report.gel(1 hunks)dbschema/project.gel(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dbschema/periodic-report.gel
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: Analyze (javascript)
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
ac6823b to
60348cd
Compare
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
60348cd to
2f2b513
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dbschema/project.gel (1)
340-343: Remove unnecessary additional report period rangeThe
additionalReportPeriodRangecreates a zero-length range fromendDatetoendDatewhich serves no purpose and could cause confusion.Apply this diff:
- additionalReportPeriodRange := ( - select range(<cal::local_date>endDate, <cal::local_date>endDate, inc_upper := true) - ) - select reportPeriodRanges union additionalReportPeriodRange + select reportPeriodRanges
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dbschema/periodic-report.gel(1 hunks)dbschema/project.gel(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dbschema/periodic-report.gel
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: Unit
- GitHub Check: Generate (head)
- GitHub Check: Generate (base)
- GitHub Check: lint
- GitHub Check: Clean
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
dbschema/project.gel (4)
156-235: Complex but well-structured trigger logicThe update trigger correctly handles all the different scenarios for MOU date and financial report period changes. The conditional logic properly manages report lifecycle and the function calls match the defined signatures.
346-359: Clean implementation for avoiding duplicate reportsThe function correctly identifies which report periods need to be created by filtering out existing ones. The use of
distinctand proper filtering logic ensures no duplicates.
361-397: Well-implemented report insertion functionThe function correctly handles the creation of both narrative and progress reports, properly avoiding duplicates and setting all required fields.
399-432: Correct implementation for financial report insertionThe function properly handles both monthly and quarterly financial report creation based on the project's financialReportPeriod setting. The conditional logic and field assignments are correct.
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
2f2b513 to
800c547
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dbschema/project.gel (2)
335-338: Remove unnecessary zero-length rangeThe
additionalReportPeriodRangecreates a zero-length range fromendDatetoendDatewhich serves no purpose and could cause confusion.- additionalReportPeriodRange := ( - select range(<cal::local_date>endDate, <cal::local_date>endDate, inc_upper := true) - ) - select reportPeriodRanges union additionalReportPeriodRange + select reportPeriodRanges
318-339: Add input validation for function parametersThe function should validate that
startDate <= endDateand thatmonthIntervalis a valid value ('1' or '3') to prevent runtime errors.function create_periodic_report_ranges(startDate: cal::local_date, endDate: cal::local_date, monthInterval: str) -> set of range<cal::local_date> using ( + assert(startDate <= endDate, + message := "Start date must be before or equal to end date"), + assert(monthInterval = '1' or monthInterval = '3', + message := "Month interval must be '1' or '3'"), with
🧹 Nitpick comments (1)
dbschema/project.gel (1)
139-229: Consider uncommenting and fixing the deletion logicThe trigger handles report creation well, but most of the deletion logic is commented out. This means reports won't be cleaned up when MOU dates contract or financial report periods change, potentially leaving orphaned reports.
The commented deletion logic appears to have been disabled due to complexity, but consider implementing at least basic cleanup for common scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dbschema/periodic-report.gel(1 hunks)dbschema/project.gel(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dbschema/periodic-report.gel
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Analyze (javascript)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: Unit
- GitHub Check: Generate (head)
- GitHub Check: Clean
- GitHub Check: Generate (base)
- GitHub Check: lint
🔇 Additional comments (3)
dbschema/project.gel (3)
125-137: LGTM - Clean trigger implementation with proper guard conditionsThe trigger correctly guards against NULL values and the function calls match their signatures properly.
370-420: LGTM - Well-structured function with proper error handlingThe function correctly handles the insertion of both narrative reports and progress reports with appropriate validation and context assignment.
422-462: LGTM - Proper conditional logic for financial report periodsThe function correctly handles different financial report periods (Monthly vs Quarterly) and includes proper null checking.
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
800c547 to
9e4eb31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dbschema/project.gel (2)
364-364: Remove debug logging from production codeDebug statements should not be left in production code as they can impact performance and clutter logs.
- debug := default::log_it("In create_periodic_report_ranges", <json>array_agg(additionalReportPeriodRange)),
134-257: Consider adding monitoring for trigger executionSince these triggers are critical for maintaining data consistency, consider implementing monitoring or alerting to ensure they execute successfully, especially in production environments where trigger failures might go unnoticed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dbschema/debug.gel(1 hunks)dbschema/periodic-report.gel(1 hunks)dbschema/project.gel(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dbschema/periodic-report.gel
🧰 Additional context used
🧠 Learnings (1)
dbschema/project.gel (1)
Learnt from: CarsonF
PR: SeedCompany/cord-api-v3#3377
File: dbschema/migrations/00015-m1obyo7.edgeql:10-29
Timestamp: 2025-04-03T15:06:36.452Z
Learning: In EdgeDB, the multirange data type (e.g., multirange<std::int64>) automatically handles merging of overlapping ranges and elimination of duplicates as part of its native functionality, eliminating the need for additional validation checks.
⏰ 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). (12)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: Unit
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Generate (base)
- GitHub Check: Generate (head)
- GitHub Check: Clean
- GitHub Check: lint
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
dbschema/debug.gel (1)
1-16: Debug logging implementation looks good!The
DebugLogtype andlog_itfunction provide a clean interface for debug logging with structured data support.dbschema/project.gel (6)
63-63: Minor formatting improvement
134-143: Well-structured trigger for initial report creation!The trigger correctly guards against missing MOU dates and properly initializes financial, narrative, and progress reports for new projects.
145-257: Comprehensive update trigger with proper safeguards!The trigger correctly handles all update scenarios including MOU date changes and financial period modifications. The deletion scope is properly limited to the current project's reports.
368-398: Robust duplicate prevention logic!The function correctly filters out existing report periods to prevent duplicates and handles edge cases with proper null checks.
400-508: Well-structured report insertion functions!All report insertion functions follow consistent patterns with proper:
- Audit field population
- Project context assignment
- Relationship handling (especially progress reports per engagement)
443-459: Verify the TODO mentioned in PR objectivesThe PR objectives mention a TODO related to FinancialReport insert and a project-level field. Please ensure this has been addressed or create a follow-up task if needed.
Do you want me to search for any TODOs or FIXMEs in the codebase related to financial reports?
bryanjnelson
changed the title
…s upon project creation
- Handles periodic report date ranges expanding and shrinking - Handles periodic report frequency changing
bryanjnelson
force-pushed
the
0933-periodic-report-trigger
branch
from
9e4eb31 to
9904995
Compare
|
Okay @CarsonF, this is ready for another look. If things can be done better/differently, or if I missed any use cases let me know. If slack is better...that's fine. |
Closes #3478.
Description
This adds
createPeriodicReportsandaddRemovePeriodicReportstriggers to create/update the appropriate number of periodic reports when a project is created, based upon projectmouStart,mouEnd, andfinancialReportPeriod.Open items: