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

[13.0][MIG] web_tree_many2one_clickable #1438

Merged
merged 15 commits into from
Nov 26, 2019

Conversation

Daemo00
Copy link
Contributor

@Daemo00 Daemo00 commented Nov 17, 2019

standard migration

pedrobaeza and others added 14 commits November 17, 2019 20:14
Clickable many2one fields for tree views
========================================

This addon provides a separate widget to allow many2one fields in a tree view
open the linked resource when clicking on their name.

You can also define a system parameter to have this behaviour for all the
existing many2one fields in tree views.

Installation
============

Install it the regular way.

Configuration
=============

If you want to have all many2one fields clickable by default, you have to
define in *Configuration > Technical > Parameters > System parameters*, a new
parameter with name `web_tree_many2one_clickable.default` and with value
`true`.

Usage
=====

For the widget option, you need to add `widget="many2one_clickable"` attribute
in the XML field definition in the tree view.

For example:

`<field name="partner_id" widget="many2one_clickable" />`

will open the linked partner in a form view.

Known issues / Roadmap
======================

* You cannot deactivate clickable behaviour for an specific many2one field if
  you configure the system parameter.
* The value of the system parameter is retrieved for each many2one field
  present in the view instead of only once.
If a list contains a node which is not a field (e.g. a button), it will
not be found in the fields so we'll have an error trying to get 'type'
from undefined.
@oca-clabot
Copy link

Hey @Daemo00, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

Copy link

@lfreeke lfreeke left a comment

Choose a reason for hiding this comment

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

Functional test 👍

Thank you

@pedrobaeza pedrobaeza added this to the 13.0 milestone Nov 18, 2019
@OCA-git-bot OCA-git-bot mentioned this pull request Nov 18, 2019
41 tasks
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I have tried on runbot and I don't get it working: no button when passing the mouse over the many2one like in 12.0:

Peek 18-11-2019 18-18

@Daemo00
Copy link
Contributor Author

Daemo00 commented Nov 18, 2019

Thanks @pedrobaeza and @lfreeke for the review! I can confirm that it is not working in the SO lines but it works for instance in the invoice lines.. Investigating deeper 🔍

@lfreeke
Copy link

lfreeke commented Nov 19, 2019

Thanks @pedrobaeza and @lfreeke for the review! I can confirm that it is not working in the SO lines but it works for instance in the invoice lines.. Investigating deeper

It is rather odd because everywhere else it is working.

@pedrobaeza
Copy link
Member

Hehe, I went directly to the only case it's failing by chance!

@hbrunn
Copy link
Member

hbrunn commented Nov 19, 2019

@pedrobaeza
Copy link
Member

thanks for the pointer, @hbrunn. I don't think so, unless that widget is defined in sale module. If it's in web module, we should inherit that widget too for adding the corresponding stuff for making it work.

@hbrunn
Copy link
Member

hbrunn commented Nov 19, 2019

it's in account (see link above). But I wonder why this doesn't pick up the changes in the prototype, theoretically, this should just work

@pedrobaeza
Copy link
Member

Well, if it can be done compatible without requiring glue module, then better to have it. But if not, at least add it to the ROADMAP for being aware.

@Daemo00
Copy link
Contributor Author

Daemo00 commented Nov 19, 2019

Hi, what I discovered is that what causes the strange behavior is actually the product_configurator widget on product_id that extends relationalFields.FieldMany2One, but this module overrides the methods of ListFieldMany2One.
I verified this by changing https://github.com/odoo/odoo/blob/22a11a6f4d0b08a56712fafd0f87a2e7bb5b00ec/addons/sale/static/src/js/product_configurator_widget.js#L18 in order to let it extend relationalFields.ListFieldMany2One.

With this change, the >> button is still not shown on field product_id because condition https://github.com/OCA/web/pull/1438/files#diff-f06f62892caad3befc7bded8af2e3c50R33 is not satisfied since the field product_id has noOpen = true, but I think this is anyway the expected behavior for this module.

I do not completely understand the product_configurator widget, would it make sense to propose a change in Odoo in order to let it inherit from relationalFields.ListFieldMany2One?
This is the first solution that I can see because as far as I know this module is explicitly intended for lists so it can't include relationalFields.FieldMany2One.

@pedrobaeza
Copy link
Member

Well, if no_open is defined, then it doesn't matter about the inheritance, so for me we can continue. I can't say about JS inheritance though.

@pedrobaeza pedrobaeza dismissed their stale review November 19, 2019 18:21

Reason for not showing up found

@Daemo00
Copy link
Contributor Author

Daemo00 commented Nov 19, 2019

Yep I can confirm that field product_id has no_open: https://github.com/odoo/odoo/blob/1b95bfe202576583d6458c6bd1a707dd988e6d95/addons/sale/views/sale_views.xml#L445

Noticing that in the beginning would have saved a few headaches about JS debugging :)

@pedrobaeza
Copy link
Member

@hbrunn do you approve this as is then or do you think it's worth to mention something in the ROADMAP?

@Daemo00
Copy link
Contributor Author

Daemo00 commented Nov 19, 2019

Anyway I'll try to submit a PR with the mentioned change to JS inheritance of the product_configurator because I've noticed that it is only used in there, we'll see what (or better, if) they respond.

@hbrunn
Copy link
Member

hbrunn commented Nov 19, 2019

I'm not sure the proposal to change the base class will be successful, because then it would be awkward to use the widget in some other view type. Like kanban, this is what lists degrade to nowadays in responsive mode. Ah, this is something you should check: How does the addon perform in responsive mode? I'd expect we won't have a link, so you'll probably have to override the kanban version too.

We'll run into this kind of situations repeatedly in the future due to the way widget classes are selected by the new framework (which is super well done btw in my opinion). I think you can solve this by adding some function _tree_many2one_clickable_setup_list to FieldMany2One, and override _renderReadonly there to call this function if we're rendered by a list or kanban view. In the override of ListFieldMany2One, call the function unconditionally. Then if some widget doesn't do something wrong or intentionally very different, it should work.

In this specific case the widget probably relies on https://github.com/odoo/odoo/blob/22a11a6f4d0b08a56712fafd0f87a2e7bb5b00ec/addons/sale/static/src/js/product_configurator_widget.js#L35 being called when rendering in read mode, you'll have to see how much trouble this causes.

Another option would be to iterate through the fields registry and depending on some conditions (inherits from FieldMany2One but not from List... and doesn't have it's own List...) just patch the function into the objects there, but I feel this might be a path to pain and misery.

@Daemo00
Copy link
Contributor Author

Daemo00 commented Nov 19, 2019

How does the addon perform in responsive mode? I'd expect we won't have a link

Exactly (invoice view):
image
But maybe this addon named web_tree_... should only focus on tree views and leave kanban views to other dedicated addons..

This said, big thanks for the proposed solutions :) that comes from some serious problem solving skills

@pedrobaeza
Copy link
Member

As it is planned (appearing when you hover over the field), I don't think we should provide an alternative on responsive mode (where there's no hover as you are on a mobile). For me it's OK to limit to current scope.

@Daemo00 Daemo00 force-pushed the 13.0-mig-web_tree_many2one_clickable branch from cd05543 to c8299a3 Compare November 24, 2019 21:45
@Daemo00 Daemo00 force-pushed the 13.0-mig-web_tree_many2one_clickable branch from c8299a3 to ca1690f Compare November 26, 2019 18:09
@pedrobaeza
Copy link
Member

/ocabot merge

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-1438-by-pedrobaeza-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 26, 2019
Signed-off-by pedrobaeza
@OCA-git-bot OCA-git-bot merged commit ca1690f into OCA:13.0 Nov 26, 2019
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at eb1c726. Thanks a lot for contributing to OCA. ❤️

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.