URB-3595: Add dematerialized encoding warning on NOTICE folder - #561
Open
WBoudabous wants to merge 3 commits into
Open
URB-3595: Add dematerialized encoding warning on NOTICE folder#561WBoudabous wants to merge 3 commits into
WBoudabous wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAdds a marker interface for licences created through the NOTICE webservice. New and migrated licences receive the marker. A registered warning condition checks it. The default profile upgrade adds the warning and French translation. ChangesNOTICE warning implementation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NOTICE
participant Licence
participant NoticeWarning
participant GenericSetup
NOTICE->>Licence: create licence
Licence->>Licence: provide ILicenceCreatedViaNoticeWS
NoticeWarning->>Licence: evaluate marker
Licence-->>NoticeWarning: return marker status
GenericSetup->>Licence: migrate annotated licences
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 2
🤖 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/warnings/conditions.py`:
- Around line 58-61: Change the condition to check for the presence of the
annotation key instead of its truthiness: instead of relying on notice =
annotations.get("notice_notification", {}) and if notice:, use a key membership
check like if "notice_notification" in annotations: return True (or adjust
existing code to use annotations.__contains__("notice_notification")). Update
the branch that currently uses the local variable notice to use this
key-presence check so empty but-present annotations still trigger the banner.
In `@src/Products/urban/migration/update_290.py`:
- Around line 417-420: The migration currently writes a hardcoded French
byte-escaped string into the warning dict (the dict with "condition":
"urban.warnings.notice", "level": "warning", "message": "..."); replace that
literal with the appropriate i18n translation key instead of the UTF-8 text —
set the "message" value to the translation key used by your i18n system (e.g.
"urban.warnings.notice" or the specific key for the message) so the UI can
resolve the localized string at runtime; update the dict in the migration (the
place where "message" is set) to use the key and remove the escaped literal.
🪄 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: 10b3cf55-701a-4f13-a11b-ced9528ac16b
📒 Files selected for processing (6)
news/URB-3595.featuresrc/Products/urban/browser/warnings/conditions.pysrc/Products/urban/browser/warnings/configure.zcmlsrc/Products/urban/locales/fr/LC_MESSAGES/urban.posrc/Products/urban/migration/update_290.pysrc/Products/urban/migration/upgrades_290.zcml
Comment on lines
+58
to
+61
| notice = annotations.get("notice_notification", {}) | ||
|
|
||
| if notice: | ||
| return True |
There was a problem hiding this comment.
Check annotation key presence instead of value truthiness.
Line 60 currently requires a truthy annotation value, so dossiers with an existing but empty notice_notification annotation won’t show the banner. The condition should match key presence.
Proposed fix
- notice = annotations.get("notice_notification", {})
-
- if notice:
+ if "notice_notification" in annotations:
return True🤖 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/warnings/conditions.py` around lines 58 - 61,
Change the condition to check for the presence of the annotation key instead of
its truthiness: instead of relying on notice =
annotations.get("notice_notification", {}) and if notice:, use a key membership
check like if "notice_notification" in annotations: return True (or adjust
existing code to use annotations.__contains__("notice_notification")). Update
the branch that currently uses the local variable notice to use this
key-presence check so empty but-present annotations still trigger the banner.
mpeeters
requested changes
Jun 15, 2026
mpeeters
left a comment
mpeeters
left a comment
Member
There was a problem hiding this comment.
Please fix the following issues and resolve conflicts
| """ | ||
|
|
||
| def evaluate(self): | ||
| events = self.licence.getAllEvents() |
Member
There was a problem hiding this comment.
This is not efficient, You should first lookup for Notice related events, also this should be cached for performance reason.
| { | ||
| "condition": "urban.warnings.notice", | ||
| "level": "warning", | ||
| "message": "Ce dossier a \xc3\xa9t\xc3\xa9 encod\xc3\xa9 de mani\xc3\xa8re d\xc3\xa9mat\xc3\xa9rialis\xc3\xa9e", |
WBoudabous
force-pushed
the
URB-3595_add_notice_warning
branch
from
June 16, 2026 09:10
f60205a to
b2223fd
Compare
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 `@src/Products/urban/migration/upgrades_290.zcml`:
- Around line 127-133: The upgrade step defining the add_notice_warning handler
at lines 127-133 duplicates the edge source="2913" destination="2914" that
already exists in the earlier upgrade step at lines 111-117, creating an
ambiguous path in GenericSetup's upgrade graph resolution. Change the source and
destination attributes of this duplicate step to the next consecutive version
pair (change source from "2913" to "2914" and destination from "2914" to "2915",
or to whatever the appropriate next version numbers are for this migration
sequence) to avoid the duplicate edge.
🪄 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: 3dae6c76-9576-42cd-9713-e25f3548da81
📒 Files selected for processing (7)
news/URB-3595.featuresrc/Products/urban/browser/licence/licenceview.pysrc/Products/urban/browser/warnings/conditions.pysrc/Products/urban/browser/warnings/configure.zcmlsrc/Products/urban/locales/fr/LC_MESSAGES/urban.posrc/Products/urban/migration/update_290.pysrc/Products/urban/migration/upgrades_290.zcml
✅ Files skipped from review due to trivial changes (1)
- news/URB-3595.feature
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Products/urban/browser/warnings/configure.zcml
- src/Products/urban/browser/warnings/conditions.py
- src/Products/urban/locales/fr/LC_MESSAGES/urban.po
Comment on lines
+127
to
+133
| <gs:upgradeStep | ||
| title="Add notice warning" | ||
| description="" | ||
| source="2913" | ||
| destination="2914" | ||
| handler=".update_290.add_notice_warning" | ||
| profile="Products.urban:default" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import xml.etree.ElementTree as ET
from collections import defaultdict
path = "src/Products/urban/migration/upgrades_290.zcml"
ns = {"gs": "http://namespaces.zope.org/genericsetup"}
root = ET.parse(path).getroot()
pairs = defaultdict(list)
for step in root.findall("gs:upgradeStep", ns):
src = step.attrib.get("source")
dst = step.attrib.get("destination")
handler = step.attrib.get("handler")
pairs[(src, dst)].append(handler)
dups = {k: v for k, v in pairs.items() if len(v) > 1}
print("Duplicate source/destination pairs:")
for (src, dst), handlers in sorted(dups.items()):
print(f" {src}->{dst}: {handlers}")
PYRepository: IMIO/Products.urban
Length of output: 198
Duplicate upgrade edge 2913 → 2914 can skip add_notice_warning handler.
Lines 127-133 define a second source="2913" and destination="2914" step that duplicates the edge at lines 111-117. GenericSetup's upgrade graph resolution treats this as an ambiguous path, so one handler can be skipped. The warning migration may never execute on upgraded sites.
Resolve by changing this step to the next version pair:
Suggested fix
<gs:upgradeStep
title="Add notice warning"
description=""
- source="2913"
- destination="2914"
+ source="2915"
+ destination="2916"
handler=".update_290.add_notice_warning"
profile="Products.urban:default" />🤖 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/migration/upgrades_290.zcml` around lines 127 - 133, The
upgrade step defining the add_notice_warning handler at lines 127-133 duplicates
the edge source="2913" destination="2914" that already exists in the earlier
upgrade step at lines 111-117, creating an ambiguous path in GenericSetup's
upgrade graph resolution. Change the source and destination attributes of this
duplicate step to the next consecutive version pair (change source from "2913"
to "2914" and destination from "2914" to "2915", or to whatever the appropriate
next version numbers are for this migration sequence) to avoid the duplicate
edge.
mpeeters
requested changes
Jun 23, 2026
| @@ -538,3 +538,22 @@ def reindex_getDecisionDate(context): | |||
| reindexIndexes(None, ["getDecisionDate"]) | |||
|
|
|||
| logger.info("upgrade step done!") | |||
| name="urban.warnings.bound_ticket_settlement" | ||
| /> | ||
|
|
||
| <adapter |
Comment on lines
+56
to
+64
| def evaluate(self): | ||
| events = self.licence.getAllEvents(interfaces.IUrbanEventNotice) | ||
|
|
||
| for event in events: | ||
| annotations = IAnnotations(event) | ||
| notice = annotations.get("notice_notification", {}) | ||
|
|
||
| if notice: | ||
| return True |
Member
There was a problem hiding this comment.
This will slower render of licences a lot, can you instead add an interface to the licence object when it is created to specify that this is a licence created through notice WS. And then use it in this condition.
| reindexIndexes(None, ["getDecisionDate"]) | ||
|
|
||
| logger.info("upgrade step done!") | ||
| def add_notice_warning(context): |
Member
There was a problem hiding this comment.
This method should also add the interface mentioned for NoticeWarning class for all existing created licences related to notice
| { | ||
| "condition": "urban.warnings.notice", | ||
| "level": "warning", | ||
| "message": "urban.warnings.notice", |
Member
There was a problem hiding this comment.
You should use the translation / translated message here and not in src/Products/urban/browser/licence/licenceview.py
mpeeters
changed the title
WBoudabous
force-pushed
the
URB-3595_add_notice_warning
branch
from
August 4, 2026 09:19
b2223fd to
e6d0ee9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Products/urban/migration/update_290.py (1)
754-761: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFree objects and add savepoints during the full-catalog scan.
The loop calls
getObject()on every licence in the site and loads all notice events of each licence. All of them stay in the ZODB cache inside one transaction. On large sites this grows memory and transaction size without bound.Add periodic savepoints and deactivate each licence after processing.
Proposed fix
catalog = api.portal.get_tool("portal_catalog") licence_brains = catalog(object_provides=IGenericLicence.__identifier__) - for licence_brain in licence_brains: + total = len(licence_brains) + for count, licence_brain in enumerate(licence_brains, start=1): licence = licence_brain.getObject() for event in licence.getAllEvents(IUrbanEventNotice): if IAnnotations(event).get("notice_notification", {}): alsoProvides(licence, ILicenceCreatedViaNoticeWS) break + if count % 500 == 0: + logger.info("processed %d/%d licences", count, total) + transaction.savepoint(optimistic=True) + licence._p_jar.cacheGC()Import
transactionif the module does not import it yet.🤖 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/migration/update_290.py` around lines 754 - 761, Update the full-catalog scan around licence_brain.getObject() to periodically create transaction savepoints and deactivate each processed licence after its events are checked. Import transaction if needed, choose a bounded interval for savepoints, and ensure deactivation occurs for every licence while preserving the existing ILicenceCreatedViaNoticeWS assignment behavior.
🤖 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/migration/update_290.py`:
- Around line 729-730: Add two blank lines between the completion log statement
and the add_notice_warning function definition to satisfy PEP8 E302, without
changing the surrounding migration logic.
- Around line 754-761: After alsoProvides in the migration loop, update the
licence persistence state by setting licence._p_changed and reindex the licence
via reindexObject(), matching the behavior in create_licence. Apply both
operations only when a notice event causes ILicenceCreatedViaNoticeWS to be
added, so catalog queries include migrated licences.
---
Nitpick comments:
In `@src/Products/urban/migration/update_290.py`:
- Around line 754-761: Update the full-catalog scan around
licence_brain.getObject() to periodically create transaction savepoints and
deactivate each processed licence after its events are checked. Import
transaction if needed, choose a bounded interval for savepoints, and ensure
deactivation occurs for every licence while preserving the existing
ILicenceCreatedViaNoticeWS assignment behavior.
🪄 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: fa85b74a-6166-440a-9580-105504b84b7b
📒 Files selected for processing (9)
news/URB-3595.featuresrc/Products/urban/browser/cron/notice.pysrc/Products/urban/browser/warnings/conditions.pysrc/Products/urban/browser/warnings/configure.zcmlsrc/Products/urban/interfaces.pysrc/Products/urban/locales/fr/LC_MESSAGES/urban.posrc/Products/urban/migration/update_290.pysrc/Products/urban/migration/upgrades_290.zcmlsrc/Products/urban/profiles/default/metadata.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- news/URB-3595.feature
- src/Products/urban/browser/warnings/configure.zcml
- src/Products/urban/locales/fr/LC_MESSAGES/urban.po
Comment on lines
729
to
+730
| logger.info("upgrade step done!") | ||
| def add_notice_warning(context): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add two blank lines before add_notice_warning.
Line 730 follows line 729 with no separation. This breaks PEP8 E302 and repeats an earlier reviewer comment on line 729.
Proposed fix
logger.info("upgrade step done!")
+
+
def add_notice_warning(context):🤖 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/migration/update_290.py` around lines 729 - 730, Add two
blank lines between the completion log statement and the add_notice_warning
function definition to satisfy PEP8 E302, without changing the surrounding
migration logic.
Comment on lines
+754
to
+761
| catalog = api.portal.get_tool("portal_catalog") | ||
| licence_brains = catalog(object_provides=IGenericLicence.__identifier__) | ||
| for licence_brain in licence_brains: | ||
| licence = licence_brain.getObject() | ||
| for event in licence.getAllEvents(IUrbanEventNotice): | ||
| if IAnnotations(event).get("notice_notification", {}): | ||
| alsoProvides(licence, ILicenceCreatedViaNoticeWS) | ||
| break |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reindex object_provides after alsoProvides.
The migration applies the marker but never reindexes the licence. create_licence in src/Products/urban/browser/cron/notice.py calls reindexObject() after alsoProvides, so migrated licences and newly created licences end up with different catalog state. Any catalog query on object_provides=ILicenceCreatedViaNoticeWS.__identifier__ will miss the migrated licences.
Also set _p_changed to match the runtime path.
Proposed fix
for event in licence.getAllEvents(IUrbanEventNotice):
if IAnnotations(event).get("notice_notification", {}):
alsoProvides(licence, ILicenceCreatedViaNoticeWS)
+ licence._p_changed = 1
+ licence.reindexObject(idxs=["object_provides"])
break📝 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.
Suggested change
| catalog = api.portal.get_tool("portal_catalog") | |
| licence_brains = catalog(object_provides=IGenericLicence.__identifier__) | |
| for licence_brain in licence_brains: | |
| licence = licence_brain.getObject() | |
| for event in licence.getAllEvents(IUrbanEventNotice): | |
| if IAnnotations(event).get("notice_notification", {}): | |
| alsoProvides(licence, ILicenceCreatedViaNoticeWS) | |
| break | |
| catalog = api.portal.get_tool("portal_catalog") | |
| licence_brains = catalog(object_provides=IGenericLicence.__identifier__) | |
| for licence_brain in licence_brains: | |
| licence = licence_brain.getObject() | |
| for event in licence.getAllEvents(IUrbanEventNotice): | |
| if IAnnotations(event).get("notice_notification", {}): | |
| alsoProvides(licence, ILicenceCreatedViaNoticeWS) | |
| licence._p_changed = 1 | |
| licence.reindexObject(idxs=["object_provides"]) | |
| break |
🤖 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/migration/update_290.py` around lines 754 - 761, After
alsoProvides in the migration loop, update the licence persistence state by
setting licence._p_changed and reindex the licence via reindexObject(), matching
the behavior in create_licence. Apply both operations only when a notice event
causes ILicenceCreatedViaNoticeWS to be added, so catalog queries include
migrated licences.
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
2 participants
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.
This pull request adds a new warning condition to display an info banner on NOTICE dossiers.
The warning displays the following message:
Ce dossier a été encodé de manière dématérialiséewhen a licence contains a
notice_notificationannotation.Summary by CodeRabbit
New Features
Translations