Conversation
📝 WalkthroughWalkthroughA new admin-only browser view, SessionAnnotationInfoView, was added to expose imio.esign session annotation data as rendered HTML. Supporting ZCML registration, a template, tests, and a persistent-to-native helper were added to collect, convert, and render session data and link UIDs. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Client)
participant View as SessionAnnotationInfoView
participant Annotations as Session Annotations Store
participant UIDLookup as uuidToObject / Catalog
participant Renderer as Template / _render_value
Browser->>View: GET /session-annotation-info
View->>Annotations: collect sessions (persistent_to_native)
Annotations-->>View: session data
View->>UIDLookup: resolve UIDs when rendering
UIDLookup-->>View: object titles/links
View->>Renderer: esign_session_html / _render_value
Renderer-->>View: formatted HTML
View-->>Browser: HTML response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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)
Comment |
de5ca03 to
31cef0e
Compare
31cef0e to
e2b3f22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/imio/esign/browser/actions.py (1)
179-190: Reuse the existing annotation indexes here.
add_files_to_session()already keepsannot["c_uids"]andannot["uids"]in sync. Walking every session and every file again makes this admin page scale with the whole annotation instead of doing direct lookups.Index-based variant
`@property` def esign_sessions(self): """Return list of (session_id, session_data) for all sessions.""" annot = get_session_annotation() - c_uid = self.context.UID() - result = [] - for session_id in annot['sessions']: - session = annot.get("sessions", {}).get(session_id) - # If any file in this session is in this context - if any(f['context_uid'] == c_uid for f in session['files']): - result.append((session_id, persistent_to_native(session))) + session_ids = { + annot["uids"][file_uid] + for file_uid in annot.get("c_uids", {}).get(self.context.UID(), []) + if file_uid in annot.get("uids", {}) + } + result = [ + (session_id, persistent_to_native(annot["sessions"][session_id])) + for session_id in session_ids + if session_id in annot.get("sessions", {}) + ] return sorted(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/actions.py` around lines 179 - 190, The esign_sessions property is iterating every session and every file instead of using the maintained indexes; change it to use the annotation indexes kept by add_files_to_session(): read c_uid = self.context.UID(), then get session_ids = annot.get('c_uids', {}).get(c_uid, []) (fall back to empty list), for each session_id fetch session = annot.get('sessions', {}).get(session_id) and if present append (session_id, persistent_to_native(session)); finally return sorted(result). Ensure you reference annot keys 'c_uids', 'sessions', and use persistent_to_native and self.context.UID() as in the original function.
🤖 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/imio/esign/tests/test_actions.py`:
- Around line 5-6: The tests import Python-2-only HTMLParser and use basestring
which breaks under Python 3; wrap the imports and symbol definitions in
compatibility try/excepts: replace the top-level "from HTMLParser import
HTMLParser" with a try block that imports HTMLParser from html.parser and falls
back to "from HTMLParser import HTMLParser" for Py2, and add a try/except to
ensure "basestring" exists (e.g. try: basestring except NameError: basestring =
(str, bytes)) so existing isinstance(..., basestring) checks in the tests (used
around the SessionAnnotationInfoView-related assertions) keep working across
Py2/3.
---
Nitpick comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 179-190: The esign_sessions property is iterating every session
and every file instead of using the maintained indexes; change it to use the
annotation indexes kept by add_files_to_session(): read c_uid =
self.context.UID(), then get session_ids = annot.get('c_uids', {}).get(c_uid,
[]) (fall back to empty list), for each session_id fetch session =
annot.get('sessions', {}).get(session_id) and if present append (session_id,
persistent_to_native(session)); finally return sorted(result). Ensure you
reference annot keys 'c_uids', 'sessions', and use persistent_to_native and
self.context.UID() as in the original function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da4e9dfb-3e6c-4f0e-96b7-e0bff6b3d0b9
📒 Files selected for processing (6)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/templates/session_annotation_info.ptsrc/imio/esign/tests/test_actions.pysrc/imio/esign/utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/imio/esign/browser/actions.py (1)
185-186: Redundant.get()lookup.Since you're iterating over
annot['sessions']keys, the session is guaranteed to exist. The current pattern could mask issues ifsessionends up as an empty dict (line 188 would then raiseKeyErroron['files']).♻️ Simplify to direct access
for session_id in annot['sessions']: - session = annot.get("sessions", {}).get(session_id) + session = annot['sessions'][session_id]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/actions.py` around lines 185 - 186, The lookup for session inside the loop over annot['sessions'] is redundant and can mask issues; replace the double .get() call used to set session (currently session = annot.get("sessions", {}).get(session_id)) with a direct access to the mapping (e.g., session = annot['sessions'][session_id]) so missing entries raise immediately and you don't silently continue with an empty dict before referencing session['files'] or other keys in the subsequent code.src/imio/esign/tests/test_actions.py (1)
250-302: Consider using more targeted assertions for the rendered HTML.The large string comparison is brittle and will fail on minor formatting or field ordering changes. Consider asserting on specific key aspects (e.g., that UIDs are converted to links, specific fields are present) rather than exact string matching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/test_actions.py` around lines 250 - 302, The current test uses one large exact string equality on HTMLParser().unescape(view.esign_session_html(esign_session[1])) which is brittle; change the test to capture the rendered string (call view.esign_session_html with esign_session[1] and unescape it) and replace the big assertEqual with several targeted assertions: check that specific keys/values exist (e.g., 'sign_id': '012345600000' and "title": u'[ia.parapheo] Session 012345600000'), verify that expected UID links appear (e.g., links to '/plone/test_session_folder/annex0' and '/plone/test_session_folder/view'), and assert presence/format of important structures like the files list and signers rather than exact whole-string equality; locate this logic around the HTMLParser().unescape(...) call and view.esign_session_html in the test_actions.py test and update assertions accordingly.
🤖 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/imio/esign/browser/actions.py`:
- Around line 173-177: The code uses basestring which is undefined in Python 3;
add a compatibility alias (e.g., define string_types = (basestring,) in a
try/except that falls back to (str,) near the top of the file, then replace the
isinstance check in the method containing the snippet—where value is checked and
self._uid_to_link is called—with isinstance(value, string_types) so the UUID
branch works on both Py2 and Py3.
---
Nitpick comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 185-186: The lookup for session inside the loop over
annot['sessions'] is redundant and can mask issues; replace the double .get()
call used to set session (currently session = annot.get("sessions",
{}).get(session_id)) with a direct access to the mapping (e.g., session =
annot['sessions'][session_id]) so missing entries raise immediately and you
don't silently continue with an empty dict before referencing session['files']
or other keys in the subsequent code.
In `@src/imio/esign/tests/test_actions.py`:
- Around line 250-302: The current test uses one large exact string equality on
HTMLParser().unescape(view.esign_session_html(esign_session[1])) which is
brittle; change the test to capture the rendered string (call
view.esign_session_html with esign_session[1] and unescape it) and replace the
big assertEqual with several targeted assertions: check that specific
keys/values exist (e.g., 'sign_id': '012345600000' and "title": u'[ia.parapheo]
Session 012345600000'), verify that expected UID links appear (e.g., links to
'/plone/test_session_folder/annex0' and '/plone/test_session_folder/view'), and
assert presence/format of important structures like the files list and signers
rather than exact whole-string equality; locate this logic around the
HTMLParser().unescape(...) call and view.esign_session_html in the
test_actions.py test and update assertions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 538d6969-c8fb-499f-bd9e-4b771390bf66
📒 Files selected for processing (6)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/templates/session_annotation_info.ptsrc/imio/esign/tests/test_actions.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/imio/esign/browser/configure.zcml
- src/imio/esign/utils.py
- src/imio/esign/browser/templates/session_annotation_info.pt
| elif isinstance(value, basestring) and re.match(r"^[0-9a-f]{32}$", value): | ||
| # Looks like a UUID | ||
| return self._uid_to_link(value) | ||
| else: | ||
| return escape(safe_unicode(pprint.pformat(value))) |
There was a problem hiding this comment.
basestring is undefined in Python 3.
This will raise a NameError when running under Python 3. Use a compatibility check:
🐛 Proposed fix
Add at the top of the file (after other imports):
try:
string_types = (basestring,)
except NameError:
string_types = (str,)Then update line 173:
- elif isinstance(value, basestring) and re.match(r"^[0-9a-f]{32}$", value):
+ elif isinstance(value, string_types) and re.match(r"^[0-9a-f]{32}$", value):🧰 Tools
🪛 Ruff (0.15.4)
[error] 173-173: Undefined name basestring
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/browser/actions.py` around lines 173 - 177, The code uses
basestring which is undefined in Python 3; add a compatibility alias (e.g.,
define string_types = (basestring,) in a try/except that falls back to (str,)
near the top of the file, then replace the isinstance check in the method
containing the snippet—where value is checked and self._uid_to_link is
called—with isinstance(value, string_types) so the UUID branch works on both Py2
and Py3.
e2b3f22 to
49b546e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/imio/esign/browser/actions.py (1)
2-4:⚠️ Potential issue | 🔴 CriticalUse one Python compatibility strategy in this module.
Line 3 adds a Python-3-only import, while Line 173 still relies on
basestringand the file still contains Python-2-only syntax (except ValueError, msgat Line 62). That means this module now breaks on at least one interpreter family before the new view can run. Please add the same compatibility aliases you already used insrc/imio/esign/tests/test_actions.pyand switch theisinstancecheck to that alias.Suggested fix
-from html import escape +try: + from html import escape +except ImportError: # Python 2 + from cgi import escape @@ import pprint import re + +try: + string_types = (basestring,) +except NameError: + string_types = (str,) @@ - elif isinstance(value, basestring) and re.match(r"^[0-9a-f]{32}$", value): + elif isinstance(value, string_types) and re.match(r"^[0-9a-f]{32}$", value):#!/bin/bash set -e sed -n '1,20p' src/imio/esign/browser/actions.py sed -n '58,66p' src/imio/esign/browser/actions.py sed -n '165,176p' src/imio/esign/browser/actions.py rg -n "from html import escape|basestring|except [^:]+," src/imio/esign/browser/actions.pyAlso applies to: 173-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/actions.py` around lines 2 - 4, This module mixes Python-2 and Python-3 constructs (you added "from html import escape" but the code still uses "basestring" and the old except syntax "except ValueError, msg"), so add the same Python compatibility aliases used in src/imio/esign/tests/test_actions.py (e.g., define a basestring alias and any other shims used there) at the top of this module, change the isinstance(...) check that currently references basestring to use that alias, and convert any old-style excepts like "except ValueError, msg" to modern syntax "except ValueError as msg" so the file consistently uses one compatibility strategy.
🤖 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/imio/esign/browser/actions.py`:
- Around line 179-190: The esign_sessions property assumes self.context.UID()
exists and will raise AttributeError for contexts without UID; update
esign_sessions to guard against missing UID by checking hasattr(self.context,
"UID") or using getattr(self.context, "UID", None) and returning an empty sorted
list if no UID is present before calling
get_session_annotation/persistent_to_native; locate the esign_sessions method
and add that guard (or alternatively narrow the view registration) so it safely
handles site-root or non-content contexts.
---
Duplicate comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 2-4: This module mixes Python-2 and Python-3 constructs (you added
"from html import escape" but the code still uses "basestring" and the old
except syntax "except ValueError, msg"), so add the same Python compatibility
aliases used in src/imio/esign/tests/test_actions.py (e.g., define a basestring
alias and any other shims used there) at the top of this module, change the
isinstance(...) check that currently references basestring to use that alias,
and convert any old-style excepts like "except ValueError, msg" to modern syntax
"except ValueError as msg" so the file consistently uses one compatibility
strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e5d2721-717d-4cc8-bf4b-ae9eeaa50960
📒 Files selected for processing (6)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/templates/session_annotation_info.ptsrc/imio/esign/tests/test_actions.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/imio/esign/browser/templates/session_annotation_info.pt
- CHANGES.rst
| @property | ||
| def esign_sessions(self): | ||
| """Return list of (session_id, session_data) for all sessions.""" | ||
| annot = get_session_annotation() | ||
| c_uid = self.context.UID() | ||
| result = [] | ||
| for session_id in annot['sessions']: | ||
| session = annot.get("sessions", {}).get(session_id) | ||
| # If any file in this session is in this context | ||
| if any(f['context_uid'] == c_uid for f in session['files']): | ||
| result.append((session_id, persistent_to_native(session))) | ||
| return sorted(result) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
sed -n '115,121p' src/imio/esign/browser/configure.zcml
sed -n '179,190p' src/imio/esign/browser/actions.py
rg -n 'name="session-annotation-info"|for="\*"|UID\(' src/imio/esign/browser/configure.zcml src/imio/esign/browser/actions.pyRepository: IMIO/imio.esign
Length of output: 1649
Guard contexts that do not expose UID().
This property assumes every traversal target has UID(), but the page is registered for for="*". Hitting @@session-annotation-info on the site root or other non-content contexts will crash with AttributeError instead of rendering an empty result. Add a guard or narrow the registration.
Suggested fix
`@property`
def esign_sessions(self):
"""Return list of (session_id, session_data) for all sessions."""
+ uid_getter = getattr(self.context, "UID", None)
+ if uid_getter is None:
+ return []
annot = get_session_annotation()
- c_uid = self.context.UID()
+ c_uid = uid_getter()
result = []
- for session_id in annot['sessions']:
- session = annot.get("sessions", {}).get(session_id)
+ for session_id, session in annot.get("sessions", {}).items():
# If any file in this session is in this context
if any(f['context_uid'] == c_uid for f in session['files']):
result.append((session_id, persistent_to_native(session)))
return sorted(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/browser/actions.py` around lines 179 - 190, The esign_sessions
property assumes self.context.UID() exists and will raise AttributeError for
contexts without UID; update esign_sessions to guard against missing UID by
checking hasattr(self.context, "UID") or using getattr(self.context, "UID",
None) and returning an empty sorted list if no UID is present before calling
get_session_annotation/persistent_to_native; locate the esign_sessions method
and add that guard (or alternatively narrow the view registration) so it safely
handles site-root or non-content contexts.
49b546e to
204890f
Compare
|
|
||
| layer = IMIO_ESIGN_INTEGRATION_TESTING | ||
|
|
||
| def setUp(self): |
There was a problem hiding this comment.
@gbastien Je sais que tu m'as dit de mettre les contenus d'exemple pour les tests dans la layer. Je gérerai ça dans une PR séparée
There was a problem hiding this comment.
@chris-adam ce n'est pas du tout urgent, en effet quand çà se calmera dans une v3 on pourra refactorer les tests pour les rendre + rapides mais c'est loin d'être prioritaire 👍
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/imio/esign/browser/actions.py (2)
176-176:⚠️ Potential issue | 🔴 CriticalReplace
basestringwith a cross-version string alias.Line 176 will raise
NameErroron Python 3, so_render_value()breaks as soon as it hits a plain string.🐛 Proposed fix
+try: + string_types = (basestring,) +except NameError: + string_types = (str,) + class SessionAnnotationInfoView(BrowserView): """Admin-only view displaying imio.esign session annotations for a specific context item.""" @@ - elif isinstance(value, basestring) and re.match(r"^[0-9a-f]{32}$", value): + elif isinstance(value, string_types) and re.match(r"^[0-9a-f]{32}$", value):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/actions.py` at line 176, The code in _render_value uses the Python2-only name basestring which raises NameError on Python 3; update the type check to use a cross-version string alias (e.g. six.string_types or a local tuple like (str,)) instead of basestring so plain strings are recognized on both Python 2 and 3; modify the isinstance call in the branch inside _render_value (the line with "elif isinstance(value, basestring) and re.match...") to reference the chosen cross-version symbol (six.string_types or the local alias) and ensure six is imported if you choose six.string_types.
182-193:⚠️ Potential issue | 🟠 MajorGuard non-content contexts before calling
UID().This view is reachable on arbitrary contexts, but
esign_sessionsassumesself.context.UID()exists. Hitting it on the site root or another non-content object will crash instead of returning an empty list.🛡️ Proposed fix
`@property` def esign_sessions(self): """Return list of (session_id, session_data) for all sessions.""" + uid_getter = getattr(self.context, "UID", None) + if uid_getter is None: + return [] annot = get_session_annotation() - c_uid = self.context.UID() + c_uid = uid_getter() result = [] - for session_id in annot['sessions']: - session = annot.get("sessions", {}).get(session_id) + for session_id, session in annot.get("sessions", {}).items(): # If any file in this session is in this context if any(f['context_uid'] == c_uid for f in session['files']): result.append((session_id, persistent_to_native(session))) return sorted(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/actions.py` around lines 182 - 193, The esign_sessions property calls self.context.UID() without guarding non-content contexts; modify esign_sessions to detect non-content contexts and return an empty list instead of raising: before calling self.context.UID(), check for a UID (e.g. uid = getattr(self.context, 'UID', None) and if not uid: return []) or use a safe IUUID(self.context, None) lookup; then proceed to use get_session_annotation(), loop over annot['sessions'], and call persistent_to_native as before. Ensure you update only esign_sessions so behavior stays the same for content objects.
🧹 Nitpick comments (1)
src/imio/esign/tests/test_actions.py (1)
206-231: Make these rendering assertions interpreter-neutral.The snapshots hard-code Python 2
pprintoutput (u'...'prefixes). On Python 3, the same values render differently, so these tests become version-specific for no functional reason.♻️ Suggested direction
- self.assertEqual(self.view._render_value(u"hello"), u"u'hello'") + rendered = self.view._render_value(u"hello") + self.assertIn(u"'hello'", rendered) @@ - self.assertEqual( - unescape(view.esign_session_html(esign_session[1])), - u"""{{ - 'acroform': True, - 'client_id': '0123456', - ... -}}""".format( - repr(esign_session[1]['last_update']), - ), - ) + rendered = unescape(view.esign_session_html(esign_session[1])) + self.assertIn(u"'client_id': '0123456'", rendered) + self.assertIn(u"'title': '[ia.parapheo] Session 012345600000'", rendered) + self.assertIn(u"Annex 0</a>", rendered) + self.assertIn(u"Annex 1</a>", rendered)Also applies to: 258-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/test_actions.py` around lines 206 - 231, The test hard-codes Python 2 pprint syntax (u'...' prefixes) in test_render_value when asserting view._render_value output, causing failures on Python 3; update the test to be interpreter-neutral by normalizing the rendered string before asserting (e.g., in test_render_value and the other similar assertions around 258-310) — call view._render_value(...) into a local variable, strip or remove any leading "u" prefixes from quoted strings (or run a simple replace like result = result.replace("u'", "'") and result = result.replace('u"', '"')) and then compare against expectations without the u prefixes (or assert against both variants), so tests pass on both Python 2 and 3 while keeping the same logical checks for _render_value.
🤖 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/imio/esign/browser/actions.py`:
- Line 176: The code in _render_value uses the Python2-only name basestring
which raises NameError on Python 3; update the type check to use a cross-version
string alias (e.g. six.string_types or a local tuple like (str,)) instead of
basestring so plain strings are recognized on both Python 2 and 3; modify the
isinstance call in the branch inside _render_value (the line with "elif
isinstance(value, basestring) and re.match...") to reference the chosen
cross-version symbol (six.string_types or the local alias) and ensure six is
imported if you choose six.string_types.
- Around line 182-193: The esign_sessions property calls self.context.UID()
without guarding non-content contexts; modify esign_sessions to detect
non-content contexts and return an empty list instead of raising: before calling
self.context.UID(), check for a UID (e.g. uid = getattr(self.context, 'UID',
None) and if not uid: return []) or use a safe IUUID(self.context, None) lookup;
then proceed to use get_session_annotation(), loop over annot['sessions'], and
call persistent_to_native as before. Ensure you update only esign_sessions so
behavior stays the same for content objects.
---
Nitpick comments:
In `@src/imio/esign/tests/test_actions.py`:
- Around line 206-231: The test hard-codes Python 2 pprint syntax (u'...'
prefixes) in test_render_value when asserting view._render_value output, causing
failures on Python 3; update the test to be interpreter-neutral by normalizing
the rendered string before asserting (e.g., in test_render_value and the other
similar assertions around 258-310) — call view._render_value(...) into a local
variable, strip or remove any leading "u" prefixes from quoted strings (or run a
simple replace like result = result.replace("u'", "'") and result =
result.replace('u"', '"')) and then compare against expectations without the u
prefixes (or assert against both variants), so tests pass on both Python 2 and 3
while keeping the same logical checks for _render_value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d53d262-034e-4229-8f4e-e903edf24398
📒 Files selected for processing (6)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/templates/session_annotation_info.ptsrc/imio/esign/tests/test_actions.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGES.rst
Summary by CodeRabbit
New Features
Tests
Chores
Refactor