-
-
Notifications
You must be signed in to change notification settings - Fork 314
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][IMP] Add analytic tags as filter for mis_builder #228
Conversation
I usually use analytic tags as subkpi (ex: departement or Business Units). |
@fclementic2c it should not break anything if you don't use the new filter. And these analytic filters are applied with a AND on move line domains that would already be in expressions. |
@benwillig could you review this one please as you are very familiar with this part of the code? |
ec5241e
to
8c05789
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review. LGTM
@apineux can you add |
Why not doing both filters as m2m. In tags is even more needed as it's usual to want to filter by several tags at the same time. It's not too much change at code level (in operator instead of =) and using m2m tags widget makes a similar UI. |
It was my first idea to use a m2m as filter but some people find confusing the fact that filtering with a m2m is searching with OR between each element selected in the m2m. M2o or M2m for these filter is a debatable topic. |
With a proper help it should be good explained, and having m2m serves for filtering one or several accounts/tags. This way, you can only filter by one. Other option is to provide a char field with domain widget for building advanced domains, or even to have 4 fields for AND and OR operations (not my best option, but preferred over not having the option). |
I am some people :) So this PR as it is now allows to filter on one tag, like one can filter on one analytic account, it's a simple extension of what exists now. Let's call this solution 0. If we allow to select more than one tag in the filter box, we have two possibilities
I'm ok with solution 0. I'm not in favor of 1. because it's a data model change, and more importantly it covers only some more use cases while raising a lot of questions at the same time (AND/OR, combinations, etc). The more I think to it the more I think solution 2. is best for this PR. And also, in another PR, investigate how hard it would be implement the domain widget. Or propose a selection of existing domain filters. That would be a completely generic solution. It can be implemented in two steps: first a domain filter in the backend view (with no widget in the preview), then add the domain filter in the preview. |
OK for solution 2 in this PR, but the OR is needed as well as said as I have that user case. Adding the domain widget is easy and I can put a JS developer for that if you want. |
Solution 2 has been implemented. Suggestion: I can add a checkbox (or selection field) to be able to choose between AND search or OR search for the tags. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rebase on 10.0 and adapt to preserve the operator?
Analytic tags are shown before analytic accounts in the widget. I kind of remember they are displayed after (or on the right of) analytic accounts in most places in Odoo. Could you check that too? |
6058f87
to
7967068
Compare
A few trailing remarks. I notice GitHub does not display them all correctly in context. They are best viewed in 7967068 |
140c883
to
c28edec
Compare
…context + UNIT tests
c28edec
to
f234a0e
Compare
@apineux tested on runbot instance http://3389032-228-9c516b.runbot1.odoo-community.org In the Demo Expenses report, when selecting an analytic account, everything works fine. Traceback (most recent call last): The display is better now than it was (cf. Julien's comment here above): the analytic tag is displayed in the field and not above anymore. |
@sbidoul @apineux I tested again with a new template to avoid the issue encoutered yesterday. |
Technical review and functional testing done at #OCAdays. Merging on 10.0-staging to do some integration testing before release. |
The improvement adds a new filter (on account.analytic.tag) in the mis builder preview.