[Gel] - Add trigger to create financial/narrative 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
📝 WalkthroughWalkthroughThis update introduces triggers and functions on the Changes
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)
dbschema/project.gel
Outdated
| interval := (select | ||
| if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' else '3'), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for financialReportPeriod values
The interval calculation assumes only Monthly (mapped to '1') or Quarterly (mapped to '3') values, but there's no validation that other enum values won't cause unexpected behavior.
- interval := (select
- if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' else '3'),
+ interval := (select
+ if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1'
+ else if __new__.financialReportPeriod = default::ReportPeriod.Quarterly then '3'
+ else (assert(false, message := "Unsupported financial report period: " ++ <str>__new__.financialReportPeriod))),📝 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.
| interval := (select | |
| if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' else '3'), | |
| interval := ( | |
| select | |
| if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' | |
| else if __new__.financialReportPeriod = default::ReportPeriod.Quarterly then '3' | |
| else ( | |
| assert( | |
| false, | |
| message := "Unsupported financial report period: " ++ | |
| <str>__new__.financialReportPeriod | |
| ) | |
| ) | |
| ), |
🤖 Prompt for AI Agents
In dbschema/project.gel around lines 134 to 135, the interval calculation
assumes financialReportPeriod is either Monthly or Quarterly without validating
other enum values. Add validation logic to explicitly check if
financialReportPeriod is one of the expected values (Monthly or Quarterly) and
handle or reject any other values to prevent unexpected behavior. This can be
done by adding a conditional check or a validation step before the interval
assignment.
| additionalReportPeriodRange := ( | ||
| select range(<cal::local_date>endDate, <cal::local_date>endDate, inc_upper := true) | ||
| ) | ||
| select reportPeriodRanges union additionalReportPeriodRange |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary additional report period range
The additionalReportPeriodRange creates a zero-length range from endDate to endDate which 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📝 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.
| additionalReportPeriodRange := ( | |
| select range(<cal::local_date>endDate, <cal::local_date>endDate, inc_upper := true) | |
| ) | |
| select reportPeriodRanges union additionalReportPeriodRange | |
| select reportPeriodRanges |
🤖 Prompt for AI Agents
In dbschema/project.gel around lines 318 to 321, remove the definition and usage
of additionalReportPeriodRange since it creates a zero-length range from endDate
to endDate that is unnecessary. Update the code to directly select
reportPeriodRanges without unioning the redundant additionalReportPeriodRange.
dbschema/project.gel
Outdated
| interval := ( | ||
| select (if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' else '3') |
There was a problem hiding this comment.
Fix inconsistent conditional logic for interval calculation
The interval calculation logic differs between the insert and update triggers - the update trigger doesn't handle the case where financialReportPeriod might be null after the when clause conditions.
- interval := (
- select (if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' else '3')
- ),
+ interval := (
+ select (
+ if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1'
+ else if __new__.financialReportPeriod = default::ReportPeriod.Quarterly then '3'
+ else (assert(false, message := "Unsupported financial report period: " ++ <str>__new__.financialReportPeriod))
+ )
+ ),📝 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.
| interval := ( | |
| select (if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' else '3') | |
| interval := ( | |
| select ( | |
| if __new__.financialReportPeriod = default::ReportPeriod.Monthly then '1' | |
| else if __new__.financialReportPeriod = default::ReportPeriod.Quarterly then '3' | |
| else (assert(false, message := "Unsupported financial report period: " ++ <str>__new__.financialReportPeriod)) | |
| ) | |
| ), |
🤖 Prompt for AI Agents
In dbschema/project.gel around lines 161 to 162, the interval calculation uses
conditional logic that is inconsistent between insert and update triggers and
does not handle null values for financialReportPeriod in the update trigger.
Modify the update trigger's interval calculation to include a null check for
financialReportPeriod and ensure it matches the insert trigger's logic by
explicitly handling the null case and setting a default interval value
accordingly.
ac6823b to
60348cd
Compare
…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
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.
| financialReports := Project::insert_financial_reports(array_agg(financialReportRanges), __new__) | ||
| select financialReports | ||
| ) else if __new__.financialReportPeriod = default::ReportPeriod.Quarterly then ( | ||
| select Project::insert_financial_reports(array_agg(reportRanges), __new__) | ||
| ) else ( | ||
| select <default::FinancialReport>{} | ||
| ) | ||
| ), | ||
| insertedNarrativeAndProgressReports := Project::insert_narrative_and_progress_reports(array_agg(reportRanges), __new__) |
There was a problem hiding this comment.
Critical: Function signature mismatch will cause runtime errors
The function calls in this trigger don't match the defined function signatures:
insert_financial_reportsis called with 2 parameters but defined to expect 4insert_narrative_and_progress_reportsis called with 2 parameters but defined to expect 3
Apply this diff to fix the function calls:
- financialReports := Project::insert_financial_reports(array_agg(financialReportRanges), __new__)
+ financialReports := Project::insert_financial_reports(__new__, (select <default::FinancialReport>{}), array_agg(financialReportRanges), (select <range<cal::local_date>>{}))- select Project::insert_financial_reports(array_agg(reportRanges), __new__)
+ select Project::insert_financial_reports(__new__, (select <default::FinancialReport>{}), (select <range<cal::local_date>>{}), array_agg(reportRanges))- insertedNarrativeAndProgressReports := Project::insert_narrative_and_progress_reports(array_agg(reportRanges), __new__)
+ insertedNarrativeAndProgressReports := Project::insert_narrative_and_progress_reports(__new__, array_agg(reportRanges), (select <default::PeriodicReport>{}))Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dbschema/project.gel around lines 144 to 152, the calls to
insert_financial_reports and insert_narrative_and_progress_reports have
incorrect argument counts that do not match their function definitions, causing
runtime errors. Update the insert_financial_reports calls to pass four
parameters as defined, and modify the insert_narrative_and_progress_reports call
to include the required third argument, ensuring all function calls match their
signatures exactly.
Monday Story
Monday Task
Description
This adds a
createPeriodicReportsOnInserttrigger to create the appropriate number of both narrative and financial reports when a project is created, based upon thefinancialReportPeriodin the project.Open items for discussion/modification:
create_periodic_report_rangesfunction be made more generic to handle other use cases? (Probably not.)Is thecreate_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?create_periodic_report_rangescould be written a bit more cleanlymouStart,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?TODOin theFinancialReportinsert; I'm unclear what that project level field is used for