Open
Conversation
📝 WalkthroughWalkthroughApprove/signing workflow logic was changed: approvals now decide post-approval transition via a new helper; guards were renamed/added; workflow definitions, migrations, wfadaptations, call sites, registry steps, and tests were updated to align with the new behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,220,255,0.5)
participant User
end
rect rgba(220,255,200,0.5)
participant Adapter as OMApprovalAdapter
participant Utils as Utils (get_post_approval_transition)
participant WF as portal_workflow
participant Catalog as portal_catalog
end
User->>Adapter: approve_file(afile, userid, ..., transition=True)
Adapter->>Adapter: update_signers / approval state
Adapter->>Utils: get_post_approval_transition(context)
Utils->>WF: getTransitionsFor(context)
WF-->>Utils: transitions list
Utils-->>Adapter: selected_transition ("set_to_print" or "propose_to_be_signed")
Adapter->>WF: do_transitions(context, [selected_transition])
Adapter->>Catalog: reindexObject(context)
Adapter-->>User: return approval result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
chris-adam
force-pushed
the
PARAF-403/to_print_wfa
branch
4 times, most recently
from
feea82a to
679fd50
Compare
chris-adam
force-pushed
the
PARAF-403/to_print_wfa
branch
from
679fd50 to
0a0def5
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
imio/dms/mail/dmsmail.py (1)
915-923:⚠️ Potential issue | 🟠 MajorDon't gate the stock
propose_to_be_signedtransition on completed approvals.
outgoingmail_workflownow routespropose_to_be_signedthrough this helper, but the default workflow still has noto_approvestate.OMApprovalAdapter.start_approval_process()only starts fromto_approve, so oncehas_approvings()becomes true there is no path that can ever makehas_approvings(all_done=True)flip toTrue; the mail is stuck increated. Please make this approval check conditional on the approval adaptation/state being present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/dmsmail.py` around lines 915 - 923, The guard in can_be_signed currently blocks the stock propose_to_be_signed transition by requiring approvals to be completed; change it to only perform the approvals check when the approval adaptation/state exists so default workflows without approvals aren’t stuck. Concretely, in can_be_signed wrap the approval logic with a presence check (e.g. if getattr(self.context, "has_approvings", None) is not None and self.context.has_approvings()) before calling has_approvings(all_done=True), or alternatively check for the approval adapter/state (OMApprovalAdapter or a "to_approve" state) before evaluating has_approvings(all_done=True); keep the existing catalog and brains checks intact and only return False for incomplete approvals when that approval capability is present.
🧹 Nitpick comments (1)
imio/dms/mail/tests/test_dmsmail.py (1)
568-580: Please cover the new approval gate andcan_set_to_print()path.This still only checks file presence. The risky behavior change in this PR is the
has_approvings(all_done=True)branch insidecan_be_signed()plus the newcan_set_to_print()guard used by the to-print adaptation, and neither branch is asserted here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/tests/test_dmsmail.py` around lines 568 - 580, Extend the test_ImioDmsOutgoingMailWfConditionsAdapter_can_be_signed test to also exercise the approval-gate branch and the to-print guard: create and transition approving workflow items so that adapted.has_approvings(all_done=True) becomes True and assert adapted.can_be_signed() both when approvals are incomplete and when all_done=True; additionally instantiate the to-print adaptation (or call the can_set_to_print() path on the same adapter) and assert its behavior before and after conditions that should allow printing. Target the ImioDmsOutgoingMailWfConditionsAdapter instance (adapted), its can_be_signed() method, the has_approvings(...) branch, and the can_set_to_print() guard 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 1308-1316: The state "to_print" is currently classified as "before
approval" by is_state_before_approve/is_state_before_or_approve but still
appears in the "after approval" buckets, causing overlap; update
is_state_after_approve and is_state_after_or_approve to exclude "to_print" from
their state checks (or otherwise align their allowed-state tuples with the
before-approve helpers) so "to_print" is only considered before approval; adjust
the tuple/list of states used in is_state_after_approve and
is_state_after_or_approve to remove "to_print" and keep consistency with
is_state_before_approve/is_state_before_or_approve.
In `@imio/dms/mail/migrations/migrate_to_3_1_2.py`:
- Around line 89-109: The migration currently overwrites any existing guards for
transitions "set_to_print" and "back_to_print" unconditionally; change the logic
in the block using applied_wfa_names / to_print_applied so that before calling
tr.setProperties(...) you check the existing guard expression and only replace
it when it equals the legacy expression (the old can_be_handsigned() check used
in the earlier loop). Locate the portal_workflow lookup (wf =
api.portal.get().portal_workflow["outgoingmail_workflow"]), inspect
tr.guard_expr or the equivalent property on the transition object, compare it to
the legacy "python:object.wf_conditions().can_be_handsigned()" string, and only
call tr.setProperties(...) with new_guard and guard_permissions when that
comparison matches; otherwise skip to preserve customized guards.
In `@imio/dms/mail/wfadaptations.py`:
- Around line 1306-1311: The transitions list in imio.dms.mail.wfadaptations
currently keeps "propose_to_be_signed" even when OMToApproveAdaptation is
applied, allowing a direct jump from to_print to to_be_signed and bypassing
approval; update the logic where transitions is built (the block using
get_applied_adaptations and the applied_wfa check) so that when
u"imio.dms.mail.wfadaptations.OMToApproveAdaptation" is present you remove or
replace "propose_to_be_signed" (e.g., remove it and ensure "propose_to_approve"
is present) to prevent the direct to_print→to_be_signed edge, and apply the same
change in the reverse-apply path that patches existing to_print states so the
edge is also removed when OMToApproveAdaptation is applied.
---
Outside diff comments:
In `@imio/dms/mail/dmsmail.py`:
- Around line 915-923: The guard in can_be_signed currently blocks the stock
propose_to_be_signed transition by requiring approvals to be completed; change
it to only perform the approvals check when the approval adaptation/state exists
so default workflows without approvals aren’t stuck. Concretely, in
can_be_signed wrap the approval logic with a presence check (e.g. if
getattr(self.context, "has_approvings", None) is not None and
self.context.has_approvings()) before calling has_approvings(all_done=True), or
alternatively check for the approval adapter/state (OMApprovalAdapter or a
"to_approve" state) before evaluating has_approvings(all_done=True); keep the
existing catalog and brains checks intact and only return False for incomplete
approvals when that approval capability is present.
---
Nitpick comments:
In `@imio/dms/mail/tests/test_dmsmail.py`:
- Around line 568-580: Extend the
test_ImioDmsOutgoingMailWfConditionsAdapter_can_be_signed test to also exercise
the approval-gate branch and the to-print guard: create and transition approving
workflow items so that adapted.has_approvings(all_done=True) becomes True and
assert adapted.can_be_signed() both when approvals are incomplete and when
all_done=True; additionally instantiate the to-print adaptation (or call the
can_set_to_print() path on the same adapter) and assert its behavior before and
after conditions that should allow printing. Target the
ImioDmsOutgoingMailWfConditionsAdapter instance (adapted), its can_be_signed()
method, the has_approvings(...) branch, and the can_set_to_print() guard when
adding these assertions.
🪄 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: 26e57cf3-e371-4180-acdf-5c01eee3cfbb
📒 Files selected for processing (6)
imio/dms/mail/adapters.pyimio/dms/mail/dmsmail.pyimio/dms/mail/migrations/migrate_to_3_1_2.pyimio/dms/mail/profiles/default/workflows/outgoingmail_workflow/definition.xmlimio/dms/mail/tests/test_dmsmail.pyimio/dms/mail/wfadaptations.py
chris-adam
force-pushed
the
PARAF-403/to_print_wfa
branch
from
0a0def5 to
31afdf0
Compare
chris-adam
force-pushed
the
PARAF-403/to_print_wfa
branch
3 times, most recently
from
568d426 to
0075559
Compare
chris-adam
force-pushed
the
PARAF-403/to_print_wfa
branch
9 times, most recently
from
b95608a to
47452a2
Compare
chris-adam
commented
Apr 13, 2026
| ) | ||
| self.common_tests() | ||
|
|
||
| def test_OMToPrintAdaptationAfterNp1(self): |
Contributor
Author
There was a problem hiding this comment.
Je ne comprends pas bien à quoi servent ces deux tests. Même avant mes changements, ce deux tests appliquent la même wfa deux fois, alors que leurs noms suggèrent l'adaptation N+1
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
imio/dms/mail/steps.py (1)
137-149:⚠️ Potential issue | 🟠 MajorPersist
omail_fieldswhen onlysignersis inserted.Line 148 only writes the registry back inside the
ISigningBehavior.esignbranch. If a site already hasISigningBehavior.esignbut is still missingISigningBehavior.signers, Line 139 updates the local list and the step exits without saving it, so rerunningactivate_esigning()leaves the form config inconsistent.Proposed fix
omf = api.portal.get_registry_record( "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_fields", default=[] ) om_fns = [dic["field_name"] for dic in omf] + updated = False if "ISigningBehavior.signers" not in om_fns: pos = om_fns.index("internal_reference_no") omf.insert(pos + 1, {"field_name": "ISigningBehavior.signers", "read_tal_condition": u"", "write_tal_condition": u""}) om_fns = [dic["field_name"] for dic in omf] + updated = True if "ISigningBehavior.esign" not in om_fns: pos = om_fns.index("ISigningBehavior.signers") omf.insert(pos + 1, {"field_name": "ISigningBehavior.esign", "read_tal_condition": u"", "write_tal_condition": u""}) + updated = True + if updated: api.portal.set_registry_record("imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_fields", omf) log.append("Updated omail_fields registry record")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/steps.py` around lines 137 - 149, The code adds "ISigningBehavior.signers" into the local omf list but only persists changes to the registry when "ISigningBehavior.esign" is also inserted; modify the logic in activate_esigning (or the block manipulating omf/om_fns) so that after inserting the "ISigningBehavior.signers" entry you call api.portal.set_registry_record("imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_fields", omf) to persist the updated omf; keep using the same omf/om_fns variables and the existing insertion position logic (pos = om_fns.index("internal_reference_no")) and ensure you still insert and recompute om_fns before persisting.imio/dms/mail/migrations/migrate_to_3_1_2.py (1)
31-60:⚠️ Potential issue | 🟠 MajorThe substitute migration can lose entries on reordered or repeated rules.
Line 41 only searches
existing_rules[idx + 1:], so if the undated base rule is earlier in the list you'll create a substitute withabsent_signer=None. On top of that, Line 31 deduplicates only on(absent_signer, substitute_signer), which drops later date windows for the same pair. Both cases silently lose substitute data during upgrade.Proposed fix
- existing_pairs = {(s["absent_signer"], s["substitute_signer"]) for s in substitutes} + existing_pairs = { + ( + s["absent_signer"], + s["substitute_signer"], + s.get("valid_from"), + s.get("valid_until"), + ) + for s in substitutes + } @@ - for candidate in existing_rules[idx + 1:]: + for c_idx, candidate in enumerate(existing_rules): + if c_idx == idx: + continue if (not candidate.get("valid_from") and not candidate.get("valid_until") and candidate["signer"] != rule["signer"] and candidate["number"] == rule["number"] and candidate["treating_groups"] == rule["treating_groups"] and candidate["mail_types"] == rule["mail_types"] and candidate["send_modes"] == rule["send_modes"] and candidate["tal_condition"] == rule["tal_condition"]): absent_signer = candidate["signer"] break - pair = (absent_signer, rule["signer"]) + if absent_signer is None: + logger.warning( + "Could not resolve absent signer for dated signer rule %r; skipping substitute migration", + rule, + ) + cleaned_rules.append(rule) + continue + pair = (absent_signer, rule["signer"], valid_from, valid_until)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/migrations/migrate_to_3_1_2.py` around lines 31 - 60, The migration currently only looks for undated bases after the dated rule (existing_rules[idx + 1:]) and deduplicates substitutes only by (absent_signer, substitute_signer), which causes missing substitutes when the undated base is earlier or when the same pair has multiple date windows; update the search over existing_rules to scan the whole list (exclude the same entry) and pick the best candidate (e.g., the nearest by index or first match) instead of only later entries, ensure you don't create a substitute with absent_signer=None (skip if no candidate found), and change existing_pairs to include the date bounds when deduping (use a key like (absent_signer, substitute_signer, valid_from, valid_until) rather than just (absent_signer, substitute_signer)) so multiple dated windows for the same pair are preserved (refer to variables/functions: existing_rules, substitutes, idx, candidate, pair, existing_pairs, new_substitutes_count).
🤖 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/utils.py`:
- Around line 1527-1537: The transition lookup in get_post_approval_transition
uses pw.getTransitionsFor(context) which is permission-filtered and can differ
from the later execution under adopt_roles(["Reviewer"]); wrap the
getTransitionsFor call in the same elevation so the decision matches execution
(use api.user.adopt_roles(["Reviewer"]) around the pw.getTransitionsFor(context)
call in get_post_approval_transition) to ensure set_to_print is chosen when it
will be executable rather than falling back to propose_to_be_signed.
---
Outside diff comments:
In `@imio/dms/mail/migrations/migrate_to_3_1_2.py`:
- Around line 31-60: The migration currently only looks for undated bases after
the dated rule (existing_rules[idx + 1:]) and deduplicates substitutes only by
(absent_signer, substitute_signer), which causes missing substitutes when the
undated base is earlier or when the same pair has multiple date windows; update
the search over existing_rules to scan the whole list (exclude the same entry)
and pick the best candidate (e.g., the nearest by index or first match) instead
of only later entries, ensure you don't create a substitute with
absent_signer=None (skip if no candidate found), and change existing_pairs to
include the date bounds when deduping (use a key like (absent_signer,
substitute_signer, valid_from, valid_until) rather than just (absent_signer,
substitute_signer)) so multiple dated windows for the same pair are preserved
(refer to variables/functions: existing_rules, substitutes, idx, candidate,
pair, existing_pairs, new_substitutes_count).
In `@imio/dms/mail/steps.py`:
- Around line 137-149: The code adds "ISigningBehavior.signers" into the local
omf list but only persists changes to the registry when "ISigningBehavior.esign"
is also inserted; modify the logic in activate_esigning (or the block
manipulating omf/om_fns) so that after inserting the "ISigningBehavior.signers"
entry you call
api.portal.set_registry_record("imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_fields",
omf) to persist the updated omf; keep using the same omf/om_fns variables and
the existing insertion position logic (pos =
om_fns.index("internal_reference_no")) and ensure you still insert and recompute
om_fns before persisting.
🪄 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: 31721781-813f-440f-b1cf-6f9ca25e6a83
📒 Files selected for processing (11)
imio/dms/mail/adapters.pyimio/dms/mail/browser/iconified_category.pyimio/dms/mail/browser/views.pyimio/dms/mail/dmsmail.pyimio/dms/mail/migrations/migrate_to_3_1_2.pyimio/dms/mail/profiles/default/workflows/outgoingmail_workflow/definition.xmlimio/dms/mail/steps.pyimio/dms/mail/tests/test_dmsmail.pyimio/dms/mail/tests/test_wfadaptations.pyimio/dms/mail/utils.pyimio/dms/mail/wfadaptations.py
🚧 Files skipped from review as they are similar to previous changes (2)
- imio/dms/mail/tests/test_dmsmail.py
- imio/dms/mail/wfadaptations.py
chris-adam
force-pushed
the
PARAF-403/to_print_wfa
branch
from
47452a2 to
a5c0ff1
Compare
Contributor
Author
|
@sgeulette Voilà, c'est prêt pour review :) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
imio/dms/mail/migrations/migrate_to_3_1_2.py (2)
27-29:⚠️ Potential issue | 🟠 Major
valid_until-only legacy rules are treated as already migrated.This fast-path only checks for
valid_from, but the conversion below handlesvalid_from or valid_until. An instance that only stored an end date will skip the migration and keep pre-3.1.2 signer rules.🛠️ Suggested fix
- if existing_rules and not any("valid_from" in rule for rule in existing_rules): + if existing_rules and not any( + ("valid_from" in rule) or ("valid_until" in rule) + for rule in existing_rules + ): logger.info("omail_signer_rules already migrated, skipping")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/migrations/migrate_to_3_1_2.py` around lines 27 - 29, The fast-path mistakenly treats rules with only a valid_until as already migrated because it only checks for "valid_from"; update the condition that uses existing_rules (the line with not any("valid_from" in rule for rule in existing_rules)) to require neither "valid_from" nor "valid_until" is present (e.g., not any(("valid_from" in r) or ("valid_until" in r) for r in existing_rules)) so rules that have only an end date will not skip the migration, and keep the logger.info("omail_signer_rules already migrated, skipping") behavior for true fully-migrated cases.
34-58:⚠️ Potential issue | 🟠 MajorDon't create substitute entries without an absent signer.
This matching logic only scans later rules and still appends a substitute when no matching undated signer is found. On reordered or partially cleaned registries, the upgrade drops the dated rule from
omail_signer_rulesand writesabsent_signer=None.🛠️ Suggested fix
for idx, rule in enumerate(existing_rules): - rule = dict(rule) + original_rule = dict(rule) + rule = dict(original_rule) valid_from = rule.pop("valid_from", None) valid_until = rule.pop("valid_until", None) if valid_from or valid_until: # Find the first undated rule after this one with same conditions absent_signer = None - for candidate in existing_rules[idx + 1:]: + for candidate in existing_rules: if (not candidate.get("valid_from") and not candidate.get("valid_until") and candidate["signer"] != rule["signer"] and candidate["number"] == rule["number"] and candidate["treating_groups"] == rule["treating_groups"] and candidate["mail_types"] == rule["mail_types"] and candidate["send_modes"] == rule["send_modes"] and candidate["tal_condition"] == rule["tal_condition"]): absent_signer = candidate["signer"] break + if absent_signer is None: + cleaned_rules.append(original_rule) + logger.warning( + "Could not infer absent signer for dated rule {}".format(original_rule) + ) + continue pair = (absent_signer, rule["signer"]) if pair not in existing_pairs: substitutes.append({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/migrations/migrate_to_3_1_2.py` around lines 34 - 58, The migration loop over existing_rules can append a substitute with absent_signer=None when no undated candidate is found; update the logic in the block that builds the substitute (inside the for idx, rule in enumerate(existing_rules) loop) so you only create/append a substitute dict and add to substitutes/existing_pairs if absent_signer is not None (i.e., after the inner candidate scan check that a matching undated signer was actually found); reference variables: existing_rules, candidate, absent_signer, substitutes, existing_pairs, and the pair tuple to gate the append.imio/dms/mail/wfadaptations.py (1)
1015-1025:⚠️ Potential issue | 🟠 Major
to_approvestill exposes the direct sign transition.
imio/dms/mail/dmsmail.py:921-932showscan_be_signed()only checks file presence and whether approvals are all done. Because these branches only addset_to_printand never removepropose_to_be_signedfromto_approve, the last approver can still skip theto_printstep in both apply orders.🛠️ Suggested fix
- transitions = ["propose_to_be_signed", "back_to_creation"] + transitions = ["back_to_creation"] # what is already applied ? already_applied = "" applied_wfa = [dic["adaptation"] for dic in get_applied_adaptations()] if u"imio.dms.mail.wfadaptations.OMServiceValidation" in applied_wfa: already_applied = "n_plus" transitions.append("back_to_n_plus_1") transitions.append("back_to_validated") if u"imio.dms.mail.wfadaptations.OMToPrintAdaptation" in applied_wfa: already_applied = "to_print" transitions.append("set_to_print") + else: + transitions.insert(0, "propose_to_be_signed")if u"imio.dms.mail.wfadaptations.OMToApproveAdaptation" in applied_wfa: transitions.append("back_to_approve") # Add "set_to_print" from to_approve ta_state = wf.states["to_approve"] - ta_transitions = list(ta_state.transitions) + ta_transitions = [tr for tr in ta_state.transitions if tr != "propose_to_be_signed"] if to_tr_id not in ta_transitions: ta_transitions.append(to_tr_id) ta_state.transitions = tuple(ta_transitions)Also applies to: 1332-1339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/wfadaptations.py` around lines 1015 - 1025, The transitions list currently always contains "propose_to_be_signed" so the final approver can skip the to_print flow; update the logic in the block that builds transitions (using get_applied_adaptations(), applied_wfa and the transitions variable) so that when u"imio.dms.mail.wfadaptations.OMToPrintAdaptation" is present you remove or do not include "propose_to_be_signed" (and likewise ensure OMServiceValidation handling doesn't leave the direct sign transition if the to_print path is required); modify the two places mentioned (the block around transitions = ["propose_to_be_signed", "back_to_creation"] and the similar block at 1332-1339) to conditionally omit "propose_to_be_signed" when OMToPrintAdaptation is applied and keep adding only the appropriate back_to_* and set_to_print transitions.
♻️ Duplicate comments (1)
imio/dms/mail/utils.py (1)
1533-1535:⚠️ Potential issue | 🟠 MajorPick the post-approval branch under Reviewer role.
portal_workflow.getTransitionsFor()is permission-filtered. From a caller that does not currently have Reviewer rights, this helper can missset_to_printand fall back topropose_to_be_signed, so the next state depends on the caller instead of the workflow.🔧 Suggested fix
def get_post_approval_transition(context): """Return the transition to trigger when all approvals are done. When OMToPrintAdaptation is applied (handsigned flow), the mail goes to to_print after approval instead of directly to to_be_signed. """ pw = api.portal.get().portal_workflow - trs = [tr['id'] for tr in pw.getTransitionsFor(context)] + with api.env.adopt_roles(["Reviewer"]): + trs = [tr["id"] for tr in pw.getTransitionsFor(context)] if "set_to_print" in trs: return "set_to_print" return "propose_to_be_signed"Run this to confirm the helper is still resolved outside the elevated role path and inspect the surrounding approval flow:
#!/bin/bash set -e rg -n -C4 --type py '\bget_post_approval_transition\s*\(' imio/dms/mail rg -n -C12 --type py 'def approve_file|adopt_roles|get_post_approval_transition|api\.content\.transition|doActionFor' imio/dms/mail/adapters.pyExpected result:
get_post_approval_transition()is evaluated before the code path that elevates to Reviewer for the actual workflow transition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/utils.py` around lines 1533 - 1535, The code uses portal_workflow.getTransitionsFor(context) (pw variable) which is permission-filtered and may not show "set_to_print" for callers without Reviewer rights, causing get_post_approval_transition behavior to depend on caller permissions; change the logic to resolve the post-approval transition outside the limited-caller path by invoking the existing helper get_post_approval_transition (or evaluate transitions under an elevated Reviewer role) before any role elevation or state-changing branch, ensuring you check for "set_to_print" vs "propose_to_be_signed" using an unfiltered transition list so the decision does not depend on the current caller's permissions.
🧹 Nitpick comments (4)
imio/dms/mail/tests/test_wfadaptations.py (4)
416-428: Consider using direct attribute access andnext()for cleaner code.Static analysis flagged minor style improvements:
- Line 417: Use
fti.localrolesinstead ofgetattr(fti, "localroles").- Line 427: Use
next(...)instead of list comprehension with[0]slice.♻️ Proposed refactor
- fti = getUtility(IDexterityFTI, name="dmsoutgoingmail") - lr = getattr(fti, "localroles") + fti = getUtility(IDexterityFTI, name="dmsoutgoingmail") + lr = fti.localroles- self.assertIn( - "to_approve", - [dic["v"] for dic in folder["om_treating"].query if dic["i"] == "review_state"][0], - ) + review_state_values = next(dic["v"] for dic in folder["om_treating"].query if dic["i"] == "review_state") + self.assertIn("to_approve", review_state_values)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/tests/test_wfadaptations.py` around lines 416 - 428, Replace the getattr(fti, "localroles") call with direct attribute access fti.localroles and simplify the final assertion by using next(...) to find the first dict in folder["om_treating"].query where dic["i"] == "review_state" and then assert "to_approve" is in that dict's "v" list (use next((dic for dic in folder["om_treating"].query if dic["i"] == "review_state"), None) to get the dict and handle it directly).
341-359: Duplicate tests:test_OMToPrintAdaptationBeforeNp1andtest_OMToPrintAdaptationAfterNp1are identical.Both tests apply the same
imiodmsmail-om_to_print_wfadaptationprofile twice and callself.common_tests(). The names suggest different scenarios (before/after N+1) but the implementations are identical. Consider removing one or clarifying their distinct purposes if they should test different scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/tests/test_wfadaptations.py` around lines 341 - 359, The two test methods test_OMToPrintAdaptationBeforeNp1 and test_OMToPrintAdaptationAfterNp1 are identical; remove or differentiate them so they test distinct scenarios: either delete one of the duplicated methods (keeping the other calling self.common_tests()) or modify one to apply the appropriate different profile or setup for the "before"/"after N+1" scenario (e.g., change the second runImportStepFromProfile call or preceding setup in test_OMToPrintAdaptationAfterNp1) so the methods exercise different states while still invoking self.common_tests(); ensure the unique profile id "imiodmsmail-om_to_print_wfadaptation" remains correct for the intended scenario.
493-511: Duplicate tests:test_OMToApproveAdaptationBeforeNp1andtest_OMToApproveAdaptationAfterNp1are identical.Same issue as in
TestOMToPrintAdaptation- both methods applyimiodmsmail-om_to_approve_wfadaptationtwice and callself.common_tests(). Consider consolidating into a single idempotency test or differentiating the scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/tests/test_wfadaptations.py` around lines 493 - 511, These two tests are duplicates: test_OMToApproveAdaptationBeforeNp1 and test_OMToApproveAdaptationAfterNp1 both run the same import step twice and call self.common_tests(); consolidate by keeping a single idempotency test (e.g., test_OMToApproveAdaptation_idempotent) that runs imiodmsmail-om_to_approve_wfadaptation twice and calls self.common_tests(), or if you intended to test a different scenario for "AfterNp1" change the second test to run the distinct adaptation or setup that reflects the "AfterNp1" condition and/or assert the differing expected state instead of duplicating the same steps.
485-486: Consider usingnext()instead of slice.Same pattern as line 427 - static analysis suggests using
next()over single-element slice.♻️ Proposed refactor
- res = [dic["v"] for dic in folder["om_treating"].query if dic["i"] == "review_state"][0] - self.assertIn("to_approve", res) + res = next(dic["v"] for dic in folder["om_treating"].query if dic["i"] == "review_state") + self.assertIn("to_approve", res)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imio/dms/mail/tests/test_wfadaptations.py` around lines 485 - 486, Replace the single-element slice pattern that retrieves the first matching dict from folder["om_treating"].query with a next() call: iterate/filter folder["om_treating"].query for dic["i"] == "review_state" and use next(...) to get the first matching element (with either a default or letting it raise if absent) before accessing ["v"]; update the line that assigns res (and the similar occurrence near line 427) to use next() instead of [0] to avoid creating a temporary list and to be more idiomatic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@imio/dms/mail/migrations/migrate_to_3_1_2.py`:
- Around line 27-29: The fast-path mistakenly treats rules with only a
valid_until as already migrated because it only checks for "valid_from"; update
the condition that uses existing_rules (the line with not any("valid_from" in
rule for rule in existing_rules)) to require neither "valid_from" nor
"valid_until" is present (e.g., not any(("valid_from" in r) or ("valid_until" in
r) for r in existing_rules)) so rules that have only an end date will not skip
the migration, and keep the logger.info("omail_signer_rules already migrated,
skipping") behavior for true fully-migrated cases.
- Around line 34-58: The migration loop over existing_rules can append a
substitute with absent_signer=None when no undated candidate is found; update
the logic in the block that builds the substitute (inside the for idx, rule in
enumerate(existing_rules) loop) so you only create/append a substitute dict and
add to substitutes/existing_pairs if absent_signer is not None (i.e., after the
inner candidate scan check that a matching undated signer was actually found);
reference variables: existing_rules, candidate, absent_signer, substitutes,
existing_pairs, and the pair tuple to gate the append.
In `@imio/dms/mail/wfadaptations.py`:
- Around line 1015-1025: The transitions list currently always contains
"propose_to_be_signed" so the final approver can skip the to_print flow; update
the logic in the block that builds transitions (using get_applied_adaptations(),
applied_wfa and the transitions variable) so that when
u"imio.dms.mail.wfadaptations.OMToPrintAdaptation" is present you remove or do
not include "propose_to_be_signed" (and likewise ensure OMServiceValidation
handling doesn't leave the direct sign transition if the to_print path is
required); modify the two places mentioned (the block around transitions =
["propose_to_be_signed", "back_to_creation"] and the similar block at 1332-1339)
to conditionally omit "propose_to_be_signed" when OMToPrintAdaptation is applied
and keep adding only the appropriate back_to_* and set_to_print transitions.
---
Duplicate comments:
In `@imio/dms/mail/utils.py`:
- Around line 1533-1535: The code uses
portal_workflow.getTransitionsFor(context) (pw variable) which is
permission-filtered and may not show "set_to_print" for callers without Reviewer
rights, causing get_post_approval_transition behavior to depend on caller
permissions; change the logic to resolve the post-approval transition outside
the limited-caller path by invoking the existing helper
get_post_approval_transition (or evaluate transitions under an elevated Reviewer
role) before any role elevation or state-changing branch, ensuring you check for
"set_to_print" vs "propose_to_be_signed" using an unfiltered transition list so
the decision does not depend on the current caller's permissions.
---
Nitpick comments:
In `@imio/dms/mail/tests/test_wfadaptations.py`:
- Around line 416-428: Replace the getattr(fti, "localroles") call with direct
attribute access fti.localroles and simplify the final assertion by using
next(...) to find the first dict in folder["om_treating"].query where dic["i"]
== "review_state" and then assert "to_approve" is in that dict's "v" list (use
next((dic for dic in folder["om_treating"].query if dic["i"] == "review_state"),
None) to get the dict and handle it directly).
- Around line 341-359: The two test methods test_OMToPrintAdaptationBeforeNp1
and test_OMToPrintAdaptationAfterNp1 are identical; remove or differentiate them
so they test distinct scenarios: either delete one of the duplicated methods
(keeping the other calling self.common_tests()) or modify one to apply the
appropriate different profile or setup for the "before"/"after N+1" scenario
(e.g., change the second runImportStepFromProfile call or preceding setup in
test_OMToPrintAdaptationAfterNp1) so the methods exercise different states while
still invoking self.common_tests(); ensure the unique profile id
"imiodmsmail-om_to_print_wfadaptation" remains correct for the intended
scenario.
- Around line 493-511: These two tests are duplicates:
test_OMToApproveAdaptationBeforeNp1 and test_OMToApproveAdaptationAfterNp1 both
run the same import step twice and call self.common_tests(); consolidate by
keeping a single idempotency test (e.g., test_OMToApproveAdaptation_idempotent)
that runs imiodmsmail-om_to_approve_wfadaptation twice and calls
self.common_tests(), or if you intended to test a different scenario for
"AfterNp1" change the second test to run the distinct adaptation or setup that
reflects the "AfterNp1" condition and/or assert the differing expected state
instead of duplicating the same steps.
- Around line 485-486: Replace the single-element slice pattern that retrieves
the first matching dict from folder["om_treating"].query with a next() call:
iterate/filter folder["om_treating"].query for dic["i"] == "review_state" and
use next(...) to get the first matching element (with either a default or
letting it raise if absent) before accessing ["v"]; update the line that assigns
res (and the similar occurrence near line 427) to use next() instead of [0] to
avoid creating a temporary list and to be more idiomatic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdb00368-2d65-424b-97dc-7c839b0b18ef
📒 Files selected for processing (11)
imio/dms/mail/adapters.pyimio/dms/mail/browser/iconified_category.pyimio/dms/mail/browser/views.pyimio/dms/mail/dmsmail.pyimio/dms/mail/migrations/migrate_to_3_1_2.pyimio/dms/mail/profiles/default/workflows/outgoingmail_workflow/definition.xmlimio/dms/mail/steps.pyimio/dms/mail/tests/test_dmsmail.pyimio/dms/mail/tests/test_wfadaptations.pyimio/dms/mail/utils.pyimio/dms/mail/wfadaptations.py
🚧 Files skipped from review as they are similar to previous changes (4)
- imio/dms/mail/browser/iconified_category.py
- imio/dms/mail/tests/test_dmsmail.py
- imio/dms/mail/adapters.py
- imio/dms/mail/dmsmail.py
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
1 participant
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.
J'ai branché cette PR sur la branche PARAF-402 parce que la nouvelle migration a été écrite là-bas...
Voir commentaire sur https://my.support.imio.be/browse/PARAF-403
Summary by CodeRabbit
Bug Fixes
Refactor
Tests