Skip to content

Manage file added again to same session, data is updated.#21

Open
gbastien wants to merge 23 commits intomainfrom
PARAF-359_file_added_to_same_session_again
Open

Manage file added again to same session, data is updated.#21
gbastien wants to merge 23 commits intomainfrom
PARAF-359_file_added_to_same_session_again

Conversation

@gbastien
Copy link
Member

@gbastien gbastien commented Mar 6, 2026

See #PARAF-359

Summary by CodeRabbit

  • Bug Fixes

    • Re-adding a file already in a session now updates the existing entry (filename/title) without creating duplicates and preserves the session.
  • Tests

    • Added tests confirming that editing and re-adding a file updates the single session entry.
  • Documentation

    • Changelog updated noting repeated file additions now update session data.
  • Chores

    • Test environment dependency versions updated.

@coderabbitai
Copy link

coderabbitai bot commented Mar 6, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Re-adding a file UID within the same session now updates the existing session entry (filename/title) instead of creating a duplicate. remove_files_from_session gained a remove_empty_session parameter to optionally preserve sessions when detaching files. A test and changelog entry were added.

Changes

Cohort / File(s) Summary
Changelog
CHANGES.rst
Added entry under 1.0a3: "Manage file added again to same session, data is updated."
Config
test-4.3.cfg
Added test dependencies: mock = 3.0.5, funcsigs = 1.0.2.
Core File Management
src/imio/esign/utils.py
add_files_to_session treats UIDs already in the current session as updates: removes old filename from in-session list, detaches previous entry without deleting the session, and re-attaches the UID. remove_files_from_session signature changed to remove_files_from_session(files_uids, remove_empty_session=True) and now only deletes the session when remove_empty_session is True and no files remain; otherwise it refreshes last_update.
Tests
src/imio/esign/tests/test_utils.py
Added test_add_files_already_exist_is_updated to assert that renaming/re-adding a file within the same session updates the single file entry (filename/title) and avoids duplicates. Note: duplicate copy of the test appears in the file.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A nibble, a hop, a file found new name,
I nudged it back in — it stayed just the same.
No doubles to chase, one session to keep,
Fresh title, fresh bytes, tucked in for sleep. 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: handling file re-addition to the same session with data updates, which is the core objective across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch PARAF-359_file_added_to_same_session_again
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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_utils.py`:
- Around line 465-475: The test currently calls add_files_to_session(signers,
(annex_uid,)) twice without supplying the existing session_id, so it may create
a new session instead of updating the same one; modify the test to capture the
returned sid from the first call, pass that sid back as the session_id argument
on the second add_files_to_session(...) invocation, then assert that the
returned sid is unchanged (identity) and that session["files"] still has length
1 with updated filename and title; reference add_files_to_session and the
session_id parameter when making the change.

In `@src/imio/esign/utils.py`:
- Around line 86-99: When replacing an already-added UID the code increments
session size before removing the old file, so remove_files_from_session()
subtracts the current annex size and leaves a stale session["size"]; to fix,
detect the existing UID in the block where annot['uids'].get(uid) == session_id,
obtain the old file size (e.g., from session["files"] entry or annex metadata
for that UID) and subtract that old size from session["size"] (or
call/remove_files_from_session in a way that uses the stored old size) before
adding/incrementing the new file size; update the logic around
remove_files_from_session, session["files"] and session["size"] (referencing
annot, uid, session, remove_files_from_session, existing_files, old_filename) so
session["size"] reflects the true post-replacement total.
- Around line 95-98: The code assumes a matching entry for uid exists in
session["files"] and directly indexes [0], causing a crash if the mapping is
stale; update the block that computes old_filename (the list comprehension using
session["files"] and uid) to safely search for the file (e.g., use next(...)
with a default None or check length of the filtered list) and only call
existing_files.remove(old_filename) and remove_files_from_session([uid], ...)
when a match is found; if no match is found, skip removing old_filename and
either log a warning or proceed to call remove_files_from_session with the uid
as needed to avoid raising an IndexError.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c53b70c0-9d70-4bb0-a1c1-9b7e3a2904d0

📥 Commits

Reviewing files that changed from the base of the PR and between 94c7b5e and df35686.

📒 Files selected for processing (4)
  • CHANGES.rst
  • src/imio/esign/tests/test_utils.py
  • src/imio/esign/utils.py
  • test-4.3.cfg

Comment on lines +465 to +475
sid, session = add_files_to_session(signers, (annex_uid,))
self.assertEqual(len(session["files"]), 1)
self.assertEqual(session["files"][0]["filename"], "annex0.pdf")
self.assertEqual(session["files"][0]["title"], "Annex 0")
# edit annex and add again, still one annex in session and data are updated
annex.file.filename = u"new_filename.pdf"
annex.setTitle('New title')
sid, session = add_files_to_session(signers, (annex_uid,))
self.assertEqual(len(session["files"]), 1)
self.assertEqual(session["files"][0]["filename"], "new_filename.pdf")
self.assertEqual(session["files"][0]["title"], "New title")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Test doesn’t strictly prove “same session” update behavior.

The second call omits session_id, so this can still pass if a new session is created. Also, sid from Line 472 is unused. Force the same-session path and assert identity/count explicitly.

💡 Proposed fix
-        sid, session = add_files_to_session(signers, (annex_uid,))
+        sid2, session = add_files_to_session(signers, (annex_uid,), session_id=sid)
+        self.assertEqual(sid2, sid)
+        self.assertEqual(len(annot["sessions"]), 1)
+        self.assertEqual(annot["uids"][annex_uid], sid)
         self.assertEqual(len(session["files"]), 1)
         self.assertEqual(session["files"][0]["filename"], "new_filename.pdf")
         self.assertEqual(session["files"][0]["title"], "New title")

Based on learnings: In src/imio/esign/utils.py, when an explicit session_id is provided to add_files_to_session(), discriminators are intentionally bypassed.

🧰 Tools
🪛 Ruff (0.15.4)

[warning] 472-472: Unpacked variable sid is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/tests/test_utils.py` around lines 465 - 475, The test
currently calls add_files_to_session(signers, (annex_uid,)) twice without
supplying the existing session_id, so it may create a new session instead of
updating the same one; modify the test to capture the returned sid from the
first call, pass that sid back as the session_id argument on the second
add_files_to_session(...) invocation, then assert that the returned sid is
unchanged (identity) and that session["files"] still has length 1 with updated
filename and title; reference add_files_to_session and the session_id parameter
when making the change.

Comment on lines +95 to +98
old_filename = path.splitext([fn for fn in session["files"]
if fn['uid'] == uid][0]['filename'])[0]
existing_files.remove(old_filename)
remove_files_from_session([uid], remove_empty_session=False)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Prevent crash when UID mapping is stale.

This lookup assumes the UID always exists in session["files"]; if annotations drift, ...[0] raises and aborts the operation. Guard the lookup before dereferencing.

💡 Proposed fix
-            old_filename = path.splitext([fn for fn in session["files"]
-                                          if fn['uid'] == uid][0]['filename'])[0]
+            old_file = next((fn for fn in session["files"] if fn["uid"] == uid), None)
+            if old_file is None:
+                logger.error(
+                    "File UID %s mapped to session %s but missing from session files.",
+                    uid, session_id,
+                )
+                continue
+            old_filename = path.splitext(old_file["filename"])[0]
🧰 Tools
🪛 Ruff (0.15.4)

[warning] 95-96: Prefer next(...) over single element slice

Replace with next(...)

(RUF015)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/utils.py` around lines 95 - 98, The code assumes a matching
entry for uid exists in session["files"] and directly indexes [0], causing a
crash if the mapping is stale; update the block that computes old_filename (the
list comprehension using session["files"] and uid) to safely search for the file
(e.g., use next(...) with a default None or check length of the filtered list)
and only call existing_files.remove(old_filename) and
remove_files_from_session([uid], ...) when a match is found; if no match is
found, skip removing old_filename and either log a warning or proceed to call
remove_files_from_session with the uid as needed to avoid raising an IndexError.

…me is not renamed if adding same file again to a session
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (3)
src/imio/esign/tests/test_utils.py (1)

465-478: ⚠️ Potential issue | 🟠 Major

Make the test explicitly target “same session” updates.

Line 472 and Line 477 re-add without session_id, so this can still pass if a different session is selected. Force session_id=sid and assert session identity/count.

Proposed fix
-        sid, session = add_files_to_session(signers, (annex_uid,))
+        sid, session = add_files_to_session(signers, (annex_uid,))
         self.assertEqual(len(session["files"]), 1)
@@
-        sid, session = add_files_to_session(signers, (annex_uid,))
+        sid2, session = add_files_to_session(signers, (annex_uid,), session_id=sid)
+        self.assertEqual(sid2, sid)
+        self.assertEqual(len(annot["sessions"]), 1)
+        self.assertEqual(annot["uids"][annex_uid], sid)
         self.assertEqual(len(session["files"]), 1)
@@
-        sid, session = add_files_to_session(signers, (annex_uid,))
+        sid3, session = add_files_to_session(signers, (annex_uid,), session_id=sid)
+        self.assertEqual(sid3, sid)
         self.assertEqual(len(session["files"]), 1)

Based on learnings: In src/imio/esign/utils.py, when an explicit session_id is provided to add_files_to_session(), all discriminators are intentionally bypassed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/tests/test_utils.py` around lines 465 - 478, The test is
re-adding files without forcing the same session so it may pass using a
different session; modify the calls to add_files_to_session to pass the existing
session id (use session_id=sid) when re-adding the annex and when adding the
exact same file, and add an assertion that the returned session id equals the
original sid to ensure the function updated the same session (refer to
add_files_to_session and the sid variable and session_id parameter).
src/imio/esign/utils.py (2)

90-93: ⚠️ Potential issue | 🟠 Major

Guard UID lookup before dereferencing list index.

At Line 90, [...][0] can raise IndexError if annot["uids"] says the UID is in the session but session["files"] is out of sync.

Proposed fix
-            old_filename = path.splitext([fn for fn in session["files"]
-                                          if fn['uid'] == uid][0]['filename'])[0]
-            existing_files.remove(old_filename)
-            remove_files_from_session([uid], remove_empty_session=False)
+            old_file = next((fn for fn in session["files"] if fn["uid"] == uid), None)
+            if old_file is None:
+                logger.error(
+                    "File UID %s mapped to session %s but missing from session files.",
+                    uid, session_id,
+                )
+                continue
+            old_filename = path.splitext(old_file["filename"])[0]
+            if old_filename in existing_files:
+                existing_files.remove(old_filename)
+            remove_files_from_session([uid], remove_empty_session=False)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/utils.py` around lines 90 - 93, The current lookup for
old_filename dereferences the zero index of a list comprehension which can raise
IndexError if session["files"] is out of sync; modify the code that computes
old_filename (the expression using session["files"] and uid) to safely find the
matching file (e.g., use next(...) with a default None or check the length of
the list before indexing), bail out or log and skip removal if no match is
found, and ensure remove_files_from_session([uid], ...) is only called when a
matching filename was located; refer to the uid variable, session["files"], the
old_filename assignment, and remove_files_from_session to locate and fix the
logic.

86-94: ⚠️ Potential issue | 🟠 Major

Replacement flow can leave session["size"] stale when file bytes changed.

Line 80 adds incoming size before replacement, and Line 93 removal subtracts current annex size, not guaranteed previous stored size. Replacing same UID after file-content change can keep an incorrect total.

Proposed fix
-    session["size"] = session.get("size", 0) + size
+    session["size"] = session.get("size", 0) + size
+    recompute_size = False
@@
         if annot['uids'].get(uid, -1) == session_id:
@@
             remove_files_from_session([uid], remove_empty_session=False)
+            recompute_size = True
@@
     if session["client_id"] is None:
@@
         if u"{session_id}" in session["title"]:
             session["title"] = session["title"].replace(u"{session_id}", str(session_id))
+    if recompute_size:
+        session["size"] = sum(get_filesize(fdic["uid"]) for fdic in session["files"])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/utils.py` around lines 86 - 94, The replacement branch that
handles annot['uids'][uid] == session_id updates session["size"] incorrectly by
adding the incoming size and later subtracting the current annex size; instead,
fetch the previous file entry from session["files"] (the same list comprehension
used to get old_filename) and read its stored size (e.g., old_entry['size'] or
equivalent metadata) as old_size, then update session["size"] by subtracting
old_size before adding the new incoming size (or replace by session["size"] +=
new_size - old_size); ensure you still remove the old_filename from
existing_files and call remove_files_from_session([uid],
remove_empty_session=False) after adjusting session["size"]; if the stored size
is missing fall back to the annex/current size but prefer the stored 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/tests/test_utils.py`:
- Around line 465-478: The test is re-adding files without forcing the same
session so it may pass using a different session; modify the calls to
add_files_to_session to pass the existing session id (use session_id=sid) when
re-adding the annex and when adding the exact same file, and add an assertion
that the returned session id equals the original sid to ensure the function
updated the same session (refer to add_files_to_session and the sid variable and
session_id parameter).

In `@src/imio/esign/utils.py`:
- Around line 90-93: The current lookup for old_filename dereferences the zero
index of a list comprehension which can raise IndexError if session["files"] is
out of sync; modify the code that computes old_filename (the expression using
session["files"] and uid) to safely find the matching file (e.g., use next(...)
with a default None or check the length of the list before indexing), bail out
or log and skip removal if no match is found, and ensure
remove_files_from_session([uid], ...) is only called when a matching filename
was located; refer to the uid variable, session["files"], the old_filename
assignment, and remove_files_from_session to locate and fix the logic.
- Around line 86-94: The replacement branch that handles annot['uids'][uid] ==
session_id updates session["size"] incorrectly by adding the incoming size and
later subtracting the current annex size; instead, fetch the previous file entry
from session["files"] (the same list comprehension used to get old_filename) and
read its stored size (e.g., old_entry['size'] or equivalent metadata) as
old_size, then update session["size"] by subtracting old_size before adding the
new incoming size (or replace by session["size"] += new_size - old_size); ensure
you still remove the old_filename from existing_files and call
remove_files_from_session([uid], remove_empty_session=False) after adjusting
session["size"]; if the stored size is missing fall back to the annex/current
size but prefer the stored value.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8985022b-cd1a-4934-88c0-aca79edfe09d

📥 Commits

Reviewing files that changed from the base of the PR and between df35686 and caf6fcd.

📒 Files selected for processing (2)
  • src/imio/esign/tests/test_utils.py
  • src/imio/esign/utils.py

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
src/imio/esign/utils.py (2)

90-92: ⚠️ Potential issue | 🟠 Major

Guard old file lookup before dereferencing.

This assumes the UID is always present in session["files"]; if mappings drift, [0] raises and aborts the update path.

💡 Proposed fix
-            old_filename = path.splitext([fn for fn in session["files"]
-                                          if fn['uid'] == uid][0]['filename'])[0]
-            existing_files.remove(old_filename)
+            old_file = next((fn for fn in session["files"] if fn["uid"] == uid), None)
+            if old_file is None:
+                logger.error(
+                    "File UID %s mapped to session %s but missing from session files.",
+                    uid, session_id,
+                )
+                continue
+            old_filename = path.splitext(old_file["filename"])[0]
+            if old_filename in existing_files:
+                existing_files.remove(old_filename)
#!/bin/bash
# Verify unsafe indexed lookup is still present and inspect guard context.
rg -n -C3 "old_filename = path\\.splitext\\(\\[fn for fn in session\\[\"files\"\\]" src/imio/esign/utils.py
rg -n -C3 "if annot\\['uids'\\]\\.get\\(uid, -1\\) == session_id" src/imio/esign/utils.py
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/utils.py` around lines 90 - 92, The code assumes a matching
file entry exists and indexes [0] directly when computing old_filename and then
removes it from existing_files; change the lookup to safely search for the entry
(e.g. use next(...) with a default None) and guard against None before calling
path.splitext or existing_files.remove — if no entry is found, skip the
removal/update path or log/raise a clear error; ensure you only call
existing_files.remove(old_filename) when old_filename is defined and present in
existing_files.

87-94: ⚠️ Potential issue | 🟠 Major

Session size can stay stale when replacing an existing UID.

When updating a UID already present in the session, size is added up front, then removal subtracts the current annex size. If content size changed since prior add, session["size"] can drift from real total and affect later auto-discrimination.

💡 Proposed fix
-    session["size"] = session.get("size", 0) + size
+    session["size"] = session.get("size", 0) + size
+    recompute_session_size = False
@@
         if annot['uids'].get(uid, -1) == session_id:
@@
             remove_files_from_session([uid], remove_empty_session=False)
+            recompute_session_size = True
@@
         annot["c_uids"].setdefault(context_uid, PersistentList()).append(uid)
+    if recompute_session_size:
+        session["size"] = sum(get_filesize(fdic["uid"]) for fdic in session["files"])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/utils.py` around lines 87 - 94, The session size drifts when
replacing a UID because the code adds the new content size then calls
remove_files_from_session which subtracts the current annex size (not the
original), so capture the original file size and adjust session["size"] using
that original value instead of relying on remove_files_from_session to compute
it. In the block guarded by if annot['uids'].get(uid, -1) == session_id:
retrieve the original file record from session["files"] (the same place you get
old_filename), read its stored size field (e.g., old_file.get('annex_size', 0)
or old_file.get('size', 0)), subtract that original size from session["size"]
(or pass it into remove_files_from_session so it uses the known old size) before
adding the new file size and removing the old file; keep existing_files removal
and calling remove_files_from_session but ensure you prevent double-size
recalculation by either disabling size updates in remove_files_from_session
(remove_empty_session=False already) or by updating remove_files_from_session to
accept an old_size parameter and use it when provided.
🤖 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/utils.py`:
- Around line 90-92: The code assumes a matching file entry exists and indexes
[0] directly when computing old_filename and then removes it from
existing_files; change the lookup to safely search for the entry (e.g. use
next(...) with a default None) and guard against None before calling
path.splitext or existing_files.remove — if no entry is found, skip the
removal/update path or log/raise a clear error; ensure you only call
existing_files.remove(old_filename) when old_filename is defined and present in
existing_files.
- Around line 87-94: The session size drifts when replacing a UID because the
code adds the new content size then calls remove_files_from_session which
subtracts the current annex size (not the original), so capture the original
file size and adjust session["size"] using that original value instead of
relying on remove_files_from_session to compute it. In the block guarded by if
annot['uids'].get(uid, -1) == session_id: retrieve the original file record from
session["files"] (the same place you get old_filename), read its stored size
field (e.g., old_file.get('annex_size', 0) or old_file.get('size', 0)), subtract
that original size from session["size"] (or pass it into
remove_files_from_session so it uses the known old size) before adding the new
file size and removing the old file; keep existing_files removal and calling
remove_files_from_session but ensure you prevent double-size recalculation by
either disabling size updates in remove_files_from_session
(remove_empty_session=False already) or by updating remove_files_from_session to
accept an old_size parameter and use it when provided.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1aa3aa45-79bc-4d05-a043-3181eb8a64d3

📥 Commits

Reviewing files that changed from the base of the PR and between 66d8df8 and 795cc16.

📒 Files selected for processing (2)
  • src/imio/esign/tests/test_utils.py
  • src/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/imio/esign/tests/test_utils.py

@gbastien gbastien marked this pull request as draft March 9, 2026 09:22
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
src/imio/esign/utils.py (1)

86-97: ⚠️ Potential issue | 🔴 Critical

Replacing an existing UID still leaves session["size"] double-counted.

session["size"] already includes the incoming file size before this block runs. Lines 95-97 then restore that inflated value after remove_files_from_session(), so the old entry is never actually subtracted. The new regression in src/imio/esign/tests/test_utils.py should expose this immediately: after Line 474, the size should stay 6968, not jump to 13936. This also skews later size-based session discrimination.

💡 Suggested fix
     if not session:
         session_id, session = create_session(
             signers, seal, acroform=acroform, title=title, annot=annot, discriminators=discriminators,
             watchers=watchers,
             create_session_custom_data=create_session_custom_data,
         )
     session["size"] = session.get("size", 0) + size
+    recompute_size = False
     existing_files = [path.splitext(f["filename"])[0] for f in session["files"]]
     for uid in files_uids:
         annex = uuidToObject(uuid=uid, unrestricted=True)
         context_uid_provider = getAdapter(annex, IContextUidProvider)
         context_uid = context_uid_provider.get_context_uid()
         # update data if adding same file to same session
         if annot['uids'].get(uid, -1) == session_id:
             logger.info('File with UID %s is already in session_id %s and data were updated!', uid, session_id)
             # remove old filename to avoid filename being renamed
             old_filename = path.splitext([fn for fn in session["files"]
                                           if fn['uid'] == uid][0]['filename'])[0]
             existing_files.remove(old_filename)
-            # session size will be updated when removing file from session
-            # so save it because it is already correct
-            saved_size = session["size"]
             remove_files_from_session([uid], remove_empty_session=False)
-            session["size"] = saved_size
+            recompute_size = True
@@
         existing_files.append(new_filename)
         annot["uids"][uid] = session_id
         annot["c_uids"].setdefault(context_uid, PersistentList()).append(uid)
+    if recompute_size:
+        session["size"] = sum(get_filesize(fdic["uid"]) for fdic in session["files"])
     if session["client_id"] is None:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/utils.py` around lines 86 - 97, The session size is left
double-counted because saved_size restores the pre-removal value which already
included the incoming file; instead, after removing the old file call you should
recalculate session["size"] from the remaining session["files"] (or subtract the
removed file's size) rather than restoring saved_size. Modify the code around
the UID-replacement branch (symbols: annot, uid, session_id, session,
remove_files_from_session, existing_files, old_filename) so that after
remove_files_from_session([uid], ...) you set session["size"] = sum(f['size']
for f in session["files"]) (or compute saved_size - removed_file_size) to ensure
the size is correct.
🤖 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/utils.py`:
- Around line 86-97: The session size is left double-counted because saved_size
restores the pre-removal value which already included the incoming file;
instead, after removing the old file call you should recalculate session["size"]
from the remaining session["files"] (or subtract the removed file's size) rather
than restoring saved_size. Modify the code around the UID-replacement branch
(symbols: annot, uid, session_id, session, remove_files_from_session,
existing_files, old_filename) so that after remove_files_from_session([uid],
...) you set session["size"] = sum(f['size'] for f in session["files"]) (or
compute saved_size - removed_file_size) to ensure the size is correct.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9e6bcb8e-ee0b-4758-8e2a-eeb194f554ef

📥 Commits

Reviewing files that changed from the base of the PR and between 795cc16 and e425dcf.

📒 Files selected for processing (2)
  • src/imio/esign/tests/test_utils.py
  • src/imio/esign/utils.py

@gbastien gbastien marked this pull request as ready for review March 12, 2026 13:22

[sources]
collective.iconifiedcategory = git ${remotes:collective}/collective.iconifiedcategory.git pushurl=${remotes:collective_push}/collective.iconifiedcategory.git
imio.annex = git ${remotes:imio}/imio.annex.git pushurl=${remotes:imio_push}/imio.annex.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imio.annex en double

return annot['sessions'][session_id]


def get_sessions_for(context_uid):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fonction déjà définie dans main (avec paramètre readonly)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didjap' de merge...

# edit annex and add again, still one annex in session and data are updated
annex0.file.filename = u"new_annex0.pdf"
annex0.setTitle('New Annex 0')
sid, session = add_files_to_session(signers, (annex0_uid,))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Est-ce qu'il ne faudrait pas tester ici que le filename et le title sont déjà mis à jour dans la session même avant de l'ajouter à nouveau ? Si oui, alors on n'est plus vraiment dans le contexte de test_utils mais plutôt test_events

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je vais compléter test_categorized_element_updated_event avec changement file, çà teste que c'est bien màj lors d'un edit, et donc ici c'est la création, on a le postulat que dans ce cas c'est bien à jour, d'ailleurs on le voit dans ce test, sinon on re-teste des choses déjà testées... Il me semble...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

à voir car création ou ajout même annexe ne passe pas par l'event de modif, c'est juste le add_files_to_session qui gère cela, çà va mettre à jour les infos correctement, le code lié à l'event est ci-dessous dans le "edit file"...
Je vais push un test_categorized_element_updated_event + complet où on teste aussi que les infos filesize sont mises à jour, mais c'est double test pour moi

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris-adam en effet le test a évolué petit à petit, et la partie "add" après le "modify" a été enlevée pour montrer que c'est màj sans re-add, j'avais lu trop vite car ton commentaire était "trop haut" dans le test, ici çà me semble bon comme çà on pourrait merger à mon sens

@gbastien gbastien requested a review from chris-adam March 13, 2026 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants