URB-3650: Error handling (+ mailing content rule) for Notice forms#584
URB-3650: Error handling (+ mailing content rule) for Notice forms#584daggelpop wants to merge 4 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 27 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds structured exception handling for NOTICE webservice failures by introducing a ChangesNOTICE Form Error Handling & Mailing Content Rule
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over NoticeResponseActionForm,NoticeResponseFailedEvent: Response Transfer Flow
NoticeResponseActionForm->>WebserviceNotice: transfer_response(data)
alt NoticeResponseException raised
WebserviceNotice-->>NoticeResponseActionForm: NoticeResponseException
NoticeResponseActionForm->>NoticeResponseActionForm: set error status (get_user_message)
NoticeResponseActionForm->>NoticeResponseFailedEvent: _notify_response_error(notice_id, error_report)
NoticeResponseFailedEvent->>ContentRule: trigger INoticeResponseFailedEvent
ContentRule->>MailAction: send failure email to support
else Generic exception raised
WebserviceNotice-->>NoticeResponseActionForm: Exception
NoticeResponseActionForm->>NoticeResponseActionForm: set generic error status + log
NoticeResponseActionForm->>NoticeResponseFailedEvent: _notify_response_error(notice_id, str(exc))
end
end
rect rgba(144, 238, 144, 0.5)
Note over NoticeResponseActionForm,WebserviceNotice: Per-File Transfer Flow
loop each file
NoticeResponseActionForm->>WebserviceNotice: transfer_notice_file(file)
alt success
WebserviceNotice-->>NoticeResponseActionForm: file metadata
NoticeResponseActionForm->>NoticeResponseActionForm: append to sent_documents
else Exception
WebserviceNotice-->>NoticeResponseActionForm: Exception
NoticeResponseActionForm->>NoticeResponseFailedEvent: _notify_response_error(notice_id, str(exc))
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
🤖 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.
Inline comments:
In `@src/Products/urban/browser/notice_forms.py`:
- Around line 218-220: The NoticeResponseFailedEvent is being emitted with an
event_wrapper built from the Urban folder context instead of the current form
context, which breaks context-driven mail template substitutions like
${parent_url}. Replace the use of api.portal.get().urban in the IContextWrapper
initialization with the appropriate current form context (such as self or the
relevant dossier/event object) so that NoticeResponseFailedEvent receives the
correct contextual information for the mail template to resolve its
substitutions properly.
- Around line 269-277: The exception handling in the `transfer_notice_file`
method call is too broad and drops typed exception handling. Instead of catching
only generic Exception, add a specific catch for NoticeResponseException before
the generic Exception handler to preserve its localized user message and
structured error reporting path. The NoticeResponseException should be handled
to properly utilize its built-in error messaging, while the generic Exception
handler can serve as a fallback for unexpected errors. This ensures that typed
exceptions from the NOTICE webservice layer are not lost in translation.
In `@src/Products/urban/locales/fr/LC_MESSAGES/urban.po`:
- Around line 314-317: The French translation in the msgstr line for the
notification error message contains the incorrect word "status" instead of the
proper French translation "statut". In the error message string in the urban.po
file, change "status inapproprié" to "statut inapproprié" to ensure proper
French language usage in the UI.
In `@src/Products/urban/services/notice.py`:
- Around line 219-271: The code directly accesses nested dictionary keys
error["code"]["code"] and error["category"] without validation, which will raise
KeyError if the SOAP WS response contains a partial error payload. Before
branching on these conditions throughout the entire error handling block,
validate that these keys exist using safe dictionary access methods like .get()
with appropriate defaults, or add explicit existence checks. Apply this
validation to all the conditional branches that check error["code"]["code"] and
error["category"] to ensure malformed responses are caught and handled
gracefully by falling through to the default NoticeResponseException case
instead of raising uncaught exceptions.
- Around line 286-293: Wrap the response.json() call and the status check (lines
286-287) in a try-except block that catches ValueError and KeyError exceptions.
When either of these exceptions occur (indicating malformed JSON body or missing
status field), instantiate and raise a NoticeResponseException to maintain the
exception hierarchy contract. This ensures that JSON parsing failures and
missing status keys are treated as NoticeResponseException rather than raw
exceptions, keeping all NOTICE WS response errors consistent within your
exception hierarchy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc3799ed-bb21-4bd3-9ee4-b61fade077f1
📒 Files selected for processing (11)
news/URB-3650.featuresrc/Products/urban/browser/notice_forms.pysrc/Products/urban/contentrules/configure.zcmlsrc/Products/urban/contentrules/notice.pysrc/Products/urban/locales/fr/LC_MESSAGES/urban.posrc/Products/urban/locales/urban.potsrc/Products/urban/migration/update_290.pysrc/Products/urban/migration/upgrades_290.zcmlsrc/Products/urban/notice/exceptions.pysrc/Products/urban/profiles/default/metadata.xmlsrc/Products/urban/services/notice.py
1 participant
Summary by CodeRabbit
New Features
Improvements