Added action to create a custom session + action to add a item to a specific session#39
Open
chris-adam wants to merge 3 commits into
Open
Added action to create a custom session + action to add a item to a specific session#39chris-adam wants to merge 3 commits into
chris-adam wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAdds two related features: a "create custom session" flow (vocabulary, z3c.form, JS/CSS overlay, registry entries, permission-gated link, tests, changelog) and an "add to custom session" overlay action (filtered sessions table, view, template, and tests). ChangesCustom e-Sign Session Creation
Add-to Custom Session Action
Sequence DiagramsequenceDiagram
participant User
participant CreateCustomSessionForm
participant uuidToObject
participant api.user.get
participant create_session
participant Portal
User->>CreateCustomSessionForm: submit (signers, title, seal)
CreateCustomSessionForm->>uuidToObject: resolve HP UIDs
CreateCustomSessionForm->>api.user.get: fetch user info
CreateCustomSessionForm->>create_session: create_session(signers, seal, title)
create_session-->>CreateCustomSessionForm: session created
CreateCustomSessionForm->>Portal: redirect @@parapheo
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
src/imio/esign/browser/forms.py (1)
109-113: ⚡ Quick winStabilize signer order before building the payload.
Selected signers come from a
Set, so iteration order is not stable. Sorting here makescreate_session()input deterministic.💡 Proposed fix
- for value in data.get("signers", []): + for value in sorted(data.get("signers", [])): info = self.extract_signer_info(value) if info is not None: signers.append(info)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/esign/browser/forms.py` around lines 109 - 113, The signer extraction loop in forms.py iterates over data.get("signers", []) which may be a Set with unstable order; to make create_session() input deterministic, convert and iterate over a stable sorted sequence (e.g., sorted(data.get("signers", []))) before calling extract_signer_info, appending results to signers; keep using the existing extract_signer_info and signers list so only the iteration source is changed to a deterministic order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/imio/esign/browser/forms.py`:
- Around line 95-99: The updateWidgets method in CreateCustomSessionForm
unconditionally sets self.widgets["title"].value to get_default_title(), which
overwrites any user-submitted title on re-render after validation errors; change
it to only assign the default when the widget has no submitted value (e.g.,
check if not self.widgets["title"].value or similar) so existing user input is
preserved, while leaving the existing seal logic (get_default_seal()) intact.
In `@src/imio/esign/tests/test_browser_forms.py`:
- Around line 121-127: The test branch that calls self._call_handleCreate(...
title: u"") fails to clear status messages at the end like the other branches,
which can leak messages into later tests; add the same final status-message
cleanup used earlier in this test file (clear the request's status messages for
the request object used by the test, e.g. the same cleanup call you used in
previous branches) after asserting session["title"] to ensure test isolation for
subsequent tests.
In `@src/imio/esign/vocabularies.py`:
- Around line 27-33: Trim the raw email before checking emptiness so
whitespace-only values are rejected: when reading the property from user
(currently assigning to email from user.getProperty("email", "")), strip
whitespace from that value first (e.g., assign a trimmed value to email_raw then
email = email_raw.strip()), perform the emptiness check on the trimmed email,
and then call safe_unicode on the trimmed email before building title (ensure
variables referenced are the same ones used in the current block:
user.getProperty, email, userid, fullname, title).
---
Nitpick comments:
In `@src/imio/esign/browser/forms.py`:
- Around line 109-113: The signer extraction loop in forms.py iterates over
data.get("signers", []) which may be a Set with unstable order; to make
create_session() input deterministic, convert and iterate over a stable sorted
sequence (e.g., sorted(data.get("signers", []))) before calling
extract_signer_info, appending results to signers; keep using the existing
extract_signer_info and signers list so only the iteration source is changed to
a deterministic order.
🪄 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: 0caaf84a-377b-441d-ae4b-ca5bc2eaee9d
📒 Files selected for processing (15)
CHANGES.rstsetup.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/forms.pysrc/imio/esign/browser/static/esign.csssrc/imio/esign/browser/static/esign.jssrc/imio/esign/browser/templates/sessions.ptsrc/imio/esign/browser/views.pysrc/imio/esign/configure.zcmlsrc/imio/esign/profiles/default/cssregistry.xmlsrc/imio/esign/profiles/default/jsregistry.xmlsrc/imio/esign/tests/base.pysrc/imio/esign/tests/test_browser_forms.pysrc/imio/esign/tests/test_browser_views.pysrc/imio/esign/vocabularies.py
Coverage Report for CI Build 26218550308Coverage decreased (-0.5%) to 84.15%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
chris-adam
force-pushed
the
PARAF-370/create-custom-session
branch
from
69f7c29 to
9039b53
Compare
chris-adam
commented
May 20, 2026
Contributor
Author
chris-adam
left a comment
chris-adam
left a comment
There was a problem hiding this comment.
Est-ce que je garde le css ici ou je déplace dans la skin iMio ?
Pour les implémentations de extract_signer_info et ActiveSignersVocabulary, je ne suis pas sûr. Soit je fais une implémentation par défaut avec les users Plone (mais on devra overwrite ça dans docs et délib), soit je fais des implémentation vides (idem), soit je fais la vraie implémentation (mais collective.contact n'est pas en dépendance donc ça complique les tests)
| """Return the default value for the title field. | ||
| Override in a subclass to change the default. | ||
| """ | ||
| return _(u"Custom session") |
Contributor
Author
There was a problem hiding this comment.
A voir si on veut mettre un titre par défaut particulier
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/imio/esign/browser/forms.py`:
- Around line 92-95: The signer-extraction code that sets email =
user.getProperty(...) can produce empty emails; add a defensive guard after
computing email: if not email, log or ignore and return None (or otherwise skip
this signer) so empty-email entries are not included in the signers payload
passed to create_session. Update the caller that collects signers to filter out
falsy/None results before building the list given to create_session so only
signers with non-empty email, e.g. from person.get_title and hp.get_full_title,
are included.
🪄 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: 7e0b7bbd-40e7-43ed-9d0d-eadacaf12bd3
📒 Files selected for processing (15)
CHANGES.rstsetup.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/forms.pysrc/imio/esign/browser/static/esign.csssrc/imio/esign/browser/static/esign.jssrc/imio/esign/browser/templates/sessions.ptsrc/imio/esign/browser/views.pysrc/imio/esign/configure.zcmlsrc/imio/esign/profiles/default/cssregistry.xmlsrc/imio/esign/profiles/default/jsregistry.xmlsrc/imio/esign/tests/base.pysrc/imio/esign/tests/test_browser_forms.pysrc/imio/esign/tests/test_browser_views.pysrc/imio/esign/vocabularies.py
✅ Files skipped from review due to trivial changes (4)
- CHANGES.rst
- src/imio/esign/profiles/default/jsregistry.xml
- src/imio/esign/browser/static/esign.css
- src/imio/esign/profiles/default/cssregistry.xml
Comment on lines
+92
to
+95
| email = user.getProperty("email", "") | ||
| fullname = person.get_title(include_person_title=False) | ||
| position = hp.get_full_title(first_index=1) | ||
| return (person.userid, email, fullname, position) |
There was a problem hiding this comment.
Add a defensive empty-email guard in signer extraction.
On Line 92, email can still be empty in edge cases (stale/tampered submit), and currently gets passed to create_session. Skip such signer entries to keep payload integrity.
💡 Proposed fix
- email = user.getProperty("email", "")
+ email = (user.getProperty("email", u"") or u"").strip()
+ if not email:
+ return None
fullname = person.get_title(include_person_title=False)
position = hp.get_full_title(first_index=1)
return (person.userid, email, fullname, position)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/imio/esign/browser/forms.py` around lines 92 - 95, The signer-extraction
code that sets email = user.getProperty(...) can produce empty emails; add a
defensive guard after computing email: if not email, log or ignore and return
None (or otherwise skip this signer) so empty-email entries are not included in
the signers payload passed to create_session. Update the caller that collects
signers to filter out falsy/None results before building the list given to
create_session so only signers with non-empty email, e.g. from person.get_title
and hp.get_full_title, are included.
Added action to add a item to a specific session
chris-adam
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/imio/esign/tests/test_actions.py (1)
374-383: ⚡ Quick winIdempotency assertion is incomplete.
This block should also assert the file appears only once in the target session; otherwise duplicate inserts can slip through while the test still passes.
Suggested test assertion
view() self.assertEqual(annot["uids"][self.annexes[0].UID()], sid_other) self.assertIn(sid_other, annot["sessions"]) + file_uids_same = [f["uid"] for f in annot["sessions"][sid_other]["files"]] + self.assertEqual(file_uids_same.count(self.annexes[0].UID()), 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/esign/tests/test_actions.py` around lines 374 - 383, The idempotency test for AddToCustomEsignSessionView is incomplete: after re-adding the same file to sid_other you must also assert the file appears only once in that session to prevent duplicates; update the test block using AddToCustomEsignSessionView(self.annexes[0], self.request) to include an assertion that counts occurrences (e.g., ensure annot["sessions"].count(sid_other) == 1 or that the annex UID appears only once in the session membership for sid_other) so the target session contains a single instance of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/imio/esign/browser/static/esign.js`:
- Around line 32-36: initSubmitToggle currently re-attaches a change handler
every time the overlay reloads, causing duplicate listeners to stack; update
initSubmitToggle to prevent duplicates by removing the previous handler (e.g.,
call .off('change') with the same selector or use a namespaced event) before
attaching the new handler for '`#add-to-custom-esign-session-form`
input[name="session_id"]', or switch to a delegated single listener bound once
at a higher container; ensure the same fix is applied to the other handler
referenced at lines 38-39 so handlers are not added repeatedly across
loadInsideOverlay cycles.
- Around line 63-65: The URL for $pbAjax.load is built by blindly appending
'?ajax_load=...' to sessionListUrl which breaks when sessionListUrl already
contains query params; change the construction to detect whether sessionListUrl
contains '?' (using sessionListUrl.includes('?') or similar) and choose '&' or
'?' as the separator, then append 'ajax_load=' + (new Date().getTime()) and the
' `#content`>*' fragment when calling $pbAjax.load (symbols: $pbAjax.load,
sessionListUrl, ajax_load).
In `@src/imio/esign/browser/templates/add_to_custom_esign_session.pt`:
- Around line 37-39: The Cancel control is a plain <input type="button"
name="form.buttons.cancel" class="standalone"> with no handler, so wire it to a
deterministic exit path: replace the button type with a submit (input
type="submit" name="form.buttons.cancel" value="Cancel") so the form handler can
detect the "form.buttons.cancel" submission server-side and perform the
cancel/redirect logic; alternatively, if you need a client-side immediate exit,
add an explicit onclick that navigates to a provided cancel_url (e.g.
onclick="window.location.href=cancel_url") and ensure the template exposes
cancel_url and keeps the i18n:attributes="value".
---
Nitpick comments:
In `@src/imio/esign/tests/test_actions.py`:
- Around line 374-383: The idempotency test for AddToCustomEsignSessionView is
incomplete: after re-adding the same file to sid_other you must also assert the
file appears only once in that session to prevent duplicates; update the test
block using AddToCustomEsignSessionView(self.annexes[0], self.request) to
include an assertion that counts occurrences (e.g., ensure
annot["sessions"].count(sid_other) == 1 or that the annex UID appears only once
in the session membership for sid_other) so the target session contains a single
instance of the file.
🪄 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: 4cdba591-a1db-4d95-ac03-ce068e633abc
📒 Files selected for processing (9)
CHANGES.rstsrc/imio/esign/browser/actions.pysrc/imio/esign/browser/configure.zcmlsrc/imio/esign/browser/static/esign.csssrc/imio/esign/browser/static/esign.jssrc/imio/esign/browser/table.pysrc/imio/esign/browser/templates/add_to_custom_esign_session.ptsrc/imio/esign/tests/test_actions.pysrc/imio/esign/tests/test_table.py
✅ Files skipped from review due to trivial changes (1)
- CHANGES.rst
Comment on lines
+32
to
+36
| function initSubmitToggle(container) { | ||
| var $container = $(container || document); | ||
| $container.find('#add-to-custom-esign-session-form input[name="session_id"]').on('change', function() { | ||
| $(this).closest('form').find('#esign-submit-btn').prop('disabled', false); | ||
| }); |
There was a problem hiding this comment.
Avoid stacking change listeners across overlay reloads.
Line 34 re-attaches handlers each loadInsideOverlay, so listeners can accumulate on repeated refresh/open cycles.
Suggested fix
function initSubmitToggle(container) {
var $container = $(container || document);
- $container.find('`#add-to-custom-esign-session-form` input[name="session_id"]').on('change', function() {
- $(this).closest('form').find('`#esign-submit-btn`').prop('disabled', false);
- });
+ $container.find('`#add-to-custom-esign-session-form`')
+ .off('change.esignSession', 'input[name="session_id"]')
+ .on('change.esignSession', 'input[name="session_id"]', function() {
+ $(this).closest('form').find('`#esign-submit-btn`').prop('disabled', false);
+ });
}Also applies to: 38-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/imio/esign/browser/static/esign.js` around lines 32 - 36,
initSubmitToggle currently re-attaches a change handler every time the overlay
reloads, causing duplicate listeners to stack; update initSubmitToggle to
prevent duplicates by removing the previous handler (e.g., call .off('change')
with the same selector or use a namespaced event) before attaching the new
handler for '`#add-to-custom-esign-session-form` input[name="session_id"]', or
switch to a delegated single listener bound once at a higher container; ensure
the same fix is applied to the other handler referenced at lines 38-39 so
handlers are not added repeatedly across loadInsideOverlay cycles.
Comment on lines
+63
to
+65
| $pbAjax.load( | ||
| sessionListUrl + '?ajax_load=' + (new Date().getTime()) + ' #content>*', | ||
| function() { |
There was a problem hiding this comment.
Build refresh URL safely when an action already has query params.
Line 64 always appends ?ajax_load=...; if sessionListUrl already includes ?, the generated URL is malformed.
Suggested fix
- $pbAjax.load(
- sessionListUrl + '?ajax_load=' + (new Date().getTime()) + ' `#content`>*',
+ var sep = sessionListUrl.indexOf('?') === -1 ? '?' : '&';
+ var refreshUrl = sessionListUrl + sep + 'ajax_load=' + Date.now();
+ $pbAjax.load(
+ refreshUrl + ' `#content`>*',
function() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/imio/esign/browser/static/esign.js` around lines 63 - 65, The URL for
$pbAjax.load is built by blindly appending '?ajax_load=...' to sessionListUrl
which breaks when sessionListUrl already contains query params; change the
construction to detect whether sessionListUrl contains '?' (using
sessionListUrl.includes('?') or similar) and choose '&' or '?' as the separator,
then append 'ajax_load=' + (new Date().getTime()) and the ' `#content`>*' fragment
when calling $pbAjax.load (symbols: $pbAjax.load, sessionListUrl, ajax_load).
Comment on lines
+37
to
+39
| <input type="button" class="standalone" name="form.buttons.cancel" | ||
| value="Cancel" | ||
| i18n:attributes="value" /> |
There was a problem hiding this comment.
Cancel button has no action.
The current cancel control does nothing (type="button" without handler). Link it to a deterministic exit path.
Suggested fix
- <input type="button" class="standalone" name="form.buttons.cancel"
- value="Cancel"
- i18n:attributes="value" />
+ <a class="standalone"
+ tal:attributes="href context/absolute_url"
+ i18n:translate="">
+ Cancel
+ </a>📝 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
| <input type="button" class="standalone" name="form.buttons.cancel" | |
| value="Cancel" | |
| i18n:attributes="value" /> | |
| <a class="standalone" | |
| tal:attributes="href context/absolute_url" | |
| i18n:translate=""> | |
| Cancel | |
| </a> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/imio/esign/browser/templates/add_to_custom_esign_session.pt` around lines
37 - 39, The Cancel control is a plain <input type="button"
name="form.buttons.cancel" class="standalone"> with no handler, so wire it to a
deterministic exit path: replace the button type with a submit (input
type="submit" name="form.buttons.cancel" value="Cancel") so the form handler can
detect the "form.buttons.cancel" submission server-side and perform the
cancel/redirect logic; alternatively, if you need a client-side immediate exit,
add an explicit onclick that navigates to a provided cancel_url (e.g.
onclick="window.location.href=cancel_url") and ensure the template exposes
cancel_url and keeps the i18n:attributes="value".
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
UI/UX
Styling & Behavior
Documentation
Tests
Dependencies