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][IMP] Add analytic tags as filter for mis_builder #228

Merged
merged 5 commits into from
Oct 11, 2019

Conversation

apineux
Copy link
Contributor

@apineux apineux commented Sep 3, 2019

The improvement adds a new filter (on account.analytic.tag) in the mis builder preview.
Capture d’écran de 2019-09-03 14-59-45

@fclementic2c
Copy link
Member

I usually use analytic tags as subkpi (ex: departement or Business Units).
Think to test it will not break in these cases.

@sbidoul
Copy link
Member

sbidoul commented Sep 3, 2019

@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.

@sbidoul
Copy link
Member

sbidoul commented Sep 3, 2019

@benwillig could you review this one please as you are very familiar with this part of the code?

Copy link
Contributor

@benwillig benwillig 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

@sbidoul
Copy link
Member

sbidoul commented Sep 3, 2019

@apineux can you add 228.feature for then changelog?

@pedrobaeza
Copy link
Member

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.

@apineux
Copy link
Contributor Author

apineux commented Sep 3, 2019

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.

@pedrobaeza
Copy link
Member

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).

@sbidoul
Copy link
Member

sbidoul commented Sep 3, 2019

some people find confusing

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

  1. filter on move lines which have one of the selected tags: in this case the analytic account filter should be changed too to work the same way for symmetry reasons; this means a data model changes and a bigger impact on existing deployments
  2. filter on move lines which have all the selected tags: this is probably the most useful for people using tags for several analytic dimensions; for example if people use tags for, say, projects and departments, then it's useful to filter on project, or on departement, or on a project within a department; in this use case with multiple dimensions, doing a OR is probably counter productive

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.

@pedrobaeza
Copy link
Member

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.

@apineux
Copy link
Contributor Author

apineux commented Sep 4, 2019

Solution 2 has been implemented.
All the tags selected in the filter must belong to the record (account.move.line) so that the record is returned.

Suggestion: I can add a checkbox (or selection field) to be able to choose between AND search or OR search for the tags.
Let me know if it is a good idea.

Copy link
Member

@sbidoul sbidoul left a 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?

mis_builder/models/mis_report_instance.py Outdated Show resolved Hide resolved
@sbidoul
Copy link
Member

sbidoul commented Sep 29, 2019

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?

@apineux apineux force-pushed the 10.0-analytic_tag_filter branch from 6058f87 to 7967068 Compare October 1, 2019 07:43
@sbidoul
Copy link
Member

sbidoul commented Oct 1, 2019

A few trailing remarks. I notice GitHub does not display them all correctly in context. They are best viewed in 7967068

@apineux apineux force-pushed the 10.0-analytic_tag_filter branch from 140c883 to c28edec Compare October 1, 2019 08:01
@apineux apineux force-pushed the 10.0-analytic_tag_filter branch from c28edec to f234a0e Compare October 1, 2019 08:06
@sbidoul
Copy link
Member

sbidoul commented Oct 1, 2019

Thanks @apineux !

@vdewulf if you are still doing mis builder doc today, you might like to test (and document?) this one: it adds support for analytic tags.

@djulien19
Copy link
Contributor

Hey,
The tags appears above, instead of inside the box:
image

@vdewulf
Copy link
Contributor

vdewulf commented Oct 1, 2019

@apineux tested on runbot instance http://3389032-228-9c516b.runbot1.odoo-community.org
I activated the analytic accounting, added some invoices with analytic account "Camp to Camp - Camptocamp" and the "Contract" analytic tag, and others with analytic account "Administrative" and "R&D" analytic tag.

In the Demo Expenses report, when selecting an analytic account, everything works fine.
When I select an analytic tag (for example "Contract", but same error occurs with the other analytic tag) and click on refresh, I've got this error:

Traceback (most recent call last):
File "/.repo_requirements/odoo/odoo/http.py", line 642, in _handle_exception
return super(JsonRequest, self)._handle_exception(exception)
File "/.repo_requirements/odoo/odoo/http.py", line 684, in dispatch
result = self._call_function(**self.params)
File "/.repo_requirements/odoo/odoo/http.py", line 334, in _call_function
return checked_call(self.db, *args, **kwargs)
File "/.repo_requirements/odoo/odoo/service/model.py", line 101, in wrapper
return f(dbname, *args, **kwargs)
File "/.repo_requirements/odoo/odoo/http.py", line 327, in checked_call
result = self.endpoint(*a, **kw)
File "/.repo_requirements/odoo/odoo/http.py", line 942, in call
return self.method(*args, **kw)
File "/.repo_requirements/odoo/odoo/http.py", line 507, in response_wrap
response = f(*args, **kw)
File "/home/odoo/odoo-10.0/addons/web/controllers/main.py", line 895, in call_kw
return self._call_kw(model, method, args, kwargs)
File "/home/odoo/odoo-10.0/addons/web/controllers/main.py", line 887, in _call_kw
return call_kw(request.env[model], method, args, kwargs)
File "/.repo_requirements/odoo/odoo/api.py", line 689, in call_kw
return call_kw_multi(method, model, args, kwargs)
File "/.repo_requirements/odoo/odoo/api.py", line 680, in call_kw_multi
result = method(recs, *args, **kwargs)
File "/home/odoo/build/OCA/mis-builder/mis_builder/models/mis_report_instance.py", line 789, in compute
kpi_matrix = self._compute_matrix()
File "/home/odoo/build/OCA/mis-builder/mis_builder/models/mis_report_instance.py", line 781, in _compute_matrix
self._add_column(aep, kpi_matrix, period, period.name, description)
File "/home/odoo/build/OCA/mis-builder/mis_builder_budget/models/mis_report_instance.py", line 64, in _add_column
aep, kpi_matrix, period, label, description)
File "/home/odoo/build/OCA/mis-builder/mis_builder/models/mis_report_instance.py", line 750, in _add_column
aep, kpi_matrix, period, label, description)
File "/home/odoo/build/OCA/mis-builder/mis_builder/models/mis_report_instance.py", line 726, in _add_column_actuals_alt
no_auto_expand_accounts=self.no_auto_expand_accounts,
File "/home/odoo/build/OCA/mis-builder/mis_builder/models/mis_report.py", line 813, in declare_and_compute_period
aml_model)
File "/home/odoo/build/OCA/mis-builder/mis_builder/models/aep.py", line 349, in do_queries
['account_id', 'company_id'], lazy=False)
File "/.repo_requirements/odoo/odoo/models.py", line 1940, in read_group
result = self._read_group_raw(domain, fields, groupby, offset=offset, limit=limit, orderby=orderby, lazy=lazy)
File "/.repo_requirements/odoo/odoo/models.py", line 1963, in _read_group_raw
query = self._where_calc(domain)
File "/.repo_requirements/odoo/odoo/models.py", line 4070, in _where_calc
e = expression.expression(domain, self)
File "/.repo_requirements/odoo/odoo/osv/expression.py", line 643, in init
self.parse()
File "/.repo_requirements/odoo/odoo/osv/expression.py", line 821, in parse
raise ValueError("Invalid field %r in leaf %r" % (left, str(leaf)))
ValueError: Invalid field u'analytic_tag_ids' in leaf "<osv.ExtendedLeaf: (u'analytic_tag_ids', 'in', [1]) on mis_committed_purchase (ctx: )>"

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
Copy link
Member

sbidoul commented Oct 1, 2019

@vdewulf the error you got is linked to the commit purchases data source (from mis_builder_demo) which is not yet adapted for analytic tag (this is covered in #222). If the column involving committed purchases is removed, it should work better.

@vdewulf
Copy link
Contributor

vdewulf commented Oct 2, 2019

@sbidoul @apineux I tested again with a new template to avoid the issue encoutered yesterday.
Everything seems to work correctly. The analytic tag is well displayed, and the combination of filters (analytic account + analytic tag) works as well.
Just have to click on refresh button every time. Not an issue but could be a nice improvement for next year!

@sbidoul sbidoul changed the base branch from 10.0 to 10.0-staging October 11, 2019 17:05
@sbidoul
Copy link
Member

sbidoul commented Oct 11, 2019

Technical review and functional testing done at #OCAdays. Merging on 10.0-staging to do some integration testing before release.

@sbidoul sbidoul merged commit b0ab9cc into OCA:10.0-staging Oct 11, 2019
@sbidoul sbidoul deleted the 10.0-analytic_tag_filter branch October 11, 2019 17:06
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.

8 participants