-
-
Notifications
You must be signed in to change notification settings - Fork 326
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
[13.0] [MIG] barcodes_generator_abstract: Migration to v13 #317
[13.0] [MIG] barcodes_generator_abstract: Migration to v13 #317
Conversation
* Rename manifest * Change openerp references to odoo * Bump version * Alphabetize imports * pyBarcode is no longer maintained, switch to viivakoodi fork (https://bitbucket.org/whitie/python-barcode/issues/16/pypi-08-release-request#comment-33978213)
* [FIX] barcodes_generator_abstract: Caching issue * Make cache method return ids & move the recordset method out of cache to fix OCA#93
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.
Diff review (750ac09?file-filters%5B%5D=.py)
LGTM. (Code review / no test)
thanks for porting this module.
:target: https://runbot.odoo-community.org/runbot/150/13.0 | ||
:alt: Try me on Runbot | ||
|
||
|badge1| |badge2| |badge3| |badge4| |badge5| | ||
|
||
This module expends Odoo functionality, allowing user to generate barcode |
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.
This module expends Odoo functionality, allowing user to generate barcode | |
This module extends Odoo functionality, allowing user to generate barcode |
@@ -99,54 +117,57 @@ class barcode_rule(models.Model): | |||
|
|||
Finally, you should inherit your model view adding buttons and fields. | |||
|
|||
Note |
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.
These Known issues / Roadmap no longer exist?
@@ -94,13 +91,11 @@ def create(self, vals): | |||
self._clear_cache(vals) | |||
return super(BarcodeRule, self).create(vals) |
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.
return super(BarcodeRule, self).create(vals) | |
return super().create(vals) |
@@ -94,13 +91,11 @@ def create(self, vals): | |||
self._clear_cache(vals) | |||
return super(BarcodeRule, self).create(vals) | |||
|
|||
@api.multi | |||
def write(self, vals): | |||
self._clear_cache(vals) | |||
return super(BarcodeRule, self).write(vals) |
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.
return super(BarcodeRule, self).write(vals) | |
return super().write(vals) |
try: | ||
import barcode | ||
except ImportError: | ||
_logger.debug("Cannot import 'python-barcode' python library.") | ||
barcode = None |
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.
try: | |
import barcode | |
except ImportError: | |
_logger.debug("Cannot import 'python-barcode' python library.") | |
barcode = None | |
import barcode |
Probably ImportError
it's impossible because if defined in manifest file.
record = super(BarcodeGenerateMixin, self).create(vals) | ||
if barcode_rule: |
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.
record = super(BarcodeGenerateMixin, self).create(vals) | |
if barcode_rule: | |
record = super().create(vals) | |
if record.barcode_rule_id: |
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.
The variable barcode_rule
is defined on same method, I think what you suggest is not correct. https://github.com/OCA/stock-logistics-barcode/pull/317/files#diff-f50419781cc0e7e9172d09cf83bef00972791e23a60c5d8579585367c4453301R43
I applied just the change on super()
@api.model | ||
def create(self, vals): | ||
self._clear_cache(vals) | ||
return super(BarcodeRule, self).create(vals) |
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.
return super(BarcodeRule, self).create(vals) | |
return super().create(vals) |
|
||
def write(self, vals): | ||
self._clear_cache(vals) | ||
return super(BarcodeRule, self).write(vals) |
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.
return super(BarcodeRule, self).write(vals) | |
return super().write(vals) |
@CarlosRoca13 @legalsylvain Isn't it time to add tests ? |
750ac09
to
da73444
Compare
@rousseldenis Ok, I will add them as soon as I can |
Thinking it better, This module needs that other depends on itself to work, so the test as in other versions should be in the dependent module. What do you think about that @rousseldenis ? |
@ernestotejeda @victoralmau changes done |
For testing abstract things, you can use for sure odoo-test-helper. |
da73444
to
3e9e251
Compare
@rousseldenis this is my first time using the odoo-test-helper I add one test |
@victoralmau Can you review it again? |
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
@pedrobaeza The merge process could not be finalized, because command
|
Currently translated at 29.6% (8 of 27 strings) Translation: stock-logistics-barcode-12.0/stock-logistics-barcode-12.0-barcodes_generator_abstract Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-barcode-12-0/stock-logistics-barcode-12-0-barcodes_generator_abstract/de/
Currently translated at 14.8% (4 of 27 strings) Translation: stock-logistics-barcode-12.0/stock-logistics-barcode-12.0-barcodes_generator_abstract Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-barcode-12-0/stock-logistics-barcode-12-0-barcodes_generator_abstract/sl/
Currently translated at 46.4% (13 of 28 strings) Translation: stock-logistics-barcode-12.0/stock-logistics-barcode-12.0-barcodes_generator_abstract Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-barcode-12-0/stock-logistics-barcode-12-0-barcodes_generator_abstract/it/
3e9e251
to
bc6273e
Compare
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at a1d20d3. Thanks a lot for contributing to OCA. ❤️ |
cc @Tecnativa TT26761
please @victoralmau @ernestotejeda review this