Conversation
gbastien
left a comment
gbastien
left a comment
There was a problem hiding this comment.
Magnifico :-)
Peut-être ajouter un test à ce sujet? :-)
gbastien
left a comment
gbastien
left a comment
There was a problem hiding this comment.
Et supprimer les traductions devenues inutiles
|
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:
📝 WalkthroughWalkthroughReplace user-facing redirects/messages with raising Unauthorized in two session views; add handling for missing seal email in external session creation (early-return sentinel); add tests for views and utils; update translation catalogs (remove two permission messages, add seal-email message); add test dependency and minor import cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant View as SessionView
participant Perm as PermissionChecker
participant Utils as create_external_session
participant ExtAPI as ExternalSigningAPI
participant Logger
Client->>View: POST /external-session-create (session_id)
View->>Perm: may_create_external_sessions(session)
alt permission denied
View-->>Client: raise Unauthorized (401)
else permission granted
View->>Utils: create_external_session(session)
alt session requires seal but no seal_email
Utils->>Logger: log error "no seal email"
Utils-->>View: return "_no_seal_email_"
View->>Client: set status message / redirect (error)
else normal path
Utils->>ExtAPI: send payload (signers/seal)
ExtAPI-->>Utils: response (success or error code)
Utils-->>View: result sentinel or data
View->>Client: success redirect or error message
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
chris-adam
force-pushed
the
may_manage_session
branch
from
aa628d7 to
4c919ba
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/esign/tests/test_browser_views.py`:
- Around line 42-44: The test currently passes a hardcoded password to
api.user.create(email="user1@sign.com", username="user1", password="password1"),
which triggers Ruff S106; update the test to avoid a static secret by generating
a password at runtime (e.g., using secrets.token_urlsafe or uuid4) and pass that
variable into api.user.create, or if you intentionally want a fixed value for
the test, add an inline suppression comment "# noqa: S106" on the
api.user.create line; reference the api.user.create(...) call in
test_browser_views.py when applying the change.
- Around line 3-16: The tests import Mock and patch from the external mock
package which isn't a declared dependency; update the imports in
src/imio/esign/tests/test_browser_views.py to use the standard library by
replacing uses of Mock and patch from mock with unittest.mock (i.e., change the
two imports referencing Mock and patch to import them from unittest.mock) so
functions/classes like Mock and patch used in the file (and any tests
referencing them) use the stdlib implementation.
|
Je me rends compte que mock n'est pas installé directement par imio.esign. Et ce n'est pas compatible python 3. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/esign/tests/test_browser_views.py`:
- Around line 37-103: The shared test layer's request.form isn't cleared between
tests causing cross-test leakage; update the TestCase setUp (the setUp method
that assigns self.request) to clear the request form at start (e.g., call
self.request.form.clear() or reset it to an empty mapping) so tests like
test_call_no_session_id that expect no session keys won't be affected by prior
tests setting request.form["session_id"] or request.form["esign_session_id"].
---
Duplicate comments:
In `@src/imio/esign/tests/test_browser_views.py`:
- Around line 15-16: Replace the external "mock" package imports with the
standard library by changing "from mock import Mock" and "from mock import
patch" to use "unittest.mock" equivalents; update any usages of Mock and patch
in this test module (test_browser_views.py) to import Mock and patch from
unittest.mock so the tests remain self-contained and work on Python 3 without
the external dependency.
| def setUp(self): | ||
| self.portal = self.layer["portal"] | ||
| self.request = self.layer["request"] | ||
| setRoles(self.portal, TEST_USER_ID, ["Manager"]) | ||
|
|
||
| # Create user for signing | ||
| api.user.create(email="user1@sign.com", username="user1", password="password1") # noqa: S106 | ||
|
|
||
| # Create content category configuration | ||
| at_folder = api.content.create( | ||
| container=self.portal, | ||
| id="annexes_types", | ||
| title="Annexes Types", | ||
| type="ContentCategoryConfiguration", | ||
| exclude_from_nav=True, | ||
| ) | ||
| category_group = api.content.create( | ||
| type="ContentCategoryGroup", | ||
| title="Annexes", | ||
| container=at_folder, | ||
| id="annexes", | ||
| ) | ||
| icon_path = os.path.join( | ||
| os.path.dirname(collective.iconifiedcategory.__file__), "tests", "icône1.png" | ||
| ) | ||
| with open(icon_path, "rb") as fl: | ||
| api.content.create( | ||
| type="ContentCategory", | ||
| title="To sign", | ||
| container=category_group, | ||
| icon=NamedBlobImage(fl.read(), filename=u"icône1.png"), | ||
| id="to_sign", | ||
| predefined_title="To be signed", | ||
| to_sign=True, | ||
| show_preview=False, | ||
| ) | ||
|
|
||
| # Create folder and annex | ||
| self.folder = api.content.create( | ||
| container=self.portal, | ||
| type="Folder", | ||
| id="test_folder", | ||
| title="Test Folder", | ||
| ) | ||
| tests_dir = os.path.dirname(__file__) | ||
| with open(os.path.join(tests_dir, "annex1.pdf"), "rb") as f: | ||
| annex = api.content.create( | ||
| container=self.folder, | ||
| type="annex", | ||
| id="test_annex", | ||
| title="Test Annex", | ||
| content_category=calculate_category_id( | ||
| self.portal["annexes_types"]["annexes"]["to_sign"] | ||
| ), | ||
| scan_id="012345600000001", | ||
| file=NamedBlobFile( | ||
| data=f.read(), | ||
| filename=u"annex1.pdf", | ||
| contentType="application/pdf", | ||
| ), | ||
| ) | ||
| self.annex_uid = annex.UID() | ||
|
|
||
| # Seed a session in the annotation | ||
| signers = [("user1", "user1@sign.com", "User 1", "Position 1")] | ||
| self.session_id, _ = add_files_to_session(signers, (self.annex_uid,)) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find testing.py configuration and IMIO_ESIGN_INTEGRATION_TESTING definition
fd -a "testing.py" -x cat {}Repository: IMIO/imio.esign
Length of output: 1761
🏁 Script executed:
#!/bin/bash
# Search for all test methods in test_browser_views.py that set or access request.form
rg -n "request\.form\[" src/imio/esign/tests/test_browser_views.py -A 2 -B 2Repository: IMIO/imio.esign
Length of output: 1982
🏁 Script executed:
#!/bin/bash
# Search for session_id or esign_session_id references in test file
rg -n "session_id|esign_session_id" src/imio/esign/tests/test_browser_views.pyRepository: IMIO/imio.esign
Length of output: 1073
🏁 Script executed:
#!/bin/bash
# Look at test_call_no_session_id methods to see what they test
rg -n "def test_call_no_session_id" -A 10 src/imio/esign/tests/test_browser_views.pyRepository: IMIO/imio.esign
Length of output: 1236
🏁 Script executed:
#!/bin/bash
# Also check if there are any assertions about form being empty or keys not present
rg -n "assertNotIn.*form|assertFalse.*form|in self.request.form" src/imio/esign/tests/test_browser_views.py -B 2 -A 2Repository: IMIO/imio.esign
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the overall structure of test_browser_views.py to see test class definitions
rg -n "^class.*Test" src/imio/esign/tests/test_browser_views.pyRepository: IMIO/imio.esign
Length of output: 277
🏁 Script executed:
#!/bin/bash
# Check if there are multiple test classes that might share a request
rg -n "class.*TestCase|class.*Unittest" src/imio/esign/tests/test_browser_views.py -A 1Repository: IMIO/imio.esign
Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check what self.layer actually is - look for layer assignment in test class
rg -n "layer.*=" src/imio/esign/tests/test_browser_views.py -B 2 -A 2Repository: IMIO/imio.esign
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Search for IMIO_ESIGN_INTEGRATION_TESTING usage in the test file
rg -n "IMIO_ESIGN_INTEGRATION_TESTING" src/imio/esign/tests/test_browser_views.pyRepository: IMIO/imio.esign
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Let's check the entire beginning of the test file to see layer definition
head -40 src/imio/esign/tests/test_browser_views.pyRepository: IMIO/imio.esign
Length of output: 1524
🏁 Script executed:
#!/bin/bash
# Verify how Plone's IntegrationTesting handles request across test methods
# Check if there's any tearDown that might clean up the request
rg -n "def tearDown|def tearDownClass" src/imio/esign/tests/test_browser_views.py -A 3Repository: IMIO/imio.esign
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the test runner or conftest to see how tests are executed
fd "conftest.py|pytest.ini|setup.cfg" -x cat {}Repository: IMIO/imio.esign
Length of output: 466
🏁 Script executed:
#!/bin/bash
# Check Plone's IntegrationTesting behavior - look for documentation or comments
rg -n "IntegrationTesting" --type py | head -20Repository: IMIO/imio.esign
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Verify if request.form is a mutable dict that persists across test methods
# by looking at how it's used elsewhere in test file
rg -n "request.form" src/imio/esign/tests/test_browser_views.py | wc -lRepository: IMIO/imio.esign
Length of output: 60
Clear request.form in setUp to ensure test isolation.
The test classes reuse self.request from the shared layer across test methods. Multiple tests explicitly set request.form["session_id"] or request.form["esign_session_id"], while test_call_no_session_id tests expect these keys to be absent. Without clearing the form dict in setUp, test execution order affects behavior, potentially causing false positives or inconsistent results.
Proposed fix
def setUp(self):
self.portal = self.layer["portal"]
self.request = self.layer["request"]
+ self.request.form.clear()
+ IStatusMessage(self.request).show()
setRoles(self.portal, TEST_USER_ID, ["Manager"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/tests/test_browser_views.py` around lines 37 - 103, The shared
test layer's request.form isn't cleared between tests causing cross-test
leakage; update the TestCase setUp (the setUp method that assigns self.request)
to clear the request form at start (e.g., call self.request.form.clear() or
reset it to an empty mapping) so tests like test_call_no_session_id that expect
no session keys won't be affected by prior tests setting
request.form["session_id"] or request.form["esign_session_id"].
gbastien
left a comment
gbastien
left a comment
There was a problem hiding this comment.
J'ai ajouté "mock" aux requires pour [test], çà devrait être OK.
Pas mal de tests bien, mais à maintenir par la suite!
Pour moi c'est bon on peut merger.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
setup.py (1)
69-71: Replace externalmockwith stdlibunittest.mockin tests.The project targets Python 3.7+ (lines 30–32), where
unittest.mockis built-in. Externalmockimports are present insrc/imio/esign/tests/test_browser_views.py(lines 15–16). Migrating tounittest.mockreduces unnecessary dependency overhead while maintaining full API compatibility.Update the test imports and remove
"mock"fromextras_require["test"].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.py` around lines 69 - 71, Remove the external "mock" test dependency and switch to the stdlib mock: delete "mock" from extras_require["test"] in setup.py and update test imports in src/imio/esign/tests/test_browser_views.py by replacing any import of the external mock package (e.g., "import mock" or "from mock import ...") with the stdlib module (use "import unittest.mock as mock" or "from unittest import mock") so existing mock usage (patch, MagicMock, etc.) continues to work without the external dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@setup.py`:
- Around line 69-71: Remove the external "mock" test dependency and switch to
the stdlib mock: delete "mock" from extras_require["test"] in setup.py and
update test imports in src/imio/esign/tests/test_browser_views.py by replacing
any import of the external mock package (e.g., "import mock" or "from mock
import ...") with the stdlib module (use "import unittest.mock as mock" or "from
unittest import mock") so existing mock usage (patch, MagicMock, etc.) continues
to work without the external dependency.
Added _no_seal_email_ error + tests
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/imio/esign/tests/test_browser_views.py (2)
15-16:⚠️ Potential issue | 🟠 Major
mockimports are still unresolved in this file.This repeats the previously reported compatibility/dependency issue; use stdlib
unittest.mockwith fallback if Python 2 support is still required.🔧 Proposed fix
-from mock import Mock -from mock import patch +try: + from unittest.mock import Mock + from unittest.mock import patch +except ImportError: # Python 2 fallback + from mock import Mock + from mock import patch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/test_browser_views.py` around lines 15 - 16, Replace the direct third-party "mock" imports with the stdlib fallback pattern: attempt to import Mock and patch from unittest.mock and fall back to importing them from mock on ImportError, so update the top-level imports that currently read "from mock import Mock" and "from mock import patch" to a try/except that assigns Mock and patch from unittest.mock first and uses mock as fallback.
37-40:⚠️ Potential issue | 🟡 MinorTest isolation risk remains: clear shared request state in
setUp.
self.requestcomes from the shared layer; without resettingrequest.form(and status queue), tests can affect each other.🔧 Proposed fix
def setUp(self): self.portal = self.layer["portal"] self.request = self.layer["request"] + self.request.form.clear() + IStatusMessage(self.request).show() setRoles(self.portal, TEST_USER_ID, ["Manager"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/test_browser_views.py` around lines 37 - 40, The setUp uses a shared request (self.request) but doesn't clear mutable state, so reset request state there: in setUp (the setUp method of the test class) assign an empty mapping to self.request.form and clear any queued response/status on self.request (e.g., reset request.response status/body or any status queue attributes) so each test starts with a fresh request; update setUp around the existing setRoles call to perform these resets.
🧹 Nitpick comments (1)
src/imio/esign/tests/test_utils.py (1)
22-23: Use stdlibunittest.mockinstead of externalmockpackage.Since the project targets Python 3.7+, the built-in
unittest.mockis available and preferred. Whilemockis already listed as a dependency insetup.py, using stdlib directly reduces an unnecessary external dependency. Update bothsrc/imio/esign/tests/test_browser_views.py(lines 15–16) andsrc/imio/esign/tests/test_utils.py(lines 22–23):Proposed fix
-from mock import Mock -from mock import patch +from unittest.mock import Mock +from unittest.mock import patch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/test_utils.py` around lines 22 - 23, Replace usages of the external "mock" package imports with the stdlib "unittest.mock": change any "from mock import Mock" and "from mock import patch" to "from unittest.mock import Mock, patch" (update both occurrences referenced in the tests, e.g., the Mock and patch imports in test_utils.py and test_browser_views.py) and run tests to ensure all mocks still resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/imio/esign/tests/test_browser_views.py`:
- Around line 15-16: Replace the direct third-party "mock" imports with the
stdlib fallback pattern: attempt to import Mock and patch from unittest.mock and
fall back to importing them from mock on ImportError, so update the top-level
imports that currently read "from mock import Mock" and "from mock import patch"
to a try/except that assigns Mock and patch from unittest.mock first and uses
mock as fallback.
- Around line 37-40: The setUp uses a shared request (self.request) but doesn't
clear mutable state, so reset request state there: in setUp (the setUp method of
the test class) assign an empty mapping to self.request.form and clear any
queued response/status on self.request (e.g., reset request.response status/body
or any status queue attributes) so each test starts with a fresh request; update
setUp around the existing setRoles call to perform these resets.
---
Nitpick comments:
In `@src/imio/esign/tests/test_utils.py`:
- Around line 22-23: Replace usages of the external "mock" package imports with
the stdlib "unittest.mock": change any "from mock import Mock" and "from mock
import patch" to "from unittest.mock import Mock, patch" (update both
occurrences referenced in the tests, e.g., the Mock and patch imports in
test_utils.py and test_browser_views.py) and run tests to ensure all mocks still
resolve.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
setup.pysrc/imio/esign/browser/views.pysrc/imio/esign/locales/en/LC_MESSAGES/imio.esign.posrc/imio/esign/locales/fr/LC_MESSAGES/imio.esign.posrc/imio/esign/locales/imio.esign.potsrc/imio/esign/tests/test_browser_views.pysrc/imio/esign/tests/test_utils.pysrc/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/imio/esign/utils.py
- src/imio/esign/locales/imio.esign.pot
- setup.py
3 participants
Summary by CodeRabbit
Bug Fixes
New Behavior
Tests
Localization
Chores