Skip to content
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

[10.0] Product Variant Default Code / Product Attribute Priority / Product Code Builder Port Merge Mangle #244

Closed
wants to merge 66 commits into from

Conversation

gdgellatly
Copy link
Contributor

@gdgellatly gdgellatly commented Mar 27, 2017

Hi,

This is a port of product_variant_default_code from odoomrp-wip 8.0 as suggested by @pedrobaeza . Lots of commits as I didn't want to lose prior history.

Main changes
Rename of Reference Mask to Partcode Template (less technical)
General porting for API and changed views
Addition of tests

UPDATE:
As suggested I have merged functionality with product_code_builder and product_attribute_priority.

Obviously it is impossible to satisfy everyone and some things get dropped in favour of others.

As a base product_variant_default_code takes priority - it is more full featured and adaptable.
Anything related to sequences was moved to product_attribute_priority after filtering out the v9 PR of product attribute priority. Note that in V10 many parts of product attribute priorirty are no longer required.
From product code builder we took;

  • the demo data,
  • the searching without active_test and
  • the codes on attributes.

We also renamed fields that were common but named differently, preferring the product_code_builder fields.

  • All the computation from product_code_builder has been removed as well as all the manual code handling as product_variant_default_code handled it more intuitively and less expensively IMHO.
  • The default separator has been set to empty string to match product_code_builder.
  • The addition of attribute codes has been added to the default reference mask computation.
  • In order to accommodate the addition of attribute codes with the original functionality of product_variant_default_code the uniq constraint on code has been removed.

@sebastienbeau
Copy link
Member

sebastienbeau commented Mar 30, 2017

Hi not sure but this module have tha same aim as this existing OCA module (generation of variant code) : https://github.com/OCA/product-attribute/tree/8.0/product_code_builder
A PR for 9 is here : #204

@gdgellatly
Copy link
Contributor Author

@sebastienbeau They are doing something similar but different.

In the simple case they are the same, however this module is much closer functionally to the original product variant part code generator, whereas the product_code_builder does not support custom code formats. I think perhaps the better thing to do is maybe merge the functionality of the two and rename this module to product_code_builder. (I actually already did some of this).

For background:
I was a fairly active maintainer of product_variant_multi and use its functionality a lot. When I did a gap analysis this was one of the first things identified (lots more to come). I reviewed what was out there and nothing really replicated it completely. So I wrote my own using jinja2 and this concept was rejected by @pedrobaeza and told to port this one and had the concept closed without further discussion. So I looked at it, and while I didn't like the non standard approach to using a mask it did everything I needed, was a little simpler than using jinja, and better evolved so I ported it with very little change.

So for identical functionality as product_code_builder but with advanced features of reference_mask I think all that needs to change would be to adjust the default reference mask so it matches product_code_builder and improve a little of the code where product_code_builder takes a better approach (mainly checking for inactive), and probably take over the demo data as well.

@pedrobaeza
Copy link
Member

Hi, Graeme, I can't find the exact PR you are talking about (and sorry if I upset you by the PR closing), but since we talked, there has been other moves like the product_code_builder. As you say, that one is more limited in some aspects and better in others. If we merge both functionalities in one it will be the best of both worlds.

@gdgellatly gdgellatly changed the title Product Variant Default Code [10.0] Product Variant Default Code / Product Attribute Priority / Product Code Builder Port Merge Mangle Apr 13, 2017
@gdgellatly
Copy link
Contributor Author

@pedrobaeza @lmignon @bealdav I've tried to make it all work for everyone.

…ors although for one looks like travis has the error
@gdgellatly
Copy link
Contributor Author

Unfortunately I need these features now and its just a hassle waiting over 3 months for any review, so I've moved all this work over to a seperate repo
https://github.com/odoonz/product/tree/10.0 .

@gdgellatly gdgellatly closed this Jul 21, 2017
@lasley
Copy link

lasley commented Jul 21, 2017

@gdgellatly - why is a merge required for your workflow? I recommend you look at git-aggregator

@gdgellatly
Copy link
Contributor Author

@lasley I have been using while waiting, certainly it is useful for dev and even demo workflow, but I can't seriously put a completely unreviewed, unmerged OCA module in production where API might change subject to review.

Better to just say, well OK it didn't get reviewed or accepted, but its survived 2 months of extensive beta testing creating and managing a few hundred thousand products, it's good enough for me, lets just keep API as it is, fork and self host and just deal with any issues as they occur and try again next version.

Besides who wants to review a 3 month old PR anyway. Personally I think they should just be closed as a matter of process. If no one is interested in 3 months on a 12 month version cycle then it just wastes everyones time.

Well definitely for me anyway, after 3 months, the mental catchup to respond hurts my brain. I had some first reviews after 18 months the other month, couldn't even respond as I no longer even had a dev copy of that version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.