From 160c89da36a7f9445ed127c8c2b9ec4984330be9 Mon Sep 17 00:00:00 2001 From: Emanuel Cino Date: Thu, 5 Nov 2026 14:36:00 +0100 Subject: [PATCH 1/2] T3646 FIX find the pages of the Cloudinary image from their borders The pages of a letter are stacked in one image with pages of different heights, which were cut in equal parts. The borders between the pages are detected, and the page is fetched from Connect if they can't be found. Assisted-by: claude-sonnet-5-5, medium, claude-code --- sbc_compassion/models/correspondence_page.py | 73 +++++++++++++++---- .../tests/test_correspondence_template.py | 39 ++++++++++ 2 files changed, 96 insertions(+), 16 deletions(-) diff --git a/sbc_compassion/models/correspondence_page.py b/sbc_compassion/models/correspondence_page.py index 0e8b2c7ec..47d44963d 100644 --- a/sbc_compassion/models/correspondence_page.py +++ b/sbc_compassion/models/correspondence_page.py @@ -25,6 +25,10 @@ BOX_SEPARATOR = "#BOX#" PAGE_SEPARATOR = "#PAGE#" +# Detection of the pages in the stacked image of Cloudinary +EDGE_WIDTH_PX = 4 +SEAM_EDGE_JUMP = 60 +SEAM_MAX_THICKNESS_PX = 12 class CorrespondencePage(models.Model): @@ -81,16 +85,9 @@ def _compute_page_image(self, image_type="original"): for page in self: cloudinary_url = getattr(page, cloudinary_url_field) page_url = getattr(page, url_field) - image = False - # A cropped image slices a stack of pages in equal parts, which is - # wrong when the pages don't have the same height: the page of the - # document is used first. - connect_first = bool(cloudinary_url and "c_crop" in cloudinary_url) - if connect_first: - image = page._get_connect_image(page_url) - if not image and cloudinary_url: - image = page._get_cloundinary_image(cloudinary_url) - if not image and not connect_first: + # Cloudinary is much faster than Connect, which is the fallback + image = page._get_cloundinary_image(cloudinary_url) + if not image: image = page._get_connect_image(page_url) if ( not image @@ -260,16 +257,60 @@ def _get_cloudinary_url(self, cloudinary_url): scanned_index = list(scanned_pages).index(self) + 1 if cloudinary_url.endswith("p1.jpg"): return cloudinary_url.replace("p1.jpg", f"p{scanned_index}.jpg") - response = requests.get(cloudinary_url, timeout=3) - if not response.ok: + try: + response = requests.get(cloudinary_url, timeout=3) + if not response.ok: + return False + image = Image.open(BytesIO(response.content)) + image.load() + except (requests.RequestException, OSError): + _logger.warning("Could not download %s", cloudinary_url, exc_info=True) + return False + bounds = self._find_page_bounds(image, len(scanned_pages)) + if not bounds: + # No reliable crop: the page is fetched from Connect return False - width, total_height = Image.open(BytesIO(response.content)).size - height = total_height // len(scanned_pages) - y_start = (scanned_index - 1) * height - crop_operator = f"c_crop,h_{height},w_{width},x_0,y_{y_start}" + y_start, height = bounds[scanned_index - 1] + crop_operator = f"c_crop,h_{height},w_{image.width},x_0,y_{y_start}" url_parts = cloudinary_url.split("upload/") return f"{url_parts[0]}upload/{crop_operator}/{url_parts[1]}" + @api.model + def _find_page_bounds(self, image, nb_pages): + """ + Cloudinary gives the pages of a letter stacked in one image, and the pages + don't always have the same height. A border is drawn at the edge of the + pages, so the brightness of the left edge changes sharply from one page + to the next one. + :param image: PIL image holding the stacked pages + :param nb_pages: number of pages in the image + :return: list of (y_start, height) for each page, or an empty list if the + pages could not be found reliably. + """ + width, total_height = image.size + if nb_pages <= 1: + return [(0, total_height)] if nb_pages == 1 else [] + edge = image.convert("L").crop((0, 0, min(width, EDGE_WIDTH_PX), total_height)) + edge = edge.resize((1, total_height), Image.Resampling.BOX) + brightness = list(edge.getdata()) + seams = [] + # The border at the top and bottom of the image is not between two pages + for y in range(SEAM_MAX_THICKNESS_PX, total_height - SEAM_MAX_THICKNESS_PX): + if abs(brightness[y] - brightness[y - 1]) < SEAM_EDGE_JUMP: + continue + if seams and y - seams[-1][-1] <= SEAM_MAX_THICKNESS_PX: + seams[-1].append(y) + else: + seams.append([y]) + if len(seams) != nb_pages - 1: + return [] + cuts = [0] + [(seam[0] + seam[-1]) // 2 for seam in seams] + [total_height] + heights = [end - start for start, end in zip(cuts, cuts[1:], strict=False)] + average = total_height / nb_pages + if any(not 0.5 * average <= height <= 1.5 * average for height in heights): + return [] + return list(zip(cuts, heights, strict=False)) + def sync_text_from_paragraphs(self): _fields = ["original_text", "english_text", "translated_text"] for page in self: diff --git a/sbc_compassion/tests/test_correspondence_template.py b/sbc_compassion/tests/test_correspondence_template.py index 96ed12bfe..80f8f3e69 100644 --- a/sbc_compassion/tests/test_correspondence_template.py +++ b/sbc_compassion/tests/test_correspondence_template.py @@ -1,3 +1,5 @@ +from PIL import Image + from odoo.tests import TransactionCase from ..models.correspondence_text_box import PAGE_WIDTH_PX @@ -108,3 +110,40 @@ def test_paragraph_filling_the_box(self): def test_nothing_fits_keeps_text(self): self.text_box.height = 1 self.assertEqual(self.text_box.split_text("word"), ("word", "")) + + +class TestPageBounds(TransactionCase): + """Pages are stacked in one image by Cloudinary, with different heights.""" + + def _stack(self, heights, width=300): + image = Image.new("L", (width, sum(heights)), 255) + y = 0 + for height in heights[:-1]: + y += height + # Dark border at the edge of the page, a few rows thick + for row in range(y - 1, y + 2): + for col in range(3): + image.putpixel((col, row), 20) + return image + + def test_pages_of_different_heights(self): + page = self.env["correspondence.page"] + bounds = page._find_page_bounds(self._stack([400, 400, 330]), 3) + self.assertEqual(len(bounds), 3) + for (start, height), (real_start, real_height) in zip( + bounds, [(0, 400), (400, 400), (800, 330)], strict=False + ): + self.assertAlmostEqual(start, real_start, delta=2) + self.assertAlmostEqual(height, real_height, delta=3) + self.assertEqual(sum(height for _start, height in bounds), 1130) + + def test_single_page(self): + page = self.env["correspondence.page"] + self.assertEqual(page._find_page_bounds(self._stack([500]), 1), [(0, 500)]) + + def test_no_reliable_crop_gives_nothing(self): + page = self.env["correspondence.page"] + # No border between the pages + self.assertEqual(page._find_page_bounds(Image.new("L", (300, 900), 255), 3), []) + # More borders than expected + self.assertEqual(page._find_page_bounds(self._stack([300, 300, 300]), 2), []) From f773ee9efc887c570d688dd91ad00520be7e231b Mon Sep 17 00:00:00 2001 From: Emanuel Cino Date: Thu, 5 Nov 2026 14:36:00 +0100 Subject: [PATCH 2/2] T3646 ADD print delay on the reports, 0 for the communications wkhtmltopdf waits 1 second (and costs 2 seconds) for the javascript of the reports, which have none. The delay can be set for each report and the migration sets it to 0 for the reports of the communications. --- partner_communication/__manifest__.py | 3 +- .../migrations/18.0.1.0.3/end-migration.py | 47 ++++++++++++++++++ partner_communication/models/__init__.py | 1 + .../models/ir_actions_report.py | 42 ++++++++++++++++ .../report/a4_communication.xml | 1 + partner_communication/tests/__init__.py | 1 + .../tests/test_ir_actions_report.py | 48 +++++++++++++++++++ .../views/ir_actions_report_view.xml | 13 +++++ .../__manifest__.py | 1 + .../data/report_print_delay.xml | 17 +++++++ sbc_compassion/models/correspondence_page.py | 47 ++++++++++++------ 11 files changed, 206 insertions(+), 15 deletions(-) create mode 100644 partner_communication/migrations/18.0.1.0.3/end-migration.py create mode 100644 partner_communication/models/ir_actions_report.py create mode 100644 partner_communication/tests/test_ir_actions_report.py create mode 100644 partner_communication/views/ir_actions_report_view.xml create mode 100644 partner_communication_compassion/data/report_print_delay.xml diff --git a/partner_communication/__manifest__.py b/partner_communication/__manifest__.py index 44b156191..f927d00f2 100644 --- a/partner_communication/__manifest__.py +++ b/partner_communication/__manifest__.py @@ -30,7 +30,7 @@ # pylint: disable=C8101 { "name": "Partner Communication", - "version": "18.0.1.0.2", + "version": "18.0.1.0.3", "category": "Other", "author": "Compassion Switzerland", "license": "AGPL-3", @@ -56,6 +56,7 @@ "views/generate_communication_wizard_view.xml", "views/ir_attachment_view.xml", "views/ir_actions_view.xml", + "views/ir_actions_report_view.xml", "views/download_print_wizard_view.xml", "views/settings_view.xml", "views/communication_snippet_view.xml", diff --git a/partner_communication/migrations/18.0.1.0.3/end-migration.py b/partner_communication/migrations/18.0.1.0.3/end-migration.py new file mode 100644 index 000000000..8797919d9 --- /dev/null +++ b/partner_communication/migrations/18.0.1.0.3/end-migration.py @@ -0,0 +1,47 @@ +import logging + +from openupgradelib import openupgrade + +_logger = logging.getLogger(__name__) + + +@openupgrade.migrate() +def migrate(env, version): + """ + The reports of our communications have no javascript, so wkhtmltopdf has + nothing to wait for: a print delay of 0 makes each of them about 2 seconds + faster. This is an end-migration so that all the modules are loaded, and it + takes the reports from what the communications use, whatever module defines + them (they may also have been added directly in the database), so that it + works for any of our instances (Switzerland, Nordic, ...). + """ + # Our code prints the reports of the communications by name: a report that + # was duplicated from the UI (same report name) can be the one that is used, + # so all the reports sharing the name of a report of the communications are + # updated. Only the ones that still have the default value (-1) change, to + # keep a value that someone would have set by hand. + env.cr.execute( + """ + UPDATE ir_act_report_xml + SET print_delay = 0 + WHERE print_delay < 0 AND report_name IN ( + SELECT report_name FROM ir_act_report_xml WHERE id IN ( + SELECT report_id FROM partner_communication_config + WHERE report_id IS NOT NULL + UNION + SELECT report_id FROM partner_communication_attachment + WHERE report_id IS NOT NULL + ) OR model IN ( + 'partner.communication.job', + 'correspondence', + 'compassion.child', + 'recurring.contract', + 'crm.claim', + 'compassion.field.office' + ) + ) + """ + ) + _logger.info( + "Print delay set to 0 for %s reports of the communications", env.cr.rowcount + ) diff --git a/partner_communication/models/__init__.py b/partner_communication/models/__init__.py index 2ed436fb8..ba477cb3a 100644 --- a/partner_communication/models/__init__.py +++ b/partner_communication/models/__init__.py @@ -15,6 +15,7 @@ communication_job, communication_snippet, ir_actions, + ir_actions_report, ir_attachment, mail_mail, res_partner, diff --git a/partner_communication/models/ir_actions_report.py b/partner_communication/models/ir_actions_report.py new file mode 100644 index 000000000..5e4d13a50 --- /dev/null +++ b/partner_communication/models/ir_actions_report.py @@ -0,0 +1,42 @@ +############################################################################## +# +# Copyright (C) 2026 Compassion CH (http://www.compassion.ch) +# Releasing children from poverty in Jesus' name +# @author: Emanuel Cino +# +# The licence is in the file __manifest__.py +# +############################################################################## +from odoo import api, fields, models + + +class IrActionsReport(models.Model): + _inherit = "ir.actions.report" + + print_delay = fields.Integer( + "Print delay (ms)", + default=-1, + help="Time that wkhtmltopdf waits for the javascript of the page before " + "printing it. Reports without javascript (all our communications) have " + "nothing to wait for and are generated about 2 seconds faster with 0. " + "-1 uses the system parameter report.print_delay (1 second by default).", + ) + + @api.model + def _run_wkhtmltopdf(self, bodies, report_ref=False, *args, **kwargs): + report = self._get_report(report_ref) if report_ref else None + if report and report.print_delay >= 0: + # The delay is read when the arguments of wkhtmltopdf are built + report_model = self.with_context(report_print_delay=report.print_delay) + return super(IrActionsReport, report_model)._run_wkhtmltopdf( + bodies, report_ref, *args, **kwargs + ) + return super()._run_wkhtmltopdf(bodies, report_ref, *args, **kwargs) + + @api.model + def _build_wkhtmltopdf_args(self, *args, **kwargs): + command_args = super()._build_wkhtmltopdf_args(*args, **kwargs) + delay = self.env.context.get("report_print_delay") + if delay is not None and "--javascript-delay" in command_args: + command_args[command_args.index("--javascript-delay") + 1] = str(delay) + return command_args diff --git a/partner_communication/report/a4_communication.xml b/partner_communication/report/a4_communication.xml index cac322a17..a1a2d6ec3 100644 --- a/partner_communication/report/a4_communication.xml +++ b/partner_communication/report/a4_communication.xml @@ -5,6 +5,7 @@ partner.communication.job A4 communication qweb-pdf + 0 partner_communication.a4_communication diff --git a/partner_communication/tests/__init__.py b/partner_communication/tests/__init__.py index 843ef3cc8..2366e3925 100644 --- a/partner_communication/tests/__init__.py +++ b/partner_communication/tests/__init__.py @@ -6,3 +6,4 @@ # The licence is in the file __manifest__.py # ############################################################################## +from . import test_ir_actions_report diff --git a/partner_communication/tests/test_ir_actions_report.py b/partner_communication/tests/test_ir_actions_report.py new file mode 100644 index 000000000..b1a8eb436 --- /dev/null +++ b/partner_communication/tests/test_ir_actions_report.py @@ -0,0 +1,48 @@ +from unittest.mock import patch + +from odoo.tests import TransactionCase + +from odoo.addons.base.models.ir_actions_report import IrActionsReport + + +class TestReportPrintDelay(TransactionCase): + def setUp(self): + super().setUp() + self.report = self.env.ref("partner_communication.report_a4_communication") + self.paperformat = self.report.get_paperformat() + + def _delay(self, args): + return args[args.index("--javascript-delay") + 1] + + def test_communication_report_has_no_delay(self): + self.assertEqual(self.report.print_delay, 0) + + def test_system_parameter_is_the_default(self): + self.env["ir.config_parameter"].sudo().set_param("report.print_delay", "750") + args = self.env["ir.actions.report"]._build_wkhtmltopdf_args( + self.paperformat, False + ) + self.assertEqual(self._delay(args), "750") + + def test_run_wkhtmltopdf_uses_the_delay_of_the_report(self): + seen = {} + + def fake_run(self, bodies, report_ref=False, *args, **kwargs): + seen["delay"] = self.env.context.get("report_print_delay") + return b"" + + with patch.object(IrActionsReport, "_run_wkhtmltopdf", fake_run): + self.env["ir.actions.report"]._run_wkhtmltopdf([], self.report.id) + self.assertEqual(seen["delay"], 0) + # -1 leaves the system parameter in charge + self.report.print_delay = -1 + self.env["ir.actions.report"]._run_wkhtmltopdf([], self.report.id) + self.assertIsNone(seen["delay"]) + + def test_delay_of_the_context_is_in_the_arguments(self): + args = ( + self.env["ir.actions.report"] + .with_context(report_print_delay=0) + ._build_wkhtmltopdf_args(self.paperformat, False) + ) + self.assertEqual(self._delay(args), "0") diff --git a/partner_communication/views/ir_actions_report_view.xml b/partner_communication/views/ir_actions_report_view.xml new file mode 100644 index 000000000..4b8fefc3d --- /dev/null +++ b/partner_communication/views/ir_actions_report_view.xml @@ -0,0 +1,13 @@ + + + + ir.actions.report.form.print.delay + ir.actions.report + + + + + + + + diff --git a/partner_communication_compassion/__manifest__.py b/partner_communication_compassion/__manifest__.py index 0652335dd..c7a989924 100644 --- a/partner_communication_compassion/__manifest__.py +++ b/partner_communication_compassion/__manifest__.py @@ -51,6 +51,7 @@ "data/communication_config.xml", "data/utm_data.xml", "data/ir_cron_data.xml", + "data/report_print_delay.xml", "views/communication_job_view.xml", "views/disaster_alert_view.xml", "views/partner_compassion_view.xml", diff --git a/partner_communication_compassion/data/report_print_delay.xml b/partner_communication_compassion/data/report_print_delay.xml new file mode 100644 index 000000000..23768edb9 --- /dev/null +++ b/partner_communication_compassion/data/report_print_delay.xml @@ -0,0 +1,17 @@ + + + + + 0 + + + 0 + + + 0 + + + 0 + + diff --git a/sbc_compassion/models/correspondence_page.py b/sbc_compassion/models/correspondence_page.py index 47d44963d..ba85717ee 100644 --- a/sbc_compassion/models/correspondence_page.py +++ b/sbc_compassion/models/correspondence_page.py @@ -237,17 +237,21 @@ def _get_cloundinary_image(self, cloudinary_url): "correspondence_id.cloudinary_final_letter_url", ) def _compute_cloudinary_url(self): + # The stacked image of a letter is analysed once for all its pages + stacked_images = {} for page in self: page.cloudinary_original_page_url = page._get_cloudinary_url( - page.correspondence_id.cloudinary_original_letter_url + page.correspondence_id.cloudinary_original_letter_url, stacked_images ) page.cloudinary_final_page_url = page._get_cloudinary_url( - page.correspondence_id.cloudinary_final_letter_url + page.correspondence_id.cloudinary_final_letter_url, stacked_images ) - def _get_cloudinary_url(self, cloudinary_url): + def _get_cloudinary_url(self, cloudinary_url, stacked_images=None): """ Returns the cloudinary url with the correct crop so that the page is displayed + :param stacked_images: optional dict to share the analysis of a stacked image + between the pages of a letter """ # Pages added to hold overflowing text are not part of the letter image letter_pages = self.correspondence_id.page_ids @@ -257,24 +261,39 @@ def _get_cloudinary_url(self, cloudinary_url): scanned_index = list(scanned_pages).index(self) + 1 if cloudinary_url.endswith("p1.jpg"): return cloudinary_url.replace("p1.jpg", f"p{scanned_index}.jpg") - try: - response = requests.get(cloudinary_url, timeout=3) - if not response.ok: - return False - image = Image.open(BytesIO(response.content)) - image.load() - except (requests.RequestException, OSError): - _logger.warning("Could not download %s", cloudinary_url, exc_info=True) - return False - bounds = self._find_page_bounds(image, len(scanned_pages)) + stacked_images = stacked_images if stacked_images is not None else {} + key = (cloudinary_url, len(scanned_pages)) + if key not in stacked_images: + stacked_images[key] = self._analyze_stacked_image( + cloudinary_url, len(scanned_pages) + ) + width, bounds = stacked_images[key] if not bounds: # No reliable crop: the page is fetched from Connect return False y_start, height = bounds[scanned_index - 1] - crop_operator = f"c_crop,h_{height},w_{image.width},x_0,y_{y_start}" + crop_operator = f"c_crop,h_{height},w_{width},x_0,y_{y_start}" url_parts = cloudinary_url.split("upload/") return f"{url_parts[0]}upload/{crop_operator}/{url_parts[1]}" + def _analyze_stacked_image(self, cloudinary_url, nb_pages): + """ + :return: tuple (width, bounds) of the stacked image, where bounds are the + (y_start, height) of the pages, empty if they can't be found. + """ + # A network error is not caught: the url is stored and would never be + # computed again. + response = requests.get(cloudinary_url, timeout=3) + if not response.ok: + return 0, [] + try: + image = Image.open(BytesIO(response.content)) + image.load() + except OSError: + _logger.warning("Not an image: %s", cloudinary_url, exc_info=True) + return 0, [] + return image.width, self._find_page_bounds(image, nb_pages) + @api.model def _find_page_bounds(self, image, nb_pages): """