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] add product template multi link #210

Merged
merged 2 commits into from
Jan 10, 2018

Conversation

legalsylvain
Copy link
Contributor

@legalsylvain legalsylvain commented Oct 6, 2017

Add a new module product_template_multi_link based on the same concept as product_multi_link, but for template, instead of variants.

In many cases, link between templates is suficient and linking each variant could be very fastidious.

See README file for the complete description

Product form
product_template_form

Linking new template (tree editable)
product_template_link_tree_edit

Extra Kanban view, for better visualization
product_template_link_kanban

Note, i didn't changed product_multi_link, to avoid to break existing instance that use it since a while.

@pedrobaeza
Copy link
Member

Isn't better to handle both links in the same module?

@bealdav
Copy link
Member

bealdav commented Oct 7, 2017

+1 to work on template on this topic 90% of use cases.
+1 to make only one module but not blocking for me.

Thanks a lot for this valuable work.

@legalsylvain
Copy link
Contributor Author

Hi @pedrobaeza and @bealdav. Thanks for your review. Indeed it could be great to handle both links in the same module (or at least make one depend of the other). I began with that approach but the problem is that I can not change product_multi_links, without breaking current behaviour (product produt will be now optional, (for the time being mandatory). And the OCA module is used by many people, and via connector. So if I change 10.0 version, I will break some shops (magento / prestashop) in production.
So, the approach I had is to make a new module in V10.

For the V11, it will be maybe interesting to merge both modules ( the branch is currently empty with a simple migration script.) if it is possible and desired. I mean :

  • because I really think that it is not trivial to handle both cases and that having two simple modules, is maybe more simple,
  • because I don't think that both features will be used in the same time : some users want to link variants together, other (90% according to @bealdav) want to simply links templates.

kind regards.

@rvalyi
Copy link
Member

rvalyi commented Oct 7, 2017

cc @guewen as you wrote or touched product_multi_links may be you have an opinion on this.

Copy link
Member

@sebastienbeau sebastienbeau left a comment

Choose a reason for hiding this comment

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

Good work !
As the existing module will need some refactor and supporting both solution will be note that easy to do if we want to have a good UI for the end user !
I think it will be better to accept this module in 10 and then to refactor in 11.
Also I confirm that adding link on variant is most of the time useless (In reality I never had a customer that ask me this feature so...). Does someone have the case?

linked_product_template_image_small = fields.Binary(
related='linked_product_template_id.image_small')

type = fields.Selection(
Copy link
Member

Choose a reason for hiding this comment

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

maybe better to use link_type as type already mean something in python

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review. type renamed in link_type.

regard.

@legalsylvain
Copy link
Contributor Author

To summarize.

Solution 1 :

2 models. One with link between product.product (variant), one with product.template (products). this is the current approach of my PR. In that case, there is nothing to mutualise between the two models, so two modules seems to be the best way to avoid to install unnecessary features.
Todo for the V11.

  • rename product_multi_link in product_product_multi_link
  • Apply last UI improvment from produt_template_multi_link into product_product_multi_link. (smart button, kanban view, constraint, etc...)

Solution 2 :

1 models with :

  • source_template_id
  • source_product_id (now optional) and with domain and constraint on source_template_id
  • linked_template_id
  • linked_product_id (now optional) and with domain and constraint on linked_template_id

Impact :

  • more complex, specially for the UI
  • require extra groups (or other configuration settings) to enable links between variants and / or templates.
  • not possible to make a PR on V10, because setting required = false on product_id will break all the instance using this module.

So my PoV : let's go accept this PR for v10, because There Is No Alternative ;-). for the V11, I let e-commerce experts choosing what is the best choice. (I'm not an e-commerce expert), but I really think that the solution 2 is very more complicated on UI implementation. (overloading variant / template form, etc...) as said by @sebastienbeau.

CC Initial commiters : @atchuthan, @guewen, @sebastienbeau

kind regards.

@pedrobaeza pedrobaeza mentioned this pull request Oct 10, 2017
22 tasks
@bealdav
Copy link
Member

bealdav commented Oct 10, 2017

@legalsylvain about naming (not blocking).
Please choose a technical name the most similar to friendly name and 'Product Product Multi Link' sounds weird for end user and erp builder.
We mainly build modules for customers/users not for technical, I mean (customer centric). KISS ;-)

@legalsylvain
Copy link
Contributor Author

Hi @bealdav

Well for the time being (after my PR) the name will be :

  • product_multi_link (*); "Product Multi Links (Variants)"
  • product_template_multi_link "Product Multi Links (Template)"

(should be renamed into product_product_multi_link or product_variant_multi_link in V11, I thinks)

I don't think that the technical name should feat perfectly with the friendly name, because technical name should be understandable for technical people, and friendly name to Functional people. I don't think either it's an OCA convention. Exemple : https://github.com/OCA/web/tree/10.0/web_tree_dynamic_colored_field

But well, if it's not correct for you, please make a better precise proposal. (for the current V10 PR)

regards.

@bealdav
Copy link
Member

bealdav commented Oct 10, 2017

@legalsylvain

You're right it's not an OCA convention, but I don't want propose any guideline without acceptance of the community.
You're example is school use case:

technical : web_tree_dynamic_colored_field
friendly name : Colorize field in tree views

What if somebody provide a module named : colorize_field_in_tree_views outside of OCA maybe.

Some more ambiguous example could be found, for sure.

Globally in think developpers use friendly name instead of summary to precise the subject of the module.

In french there is proverb: call a cat, a cat Appeler un chat, un chat ;-)

It seems to me easier when simple things stay simple (in computer engineering some things are not simple).
I'm happy when it can be KISS.

But as told above, if I can't convince others, probably that my point of view isn't good enough, then no needs more guideline

Thanks for sharing you opinion @pedrobaeza @sebastienbeau etc, etc.

@bealdav
Copy link
Member

bealdav commented Oct 10, 2017

@legalsylvain as said not blocking for me

@legalsylvain legalsylvain force-pushed the 10.0_ADD_product_template_multi_link branch from 31fae83 to e69602d Compare October 10, 2017 21:35
Copy link
Sponsor Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

A great module!

@legalsylvain legalsylvain merged commit efaeec8 into OCA:10.0 Jan 10, 2018
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.

None yet

6 participants