[ADD] report_barcode_elaphe: Module to generate barcode with other lib #72

jefmoura · 2016-09-19T14:27:26Z

The current barcode generating method in Odoo uses the Reportlab and if I want to generate e.g a Pharmacode I cannot; because, the lib doesn't provide this type nor configurations for it. To provide more options and settings for barcodes, we create this module that uses the elaphe library and overrides the controller that makes barcode. It uses other library to generate barcodes but it will keep the current behavior and add options/settings at the same time.

oca-clabot · 2016-09-19T14:27:35Z

Hey @jefmoura, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

@jefmoura (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

jefmoura · 2016-09-22T07:26:08Z

I have already signed and sent back the Contributor License Agreement.
As you can see the new module needs a external dependency.
I'm waiting for the next steps.

lasley

Hi @jefmoura - thanks for the submission! I put some comments inline & will create an issue to resolve the Runbot dependency

lasley · 2016-10-19T18:48:52Z

report_barcode_elaphe/controllers/main.py

+        :param extraopts: Accepted different params per type of barcode. Eg: extraopts='{eclevel:M,version:8}'
+        """
+        try:
+            width, height, scale, margin = int(width), int(height), float(


These should all be declared on independent lines.

lasley · 2016-10-19T18:50:27Z

report_barcode_elaphe/controllers/main.py

+        :param scale: Accepted positive float values
+        :param extraopts: Accepted different params per type of barcode. Eg: extraopts='{eclevel:M,version:8}'
+        """
+        try:


This guard is too long & not very Pythonic IMO. Please try to isolate what you are guarding for, and leave everything else outside of it. Suggestions are to move the variable declarations and nested try/except out of this try/except

lasley · 2016-10-19T18:51:40Z

report_barcode_elaphe/controllers/main.py

+                extra_opts = {}
+                if kw.get('extraopts', False):
+                    for opt in kw['extraopts'].split(','):
+                        key = opt.split(':')[0]


opt.split is being called a lot of times on the same object. I recommend you assign it to a variable and use that instead

lasley · 2016-10-19T18:53:52Z

report_barcode_elaphe/README.md

@@ -0,0 +1,61 @@
+![Licence](https://img.shields.io/badge/licence-AGPL--3-blue.svg)


Please remove this file, only the RST is necessary

lasley · 2016-10-19T18:54:15Z

report_barcode_elaphe/__init__.py

@@ -0,0 +1,21 @@
+# -*- coding: utf-8 -*-
+##############################################################################


Please use the shorter header & apply to all - https://github.com/OCA/maintainer-tools/blob/master/template/module/__init__.py#L3

lasley · 2016-10-19T19:00:37Z

report_barcode_elaphe/controllers/main.py

+
+try:
+    from elaphe import barcode
+except Exception:


Always try to be as specific in your guards as possible. except ImportError:

lasley · 2016-10-19T21:23:23Z

report_barcode_elaphe/controllers/main.py

+                        key = opt.split(':')[0]
+                        if opt.split(':')[1] == 'True':
+                            values = True
+                        elif opt.split(':')[1] == 'False':


IMO you should use openerp.tools.safe_eval instead of this set of conditionals. Will be more flexible & more efficient

lasley · 2016-10-19T21:28:03Z

report_barcode_elaphe/controllers/main.py

+
+    @route()
+    def report_barcode(self, type, value, width=0, height=0,
+                       humanreadable=0, **kw):


Completely re-implementing a method is not desirable in a lot of instances, particularly when unknown modules could depend on, or add to via inheritance, implementation on the parent. You can't guarantee module load order, so inconsistent results are common if not calling the superclass.

I would prefer if there were an argument to explicitly use this implementation & send to super if not defined. This will guard against unpredictable behavior while still allowing for the fanciness that this module provides 😄

lasley · 2016-10-19T21:30:36Z

report_barcode_elaphe/controllers/main.py

+                description='Cannot convert into barcode.')
+
+        return request.make_response(barcode_out.getvalue(),
+                                     headers=[('Content-Type', 'image/png')])


Please add a newline to end of file - https://robots.thoughtbot.com/no-newline-at-end-of-file

lasley · 2016-10-19T21:32:53Z

report_barcode_elaphe/readme.rst

+
+=====================
+Report Barcode Elaphe
+=====================


Please use OCA ReadMe template (Usage section, newer bug tracker note, etc) - https://raw.githubusercontent.com/OCA/maintainer-tools/master/template/module/README.rst

lasley · 2016-10-19T21:36:46Z

@jefmoura - Travis dependency you can resolve. Just add a pip install elaphe in the Travis file install section, after the travis_install_nightly

We will also need some tests in order to make sure everything works. Please let me know if you have questions on how to test. What to test should become apparent once Travis passes and Codecov drops a comment in this PR.

Also note I did the review here, but I think this module may be better in the stock-logistics-barcode repo. @pedrobaeza , @OCA/logistics-maintainers - thoughts?

pedrobaeza · 2016-10-20T06:38:14Z

About Travis, better to add a regular requirements.txt file like Python packages.

About the repo, this is not something for dealing with barcodes in the backend, but for "printing" them, so I think this is the right place.

lasley · 2016-10-20T15:18:03Z

Thanks for the confirmation @pedrobaeza

@jefmoura - Let's perform the QA process here, then we can migrate to the correct repo for merge. Thanks again for the contribution! Drop a message here when you're ready for another review or need help.

pedrobaeza · 2016-10-20T15:22:23Z

@lasley, I think you haven't read correctly my comment: this is the correct repository.

lasley · 2016-10-20T16:04:12Z

Oh whoops - I should stop reading my Github messages until after the morning coffee 😆

jefmoura · 2016-12-14T12:39:39Z

@lasley I took long to reply but I'm almost done with the changes. I have one question: How to test it? I have created unit test as you can see here 07218a5
Is it enough? Thanks!

lasley

Thanks for the update @jefmoura

Looking at the code, there's still a few missing spots in coverage. Biggest one is in the controller. I typically just mock out the parent of the controller I'm testing then go from there. You can then just call the methods you need, providing the proper returns from parent when you need it.

There's also some failing lints - https://travis-ci.org/OCA/reporting-engine/jobs/183906326#L461

Realistically I'm also good with pushing the controller tests back to the next migration in order to speed up merge. You're already raising the coverage of the repo quite a bit here. Lints are a blocker though definitely.

lasley · 2016-12-28T21:09:33Z

report_barcode_elaphe/models/report.py

+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
+
+from openerp import models
+from openerp.tools.safe_eval import safe_eval as eval


I would prefer this left as the name safe_eval. Overriding namespaces you don't own isn't good IMO & when I saw it later in the code I had to jump back up to the imports to see if it was being done right. Best to leave it explicit

github-actions · 2021-10-24T12:23:04Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

report_barcode_elaphe module

c1f092a

lasley suggested changes Oct 19, 2016

View reviewed changes

lasley added the needs review label Oct 19, 2016

lasley modified the milestone: 8.0 Oct 19, 2016

lasley mentioned this pull request Oct 19, 2016

Pip install elaphe on Runbot #81

Closed

lasley added needs fixing and removed needs review labels Oct 20, 2016

Jeferson Moura added 2 commits October 27, 2016 09:22

Changes after the code review in #72 and improvements

09d4021

Added unit tests

07218a5

Solved Travis dependency

f9caea6

lasley reviewed Dec 28, 2016

View reviewed changes

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 24, 2021

github-actions bot closed this Nov 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADD] report_barcode_elaphe: Module to generate barcode with other lib #72

[ADD] report_barcode_elaphe: Module to generate barcode with other lib #72

jefmoura commented Sep 19, 2016

oca-clabot commented Sep 19, 2016

jefmoura commented Sep 22, 2016 •

edited

lasley left a comment

lasley Oct 19, 2016

lasley Oct 19, 2016

lasley Oct 19, 2016

lasley Oct 19, 2016

lasley Oct 19, 2016

lasley Oct 19, 2016

lasley Oct 19, 2016

lasley Oct 19, 2016

lasley Oct 19, 2016 •

edited

lasley Oct 19, 2016

lasley commented Oct 19, 2016 •

edited

pedrobaeza commented Oct 20, 2016

lasley commented Oct 20, 2016

pedrobaeza commented Oct 20, 2016

lasley commented Oct 20, 2016

jefmoura commented Dec 14, 2016 •

edited

lasley left a comment •

edited

lasley Dec 28, 2016

github-actions bot commented Oct 24, 2021

		@@ -0,0 +1,61 @@
		![Licence](https://img.shields.io/badge/licence-AGPL--3-blue.svg)

		@@ -0,0 +1,21 @@
		# -- coding: utf-8 --
		##############################################################################

[ADD] report_barcode_elaphe: Module to generate barcode with other lib #72

[ADD] report_barcode_elaphe: Module to generate barcode with other lib #72

Conversation

jefmoura commented Sep 19, 2016

oca-clabot commented Sep 19, 2016

jefmoura commented Sep 22, 2016 • edited

lasley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley Oct 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley commented Oct 19, 2016 • edited

pedrobaeza commented Oct 20, 2016

lasley commented Oct 20, 2016

pedrobaeza commented Oct 20, 2016

lasley commented Oct 20, 2016

jefmoura commented Dec 14, 2016 • edited

lasley left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 24, 2021

jefmoura commented Sep 22, 2016 •

edited

lasley Oct 19, 2016 •

edited

lasley commented Oct 19, 2016 •

edited

jefmoura commented Dec 14, 2016 •

edited

lasley left a comment •

edited