Handled multiple sessions for one context#95
Merged
Conversation
chris-adam
commented
Feb 23, 2026
| self.annot["session_ids"] = PersistentList(new_session_ids) | ||
| session_len = len(session_file_uids) | ||
| if session_len > 1: | ||
| return True, _("${count} files added to session number ${session_id}", |
Contributor
Author
There was a problem hiding this comment.
Traduction à supprimer
chris-adam
commented
Feb 23, 2026
| mapping={"count": session_len, "session_id": session_id}) | ||
| n_sessions = len(new_session_ids) | ||
| if n_sessions == 1: | ||
| return True, _("${count} file(s) added to session number ${session_id}", |
Contributor
Author
There was a problem hiding this comment.
Traduction à ajouter
chris-adam
force-pushed
the
PARAF-345/one_context_multiple_sessions
branch
from
643168f to
ac9d042
Compare
|
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:
📝 WalkthroughWalkthroughOMApprovalAdapter replaces scalar Changes
Sequence Diagram(s)sequenceDiagram
participant Adapter as OMApprovalAdapter
participant Subscriber as dmsoutgoingmail_transition
participant SessionView as ExternalSessionCreateView
rect rgba(200,220,255,0.5)
Note over Adapter: initialization
Adapter->>Adapter: annot["session_ids"] = PersistentList()
end
rect rgba(200,255,220,0.5)
Note over Adapter: add_mail_files_to_session (per-file)
loop for each file
Adapter->>Adapter: create session for file
Adapter->>Adapter: append new session_id to annot["session_ids"]
end
Adapter->>Subscriber: expose approval.session_ids
end
rect rgba(255,230,200,0.5)
Note over Subscriber: external session creation
loop for each session_id in approval.session_ids
Subscriber->>SessionView: instantiate ExternalSessionCreateView(session_id)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
📝 Coding Plan for PR comments
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: 1
🧹 Nitpick comments (1)
imio/dms/mail/adapters.py (1)
1871-1871: Rename ambiguous loop variable (l).
This avoids E741 and improves readability.♻️ Suggested tweak
- session_len = sum(len(l) for l in self.pdf_files_uids) + session_len = sum(len(pdf_list) for pdf_list in self.pdf_files_uids)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/adapters.py` at line 1871, The variable name "l" in the comprehension that computes session_len is ambiguous and can shadow or be confused with the digit "1"; update the comprehension in the context where session_len is computed (using session_len and self.pdf_files_uids) to use a clear name like "uids" or "files" instead of "l" so it becomes sum(len(uids) for uids in self.pdf_files_uids), improving readability and avoiding E741.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@imio/dms/mail/adapters.py`:
- Around line 1858-1877: The current code overwrites stored session ids
(self.annot["session_ids"]) when new_session_ids is non-empty, dropping
previously created sessions; change it to merge existing IDs with new ones
(preserving order and uniqueness) instead of replacing them: fetch existing =
list(self.session_ids) or [] , extend it with any ids from new_session_ids that
are not already present, and then assign the merged list back to
self.annot["session_ids"] as a PersistentList; references: add_files_to_session
(where new_session_ids is built), new_session_ids, self.annot["session_ids"],
and self.session_ids.
---
Nitpick comments:
In `@imio/dms/mail/adapters.py`:
- Line 1871: The variable name "l" in the comprehension that computes
session_len is ambiguous and can shadow or be confused with the digit "1";
update the comprehension in the context where session_len is computed (using
session_len and self.pdf_files_uids) to use a clear name like "uids" or "files"
instead of "l" so it becomes sum(len(uids) for uids in self.pdf_files_uids),
improving readability and avoiding E741.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
imio/dms/mail/adapters.pyimio/dms/mail/locales/en/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/fr/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/generated.potimio/dms/mail/locales/imio.dms.mail.potimio/dms/mail/skins/imio_dms_mail/imiodmsmail.css.dtmlimio/dms/mail/subscribers.pyimio/dms/mail/tests/test_adapters.pyimio/dms/mail/tests/test_subscribers.py
chris-adam
force-pushed
the
PARAF-345/one_context_multiple_sessions
branch
from
ac9d042 to
b4342ed
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
imio/dms/mail/tests/test_adapters.py (1)
1454-1456: Redundant assertions — line 1456 subsumes lines 1454 and 1455.
assertEqual(list(self.approval.session_ids), [0])already verifies both length (1) and content (first element is 0), making the preceding two assertions redundant.♻️ Simplification
- self.assertEqual(len(self.approval.session_ids), 1) - self.assertEqual(self.approval.session_ids[0], 0) self.assertEqual(list(self.approval.session_ids), [0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/tests/test_adapters.py` around lines 1454 - 1456, The three assertions on session_ids are redundant; remove the first two and keep the single comprehensive check `assertEqual(list(self.approval.session_ids), [0])`. Locate the assertions referencing self.approval.session_ids in the test function in imio/dms/mail/tests/test_adapters.py and delete the lines with `assertEqual(len(self.approval.session_ids), 1)` and `assertEqual(self.approval.session_ids[0], 0)` so only the final list equality assertion remains.imio/dms/mail/adapters.py (2)
1866-1869:set.union()does not preserve insertion order of existing session IDs.Converting
self.session_idsandnew_session_idstosetbefore merging is non-deterministic by Python spec. The previous review explicitly suggested an ordered list merge to maintain stable ordering. The current approach works for small integers in CPython (due to identity hashing), but is fragile.♻️ Ordered merge preserving insertion order
- if new_session_ids: - session_ids = set(self.session_ids).union(set(new_session_ids)) - self.annot["session_ids"] = PersistentList(list(session_ids)) + if new_session_ids: + existing = list(self.session_ids) + merged = existing + [sid for sid in new_session_ids if sid not in existing] + self.annot["session_ids"] = PersistentList(merged) + new_session_ids = merged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/adapters.py` around lines 1866 - 1869, The current merge converts self.session_ids and new_session_ids to sets which loses insertion order; change the merge in the block handling new_session_ids so it preserves existing ordering by keeping self.session_ids in order and appending only those ids from new_session_ids that are not already present (i.e. build an ordered list starting from self.session_ids then extend with each id from new_session_ids if id not in existing), and store that result back into self.annot["session_ids"] as a PersistentList; update references in this function to use this ordered-merge logic for session_ids and new_session_ids.
1165-1201: Data migration concern: objects with old"session_id"key lose their session reference.
__init__usessetdefault, so existing annotations (created before this PR) retain the old"session_id"(singular) key without gaining"session_ids". Thesession_idsproperty doesself.annot.get("session_ids", PersistentList()), which silently returns[]for those objects instead of the previously stored ID.Any subscriber or downstream logic reading
approval.session_idson an in-progress object that was signed under the old schema will see an empty list — losing the session reference entirely.Consider adding an upgrade step (similar to existing
migrations/migrate_to_*.py) that iterates outgoing mail objects and converts:# existing: annot["idm.approval"]["session_id"] = 0 # desired: annot["idm.approval"]["session_ids"] = PersistentList([0])#!/bin/bash # Verify whether a migration step for session_id -> session_ids exists rg -n "session_id" --type=py migrations/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/adapters.py` around lines 1165 - 1201, The annotations initializer (__init__) uses setdefault for "idm.approval" but older objects may still have a legacy "session_id" key causing session_ids property (session_ids) to return an empty PersistentList; add a migration script (following the pattern in migrations/migrate_to_*.py) that iterates outgoing mail objects, reads annot = IAnnotations(obj).get("idm.approval", {}), and when annot contains "session_id" but not "session_ids" converts the value into annot["session_ids"] = PersistentList([annot.pop("session_id")]); also update the runtime accessor session_ids to defensively check for the legacy "session_id" key (e.g., return annot.get("session_ids") or PersistentList([annot.get("session_id")]) if present) so in-flight objects work before migration runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@imio/dms/mail/adapters.py`:
- Around line 1866-1869: The current merge converts self.session_ids and
new_session_ids to sets which loses insertion order; change the merge in the
block handling new_session_ids so it preserves existing ordering by keeping
self.session_ids in order and appending only those ids from new_session_ids that
are not already present (i.e. build an ordered list starting from
self.session_ids then extend with each id from new_session_ids if id not in
existing), and store that result back into self.annot["session_ids"] as a
PersistentList; update references in this function to use this ordered-merge
logic for session_ids and new_session_ids.
- Around line 1165-1201: The annotations initializer (__init__) uses setdefault
for "idm.approval" but older objects may still have a legacy "session_id" key
causing session_ids property (session_ids) to return an empty PersistentList;
add a migration script (following the pattern in migrations/migrate_to_*.py)
that iterates outgoing mail objects, reads annot =
IAnnotations(obj).get("idm.approval", {}), and when annot contains "session_id"
but not "session_ids" converts the value into annot["session_ids"] =
PersistentList([annot.pop("session_id")]); also update the runtime accessor
session_ids to defensively check for the legacy "session_id" key (e.g., return
annot.get("session_ids") or PersistentList([annot.get("session_id")]) if
present) so in-flight objects work before migration runs.
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1454-1456: The three assertions on session_ids are redundant;
remove the first two and keep the single comprehensive check
`assertEqual(list(self.approval.session_ids), [0])`. Locate the assertions
referencing self.approval.session_ids in the test function in
imio/dms/mail/tests/test_adapters.py and delete the lines with
`assertEqual(len(self.approval.session_ids), 1)` and
`assertEqual(self.approval.session_ids[0], 0)` so only the final list equality
assertion remains.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
imio/dms/mail/adapters.pyimio/dms/mail/locales/en/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/fr/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/generated.potimio/dms/mail/locales/imio.dms.mail.potimio/dms/mail/skins/imio_dms_mail/imiodmsmail.css.dtmlimio/dms/mail/subscribers.pyimio/dms/mail/tests/test_adapters.pyimio/dms/mail/tests/test_subscribers.py
🚧 Files skipped from review as they are similar to previous changes (5)
- imio/dms/mail/subscribers.py
- imio/dms/mail/locales/fr/LC_MESSAGES/imio.dms.mail.po
- imio/dms/mail/skins/imio_dms_mail/imiodmsmail.css.dtml
- imio/dms/mail/locales/en/LC_MESSAGES/imio.dms.mail.po
- imio/dms/mail/tests/test_subscribers.py
chris-adam
force-pushed
the
PARAF-345/one_context_multiple_sessions
branch
from
b4342ed to
613d8fd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
imio/dms/mail/adapters.py (1)
1866-1868:set.union()loses insertion order of session IDs.
set(self.session_ids).union(set(new_session_ids))does not guarantee iteration order. For single-session mails this is harmless, but with multiple sessions any downstream code (e.g.,subscribers.pyiteratingapproval.session_idsto createExternalSessionCreateViewinstances) will see a non-deterministic order across different runs.♻️ Proposed order-preserving merge
- if new_session_ids: - session_ids = set(self.session_ids).union(set(new_session_ids)) - self.annot["session_ids"] = PersistentList(list(session_ids)) + if new_session_ids: + merged = list(self.session_ids) + for sid in new_session_ids: + if sid not in merged: + merged.append(sid) + self.annot["session_ids"] = PersistentList(merged)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/adapters.py` around lines 1866 - 1868, The current use of set(self.session_ids).union(set(new_session_ids)) loses insertion order; replace that with an order-preserving merge so existing session order is kept and only new, non-duplicate IDs are appended. Concretely, build a list starting from self.session_ids (or [] if None), iterate new_session_ids and append each id only if not already present, then assign self.annot["session_ids"] = PersistentList(merged_list); this preserves order for symbols like session_ids, new_session_ids and the stored PersistentList used by downstream code (e.g., approval.session_ids → ExternalSessionCreateView).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@imio/dms/mail/adapters.py`:
- Around line 1866-1868: The current use of
set(self.session_ids).union(set(new_session_ids)) loses insertion order; replace
that with an order-preserving merge so existing session order is kept and only
new, non-duplicate IDs are appended. Concretely, build a list starting from
self.session_ids (or [] if None), iterate new_session_ids and append each id
only if not already present, then assign self.annot["session_ids"] =
PersistentList(merged_list); this preserves order for symbols like session_ids,
new_session_ids and the stored PersistentList used by downstream code (e.g.,
approval.session_ids → ExternalSessionCreateView).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
imio/dms/mail/adapters.pyimio/dms/mail/locales/en/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/fr/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/generated.potimio/dms/mail/locales/imio.dms.mail.potimio/dms/mail/skins/imio_dms_mail/imiodmsmail.css.dtmlimio/dms/mail/subscribers.pyimio/dms/mail/tests/test_adapters.pyimio/dms/mail/tests/test_subscribers.py
🚧 Files skipped from review as they are similar to previous changes (5)
- imio/dms/mail/locales/fr/LC_MESSAGES/imio.dms.mail.po
- imio/dms/mail/skins/imio_dms_mail/imiodmsmail.css.dtml
- imio/dms/mail/locales/imio.dms.mail.pot
- imio/dms/mail/subscribers.py
- imio/dms/mail/locales/generated.pot
chris-adam
force-pushed
the
PARAF-345/one_context_multiple_sessions
branch
from
613d8fd to
b606f20
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
imio/dms/mail/tests/test_adapters.py (1)
1356-1460: Add one explicit multi-session assertion path in this test.Line [1391] and Line [1459] validate only the single-session message path. Given this PR’s multi-session objective, add a case asserting the multi-session message (
"${count} file(s) added to ${n} sessions") andlen(self.approval.session_ids) > 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/tests/test_adapters.py` around lines 1356 - 1460, Update test_add_mail_files_to_session to include an explicit multi-session assertion: after the existing single-session happy-path calls to self.approval.add_mail_files_to_session(), create the conditions that cause a second session to be created (so that len(self.approval.session_ids) > 1), call self.approval.add_mail_files_to_session() again, and assert the returned message equals the multi-session string "${count} file(s) added to ${n} sessions" and that len(self.approval.session_ids) > 1; reference the tested method add_mail_files_to_session() and the session list self.approval.session_ids when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@imio/dms/mail/adapters.py`:
- Around line 1867-1869: The code replaces session_ids by taking a set union
which loses ordering; update the merge in the block that handles
self.session_ids and new_session_ids so it preserves the existing order and
appends any new session IDs in their incoming order (e.g., start from
list(self.session_ids), iterate new_session_ids and append only those not
already present) and then assign that ordered list back into
self.annot["session_ids"] as a PersistentList; ensure you operate on the symbols
self.session_ids, new_session_ids, self.annot["session_ids"], and PersistentList
so the "first" session id remains deterministic.
- Around line 1872-1879: When computing n_sessions from new_session_ids in the
block using session_ids and pdf_files_uids, add an explicit guard for n_sessions
== 0 before the n_sessions == 1 check: detect the case where no sessions were
created (new_session_ids is empty and session_len may be 0) and return True with
a clear localized message indicating that no sessions were created or no files
were added to any session (use the same translation pattern and mapping keys as
the other branches). Update the logic around new_session_ids, session_len, and
n_sessions to avoid returning "added to 0 sessions" and ensure the message
references the relevant symbols (session_ids, pdf_files_uids, new_session_ids,
n_sessions).
In `@imio/dms/mail/subscribers.py`:
- Around line 452-453: The loop over approval.session_ids may re-invoke
ExternalSessionCreateView for session IDs already processed; update the code in
the block that calls ExternalSessionCreateView(mail,
mail.REQUEST)(session_id=sid) to avoid duplicates by deduplicating and/or
checking for existing sessions before calling: convert approval.session_ids to a
set or iterate only new IDs, or query the ExternalSession (or equivalent) model
to skip session_ids that already have an external session entry; ensure you
reference approval.session_ids and ExternalSessionCreateView(mail, mail.REQUEST)
when implementing the guard so previously created external sessions are not
re-created on retry/backtrack.
---
Nitpick comments:
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1356-1460: Update test_add_mail_files_to_session to include an
explicit multi-session assertion: after the existing single-session happy-path
calls to self.approval.add_mail_files_to_session(), create the conditions that
cause a second session to be created (so that len(self.approval.session_ids) >
1), call self.approval.add_mail_files_to_session() again, and assert the
returned message equals the multi-session string "${count} file(s) added to ${n}
sessions" and that len(self.approval.session_ids) > 1; reference the tested
method add_mail_files_to_session() and the session list
self.approval.session_ids when adding these assertions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
imio/dms/mail/adapters.pyimio/dms/mail/locales/en/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/fr/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/generated.potimio/dms/mail/locales/imio.dms.mail.potimio/dms/mail/skins/imio_dms_mail/imiodmsmail.css.dtmlimio/dms/mail/subscribers.pyimio/dms/mail/tests/test_adapters.pyimio/dms/mail/tests/test_subscribers.py
🚧 Files skipped from review as they are similar to previous changes (3)
- imio/dms/mail/locales/fr/LC_MESSAGES/imio.dms.mail.po
- imio/dms/mail/locales/imio.dms.mail.pot
- imio/dms/mail/locales/generated.pot
Comment on lines
+1867
to
+1869
| if new_session_ids: | ||
| session_ids = set(self.session_ids).union(set(new_session_ids)) | ||
| self.annot["session_ids"] = PersistentList(list(session_ids)) |
There was a problem hiding this comment.
Set union discards session ID ordering.
set(self.session_ids).union(set(new_session_ids)) does not preserve insertion order, so self.annot["session_ids"] ends up in an arbitrary order on every call. Line 1877 already reads new_session_ids[0] (the "first" session ID) for the single-session message — after a set round-trip that element is non-deterministic if existing IDs are present.
Use an order-preserving merge instead (as the previous review suggested):
🔧 Proposed fix — ordered merge
- if new_session_ids:
- session_ids = set(self.session_ids).union(set(new_session_ids))
- self.annot["session_ids"] = PersistentList(list(session_ids))
+ if new_session_ids:
+ existing = list(self.session_ids)
+ merged = existing + [sid for sid in new_session_ids if sid not in existing]
+ self.annot["session_ids"] = PersistentList(merged)
+ new_session_ids = merged📝 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
| if new_session_ids: | |
| session_ids = set(self.session_ids).union(set(new_session_ids)) | |
| self.annot["session_ids"] = PersistentList(list(session_ids)) | |
| if new_session_ids: | |
| existing = list(self.session_ids) | |
| merged = existing + [sid for sid in new_session_ids if sid not in existing] | |
| self.annot["session_ids"] = PersistentList(merged) | |
| new_session_ids = merged |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@imio/dms/mail/adapters.py` around lines 1867 - 1869, The code replaces
session_ids by taking a set union which loses ordering; update the merge in the
block that handles self.session_ids and new_session_ids so it preserves the
existing order and appends any new session IDs in their incoming order (e.g.,
start from list(self.session_ids), iterate new_session_ids and append only those
not already present) and then assign that ordered list back into
self.annot["session_ids"] as a PersistentList; ensure you operate on the symbols
self.session_ids, new_session_ids, self.annot["session_ids"], and PersistentList
so the "first" session id remains deterministic.
Comment on lines
+1872
to
+1879
| new_session_ids = list(self.session_ids) | ||
| session_len = sum(len(p) for p in self.pdf_files_uids) | ||
| n_sessions = len(new_session_ids) | ||
| if n_sessions == 1: | ||
| return True, _("${count} file(s) added to session number ${session_id}", | ||
| mapping={"count": session_len, "session_id": new_session_ids[0]}) | ||
| return True, _("${count} file(s) added to ${n} sessions", | ||
| mapping={"count": session_len, "n": n_sessions}) |
There was a problem hiding this comment.
n_sessions == 0 produces a misleading return message.
If session_file_uids is empty (all PDFs already existed) and self.session_ids is also empty (e.g., annotation was reset), then new_session_ids = list(self.session_ids) → [], n_sessions = 0, and the function returns True, _("${count} file(s) added to ${n} sessions", mapping={"count": session_len, "n": 0}) — semantically "added to 0 sessions".
Add an explicit guard:
🔧 Proposed fix
+ if not new_session_ids:
+ # nothing was created or reused — should not happen in normal flow
+ return False, _("No session available")
session_len = sum(len(p) for p in self.pdf_files_uids)
n_sessions = len(new_session_ids)
if n_sessions == 1:📝 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
| new_session_ids = list(self.session_ids) | |
| session_len = sum(len(p) for p in self.pdf_files_uids) | |
| n_sessions = len(new_session_ids) | |
| if n_sessions == 1: | |
| return True, _("${count} file(s) added to session number ${session_id}", | |
| mapping={"count": session_len, "session_id": new_session_ids[0]}) | |
| return True, _("${count} file(s) added to ${n} sessions", | |
| mapping={"count": session_len, "n": n_sessions}) | |
| new_session_ids = list(self.session_ids) | |
| if not new_session_ids: | |
| # nothing was created or reused — should not happen in normal flow | |
| return False, _("No session available") | |
| session_len = sum(len(p) for p in self.pdf_files_uids) | |
| n_sessions = len(new_session_ids) | |
| if n_sessions == 1: | |
| return True, _("${count} file(s) added to session number ${session_id}", | |
| mapping={"count": session_len, "session_id": new_session_ids[0]}) | |
| return True, _("${count} file(s) added to ${n} sessions", | |
| mapping={"count": session_len, "n": n_sessions}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@imio/dms/mail/adapters.py` around lines 1872 - 1879, When computing
n_sessions from new_session_ids in the block using session_ids and
pdf_files_uids, add an explicit guard for n_sessions == 0 before the n_sessions
== 1 check: detect the case where no sessions were created (new_session_ids is
empty and session_len may be 0) and return True with a clear localized message
indicating that no sessions were created or no files were added to any session
(use the same translation pattern and mapping keys as the other branches).
Update the logic around new_session_ids, session_len, and n_sessions to avoid
returning "added to 0 sessions" and ensure the message references the relevant
symbols (session_ids, pdf_files_uids, new_session_ids, n_sessions).
Comment on lines
+452
to
+453
| for sid in approval.session_ids: | ||
| ExternalSessionCreateView(mail, mail.REQUEST)(session_id=sid) |
There was a problem hiding this comment.
Prevent duplicate external session creation calls.
Line [452]-Line [453] iterates all stored approval.session_ids, including old ones. If this transition is retried/backtracked, previously created external sessions can be called again.
🔧 Proposed fix
- added, msg = approval.add_mail_files_to_session()
+ previous_session_ids = set(approval.session_ids)
+ added, msg = approval.add_mail_files_to_session()
+ new_session_ids = [sid for sid in approval.session_ids if sid not in previous_session_ids]
msg2 = ""
if added:
if not get_registry_seal_code():
msg2 = _("Seal code must be defined in eSign settings befode sending session")
else:
- for sid in approval.session_ids:
+ for sid in new_session_ids:
ExternalSessionCreateView(mail, mail.REQUEST)(session_id=sid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@imio/dms/mail/subscribers.py` around lines 452 - 453, The loop over
approval.session_ids may re-invoke ExternalSessionCreateView for session IDs
already processed; update the code in the block that calls
ExternalSessionCreateView(mail, mail.REQUEST)(session_id=sid) to avoid
duplicates by deduplicating and/or checking for existing sessions before
calling: convert approval.session_ids to a set or iterate only new IDs, or query
the ExternalSession (or equivalent) model to skip session_ids that already have
an external session entry; ensure you reference approval.session_ids and
ExternalSessionCreateView(mail, mail.REQUEST) when implementing the guard so
previously created external sessions are not re-created on retry/backtrack.
Contributor
Author
There was a problem hiding this comment.
This is part of another ticket and it will be handled in another PR
There was a problem hiding this comment.
@chris-adam, understood! I acknowledge your decision to handle the duplicate session creation prevention in a separate PR.
Would you like me to create a follow-up issue to track this concern? If so, I can document the potential duplicate invocation scenario and reference this discussion for context.
There was a problem hiding this comment.
@chris-adam, noted! I won't create a follow-up issue. Thank you for clarifying.
chris-adam
force-pushed
the
PARAF-345/one_context_multiple_sessions
branch
from
b606f20 to
2d99573
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
imio/dms/mail/adapters.py (1)
1867-1869:⚠️ Potential issue | 🟠 MajorPreserve session ID order when merging lists.
Line [1868] uses a set union, which discards ordering and makes persisted
session_idsnon-deterministic.🔧 Proposed fix
- if new_session_ids: - session_ids = set(self.session_ids).union(set(new_session_ids)) - self.annot["session_ids"] = PersistentList(list(session_ids)) + if new_session_ids: + existing_session_ids = list(self.session_ids) + merged_session_ids = existing_session_ids + [ + sid for sid in new_session_ids if sid not in existing_session_ids + ] + self.annot["session_ids"] = PersistentList(merged_session_ids) + new_session_ids = merged_session_ids🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/adapters.py` around lines 1867 - 1869, The merge currently uses set union on self.session_ids and new_session_ids (in code touching self.session_ids, new_session_ids, and self.annot["session_ids"] with PersistentList), which loses order; change it to produce an ordered, deduplicated list by iterating self.session_ids then new_session_ids and appending only items not already seen (or use OrderedDict.fromkeys over the concatenation) so original order is preserved, then assign that ordered list wrapped in PersistentList back to self.annot["session_ids"].imio/dms/mail/subscribers.py (1)
459-482:⚠️ Potential issue | 🟠 MajorAvoid re-sending already-created external sessions.
Line [481] iterates all persisted
approval.session_ids, so retries/backtracks can callExternalSessionCreateViewagain for old sessions.🔧 Proposed fix
- added, msg = approval.add_mail_files_to_session() + previous_session_ids = set(approval.session_ids) + added, msg = approval.add_mail_files_to_session() + new_session_ids = [sid for sid in approval.session_ids if sid not in previous_session_ids] api.portal.show_message( message=msg, request=mail.REQUEST, type=added and "info" or "error", ) @@ - if seal_code and seal_email: - for sid in approval.session_ids: + if seal_code and seal_email: + for sid in new_session_ids: ExternalSessionCreateView(mail, mail.REQUEST)(session_id=sid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/subscribers.py` around lines 459 - 482, The code is re-sending ExternalSessionCreateView for all approval.session_ids (so retries re-send old sessions); change the logic to only call ExternalSessionCreateView for sessions newly created by approval.add_mail_files_to_session (or otherwise not already linked to an external session). Update add_mail_files_to_session to return the list of created session IDs (e.g., created_ids) or, if such a helper exists, filter approval.session_ids by a predicate like "not already has external id" before the loop; then iterate created_ids (or the filtered list) when invoking ExternalSessionCreateView(mail, mail.REQUEST)(session_id=sid) so existing external sessions are not re-sent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@imio/dms/mail/adapters.py`:
- Around line 1867-1869: The merge currently uses set union on self.session_ids
and new_session_ids (in code touching self.session_ids, new_session_ids, and
self.annot["session_ids"] with PersistentList), which loses order; change it to
produce an ordered, deduplicated list by iterating self.session_ids then
new_session_ids and appending only items not already seen (or use
OrderedDict.fromkeys over the concatenation) so original order is preserved,
then assign that ordered list wrapped in PersistentList back to
self.annot["session_ids"].
In `@imio/dms/mail/subscribers.py`:
- Around line 459-482: The code is re-sending ExternalSessionCreateView for all
approval.session_ids (so retries re-send old sessions); change the logic to only
call ExternalSessionCreateView for sessions newly created by
approval.add_mail_files_to_session (or otherwise not already linked to an
external session). Update add_mail_files_to_session to return the list of
created session IDs (e.g., created_ids) or, if such a helper exists, filter
approval.session_ids by a predicate like "not already has external id" before
the loop; then iterate created_ids (or the filtered list) when invoking
ExternalSessionCreateView(mail, mail.REQUEST)(session_id=sid) so existing
external sessions are not re-sent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
imio/dms/mail/adapters.pyimio/dms/mail/locales/en/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/fr/LC_MESSAGES/imio.dms.mail.poimio/dms/mail/locales/generated.potimio/dms/mail/locales/imio.dms.mail.potimio/dms/mail/skins/imio_dms_mail/imiodmsmail.css.dtmlimio/dms/mail/subscribers.pyimio/dms/mail/tests/test_adapters.pyimio/dms/mail/tests/test_subscribers.py
🚧 Files skipped from review as they are similar to previous changes (4)
- imio/dms/mail/tests/test_adapters.py
- imio/dms/mail/locales/imio.dms.mail.pot
- imio/dms/mail/tests/test_subscribers.py
- imio/dms/mail/locales/en/LC_MESSAGES/imio.dms.mail.po
sgeulette
approved these changes
Mar 12, 2026
Member
sgeulette
left a comment
sgeulette
left a comment
There was a problem hiding this comment.
J'ai simplifié et corrigé le code + tests
| return True, _("${count} file added to session number ${session_id}", | ||
| mapping={"count": session_len, "session_id": session_id}) | ||
| # All PDFs already existed — reuse the previously stored session ids | ||
| new_session_ids = list(self.session_ids) |
Member
There was a problem hiding this comment.
pas correct car il n'est pas dit que toutes les sessions existantes ont été utilisées pour les fichiers ajoutés
| mapping={"count": session_len, "session_id": session_id}) | ||
| # All PDFs already existed — reuse the previously stored session ids | ||
| new_session_ids = list(self.session_ids) | ||
| session_len = sum(len(p) for p in self.pdf_files_uids) |
Member
There was a problem hiding this comment.
si des fichiers existaient préalablement, ils sont comptabilisés ?? Pas correct
| from imio.dms.mail import IM_EDITOR_SERVICE_FUNCTIONS | ||
| from imio.dms.mail import IM_READER_SERVICE_FUNCTIONS | ||
| from imio.dms.mail.adapters import OMApprovalAdapter | ||
|
|
Member
There was a problem hiding this comment.
régler l'IDE pour ne pas ajouter une ligne vide...
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
Style
Tests
Documentation