Added option to define signers from templates#124
Conversation
📝 WalkthroughWalkthroughThis PR introduces the ability to define outgoing-mail signers from POD template configuration instead of using signer rules. It adds a registry flag ChangesTemplate Signer Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
chris-adam
force-pushed
the
PARAF-426/template_signers
branch
from
ac0b199 to
8573e81
Compare
| </property> | ||
| <property name="behaviors" purge="False"> | ||
| <element value="imio.dms.mail.content.behaviors.IDefaultContentCategoryBehavior"/> | ||
| <element value="imio.dms.mail.content.behaviors.ISigningBehavior"/> |
There was a problem hiding this comment.
Est-ce qu'une migration est en cours de préparation ou il faut en créer une nouvelle ?
chris-adam
force-pushed
the
PARAF-426/template_signers
branch
from
8573e81 to
4491d66
Compare
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@imio/dms/mail/browser/overrides.zcml`:
- Around line 168-177: The browser:page registration for class
imio.dms.mail.browser.documentgenerator.DmsViewConfigurablePodTemplate currently
uses layer=".interfaces.ILayer" which resolves to the wrong package; update the
layer attribute to the fully qualified interface
imio.dms.mail.interfaces.IImioDmsMailLayer to match the other registrations and
keep layer usage consistent across the overrides (adjust the layer="..." value
in that browser:page element referencing DmsViewConfigurablePodTemplate and
template="templates/documentviewerdx.pt").
🪄 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: abc8f84d-caad-4074-ba09-a09c192cecaf
📒 Files selected for processing (11)
CHANGES.rstimio/dms/mail/browser/configure.zcmlimio/dms/mail/browser/documentgenerator.pyimio/dms/mail/browser/overrides.zcmlimio/dms/mail/browser/settings.pyimio/dms/mail/content/behaviors.pyimio/dms/mail/profiles/default/types/ConfigurablePODTemplate.xmlimio/dms/mail/subscribers.pyimio/dms/mail/tests/test_behaviors.pyimio/dms/mail/tests/test_documentgenerator.pyimio/dms/mail/tests/test_subscribers.py
✅ Files skipped from review due to trivial changes (1)
- CHANGES.rst
🚧 Files skipped from review as they are similar to previous changes (8)
- imio/dms/mail/profiles/default/types/ConfigurablePODTemplate.xml
- imio/dms/mail/content/behaviors.py
- imio/dms/mail/browser/configure.zcml
- imio/dms/mail/subscribers.py
- imio/dms/mail/tests/test_subscribers.py
- imio/dms/mail/tests/test_documentgenerator.py
- imio/dms/mail/browser/settings.py
- imio/dms/mail/tests/test_behaviors.py
| <configure package="collective.documentviewer"> | ||
| <browser:page | ||
| name="documentviewer" | ||
| for="collective.documentgenerator.content.pod_template.IConfigurablePODTemplate" | ||
| class="imio.dms.mail.browser.documentgenerator.DmsViewConfigurablePodTemplate" | ||
| template="templates/documentviewerdx.pt" | ||
| permission="zope2.View" | ||
| layer=".interfaces.ILayer" | ||
| /> | ||
| </configure> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other overrides in this package handle layer registration
rg -n "layer=" imio/dms/mail/browser/overrides.zcml | head -20
# Check if there's a pattern for package-scoped overrides
rg -n "configure package=" imio/dms/mail/browser/overrides.zcml -A5Repository: IMIO/imio.dms.mail
Length of output: 3435
Standardize layer specification to imio.dms.mail for consistency.
Line 175 uses layer=".interfaces.ILayer" (resolving to collective.documentviewer.interfaces.ILayer), while lines 183 and 224 both use the fully qualified imio.dms.mail.interfaces.IImioDmsMailLayer. Since all three registrations are in the same overrides file and serve as imio.dms.mail customizations, use imio.dms.mail.interfaces.IImioDmsMailLayer at line 175 for consistency.
🤖 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 `@imio/dms/mail/browser/overrides.zcml` around lines 168 - 177, The
browser:page registration for class
imio.dms.mail.browser.documentgenerator.DmsViewConfigurablePodTemplate currently
uses layer=".interfaces.ILayer" which resolves to the wrong package; update the
layer attribute to the fully qualified interface
imio.dms.mail.interfaces.IImioDmsMailLayer to match the other registrations and
keep layer usage consistent across the overrides (adjust the layer="..." value
in that browser:page element referencing DmsViewConfigurablePodTemplate and
template="templates/documentviewerdx.pt").
There was a problem hiding this comment.
The ZCML registration for DmsViewConfigurablePodTemplate in browser/configure.zcml used layer="imio.dms.mail.interfaces.IImioDmsMailLayer", but the collective.documentgenerator registration it was trying to override used layer="collective.documentviewer.interfaces.ILayer". These are different discriminators, so Zope treated them as two separate registrations rather than an override. At runtime, the DG registration won the adapter lookup.
There was a problem hiding this comment.
@chris-adam, that's a great clarification — you're absolutely right, and my suggestion was incorrect.
In Zope's adapter/page registry, all discriminators (including layer) form the lookup key. To override an existing registration, the overriding registration must use the same discriminators as the original. If collective.documentgenerator registers the documentviewer page with layer="collective.documentviewer.interfaces.ILayer", then matching that exact layer is the only way to replace it. Using imio.dms.mail.interfaces.IImioDmsMailLayer would register a parallel entry under a different discriminator set, leaving the original DG registration still active and winning the lookup — which is exactly the bug you observed.
The .interfaces.ILayer inside <configure package="collective.documentviewer"> resolving to collective.documentviewer.interfaces.ILayer is therefore intentional and correct. I'll note this for future reviews.
✏️ Learnings added
Learnt from: chris-adam
Repo: IMIO/imio.dms.mail PR: 124
File: imio/dms/mail/browser/overrides.zcml:168-177
Timestamp: 2026-05-18T12:49:17.654Z
Learning: In Zope ZCML, overriding a `browser:page` registration requires matching ALL discriminators (including `layer`) of the original registration. In `imio/dms/mail/browser/overrides.zcml`, the `DmsViewConfigurablePodTemplate` page registration for `IConfigurablePODTemplate` must use `layer=".interfaces.ILayer"` (resolving to `collective.documentviewer.interfaces.ILayer`) to properly override the existing `collective.documentgenerator`/`collective.documentviewer` registration. Using `imio.dms.mail.interfaces.IImioDmsMailLayer` instead creates a separate registration and leaves the original DG registration active, so the override never takes effect at runtime.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Summary by CodeRabbit
New Features
Configuration
Behavior / Validation
Tests