Added download QR code page to docx and pdf files to be signed#98
Open
chris-adam wants to merge 1 commit intomasterfrom
Open
Added download QR code page to docx and pdf files to be signed#98chris-adam wants to merge 1 commit intomasterfrom
chris-adam wants to merge 1 commit intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdds rendering and merging of a QR-code download page into PDFs produced during the approval workflow, extends _create_pdf_file and related flows to merge that page for PDFs/converted docs, adds tests for PDF/ODT/DOC flows, and updates imio.helpers dependency to include the pdf extra. Changes
Sequence DiagramsequenceDiagram
participant Client
participant OMApprovalAdapter as OMApprovalAdapter
participant Converter as Doc Converter
participant TemplateRenderer as Template Renderer
participant PDFMerger as PDF Merger
participant Storage as Content Storage
Client->>OMApprovalAdapter: submit document for approval
alt source is PDF
OMApprovalAdapter->>TemplateRenderer: render download QR PDF (uid)
TemplateRenderer-->>OMApprovalAdapter: QR PDF bytes
OMApprovalAdapter->>PDFMerger: merge(original PDF, QR PDF)
PDFMerger-->>OMApprovalAdapter: merged PDF bytes
else source is ODT/DOC
OMApprovalAdapter->>Converter: convert to PDF
Converter-->>OMApprovalAdapter: converted PDF bytes
OMApprovalAdapter->>TemplateRenderer: render download QR PDF (uid)
TemplateRenderer-->>OMApprovalAdapter: QR PDF bytes
OMApprovalAdapter->>PDFMerger: merge(converted PDF, QR PDF)
PDFMerger-->>OMApprovalAdapter: merged PDF bytes
end
OMApprovalAdapter->>Storage: create content with merged PDF & set esign attributes
Storage-->>OMApprovalAdapter: stored content metadata
OMApprovalAdapter-->>Client: continue approval flow with merged PDF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 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)
Comment |
chris-adam
force-pushed
the
PARAF-287
branch
5 times, most recently
from
a5dd17f to
72801a8
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: 4
🧹 Nitpick comments (1)
imio/dms/mail/tests/test_adapters.py (1)
1582-1646: Test validates DOCX path correctly, but deeply nested patches reduce readability.The test properly validates the non-ODT code path and uses try/finally for cleanup. Consider using
contextlib.ExitStackorunittest.mock.patch.multipleto flatten the patch nesting for better readability.Example refactor using ExitStack
from contextlib import ExitStack def test_create_pdf_file_from_docx(self): """For non-ODT files (DOCX), the QR page is appended after conversion.""" # ... setup code ... with ExitStack() as stack: stack.enter_context(patch("imio.dms.mail.adapters.convert_and_save_file", return_value=mock_pdf_file)) stack.enter_context(patch("imio.dms.mail.adapters.get_original_template", return_value=None)) stack.enter_context(patch("imio.dms.mail.adapters.update_categorized_elements")) stack.enter_context(patch("imio.dms.mail.adapters.get_category_object", return_value=Mock())) stack.enter_context(patch.object(self.portal.templates.om, "get", return_value=mock_template)) stack.enter_context(patch.object(self.approval, "_render_download_template_to_pdf", return_value=fake_sub_pdf)) stack.enter_context(patch("imio.dms.mail.adapters.merge_pdf", return_value=fake_merged)) mock_converter_cls = stack.enter_context(patch("imio.dms.mail.adapters.Converter")) mock_converter_cls.return_value = Mock() self.approval._create_pdf_file(...)🤖 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 1582 - 1646, The test test_create_pdf_file_from_docx currently uses many nested patch context managers around the call to _create_pdf_file which reduces readability; replace the nested with a single ExitStack (from contextlib) or unittest.mock.patch.multiple to enter all patches at once for convert_and_save_file, get_original_template, update_categorized_elements, get_category_object, portal.templates.om.get, _render_download_template_to_pdf, merge_pdf and Converter so the setup is flat and then call self.approval._create_pdf_file(...) inside that single context; ensure mock_converter_cls is captured from the stack so you can assert mock_converter_cls.assert_called_once_with(mock_pdf_file) and mock_converter_cls.return_value.assert_called_once() as before.
🤖 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`:
- Line 1942: The call to add_files_to_session unpacks (session_id, session) but
session is never used; change the unpacked variable name to _session (e.g.,
(session_id, _session) = add_files_to_session(...)) or otherwise prefix it with
an underscore so the unused variable is explicit; update the unpacking site
where add_files_to_session is called to reference session_id and _session
accordingly.
- Line 1799: The line unpacks get_file_download_url(new_uid,
short_uid=get_suid_from_uuid(new_uid)) into download_url, s_uid but s_uid is
never used; change the unpack to download_url, _s_uid (or download_url, _) to
mark it intentionally unused so linters won't warn and intent is clear—update
the occurrence where get_file_download_url is called (referencing
get_file_download_url, new_uid, get_suid_from_uuid, and s_uid) accordingly.
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1538-1539: The test stub test_create_pdf_file_from_pdf is empty
and the docstrings/names for test_create_pdf_file_from_pdf and
test_create_pdf_file_from_odt appear inconsistent; implement the missing test
and/or fix the swapped docstrings. Add a concrete test in
test_create_pdf_file_from_pdf that constructs a sample PDF input (or uses a test
fixture), calls the adapter method under test (e.g., create_pdf_file or the
adapter class/method referenced in adjacent tests), and asserts expected
outputs: that a file-like/Document object is returned, content-type is PDF, size
> 0, and any metadata (pages, title) as appropriate; also verify
test_create_pdf_file_from_odt has the correct docstring describing ODT input.
Ensure you reference the existing helper fixtures or factory functions used in
neighboring tests so the new test integrates with the test suite.
- Around line 1541-1580: The test function test_create_pdf_file_from_odt has a
mismatched name/docstring: it creates and asserts behavior for a PDF input (uses
example.pdf and asserts PDF path behavior). Rename the test function to
test_create_pdf_file_from_pdf (and update its docstring to "When input is a
PDF...") or remove/rename the existing stub test_create_pdf_file_from_pdf to
avoid duplicates; ensure any references (e.g., in test suites) use the new name
and keep the body, mocks, and assertions in _create_pdf_file invocation
unchanged.
---
Nitpick comments:
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1582-1646: The test test_create_pdf_file_from_docx currently uses
many nested patch context managers around the call to _create_pdf_file which
reduces readability; replace the nested with a single ExitStack (from
contextlib) or unittest.mock.patch.multiple to enter all patches at once for
convert_and_save_file, get_original_template, update_categorized_elements,
get_category_object, portal.templates.om.get, _render_download_template_to_pdf,
merge_pdf and Converter so the setup is flat and then call
self.approval._create_pdf_file(...) inside that single context; ensure
mock_converter_cls is captured from the stack so you can assert
mock_converter_cls.assert_called_once_with(mock_pdf_file) and
mock_converter_cls.return_value.assert_called_once() as before.
4976dd1 to
e6c1517
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
imio/dms/mail/tests/test_adapters.py (1)
1667-1691: Consider reducing context manager nesting depth.The test has 9 levels of nested
withstatements, which hurts readability. Consider using@patchdecorators on the test method orcontextlib.ExitStackto flatten the structure.Example using ExitStack
from contextlib import ExitStack def test_create_pdf_file_from_docx(self): """For non-ODT files (DOCX), the QR page is appended after conversion.""" # ... setup code ... with ExitStack() as stack: stack.enter_context(patch("imio.dms.mail.adapters.convert_and_save_file", return_value=mock_pdf_file)) stack.enter_context(patch("imio.dms.mail.adapters.get_original_template", return_value=None)) stack.enter_context(patch("imio.dms.mail.adapters.update_categorized_elements")) stack.enter_context(patch("imio.dms.mail.adapters.get_category_object", return_value=Mock())) stack.enter_context(patch.object(self.portal.templates.om, "get", return_value=mock_template)) stack.enter_context(patch.object(self.approval, "_render_download_template_to_pdf", return_value=fake_sub_pdf)) stack.enter_context(patch("imio.dms.mail.adapters.merge_pdf", return_value=fake_merged)) mock_converter_cls = stack.enter_context(patch("imio.dms.mail.adapters.Converter")) mock_converter_cls.return_value = Mock() self.approval._create_pdf_file(...)🤖 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 1667 - 1691, The test currently nests nine with-patch context managers around the call to self.approval._create_pdf_file which reduces readability; replace the deeply nested with blocks by using contextlib.ExitStack (enter_context for each patch) or convert appropriate patches to `@patch` decorators on the test method, applying patches for convert_and_save_file, get_original_template, update_categorized_elements, get_category_object, portal.templates.om.get, _render_download_template_to_pdf, merge_pdf, and imio.dms.mail.adapters.Converter so the call to _create_pdf_file remains the same but the setup is flattened and easier to read and maintain.
🤖 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 1726-1749: The docstring for _render_download_template_to_pdf
claims it returns PDF bytes or an empty string on failure, but exceptions from
generate_barcode, download_template.get_file(), or convert_odt can propagate;
update _render_download_template_to_pdf to catch exceptions (wrap the current
body in try/except), log the exception (e.g., logger.exception or
processLogger.error with the exception), and return an empty string on failure
so callers that check "if sub_pdf_data:" behave as documented; keep existing
variables (helper_view, gen_context, template_file) and ensure the except block
returns "".
---
Nitpick comments:
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1667-1691: The test currently nests nine with-patch context
managers around the call to self.approval._create_pdf_file which reduces
readability; replace the deeply nested with blocks by using contextlib.ExitStack
(enter_context for each patch) or convert appropriate patches to `@patch`
decorators on the test method, applying patches for convert_and_save_file,
get_original_template, update_categorized_elements, get_category_object,
portal.templates.om.get, _render_download_template_to_pdf, merge_pdf, and
imio.dms.mail.adapters.Converter so the call to _create_pdf_file remains the
same but the setup is flattened and easier to read and maintain.
224bac8 to
cc8c3d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
imio/dms/mail/tests/test_adapters.py (1)
1547-1548: Prefer delta-based assertions over absolute container counts.Line 1548, Line 1584, and Line 1637 hardcode totals tied to current fixture contents, which can make these tests fragile as setup evolves.
♻️ Stabilization example
def test_create_pdf_file_from_pdf(self): + initial_count = len(list(self.omail.objectIds())) ... - self.assertEqual(len(list(self.omail.objectIds())), 4) + self.assertEqual(len(list(self.omail.objectIds())), initial_count + 2)Apply the same pattern to the ODT/DOC tests (
+1for ODT,+2for DOC in current flows).Also applies to: 1583-1584, 1636-1637
🤖 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 1547 - 1548, Replace fragile absolute container-count assertions that check len(list(self.omail.objectIds())) == 4 with delta-based assertions: capture the initial count via initial = len(list(self.omail.objectIds())) before the operation, perform the operation that adds ODT/DOC/PDF objects, then assert len(list(self.omail.objectIds())) == initial + N (use +1 for an added ODT, +2 for added DOC, +1 for added PDF/merged as appropriate). Update all occurrences that hardcode totals (the assertions around self.omail.objectIds() at the three spots called out) to follow this pattern so tests remain stable as fixtures change.
🤖 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 1801-1830: The flag doc_cb_download_added is set to True even when
no doc_cb_download subtemplate is resolved; only set it after a
download_template is found and the generation context is actually updated. Move
the update_dict_with_validation(...) call that injects "doc_cb_download" (which
uses generate_barcode(download_url), get_max_download_date(...)) into the block
guarded by if download_template: and then set doc_cb_download_added = True
immediately after that update; apply the same change for the analogous block
around lines 1863-1867 so doc_cb_download_added is only True when
uuidToObject(...) returned a template and update_dict_with_validation was
executed.
- Line 1872: The NotImplementedError raised when a file cannot be converted to
PDF (the raise that references nbf.contentType) should not bubble up; update the
caller add_mail_files_to_session (or the function that wraps the conversion) to
catch NotImplementedError and handle it the same way other failures are handled
by returning (False, msg). Specifically, wrap the conversion/attachment call
that may raise NotImplementedError, build a user-friendly message including the
offending nbf.contentType, and return (False, msg) instead of letting the
exception propagate.
---
Nitpick comments:
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1547-1548: Replace fragile absolute container-count assertions
that check len(list(self.omail.objectIds())) == 4 with delta-based assertions:
capture the initial count via initial = len(list(self.omail.objectIds())) before
the operation, perform the operation that adds ODT/DOC/PDF objects, then assert
len(list(self.omail.objectIds())) == initial + N (use +1 for an added ODT, +2
for added DOC, +1 for added PDF/merged as appropriate). Update all occurrences
that hardcode totals (the assertions around self.omail.objectIds() at the three
spots called out) to follow this pattern so tests remain stable as fixtures
change.
4215a83 to
212fe62
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
imio/dms/mail/tests/test_adapters.py (1)
1638-1638: Avoid relying on container order (values()[-1]) to locate the generated PDF.Line 1638 is order-sensitive and can become flaky. Prefer resolving from
self.approval.annot["pdf_files"](as done in the PDF test) and fetching by UID.🤖 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` at line 1638, The test currently picks the generated PDF via an order-sensitive container access (pdf_file = self.omail.values()[-1]); instead, derive the PDF UID from the approval annotation and use that to retrieve the file from self.omail. Concretely, read the PDF UID from self.approval.annot["pdf_files"] (e.g. the last entry or by matching metadata), then replace the values()[-1] lookup with a UID-based lookup in self.omail (e.g. self.omail[<uid>]) so the test no longer depends on dict ordering.
🤖 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 1729-1731: The docstring parameters are out of sync with the
method signature: update the docstring for the method in
imio/dms/mail/adapters.py that now accepts uid (the function whose signature
includes the uid parameter) by removing references to download_template and
download_url, adding a clear :param uid: description (what uid represents and
how it is used), and ensuring the :return: description still accurately states
"PDF bytes, or empty string on failure" (and note any exceptions or side-effects
if applicable); keep the rest of the docstring consistent with the method's
actual behavior.
- Line 1753: In imio.dms.mail.adapters update the exception handler that
currently reads "except Exception as e:" to remove the unused alias and use
"except Exception:" instead; ensure the except block (the one that calls
logger.exception()) no longer references "e" anywhere and run tests to confirm
no other code depends on that variable.
- Around line 1788-1804: The QR-rendering branch skips update_esign_attributes
when download_page is falsy, leaving esign flags out of sync; to fix, initialize
pdf_file = orig_fobj before the download_page check, keep the existing creation
path (creating pdf_file and annot only when download_page truthy), and then
unconditionally call update_esign_attributes(pdf_file, orig_fobj) after the
if/else so the original file (when no new PDF was produced) also gets its
signing/approval attributes synchronized; refer to symbols download_page,
pdf_file, orig_fobj, update_esign_attributes, and annot to locate and adjust the
code.
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1602-1632: Before overwriting the registry key
"imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats", capture
its current value into a variable (e.g., previous_omail_esign_formats =
api.portal.get_registry_record(...)), then set the temporary value as you do; in
the finally block restore the original value by calling
api.portal.set_registry_record with that saved variable instead of the
hard-coded ["odt", "pdf"]; reference the test code around the calls to
api.portal.set_registry_record and the finally block that currently forces the
fallback.
---
Nitpick comments:
In `@imio/dms/mail/tests/test_adapters.py`:
- Line 1638: The test currently picks the generated PDF via an order-sensitive
container access (pdf_file = self.omail.values()[-1]); instead, derive the PDF
UID from the approval annotation and use that to retrieve the file from
self.omail. Concretely, read the PDF UID from self.approval.annot["pdf_files"]
(e.g. the last entry or by matching metadata), then replace the values()[-1]
lookup with a UID-based lookup in self.omail (e.g. self.omail[<uid>]) so the
test no longer depends on dict ordering.
Comment on lines
+1602
to
+1632
| api.portal.set_registry_record( | ||
| "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats", | ||
| ["odt", "pdf", "doc"], | ||
| ) | ||
|
|
||
| try: | ||
| # Remove existing ODT files from approval, then add a DOC file | ||
| self.approval.remove_file_from_approval(self.files[0].UID()) | ||
| self.approval.remove_file_from_approval(self.files[1].UID()) | ||
| ct = self.portal["annexes_types"]["outgoing_dms_files"]["outgoing-dms-file"] | ||
| with open("%s/batchimport/toprocess/incoming-mail/in-courrier4.doc" % PRODUCT_DIR, "rb") as fo: | ||
| doc_data = fo.read() | ||
| doc_fobj = createContentInContainer( | ||
| self.omail, | ||
| "dmsommainfile", | ||
| id="docfile", | ||
| scan_id="012999900000601", | ||
| file=NamedBlobFile(doc_data, contentType="application/msword", filename=u"in-courrier4.doc"), | ||
| content_category=calculate_category_id(ct), | ||
| ) | ||
|
|
||
| self.pw.doActionFor(self.omail, "propose_to_approve") | ||
| # Signer 0 (dirg) approves — no session creation yet | ||
| self.approval.approve_file(doc_fobj, "dirg") | ||
| # Signer 1 (bourgmestre) approves — triggers add_mail_files_to_session() | ||
| self.approval.approve_file(doc_fobj, "bourgmestre") | ||
| finally: | ||
| api.portal.set_registry_record( | ||
| "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats", | ||
| ["odt", "pdf"], | ||
| ) |
There was a problem hiding this comment.
Restore the previous registry value instead of a hard-coded fallback.
Lines 1629–1632 force ["odt", "pdf"] regardless of prior state, which can leak config changes across tests if defaults evolve.
💡 Proposed fix
- api.portal.set_registry_record(
+ previous_formats = api.portal.get_registry_record(
+ "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats"
+ )
+ api.portal.set_registry_record(
"imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats",
["odt", "pdf", "doc"],
)
@@
finally:
api.portal.set_registry_record(
"imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats",
- ["odt", "pdf"],
+ previous_formats,
)🤖 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 1602 - 1632, Before
overwriting the registry key
"imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats", capture
its current value into a variable (e.g., previous_omail_esign_formats =
api.portal.get_registry_record(...)), then set the temporary value as you do; in
the finally block restore the original value by calling
api.portal.set_registry_record with that saved variable instead of the
hard-coded ["odt", "pdf"]; reference the test code around the calls to
api.portal.set_registry_record and the finally block that currently forces the
fallback.
9f2de19 to
0f49254
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
imio/dms/mail/adapters.py (3)
1729-1731:⚠️ Potential issue | 🟡 MinorUpdate docstring params to match the
uid-based signature.The method now accepts
uid, but the docstring still documentsdownload_templateanddownload_url.🛠️ Proposed docstring fix
def _render_download_template_to_pdf(self, uid): """Render the download subtemplate (QR code page) to PDF bytes. - :param download_template: the POD template object to render - :param download_url: the URL to encode in the QR/barcode + :param uid: UUID used to build the file download URL and barcode :return: PDF bytes, or empty string on failure """🤖 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 1729 - 1731, The docstring still documents download_template and download_url but the method signature now accepts uid; update the docstring for that method to remove/replace download_template and download_url entries and add a param entry for uid (explain what uid is/represents), ensure the return description remains accurate (PDF bytes or empty string on failure), and adjust any examples or descriptions mentioning the old parameters so they reference uid where appropriate.
1788-1804:⚠️ Potential issue | 🟠 MajorApply e-sign attribute sync even when PDF QR rendering fails.
If
download_pageis falsy,pdf_fileremainsorig_fobjandupdate_esign_attributes(...)is skipped, which can leave signing flags/category sync inconsistent in the fallback path.🛠️ Proposed fix
if nbf.contentType == "application/pdf": pdf_file = orig_fobj new_uid = uuid.uuid4().hex download_page = self._render_download_template_to_pdf(new_uid) if download_page: merged = merge_pdf(nbf.data, download_page) file_object = NamedBlobFile(merged, filename=safe_unicode(new_filename)) pdf_file = createContentInContainer( self.context, orig_fobj.portal_type, title=safe_unicode(new_filename), file=file_object, content_category=orig_fobj.content_category, scan_id=orig_fobj.scan_id, conv_from_uid=f_uid, **{"_plone.uuid": new_uid} ) annot = IAnnotations(pdf_file) annot["documentgenerator"] = {"conv_from_uid": f_uid} - update_esign_attributes(pdf_file, orig_fobj) + update_esign_attributes(pdf_file, orig_fobj)🤖 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 1788 - 1804, When download_page is falsy the code skips update_esign_attributes, leaving signing flags out of sync; ensure that in the fallback path pdf_file is treated as orig_fobj and update_esign_attributes is still called. Concretely, after the merge_pdf block (or in an else for download_page) set pdf_file = orig_fobj (if not already set) and call update_esign_attributes(pdf_file, orig_fobj); keep the existing annotation logic only for the merged branch (IAnnotations/pdf_file assignment stays inside the download_page branch).
1753-1753:⚠️ Potential issue | 🟡 MinorRemove the unused exception alias.
eis never used in this handler.🛠️ Proposed fix
- except Exception as e: + except Exception: logger.exception("Could not render download template to PDF") return ""🤖 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 1753, The except block currently binds an unused exception variable with "except Exception as e:"; remove the unused alias by changing that clause to "except Exception:" in the relevant handler in imio/dms/mail/adapters.py (locate the try/except surrounding the code where the comment points to) so the exception is handled without an unused variable.imio/dms/mail/tests/test_adapters.py (1)
1602-1632:⚠️ Potential issue | 🟡 MinorRestore the original registry value instead of a hardcoded fallback.
The test currently forces
["odt", "pdf"]infinally, which can leak state if defaults evolve.🛠️ Proposed fix
+ previous_formats = api.portal.get_registry_record( + "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats" + ) api.portal.set_registry_record( "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats", ["odt", "pdf", "doc"], ) @@ finally: api.portal.set_registry_record( "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats", - ["odt", "pdf"], + previous_formats, )🤖 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 1602 - 1632, Save the original registry value returned by api.portal.get_registry_record (for the key "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats") before you call api.portal.set_registry_record to change it, and in the finally block restore that saved original value instead of setting a hardcoded ["odt","pdf"]; update the test around the set/reset calls (references: api.portal.set_registry_record, the registry key string, and the surrounding try/finally) so the original value is preserved and restored even if defaults change.
🧹 Nitpick comments (1)
imio/dms/mail/tests/test_adapters.py (1)
37-37: Importing from test module internals should be avoided; refactor to use a local helper.
imio.helpers.tests.test_pdf._pdf_page_countis a private test utility (underscore prefix) from an external package's test module and may not be available in packaged distributions. However, sinceimio.helpersis an established dependency and the project already imports from its test modules (e.g.,ImioTestHelpers), consider creating a local_pdf_page_count()helper function inimio/dms/mail/tests/to count PDF pages independently, using available PDF libraries (e.g., PyPDF2, which may already be transitively available through dependencies).🤖 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` at line 37, The test imports a private helper imio.helpers.tests.test_pdf._pdf_page_count which is fragile; replace it by adding a local helper function named _pdf_page_count in imio/dms/mail/tests that uses a PDF library (e.g., PyPDF2/PdfReader) to open a bytes/stream and return the page count, then update imio/dms/mail/tests/test_adapters.py to import or call the new local _pdf_page_count instead of imio.helpers.tests.test_pdf._pdf_page_count; ensure the helper accepts the same input type your tests pass (bytes or file-like) and add a small unit test exercising the helper so it’s packaged with your tests.
🤖 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 1729-1731: The docstring still documents download_template and
download_url but the method signature now accepts uid; update the docstring for
that method to remove/replace download_template and download_url entries and add
a param entry for uid (explain what uid is/represents), ensure the return
description remains accurate (PDF bytes or empty string on failure), and adjust
any examples or descriptions mentioning the old parameters so they reference uid
where appropriate.
- Around line 1788-1804: When download_page is falsy the code skips
update_esign_attributes, leaving signing flags out of sync; ensure that in the
fallback path pdf_file is treated as orig_fobj and update_esign_attributes is
still called. Concretely, after the merge_pdf block (or in an else for
download_page) set pdf_file = orig_fobj (if not already set) and call
update_esign_attributes(pdf_file, orig_fobj); keep the existing annotation logic
only for the merged branch (IAnnotations/pdf_file assignment stays inside the
download_page branch).
- Line 1753: The except block currently binds an unused exception variable with
"except Exception as e:"; remove the unused alias by changing that clause to
"except Exception:" in the relevant handler in imio/dms/mail/adapters.py (locate
the try/except surrounding the code where the comment points to) so the
exception is handled without an unused variable.
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1602-1632: Save the original registry value returned by
api.portal.get_registry_record (for the key
"imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats") before
you call api.portal.set_registry_record to change it, and in the finally block
restore that saved original value instead of setting a hardcoded ["odt","pdf"];
update the test around the set/reset calls (references:
api.portal.set_registry_record, the registry key string, and the surrounding
try/finally) so the original value is preserved and restored even if defaults
change.
---
Nitpick comments:
In `@imio/dms/mail/tests/test_adapters.py`:
- Line 37: The test imports a private helper
imio.helpers.tests.test_pdf._pdf_page_count which is fragile; replace it by
adding a local helper function named _pdf_page_count in imio/dms/mail/tests that
uses a PDF library (e.g., PyPDF2/PdfReader) to open a bytes/stream and return
the page count, then update imio/dms/mail/tests/test_adapters.py to import or
call the new local _pdf_page_count instead of
imio.helpers.tests.test_pdf._pdf_page_count; ensure the helper accepts the same
input type your tests pass (bytes or file-like) and add a small unit test
exercising the helper so it’s packaged with your tests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGES.rstimio/dms/mail/adapters.pyimio/dms/mail/tests/test_adapters.pysetup.py
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGES.rst
- setup.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
imio/dms/mail/adapters.py (1)
1733-1740: Add an explicit guard for missingdownload_barcodetemplate.At Line 1739,
download_template.UID()is dereferenced without an explicitNonecheck. It currently falls into the generic exception path, which makes config problems less clear and silently skips QR-page generation.Proposed tweak
try: download_template = api.portal.get().templates.om.get("download_barcode") + if not download_template: + logger.error("Missing template 'templates/om/download_barcode'; QR page cannot be rendered") + return "" download_url, _short_uid = get_file_download_url(uid, short_uid=get_suid_from_uuid(uid)) helper_view = getMultiAdapter( (self.context, self.context.REQUEST), name="document_generation_helper_view", )🤖 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 1733 - 1740, The code dereferences download_template.UID() without checking for a missing "download_barcode" template; add an explicit guard after obtaining download_template (from api.portal.get().templates.om.get("download_barcode")) to handle None: if download_template is None, log an explicit error or raise a clear ConfigurationError (or return/skip with a specific log) so QR-page generation isn't silently skipped, and only set helper_view.pod_template = download_template.UID() when download_template is not None; keep the rest of the flow (get_file_download_url, get_suid_from_uuid, document_generation_helper_view usage) unchanged.
🤖 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 1733-1740: The code dereferences download_template.UID() without
checking for a missing "download_barcode" template; add an explicit guard after
obtaining download_template (from
api.portal.get().templates.om.get("download_barcode")) to handle None: if
download_template is None, log an explicit error or raise a clear
ConfigurationError (or return/skip with a specific log) so QR-page generation
isn't silently skipped, and only set helper_view.pod_template =
download_template.UID() when download_template is not None; keep the rest of the
flow (get_file_download_url, get_suid_from_uuid, document_generation_helper_view
usage) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGES.rstimio/dms/mail/adapters.pyimio/dms/mail/tests/test_adapters.pysetup.py
🚧 Files skipped from review as they are similar to previous changes (3)
- imio/dms/mail/tests/test_adapters.py
- setup.py
- CHANGES.rst
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
imio/dms/mail/tests/test_adapters.py (1)
1594-1624:⚠️ Potential issue | 🟡 MinorRestore the previous registry value instead of a hard-coded fallback.
This test mutates a global registry key and restores
["odt", "pdf"]unconditionally. That can leak test state when defaults/config evolve.💡 Suggested fix
+ previous_formats = api.portal.get_registry_record( + "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats" + ) api.portal.set_registry_record( "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats", ["odt", "pdf", "doc"], ) @@ finally: api.portal.set_registry_record( "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats", - ["odt", "pdf"], + previous_formats, )🤖 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 1594 - 1624, Before mutating the registry key "imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats" capture its current value into a local variable (e.g., original_formats) prior to calling api.portal.set_registry_record, then in the finally block restore that captured value instead of the hard-coded ["odt","pdf"]; update the test around the createContentInContainer/approve_file flow (in test_adapters.py where omail_esign_formats is set) to use the saved original_formats for restoration so test state is not leaked if defaults change.
🧹 Nitpick comments (1)
imio/dms/mail/tests/test_adapters.py (1)
1630-1631: Avoid relying on container ordering to select the generated PDF.Using
self.omail.values()[-1]makes the test sensitive to ordering. Prefer resolving the object fromself.approval.annot["pdf_files"].💡 Suggested refactor
- pdf_file = self.omail.values()[-1] + pdf_uid = self.approval.annot["pdf_files"][0][0] + catalog = getToolByName(self.portal, "portal_catalog") + pdf_file = catalog(UID=pdf_uid)[0].getObject()🤖 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 1630 - 1631, The test currently picks the generated PDF by relying on container ordering with self.omail.values()[-1], which is fragile; instead resolve the PDF using the approval annotation: read the key(s) from self.approval.annot["pdf_files"] and use that key to fetch the object from self.omail (or iterate self.omail items to match the annotated filename/uid) before calling _pdf_page_count on pdf_file.file.data; update the test to reference self.approval.annot["pdf_files"] and avoid using values()[-1] so the lookup is deterministic.
🤖 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 1745-1765: The code currently assumes download_template exists and
relies on the exception handler when it's missing; update the block starting at
download_template = api.portal.get().templates.om.get("download_barcode") to
explicitly guard for a missing template (e.g., if not download_template) and
short-circuit (log a clear debug/info message and return/skip PDF generation)
before calling get_file_download_url, generate_barcode, NamedBlobFile or
convert_odt; keep the broad try/except for unexpected errors but do not use it
to control the normal missing-template flow.
---
Duplicate comments:
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1594-1624: Before mutating the registry key
"imio.dms.mail.browser.settings.IImioDmsMailConfig.omail_esign_formats" capture
its current value into a local variable (e.g., original_formats) prior to
calling api.portal.set_registry_record, then in the finally block restore that
captured value instead of the hard-coded ["odt","pdf"]; update the test around
the createContentInContainer/approve_file flow (in test_adapters.py where
omail_esign_formats is set) to use the saved original_formats for restoration so
test state is not leaked if defaults change.
---
Nitpick comments:
In `@imio/dms/mail/tests/test_adapters.py`:
- Around line 1630-1631: The test currently picks the generated PDF by relying
on container ordering with self.omail.values()[-1], which is fragile; instead
resolve the PDF using the approval annotation: read the key(s) from
self.approval.annot["pdf_files"] and use that key to fetch the object from
self.omail (or iterate self.omail items to match the annotated filename/uid)
before calling _pdf_page_count on pdf_file.file.data; update the test to
reference self.approval.annot["pdf_files"] and avoid using values()[-1] so the
lookup is deterministic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGES.rstimio/dms/mail/adapters.pyimio/dms/mail/tests/test_adapters.pysetup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGES.rst
Comment on lines
+1745
to
+1765
| download_template = api.portal.get().templates.om.get("download_barcode") | ||
| download_url, _short_uid = get_file_download_url(uid, short_uid=get_suid_from_uuid(uid)) | ||
| helper_view = getMultiAdapter( | ||
| (self.context, self.context.REQUEST), | ||
| name="document_generation_helper_view", | ||
| ) | ||
| helper_view.pod_template = download_template.UID() | ||
| helper_view.output_format = "pdf" | ||
| gen_context = { | ||
| "context": self.context, | ||
| "portal": api.portal.get(), | ||
| "view": helper_view, | ||
| "download_barcode": generate_barcode(download_url).read(), | ||
| "download_url": download_url, | ||
| "max_download_date": get_max_download_date(None, adate=datetime.date.today()), | ||
| "render_download_barcode": True, | ||
| } | ||
| template_file = NamedBlobFile(download_template.get_file().data, filename=u"download_template.odt") | ||
| return convert_odt(template_file, fmt="pdf", gen_context=gen_context) | ||
| except Exception: | ||
| logger.exception("Could not render download template to PDF") |
There was a problem hiding this comment.
Guard missing download_barcode template before relying on exception flow.
If the template is absent, this path currently throws and logs a full exception stack on every call. It’s cleaner to short-circuit early and skip URL generation in that case.
💡 Suggested fix
- download_template = api.portal.get().templates.om.get("download_barcode")
- download_url, _short_uid = get_file_download_url(uid, short_uid=get_suid_from_uuid(uid))
+ download_template = api.portal.get().templates.om.get("download_barcode")
+ if not download_template:
+ logger.warning("Template 'download_barcode' is not configured; skipping QR page merge.")
+ return ""
+ download_url, _short_uid = get_file_download_url(uid, short_uid=get_suid_from_uuid(uid))🧰 Tools
🪛 Ruff (0.15.2)
[warning] 1764-1764: Do not catch blind exception: Exception
(BLE001)
🤖 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 1745 - 1765, The code currently
assumes download_template exists and relies on the exception handler when it's
missing; update the block starting at download_template =
api.portal.get().templates.om.get("download_barcode") to explicitly guard for a
missing template (e.g., if not download_template) and short-circuit (log a clear
debug/info message and return/skip PDF generation) before calling
get_file_download_url, generate_barcode, NamedBlobFile or convert_odt; keep the
broad try/except for unexpected errors but do not use it to control the normal
missing-template flow.
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.
Le code me parait très compliqué. Mais je ne vois pas vraiment comment faire plus simple
Summary by CodeRabbit
New Features
Tests
Chores
Documentation