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

[ADD] product_internal_reference_generator #309

Conversation

ilyasProgrammer
Copy link
Member

@ilyasProgrammer ilyasProgrammer commented Oct 26, 2023

This module allows to generate internal references for Product templates and variants using sequences, setting codes as read-only.

In product template, it's possible to choose among different Internal Reference Templates related to a sequence, and then generate an internal reference with the following structure:

Internal Reference Prefix + progressive number for variant, eg: “Main0001001”, where:

"Main0001" is the prefix generated by sequence and assigned to product template, and

"001" the variant identifier.

Every time a new variant is created, a new internal reference is automatically assigned with progressive variant code.

A specific access rights allows specific users to change internal reference template for a product template once an internal reference has been generated, as well as editing existing ones.

@ilyasProgrammer ilyasProgrammer force-pushed the 14.0-product_internal_reference_generator branch 2 times, most recently from 096e10a to bab2b14 Compare October 26, 2023 15:28
@ilyasProgrammer ilyasProgrammer force-pushed the 14.0-product_internal_reference_generator branch 3 times, most recently from bcc0e9f to f9eba98 Compare November 3, 2023 08:48
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional ok!

Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review, a few suggestions.

Thanks for your contribution, this must not have been easy to implement!

product_internal_reference_generator/__manifest__.py Outdated Show resolved Hide resolved
product_internal_reference_generator/__manifest__.py Outdated Show resolved Hide resolved
product_internal_reference_generator/views/product.xml Outdated Show resolved Hide resolved
@@ -0,0 +1,18 @@
{
"name": "Product Internal Reference Generator",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: should this exclude product_variant_default_code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review! Yes these modules makes no sence to be intalled together.

product_internal_reference_generator/views/product.xml Outdated Show resolved Hide resolved
@ilyasProgrammer ilyasProgrammer force-pushed the 14.0-product_internal_reference_generator branch 6 times, most recently from ece3380 to 6f7d75b Compare November 3, 2023 12:15
Copy link

@geomer198 geomer198 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix my comments.

@ilyasProgrammer ilyasProgrammer force-pushed the 14.0-product_internal_reference_generator branch from 6f7d75b to f7e2203 Compare November 3, 2023 12:54
Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review, LGTM. Nice refactor in the tests!

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link

@geomer198 geomer198 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional is good!

@francesco-ooops
Copy link
Contributor

@pedrobaeza can we proceed with this merge? thank you!

@pedrobaeza pedrobaeza added this to the 14.0 milestone Nov 8, 2023
@pedrobaeza
Copy link
Member

What's the difference with https://github.com/OCA/product-attribute/tree/14.0/product_sequence and why here instead of OCA/product-attribute?

@francesco-ooops
Copy link
Contributor

Hi @pedrobaeza ,

  1. first of all, we published here since it has a similar goal as https://github.com/OCA/product-variant/tree/14.0/product_variant_default_code so we thought this was the most correct repo - It's not easy to understand the difference between the two repos as both miss a repo description...
    We wouldn't mind switching repo if requested.

  2. secondly, yes in this case I completely missed the existence of the module you pointed out!
    But once again it's difficult from a functional perspective to evaluate whether an existing module does what you need when documentation only tells you how to setup, but not what the module actually does or the use cases that are covered - as is the case for the module you linked.

I tested product_sequence and it has a similar approach, but with the following downsides compared to this module:

  • Sequence is only managed by product category, you cannot have two products in the same category with a different sequence (on the contrary, this module can be extended to automatically assign a reference prefix - and we’ll probably do that)

  • Once an internal reference has been assigned to a product template, I don’t see any established flow to change it, as an error is raised on product.product . So if you assign the wrong category to the product by mistake, you’re stuck with the wrong sequence

  • Internal reference is not read-only for most users thus allowing getting out of the sequence, also allowing the risk of assigning a code that has yet to be assigned by the sequence - leading to an error further down the road

I agree that overall they are not very different, but also they're not the same. I think this could be merged and then see if there will be interest in future migrations to keep only one or both.

What do you think? :)

@pedrobaeza
Copy link
Member

OK, as asking you to integrate the missing features into the other module, you should at least move it to product-attribute repo. This one is for features specific to handle product variant things. The other module you pointed is because the references are assigned to the variants. It's true that the -attribute part of the repo name confuses, but it was done with other purpose, as the original idea was to have a repo for the product and its attributes, but not the variant attributes. For example, the manufacturer of the product, and similar things.

@francesco-ooops
Copy link
Contributor

@pedrobaeza thanks for your feedback, but it's not clear to me what's the next step: should we move this PR in product-attribute?

Is it ok to keep this as a new module? I don't think it's worth the effort try and improve the old one.

@ilyasProgrammer
Copy link
Member Author

moved to OCA/product-attribute#1453

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

Successfully merging this pull request may close these issues.

6 participants