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

[PORT] Migrate product_barcode_generator to 9.0 #32

Closed
wants to merge 13 commits into
base: 9.0
from

Conversation

Projects
None yet
7 participants
@yelizariev
Copy link
Member

yelizariev commented Jul 17, 2016

No description provided.

@yelizariev yelizariev changed the title [PORT] Migrate module to 9.0 [PORT] Migrate product_barcode_generator to 9.0 Jul 17, 2016

@@ -7,7 +7,7 @@
<field name="model">res.company</field>
<field name="inherit_id" ref="base.view_company_form"/>
<field name="arch" type="xml">
<page string="Configuration" position="inside">
<page name="configuration" position="inside">

This comment has been minimized.

@pedrobaeza

pedrobaeza Jul 17, 2016

Contributor

Don't remove string attribute

This comment has been minimized.

@yelizariev

yelizariev Jul 17, 2016

Member

Why? It's used for inheritance, the view doesn't add new Page

This comment has been minimized.

@yelizariev

yelizariev Jul 17, 2016

Member

You know, that it's impossible to inherit via string in 9.0

This comment has been minimized.

@pedrobaeza

pedrobaeza Jul 17, 2016

Contributor

Sorry, I though it was a new page. Sunday mistakes...

@@ -21,10 +21,10 @@

{
"name": 'Product barcode generator',
"license": "GPL-3",
"license": "AGPL-3",

This comment has been minimized.

@pedrobaeza

pedrobaeza Jul 17, 2016

Contributor

You should check if the license can be changed

<field name="name">EAN13 code</field>
<field name="code">product.ean13.code</field>
</record>
<!--<record id="seq_type_ean13_sequence" model="ir.sequence.type">-->

This comment has been minimized.

@pedrobaeza

pedrobaeza Jul 17, 2016

Contributor

Remove this instead of commenting

@@ -36,21 +36,20 @@ class ProductCategory(models.Model):
class ProductProduct(models.Model):
_inherit = 'product.product'

ean13 = fields.Char(copy=False)
ean_sequence_id = fields.Many2one('ir.sequence', string='Ean sequence')

This comment has been minimized.

@pedrobaeza

pedrobaeza Jul 17, 2016

Contributor

What's the meaning of the sequence here? Shouldn't be enough at company level (although I miss the possibility of putting more than one sequence there)?

This comment has been minimized.

@yelizariev

yelizariev Jul 17, 2016

Member

Check _get_ean_next_code -- it allows to use different sequence for the product. But it's strange to have a separate sequence on a product level.
Anyway, this PR is intended to port module rather than improve its functionality

This comment has been minimized.

@pedrobaeza

pedrobaeza Jul 17, 2016

Contributor

Each time we migrate a module, we can (and should) improve the functionality, because this is the best chance of rethinking things.

This comment has been minimized.

@yelizariev

yelizariev Jul 17, 2016

Member

I still believe that OCA should speed PR merging up someway. One of ideas is creating new issues instead of waiting until every concern is resolved

This comment has been minimized.

@pedrobaeza

pedrobaeza Sep 14, 2016

Contributor

OK, let's stay as is for now.

x620 and others added some commits Jul 18, 2016

x620
[FIX] Return license in the __openerp__.py, remov unused code in the …
…ean_sequence.xml, add contributor in README.rst
Merge pull request #4 from x620/9.0-product_barcode_generator_2
[FIX] 9.0 product_barcode_generator
@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Jul 18, 2016

Why does runbot make build without tests?
@gurneyalex

x620 and others added some commits Jul 18, 2016

Merge pull request #5 from x620/9.0-product_barcode_generator_2
[PORT] product_barcode_generator: migrate tests
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 18, 2016

Coverage Status

Coverage remained the same at 94.643% when pulling 0b9320f on it-projects-llc:9.0-product_barcode_generator into ca2b48d on OCA:9.0.

@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Jul 19, 2016

Runbot is not executing tests because we have already Travis for that, and this way, we make the builds lighter, faster, and cheaper (Travis infrastructure is free, Runbot one is not).

@moylop260

This comment has been minimized.

Copy link

moylop260 commented Jul 20, 2016

Could you check the lint build of travis?
https://travis-ci.org/OCA/stock-logistics-barcode/jobs/145529529

x620 and others added some commits Jul 21, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage remained the same at 94.643% when pulling 94674b4 on it-projects-llc:9.0-product_barcode_generator into ca2b48d on OCA:9.0.

x620 and others added some commits Jul 21, 2016

Merge pull request #7 from x620/9.0-product_barcode_generator_2
[FIX] product_barcode_generator: fix deprecated elements
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage remained the same at 94.737% when pulling 600eee0 on it-projects-llc:9.0-product_barcode_generator into ca2b48d on OCA:9.0.

@lasley lasley added the needs fixing label Aug 16, 2016

@lasley lasley added this to the 9.0 milestone Aug 16, 2016

@lasley

This comment has been minimized.

Copy link
Member

lasley commented Sep 13, 2016

@yelizariev - Can you take a look at the Travis build please? I'm 👍 other than that.

@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Sep 14, 2016

I need advice how to fix long line error in author attribute:

"author": 'IT-Projects LLC, Julius Network Solutions, Odoo Community Association (OCA)',

travis:

 product_barcode_generator/__openerp__.py:27:80: E501 line too long (92 > 79 characters)
@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Sep 14, 2016

Put:

    "author": 'IT-Projects LLC, '
              'Julius Network Solutions, '
              'Odoo Community Association (OCA)',
@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Sep 14, 2016

@pedrobaeza thank you!

@@ -2,21 +2,21 @@
<openerp>

This comment has been minimized.

@pedrobaeza

pedrobaeza Sep 14, 2016

Contributor

Put <odoo> and remove <data> tag

This comment has been minimized.

@yelizariev

yelizariev Sep 14, 2016

Member

Can someone make a script and fix modules (in all repositories) at once, instead of manual fixing each time?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 94.737% when pulling ec7c048 on it-projects-llc:9.0-product_barcode_generator into ca2b48d on OCA:9.0.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 13, 2016

Coverage Status

Coverage remained the same at 83.333% when pulling 3156649 on it-projects-llc:9.0-product_barcode_generator into ca2b48d on OCA:9.0.

@lasley

lasley approved these changes Oct 13, 2016

@lasley lasley added needs review and removed needs fixing labels Oct 13, 2016

@lasley

This comment has been minimized.

Copy link
Member

lasley commented Oct 13, 2016

Thanks @yelizariev

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Nov 9, 2016

Hi @yelizariev.

I just discovered this module you're porting. I realized recently an other module, named 'barcodes_generator_product' (and the same for partners). See #49.
I didn't know the module product_barcode_generator because when I began to make my module, I look for an existing OCA module but based on barcode.rule. (on odoo-code-search). And 'product_barcode_generator' do not extend barcode.rule. The module provides abstract feature to generate barcodes for any models. (product and partner are implemented for the time being, but we can imagine to generate barcodes for stock location, for stock picking, ...)
I took a look on this module and my PoV is that barcodes_generator_product provides same features, but in a more generic way. (barcode rule pattern).
I suppose that barcodes_generator_product could supersed product_barcode_generator for version 9.0 +, but I'd like to have your PoV about that point. Do the new module feat with your needs ? Maybe I missed some features that are not in the module you're porting. If yes, feel free to ask improvment on my PR.

Kind regards.

@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Nov 9, 2016

@legalsylvain I am far from this module and not able to compare it with yours. Let me rely on your opinion at this moment. We would return to this module or suggest improvements for your modules in the future if needed.

@rafaelbn

This comment has been minimized.

Copy link
Member

rafaelbn commented Jan 13, 2017

Hi,

We would like yo use this module https://github.com/OCA/stock-logistics-barcode/tree/9.0 or this PR for EAN-13. @yelizariev did you check if this PR is duplicated? If so maybe is better to close this PR

Let me know please

Thanks!

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Jan 13, 2017

Hi. Well, it's not decided for the time being, but I propose to replace this module (product_barcode_generator) by barcode_generator_product.

  • barcode_generator_product is available in 9.0 link
  • barcode_generator_product is being ported in 10.0 PR by @lasley
  • I'll port this module for V8.0 in a few weeks.

From my PoV, the new module is more generic, allow more features. But as I wrote it, I would prefer to let community compare and accept my proposal.

Any external point of view is so welcome. @rafaelbn, could you benchmark ?

Kind regards.

@lasley

This comment has been minimized.

Copy link
Member

lasley commented Jan 13, 2017

My thoughts point towards the abstract module too. Having already been able to add a new barcode type to an entity, I think this is a total win 👍

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Jan 17, 2017

Ok. I propose to close this PR. If you're agree, please accept too this #74. (and #73).

kind regards.

@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Jan 18, 2017

Closing this one as deprecated and following the other approach. Sorry for the time lost, @yelizariev, but we all think than the new approach is more consistent with Odoo itself than to continue with this one.

@pedrobaeza pedrobaeza closed this Jan 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment