Skip to content

URB-3659: Add due date to imported summary report events#590

Open
daggelpop wants to merge 1 commit into
2.9.xfrom
URB-3659_add_due_date_to_RS
Open

URB-3659: Add due date to imported summary report events#590
daggelpop wants to merge 1 commit into
2.9.xfrom
URB-3659_add_due_date_to_RS

Conversation

@daggelpop

@daggelpop daggelpop commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Note: so far, only summary report notifications have had data in the dueDate field.

Summary by CodeRabbit

  • New Features

    • Imported summary report events now include due-date information when available.
    • Notifications can now surface a due date from incoming data, making event deadlines more complete.
  • Bug Fixes

    • Summary report event details now carry the due date into the event record, improving accuracy for downstream viewing and tracking.

@daggelpop daggelpop requested a review from mpeeters June 26, 2026 09:05
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds due_date parsing to notice notifications and copies that value onto imported summary report events as UltimeDate. The feature note is updated to mention due dates for [daggelpop].

Changes

Due date propagation

Layer / File(s) Summary
Parse notification due date
src/Products/urban/notice/notification.py
NoticeNotification reads dueDate, parses the first 19 characters with %Y-%m-%dT%H:%M:%S, and returns a date or None.
Set event due date
src/Products/urban/browser/cron/notice.py, news/URB-3659.feature
SummaryReportHandler.fill_incoming_event converts notification.due_date to DateTime and assigns it to event.UltimeDate; the feature note records due dates for imported summary report events.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • IMIO/Products.urban#567: Adds cron notice routing into SummaryReportHandler, which is the same flow updated here to set UltimeDate from notification.due_date.
  • IMIO/Products.urban#568: Routes another notice variant into SummaryReportHandler, sharing the imported summary-report event path adjusted in this PR.

Suggested reviewers

  • mpeeters

Poem

I sniffed a due date by the moon,
and tucked it in the event quite soon.
Hop hop, the notice learns its day,
then bounds along the cron relay. 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding due date support to imported summary report events.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch URB-3659_add_due_date_to_RS

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/Products/urban/browser/cron/notice.py (1)

609-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the UltimeDate propagation path with a handler test.

The feature value is in the cross-layer assignment, so a handler/import test asserting that a notification dueDate ends up on event.UltimeDate would protect this behavior better than parser-only coverage.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Products/urban/browser/cron/notice.py` around lines 609 - 611, The
UltimeDate propagation in the notification handling path is only covered
indirectly, so add a handler/import test that exercises the cross-layer
assignment from notification dueDate to event.UltimeDate. Use the existing
notification processing flow around the code in notice.py, and assert that when
self.notification.due_date is present, the resulting event.setUltimeDate call
receives the expected DateTime value. Keep the test focused on the handler-level
behavior rather than parser-only coverage.
src/Products/urban/notice/notification.py (1)

245-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a regression test for dueDate parsing.

This new property is now part of the import contract, but the supplied changes do not add coverage for valid dueDate values or the None case already present in test fixtures. A small parser-level test here would make future format changes much safer.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Products/urban/notice/notification.py` around lines 245 - 250, Add a
regression test around Notification.due_date to cover both valid dueDate parsing
and the existing None/absent case from fixtures. Update the relevant
notification parser tests to instantiate or load a Notification object with a
sample dueDate string and assert the returned date is parsed correctly, then add
a case where dueDate is missing or null and confirm due_date stays falsy/None.
Use the due_date property and _get_data("dueDate") behavior as the key entry
points when locating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/Products/urban/browser/cron/notice.py`:
- Around line 609-611: The UltimeDate propagation in the notification handling
path is only covered indirectly, so add a handler/import test that exercises the
cross-layer assignment from notification dueDate to event.UltimeDate. Use the
existing notification processing flow around the code in notice.py, and assert
that when self.notification.due_date is present, the resulting
event.setUltimeDate call receives the expected DateTime value. Keep the test
focused on the handler-level behavior rather than parser-only coverage.

In `@src/Products/urban/notice/notification.py`:
- Around line 245-250: Add a regression test around Notification.due_date to
cover both valid dueDate parsing and the existing None/absent case from
fixtures. Update the relevant notification parser tests to instantiate or load a
Notification object with a sample dueDate string and assert the returned date is
parsed correctly, then add a case where dueDate is missing or null and confirm
due_date stays falsy/None. Use the due_date property and _get_data("dueDate")
behavior as the key entry points when locating the code.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 34a711ad-7735-4a21-8f38-ec7256bc91c8

📥 Commits

Reviewing files that changed from the base of the PR and between 328638e and 1dd017a.

📒 Files selected for processing (3)
  • news/URB-3659.feature
  • src/Products/urban/browser/cron/notice.py
  • src/Products/urban/notice/notification.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant