Added action RecreateSession#36
Open
chris-adam wants to merge 3 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a RecreateSession admin action: new RecreateSessionView that builds a fresh session from an existing non-draft session (rebuilds signers, copies files, audits, redirects), UI wiring and ZCML registration, tests, CHANGES entry, and an adjusted get_sessions_for implementation. ChangesSession Recreation Feature
Sequence DiagramsequenceDiagram
actor Admin
participant RecreateSessionView
participant SessionStorage
participant FileManager
participant AuditLog
participant UI
Admin->>RecreateSessionView: Click "Recreate Session" (confirm)
RecreateSessionView->>RecreateSessionView: may_recreate_session (permission)
RecreateSessionView->>SessionStorage: Fetch source session
RecreateSessionView->>RecreateSessionView: Validate state != "draft" and build signer data
RecreateSessionView->>SessionStorage: create_session(new session)
RecreateSessionView->>FileManager: copy files from old -> new session
RecreateSessionView->>AuditLog: record recreation event
RecreateSessionView->>UI: set info message and redirect to paraphe page
UI->>Admin: Navigate to new session paraphe view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
Coverage Report for CI Build 26227542277Coverage increased (+0.2%) to 84.874%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
chris-adam
force-pushed
the
PARAF-420/recreate_session
branch
4 times, most recently
from
b5ed49f to
65a4670
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: 2
🧹 Nitpick comments (1)
src/imio/esign/browser/actions.py (1)
17-18: 💤 Low valueUnused imports.
PersistentListandPersistentMappingare imported but not used in this file. Thecreate_sessionfunction handles persistent data structures internally.♻️ Remove unused imports
from imio.helpers.security import check_zope_admin -from persistent.list import PersistentList -from persistent.mapping import PersistentMapping from plone import api🤖 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/imio/esign/browser/actions.py` around lines 17 - 18, The file imports PersistentList and PersistentMapping but they are unused; remove the unused imports (PersistentList and PersistentMapping) from the import statement so only needed symbols remain, leaving create_session and any other used names untouched to avoid changing 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/imio/esign/browser/actions.py`:
- Around line 246-249: The code converts session_id with session_id =
int(session_id) without guarding against non-numeric input; wrap the conversion
in a try/except ValueError around the line that casts session_id to int, and on
except call api.portal.show_message(_("Invalid session ID provided!"),
request=self.request, type="error") and return the same redirect as the
empty-case (self.context.absolute_url() + "/@@parapheo"); keep the existing
branch for the falsy session_id unchanged and only add the try/except handling
for the int(...) conversion to avoid raising on bad input.
- Around line 242-285: The __call__ method currently returns URL strings instead
of performing HTTP redirects; replace the three returns that return
self.context.absolute_url() + "/@@parapheo" with actual redirects by calling
self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo")
(keep existing logic checks around may_recreate_session(), session lookup, and
draft check intact); update the branches after the missing session_id check,
session not found check, and draft-session check to call
self.request.RESPONSE.redirect(...) and then return (or otherwise exit) to match
RemoveFromSessionView._finished() behavior and ensure callers of
create_session/add_files_to_session keep current flow.
---
Nitpick comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 17-18: The file imports PersistentList and PersistentMapping but
they are unused; remove the unused imports (PersistentList and
PersistentMapping) from the import statement so only needed symbols remain,
leaving create_session and any other used names untouched to avoid changing
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: d9b4f04a-6fd6-4a29-8682-76ff51af9a16
📒 Files selected for processing (7)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/table.pysrc/imio/esign/tests/test_actions.pysrc/imio/esign/tests/test_utils.pysrc/imio/esign/utils.py
chris-adam
force-pushed
the
PARAF-420/recreate_session
branch
from
65a4670 to
6bd272e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/imio/esign/browser/actions.py (1)
240-286:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
self.request.RESPONSE.redirect()instead of returning URL strings.The
__call__method returns URL strings directly at lines 246, 249, 255, 259, and 286. This renders the URL as HTML response content rather than triggering an actual HTTP redirect. This is inconsistent withRemoveFromSessionView._finished()(line 111) and other similar views in this codebase.🔧 Proposed fix
def __call__(self): if not self.may_recreate_session(): raise Unauthorized session_id = self.request.form.get("esign_session_id") if not session_id: api.portal.show_message(_("No session ID provided!"), request=self.request, type="error") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo") + return if not session_id.isdigit(): api.portal.show_message(_("Invalid session ID!"), request=self.request, type="error") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo") + return session_id = int(session_id) annot = get_session_annotation() sessions = annot["sessions"] if session_id not in sessions: api.portal.show_message(_("Session not found!"), request=self.request, type="error") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo") + return old = sessions[session_id] if old["state"] == "draft": api.portal.show_message(_("Cannot recreate a draft session!"), request=self.request, type="warning") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo") + return # ... rest of the method ... api.portal.show_message( _("New session ${nid} created from session ${oid}", mapping={"nid": new_id, "oid": session_id}), request=self.request, type="info", ) - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo")🤖 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/imio/esign/browser/actions.py` around lines 240 - 286, The __call__ method is returning URL strings instead of issuing HTTP redirects; replace each return of a URL string (all early-error/flow exits and the final success return in imio.esign.browser.actions.RecreateSessionView.__call__) with a proper redirect by calling self.request.RESPONSE.redirect(target_url) (use the same target URLs currently returned, e.g. self.context.absolute_url() + "/@@parapheo"), then stop further processing by returning None (or an empty string) after the redirect; ensure every branch that currently returns a URL uses this pattern so behavior matches RemoveFromSessionView._finished() and other views.
🤖 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.
Duplicate comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 240-286: The __call__ method is returning URL strings instead of
issuing HTTP redirects; replace each return of a URL string (all
early-error/flow exits and the final success return in
imio.esign.browser.actions.RecreateSessionView.__call__) with a proper redirect
by calling self.request.RESPONSE.redirect(target_url) (use the same target URLs
currently returned, e.g. self.context.absolute_url() + "/@@parapheo"), then stop
further processing by returning None (or an empty string) after the redirect;
ensure every branch that currently returns a URL uses this pattern so behavior
matches RemoveFromSessionView._finished() and other views.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57fe20e9-ce37-4f79-a5e4-350f36116091
📒 Files selected for processing (7)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/table.pysrc/imio/esign/tests/test_actions.pysrc/imio/esign/tests/test_utils.pysrc/imio/esign/utils.py
chris-adam
force-pushed
the
PARAF-420/recreate_session
branch
from
6bd272e to
7dfc7d7
Compare
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/imio/esign/browser/actions.py`:
- Around line 263-268: The code passes old.get("title") into create_session
which copies the already-materialized title (with previous session/sign IDs)
into the new draft; instead, regenerate the title from the original title
template or construct a fresh title based on new_id before calling
create_session so the new session UI name and subsequent
create_external_session() sessionName do not retain the old id. Update the call
site that invokes create_session (the new_id, _new_session = create_session(...)
block) to compute a new title (or fallback) rather than reusing old["title"],
referencing create_session, old["title"], new_id and
create_external_session()/sessionName to guide where to change.
- Around line 274-278: Re-adding the same file UIDs into a new session
overwrites annot["uids"][uid] and re-appends the raw UID into annot["c_uids"],
breaking remove/update flows; update the annotation model and the add/remove
functions to be session-aware: change annot["uids"] to map each uid to a
set/list of session IDs (or store per-session metadata) and modify
add_files_to_session so it appends the new session id to annot["uids"][uid]
instead of overwriting and avoids duplicating entries in annot["c_uids"]; then
update RemoveItemFromSessionView, remove_files_from_session, and
remove_context_from_session to consult the session-aware mapping and only remove
entries for the targeted session (and clean up uid entries when no sessions
remain).
🪄 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: f034ce5a-2afd-4efb-bf92-1021d007d922
📒 Files selected for processing (7)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/table.pysrc/imio/esign/tests/test_actions.pysrc/imio/esign/tests/test_utils.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/imio/esign/browser/table.py
- CHANGES.rst
Comment on lines
+274
to
+278
| add_files_to_session( | ||
| signers=signers, | ||
| files_uids=[f["uid"] for f in old["files"]], | ||
| session_id=new_id, | ||
| ) |
There was a problem hiding this comment.
Recreation breaks the UID indexes used by remove/update flows.
This re-adds the same file UIDs into a second session, but add_files_to_session() still overwrites annot["uids"][uid] and appends the raw UID again in annot["c_uids"]. After that, the original session still contains the file while RemoveItemFromSessionView, remove_files_from_session(), and remove_context_from_session() can only target the newest session or leave stale entries behind. The annotation model needs multi-session tracking before this flow is safe.
🤖 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/imio/esign/browser/actions.py` around lines 274 - 278, Re-adding the same
file UIDs into a new session overwrites annot["uids"][uid] and re-appends the
raw UID into annot["c_uids"], breaking remove/update flows; update the
annotation model and the add/remove functions to be session-aware: change
annot["uids"] to map each uid to a set/list of session IDs (or store per-session
metadata) and modify add_files_to_session so it appends the new session id to
annot["uids"][uid] instead of overwriting and avoids duplicating entries in
annot["c_uids"]; then update RemoveItemFromSessionView,
remove_files_from_session, and remove_context_from_session to consult the
session-aware mapping and only remove entries for the targeted session (and
clean up uid entries when no sessions remain).
7dfc7d7 to
0b8c6cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/imio/esign/browser/actions.py (2)
240-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
self.request.RESPONSE.redirect()instead of returning URL strings.The
__call__method returns URL strings directly at lines 246, 249, 255, 259, and 285. This renders the URL as plain text in the response body rather than triggering an HTTP redirect. Other views in this file (e.g.,RemoveFromSessionView._finished()at line 111) correctly useself.request.RESPONSE.redirect().🐛 Proposed fix
def __call__(self): if not self.may_recreate_session(): raise Unauthorized session_id = self.request.form.get("esign_session_id") + redirect_url = self.context.absolute_url() + "/@@parapheo" if not session_id: api.portal.show_message(_("No session ID provided!"), request=self.request, type="error") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(redirect_url) + return if not session_id.isdigit(): api.portal.show_message(_("Invalid session ID!"), request=self.request, type="error") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(redirect_url) + return session_id = int(session_id) annot = get_session_annotation() sessions = annot["sessions"] if session_id not in sessions: api.portal.show_message(_("Session not found!"), request=self.request, type="error") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(redirect_url) + return old = sessions[session_id] if old["state"] == "draft": api.portal.show_message(_("Cannot recreate a draft session!"), request=self.request, type="warning") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(redirect_url) + return # ... rest of method ... api.portal.show_message( _("New session ${nid} created from session ${oid}", mapping={"nid": new_id, "oid": session_id}), request=self.request, type="info", ) - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(redirect_url)🤖 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/imio/esign/browser/actions.py` around lines 240 - 259, In the __call__ method of the view, replace the plain returns of URL strings (the branches that currently return self.context.absolute_url() + "/@@parapheo") with calls to self.request.RESPONSE.redirect(...) so the client is actually redirected; update each branch that shows messages and then returns (invalid/missing session_id, session not found, draft session, etc.) to call self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo") and then return immediately after the redirect to stop processing (mirror the pattern used in RemoveFromSessionView._finished()). Ensure you keep the existing api.portal.show_message(...) calls before redirecting and reference the __call__ function and RemoveFromSessionView._finished for the pattern.
273-277:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUID index overwrites may cause stale references in the old session.
When
add_files_to_sessionis called for the new session,annot["uids"][uid]is overwritten to point to the new session (utils.py line 138). The old session still references these file UIDs in itsfileslist, but the global UID→session mapping now points only to the new session. This can cause issues when attempting to remove files from the old session viaRemoveItemFromSessionVieworremove_files_from_session().The updated
get_sessions_formitigates this for session lookups, but the single-sessionannot["uids"]mapping remains inconsistent.#!/bin/bash # Check how remove_files_from_session uses the uids mapping rg -n -A10 'def remove_files_from_session' src/imio/esign/utils.py # Check if there are any other places that rely on annot["uids"] being authoritative rg -n 'annot\["uids"\]' src/imio/esign/🤖 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/imio/esign/browser/actions.py` around lines 273 - 277, The call to add_files_to_session currently overwrites annot["uids"][uid] with the new session id, breaking the old session's uid mapping and causing remove_files_from_session / RemoveItemFromSessionView to see stale references; change the uid mapping to be multi-valued instead of a single id: in add_files_to_session (and the utils.py code that writes annot["uids"] around line 138) store a list or set of session ids per uid (e.g., annot["uids"].setdefault(uid, set/list).add/append(new_id)) rather than assigning new_id, and update any readers (remove_files_from_session, RemoveItemFromSessionView, and any code that assumes a single session id) to consult this collection or use get_sessions_for to determine the correct session(s) before removing files.
🤖 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/imio/esign/browser/actions.py`:
- Around line 263-271: The create_session call is missing the title so the
recreated session gets a default name; update the create_session invocation (the
new_id, _new_session = create_session(...) call) to pass title=old.get("title")
(or old["title"] if guaranteed) so the new session preserves the original title
and include that same title when constructing create_session_custom_data if
needed to track recreation from session_id.
---
Duplicate comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 240-259: In the __call__ method of the view, replace the plain
returns of URL strings (the branches that currently return
self.context.absolute_url() + "/@@parapheo") with calls to
self.request.RESPONSE.redirect(...) so the client is actually redirected; update
each branch that shows messages and then returns (invalid/missing session_id,
session not found, draft session, etc.) to call
self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo") and
then return immediately after the redirect to stop processing (mirror the
pattern used in RemoveFromSessionView._finished()). Ensure you keep the existing
api.portal.show_message(...) calls before redirecting and reference the __call__
function and RemoveFromSessionView._finished for the pattern.
- Around line 273-277: The call to add_files_to_session currently overwrites
annot["uids"][uid] with the new session id, breaking the old session's uid
mapping and causing remove_files_from_session / RemoveItemFromSessionView to see
stale references; change the uid mapping to be multi-valued instead of a single
id: in add_files_to_session (and the utils.py code that writes annot["uids"]
around line 138) store a list or set of session ids per uid (e.g.,
annot["uids"].setdefault(uid, set/list).add/append(new_id)) rather than
assigning new_id, and update any readers (remove_files_from_session,
RemoveItemFromSessionView, and any code that assumes a single session id) to
consult this collection or use get_sessions_for to determine the correct
session(s) before removing files.
🪄 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: f3a24e0c-6dd7-40e9-a978-d01f30cc0336
📒 Files selected for processing (7)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/table.pysrc/imio/esign/tests/test_actions.pysrc/imio/esign/tests/test_utils.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/esign/browser/configure.zcml
chris-adam
force-pushed
the
PARAF-420/recreate_session
branch
from
0b8c6cd to
d941359
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/imio/esign/browser/actions.py (2)
273-277:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFile UID tracking may conflict when files exist in multiple sessions.
Re-adding the same file UIDs to a new session via
add_files_to_sessionoverwritesannot["uids"][uid]with the new session ID. This meansRemoveItemFromSessionViewand similar views will only target the newest session, leaving stale entries in the original session.If files belonging to multiple sessions is an intended use case, the annotation model may need refactoring to track session IDs per UID.
#!/bin/bash # Check how annot["uids"] is used in removal flows rg -n -A5 'annot\["uids"\]' src/imio/esign/🤖 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/imio/esign/browser/actions.py` around lines 273 - 277, add_files_to_session currently writes a single session id into annot["uids"][uid], which overwrites prior sessions and breaks removal logic (e.g., RemoveItemFromSessionView) for files shared across sessions; change the model to store a collection of session ids per uid (e.g., a list or set) and update add_files_to_session to append/merge session ids instead of replacing them, then update removal flows (RemoveItemFromSessionView and any readers of annot["uids"]) to remove only the session id for the given operation and to treat annot["uids"][uid] as a collection; ensure persistence/serialization handles the new shape and add defensive checks where annot["uids"] is read.
240-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
self.request.RESPONSE.redirect()instead of returning URL strings.The
__call__method returns URL strings directly (lines 246, 249, 255, 259, 285), which renders the URL as response content rather than triggering an HTTP redirect. This is inconsistent with the_finished()pattern used inRemoveFromSessionView(line 111) andRemoveItemFromSessionView(line 139).🐛 Proposed fix
def __call__(self): if not self.may_recreate_session(): raise Unauthorized session_id = self.request.form.get("esign_session_id") if not session_id: api.portal.show_message(_("No session ID provided!"), request=self.request, type="error") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo") + return if not session_id.isdigit(): api.portal.show_message(_("Invalid session ID!"), request=self.request, type="error") - return self.context.absolute_url() + "/@@parapheo" + self.request.RESPONSE.redirect(self.context.absolute_url() + "/@@parapheo") + return # ... similar changes for lines 255, 259, 285🤖 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/imio/esign/browser/actions.py` around lines 240 - 285, The view's __call__ currently returns URL strings; change each return of self.context.absolute_url() + "/@@parapheo" to perform an HTTP redirect via self.request.RESPONSE.redirect(url) (e.g. url = self.context.absolute_url() + "/@@parapheo"; self.request.RESPONSE.redirect(url)) and then return immediately (return None or an empty string) so the response is a proper redirect. Update every early-exit and the success path in this function (the __call__ method that calls create_session, add_files_to_session, sets self._new_session_id and calls audit) to use self.request.RESPONSE.redirect instead of returning the URL string. Ensure no other callers expect the URL string to be returned.
🧹 Nitpick comments (1)
src/imio/esign/tests/test_actions.py (1)
367-371: 💤 Low valueConsider adding
strict=Truetozip()for safer iteration.Using
zip()withoutstrict=Truecould silently truncate if the signer lists have different lengths. While this is unlikely given the test setup, addingstrict=True(Python 3.10+) would catch any mismatch explicitly.♻️ Proposed fix
- for nu, ou in zip(new_session["signers"], old["signers"]): + for nu, ou in zip(new_session["signers"], old["signers"], strict=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/imio/esign/tests/test_actions.py` around lines 367 - 371, The test loop using zip(new_session["signers"], old["signers"]) can silently truncate if the signer lists differ; update the iteration in the test (the for loop that zips new_session["signers"] and old["signers"] in src/imio/esign/tests/test_actions.py) to use zip(..., strict=True) so a length mismatch raises an error and the assertion checks all entries reliably.
🤖 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.
Duplicate comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 273-277: add_files_to_session currently writes a single session id
into annot["uids"][uid], which overwrites prior sessions and breaks removal
logic (e.g., RemoveItemFromSessionView) for files shared across sessions; change
the model to store a collection of session ids per uid (e.g., a list or set) and
update add_files_to_session to append/merge session ids instead of replacing
them, then update removal flows (RemoveItemFromSessionView and any readers of
annot["uids"]) to remove only the session id for the given operation and to
treat annot["uids"][uid] as a collection; ensure persistence/serialization
handles the new shape and add defensive checks where annot["uids"] is read.
- Around line 240-285: The view's __call__ currently returns URL strings; change
each return of self.context.absolute_url() + "/@@parapheo" to perform an HTTP
redirect via self.request.RESPONSE.redirect(url) (e.g. url =
self.context.absolute_url() + "/@@parapheo";
self.request.RESPONSE.redirect(url)) and then return immediately (return None or
an empty string) so the response is a proper redirect. Update every early-exit
and the success path in this function (the __call__ method that calls
create_session, add_files_to_session, sets self._new_session_id and calls audit)
to use self.request.RESPONSE.redirect instead of returning the URL string.
Ensure no other callers expect the URL string to be returned.
---
Nitpick comments:
In `@src/imio/esign/tests/test_actions.py`:
- Around line 367-371: The test loop using zip(new_session["signers"],
old["signers"]) can silently truncate if the signer lists differ; update the
iteration in the test (the for loop that zips new_session["signers"] and
old["signers"] in src/imio/esign/tests/test_actions.py) to use zip(...,
strict=True) so a length mismatch raises an error and the assertion checks all
entries reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9b98bb0-680d-4f34-8def-20e6ac8b0eeb
📒 Files selected for processing (7)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/table.pysrc/imio/esign/tests/test_actions.pysrc/imio/esign/tests/test_utils.pysrc/imio/esign/utils.py
✅ Files skipped from review due to trivial changes (1)
- CHANGES.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- src/imio/esign/tests/test_utils.py
- src/imio/esign/utils.py
chris-adam
force-pushed
the
PARAF-420/recreate_session
branch
from
d941359 to
71fdfc9
Compare
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation