URB-3480: Add content rule events for successful and broken NOTICe handling#533
Open
daggelpop wants to merge 3 commits into
Open
URB-3480: Add content rule events for successful and broken NOTICe handling#533daggelpop wants to merge 3 commits into
daggelpop wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAdds Plone content-rule events and substitutions for NOTICe import lifecycle, emits NoticeImportSucceededEvent and NoticeImportFailedEvent from the notice import cron flow, registers ZCML bindings for subscribers and substitutions, and adds French translations and a feature note. Changes
Sequence DiagramsequenceDiagram
participant Cron as Notice Import Process
participant Wrapper as IContextWrapper
participant Emitter as Event Dispatcher
participant RuleEngine as Plone Content Rules
participant Action as Rule Action Handler
Cron->>Wrapper: Wrap notice details (notice_id, notice_type, context)
Wrapper->>Emitter: Emit NoticeImportSucceededEvent / NoticeImportFailedEvent
Emitter->>RuleEngine: Dispatch event to rule engine
RuleEngine->>RuleEngine: Evaluate conditions (using notice_id / notice_type substitutions)
RuleEngine->>Action: Execute matching actions (email, notifications, etc.)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Products/urban/browser/cron/notice.py`:
- Around line 164-171: The helper _notify_import_error is never called so
NoticeImportFailedEvent is never emitted; locate the import workflow (e.g., the
function/method that performs the notice import—search for import_notice,
process_notice_import, or similar in the same module/class) and call
_notify_import_error(notice_id=..., notice_type=...) inside the exception/error
handling path where imports fail (catch the exception or detect import failure),
ensuring you pass the relevant notice_id and notice_type before re-raising or
handling the error so rules bound to NoticeImportFailedEvent will run.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
news/URB-3480.featuresrc/Products/urban/browser/cron/notice.pysrc/Products/urban/contentrules/configure.zcmlsrc/Products/urban/contentrules/notice.pysrc/Products/urban/locales/fr/LC_MESSAGES/urban.posrc/Products/urban/locales/urban.pot
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Products/urban/browser/cron/notice.py`:
- Around line 79-80: The code is overloading notice_type with failure reasons
(e.g., "failed retry", "failed import"); change the data model to keep
notice_type as the actual notice type and add a separate field (e.g.,
failure_reason or import_status) for failure details: update assignments that
set notice_type = "failed retry"/"failed import" to instead set failure_reason =
"failed retry"/"failed import" (or pass that string as a new parameter), adjust
calls to _notify_import_error to accept/forward the failure_reason, and update
any place that reads notice_type (including uses around failed_notice_id,
remaining_failed, and the _notify_import_error implementation) to use the new
failure_reason field for filtering and messages. Ensure all occurrences
referenced in the diff (the blocks using failed_notice_id, remaining_failed and
calls to _notify_import_error) are updated consistently.
- Around line 79-80: Make failed-event emission best-effort: wrap calls that
emit failure events (calls to IContextWrapper(...) and notify(...) and the
helper _notify_import_error) in their own try/except so any exception from
emitting the error is caught and logged but not allowed to propagate or
interrupt bookkeeping (e.g., the remaining_failed.append flow). Specifically,
around the places that call IContextWrapper(...) / notify(...) (including the
block that calls self._notify_import_error and the other locations referenced),
catch Exception, log the emission failure with context, and continue so the
original import exception handling and remaining_failed/other bookkeeping always
run; do this for all occurrences (lines referenced 122-123, 167-174 and the
shown call to self._notify_import_error) so event emission is non-blocking.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Products/urban/browser/cron/notice.py (1)
79-80:⚠️ Potential issue | 🟠 MajorKeep
notice_typesemantic; use a separate failure field.On Line 79, Line 122, and Line 156,
notice_typeis populated with failure reasons instead of the actual NOTICe type. This breaks type-based content-rule filtering and makes${notice_type}substitutions misleading for failed imports.🔧 Suggested direction
- def _notify_import_error(self, notice_id="", notice_type=""): + def _notify_import_error(self, notice_id="", notice_type="", failure_reason=""): try: args = { "notice_id": notice_id, "notice_type": notice_type, + "failure_reason": failure_reason, } urban_folder = api.portal.get().urban event_wrapper = IContextWrapper(urban_folder)(**args) notify(NoticeImportFailedEvent(event_wrapper))- self._notify_import_error(failed_notice_id, "failed retry") + self._notify_import_error( + notice_id=failed_notice_id, + notice_type="", # unknown here unless you persist/recover it + failure_reason="failed retry", + )- self._notify_import_error(notice_id, "failed import") + self._notify_import_error( + notice_id=notice_id, + notice_type="", # pass real type when available + failure_reason="failed import", + )Also applies to: 122-123, 156-157, 176-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Products/urban/browser/cron/notice.py` around lines 79 - 80, The code is incorrectly overwriting notice_type with failure reasons; keep notice_type as the actual NOTICE type and add a separate failure field (e.g., import_failure or failure_reason) to carry error text. Update places that call _notify_import_error (the call that currently passes "failed retry" or other messages alongside remaining_failed) so they pass the real notice_type unchanged and an extra parameter or payload key for the failure reason, and modify _notify_import_error to accept and log that new failure field instead of using notice_type for failures; ensure any message templating or substitutions still use notice_type for the original type and use the new field where failure details are needed (references: _notify_import_error, remaining_failed, and the blocks that append failed_notice_id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Products/urban/browser/cron/notice.py`:
- Around line 79-80: The code is incorrectly overwriting notice_type with
failure reasons; keep notice_type as the actual NOTICE type and add a separate
failure field (e.g., import_failure or failure_reason) to carry error text.
Update places that call _notify_import_error (the call that currently passes
"failed retry" or other messages alongside remaining_failed) so they pass the
real notice_type unchanged and an extra parameter or payload key for the failure
reason, and modify _notify_import_error to accept and log that new failure field
instead of using notice_type for failures; ensure any message templating or
substitutions still use notice_type for the original type and use the new field
where failure details are needed (references: _notify_import_error,
remaining_failed, and the blocks that append failed_notice_id).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation