-
Notifications
You must be signed in to change notification settings - Fork 23.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] stock: fix text product label reports with special characters #152657
[FIX] stock: fix text product label reports with special characters #152657
Conversation
@robodoo override=ci/security |
@robinengels Ok for me, I can't override the runbot warning though (as far as I know at least). You will probably have to contact the runbot guys for that. @robodoo r+ |
@ticodoo you may want to rebuild or fix this PR as it has failed CI. |
@robinengels also based on looking at the pictures the customer posted, it looks like they're also trying to print unicode (e.g. "è", "ç", etc. since it's in French) characters which aren't fixed by your PR. You probably just need to add |
93160c5
to
1dc197b
Compare
@odoo/rd-runbot Hello, do I need to write an upgrade script because I added |
Upgrade exception #302 added. Waiting the forward-port of this pr, the exception will be applied on all builds, and create an inconsistent state for migrations. If you need to apply any change before it reaches master, please notify runbot team. Details:
|
@ticodoo you might need to r+ it again |
@robinengels your current fix is no longer correct. Please consider the case where more than 1 product is having its label printed |
bcebc02
to
93bf6f6
Compare
@ticodoo This should be good now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I still found some issues:
- You forgot about the
custom_barcodes
section (used for printing made from a picking). They're still 2-tuple, so in the template it will crash as you're trying to access[2]
. - A similar change needs to be applied to
default_code
as well, otherwise it could still display the same oddities as in the name.
Make sure to properly test the different use-cases as those labels can be generated in different places. 😉
93bf6f6
to
8ef9a9d
Compare
8ef9a9d
to
215ecf0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid not. 🙁
Please, take the time to properly test your solution.
def _get_markup_default_code(self, product): | ||
if product.default_code and len(product.default_code) > 15: | ||
return (markupsafe.Markup(product.default_code[:15]), markupsafe.Markup(product.default_code[15:30])) | ||
else: | ||
return (markupsafe.Markup(product.default_code or '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any good reason to go through that hassle instead of just splitting it in the label like it was done before? Given that you still need to proceed in two separate ways anyway once in the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to mark it as markup safe before putting it in the template. So I thought it would be cleaner to do it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about the markup part. What I meant was to do the conversion like it is done with display_name_markup
and then splice it in the template directly. Little less code for the same end result.
'display_name_markup': markupsafe.Markup(product.display_name), | ||
'default_code': self._get_markup_default_code(product) | ||
} | ||
quantity_by_product[product] += (barcodes_qtys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it still in a tuple since above you append a dict?
Edit: Woops, only meant to select line 49.
barcodes_qtys[0] = { | ||
'barcode': barcodes_qtys[0][0], | ||
'quantity': barcodes_qtys[0][1], | ||
'display_name_markup': markupsafe.Markup(product.display_name), | ||
'default_code': self._get_markup_default_code(product) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As hinted by the plural form of barcodes_qtys
, it is a list. Meaning that there could be more than a single lot in there. 😉
714ab1b
to
dd8689d
Compare
@clesgow I hope it's good now, I tested printing labels of a delivery with multiple products and different lots for each products. I also tested printing labels directly on product page and same for lots. Did I miss a place where we could print labels ? |
@clesgow I added some test, let me know if I should add some more cases. |
c36f27a
to
61c0965
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last thing and we're good to go:
Since the ZPL code is not very easy to understand from a simple look, could you add a little description of what your tests are supposed to test ? Since the error tests The rendering is not good
isn't really explicit on what went wrong / what was actually checked.
addons/stock/tests/test_report.py
Outdated
def test_product_label_custom_barcode_reports(self): | ||
report = self.env.ref('stock.label_product_product') | ||
target = b'\n\t\t\n\n\n\n\n\n^XA^CI28\n^FT100,80^A0N,40,30^FD[C4181234""154654654654]Mellohi"^FS\n\n^FT100,115^A0N,30,24^FDC4181234""15465^FS\n^FT100,150^A0N,30,24^FD4654654^FS\n\n\n\n^FO100,160^BY3\n^BCN,100,Y,N,N\n^FDscan""me^FS\n\n^XZ\n\n\n\n^XA^CI28\n^FT100,80^A0N,40,30^FD[C4181234""154654654654]Mellohi"^FS\n\n^FT100,115^A0N,30,24^FDC4181234""15465^FS\n^FT100,150^A0N,30,24^FD4654654^FS\n\n\n\n^FO100,160^BY3\n^BCN,100,Y,N,N\n^FDscan""me^FS\n\n^XZ\n\n\n\n\n\n\n^XA^CI28\n^FT100,80^A0N,40,30^FD[C4181234""154654654654]Mellohi"^FS\n\n^FT100,115^A0N,30,24^FDC4181234""15465^FS\n^FT100,150^A0N,30,24^FD4654654^FS\n\n\n\n^FO100,160^BY3\n^BCN,100,Y,N,N\n^FD123barcode^FS\n\n^XZ\n\n\n\n^XA^CI28\n^FT100,80^A0N,40,30^FD[C4181234""154654654654]Mellohi"^FS\n\n^FT100,115^A0N,30,24^FDC4181234""15465^FS\n^FT100,150^A0N,30,24^FD4654654^FS\n\n\n\n^FO100,160^BY3\n^BCN,100,Y,N,N\n^FD123barcode^FS\n\n^XZ\n\n\n\n\n' | ||
rendering, qweb_type = report._render_qweb_text(self.product1.product_tmpl_id.id, {'custom_barcodes': {self.product1.product_tmpl_id.id: [("123barcode", 2)]}, 'quantity_by_product': {self.product1.product_tmpl_id.id: 2}, 'active_model': 'product.template'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Would be nice to try it with at least two custom_barcodes
, as it was this case that broke before.
addons/stock/tests/test_report.py
Outdated
self.assertEqual(target, rendering.replace(b' ', b''), 'The rendering is not good') | ||
self.assertEqual(qweb_type, 'text', 'the report type is not good') | ||
|
||
def test_product_label_without_default_code_reports(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the purpose of this test ? Is it solely to check:
https://github.com/odoo/odoo/blob/61c0965d1de8d185a7c7115a36433ce3a3b455fd/addons/stock/report/product_label_report.py#L26
Because if that's the case, not sure it warrants a test. 🙂
61c0965
to
5d3ed1b
Compare
Current behavior: When printing products ZPL Labels, special characters are not printed correctly. e.g. quotes become ' Steps to reproduce: - Modify a product name with special characters - Print a product label - Select ZPL Labels As the report is only rendered as text, the special characters are not dangerous and can be printed as is. opw-3684870
5d3ed1b
to
c600728
Compare
@robodoo r+ |
Current behavior: When printing products ZPL Labels, special characters are not printed correctly. e.g. quotes become ' Steps to reproduce: - Modify a product name with special characters - Print a product label - Select ZPL Labels As the report is only rendered as text, the special characters are not dangerous and can be printed as is. opw-3684870 closes #152657 Signed-off-by: Quentin Wolfs (quwo) <quwo@odoo.com>
@robinengels @clesgow this pull request has forward-port PRs awaiting action (not merged or closed): |
1 similar comment
@robinengels @clesgow this pull request has forward-port PRs awaiting action (not merged or closed): |
Hello @clesgow Can you please forwardport this asap? ;) Edit: just noticed that the forwarport pr was active nevermind |
@robinengels @clesgow this pull request has forward-port PRs awaiting action (not merged or closed): |
@robinengels @clesgow this pull request has forward-port PRs awaiting action (not merged or closed): |
Current behavior:
When printing products ZPL Labels, special characters are not printed correctly. e.g. quotes become '
Steps to reproduce:
As the report is only rendered as text, the special characters are not dangerous and can be printed as is.
opw-3684870
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr