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

[FIX][12.0] search_panel view: Do not apply model domain to comodel #1798

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

katyukha
Copy link

@katyukha katyukha commented Feb 2, 2021

Description

This commit fixes incorrect bechavior (error thrown) in case when
additional domain provided to action (ir.actions.act_window)
that displays view with search panel enabled.

Before this commit

For example we have following models:

  • My Category
  • My Model

And in category view, we have stat-button that display number of records
of records in this category. On click, it have to open view for My
Model, with domain like [('category_id', '=', 42)].
In this case, following error will be raised:

Error:
Odoo Server Error

Traceback (most recent call last):
  ...
  File "/opt/odoo/custom_addons/web_view_searchpanel/models/base.py", line 60, in search_panel_select_range
    hierarchical_naming=False).search_read(model_domain, fields),
  File "/opt/odoo/odoo/odoo/models.py", line 4615, in search_read
    records = self.search(domain or [], offset=offset, limit=limit, order=order)
  File "/opt/odoo/odoo/odoo/models.py", line 1581, in search
    res = self._search(args, offset=offset, limit=limit, order=order, count=count)
  File "/opt/odoo/odoo/odoo/models.py", line 4147, in _search
    query = self._where_calc(args)
  File "/opt/odoo/odoo/odoo/models.py", line 3939, in _where_calc
    e = expression.expression(domain, self)
  File "/opt/odoo/odoo/odoo/osv/expression.py", line 673, in __init__
    self.parse()
  File "/opt/odoo/odoo/odoo/osv/expression.py", line 854, in parse
    raise ValueError("Invalid field %r in leaf %r" % (left, str(leaf)))
ValueError: Invalid field 'category_id' in leaf "<osv.ExtendedLeaf: ('category_id', '=', 26) on bureaucrat_knowledge_category (ctx: )>"

Diagnostics

It seems that model domain was passed to comodel, thus system cannot
find field related to model in comodel. See code
(web_view_searchpanel/models/base.py", line 60, in
search_panel_select_range)

As tested, the search_domain causes this bug.

But if we look at implementation of same method
(search_panel_select_range) in Odoo 13.0 (see
code)
then we can see, that there is only empty domain applied for search.

Solution

It seems, that variable model_domain could be simply removed, and
we could do the search in comodel without any extra domain in this case.

So, this commit, only makes imlementation of this method look same as in
Odoo 13.0

After this commit

Everything is working fine.

Description
-----------

This commit fixes incorrect bechavior (error thrown) in case when
additional `domain` provided to action (`ir.actions.act_window`)
that displays view with search panel enabled.

Before this commit
------------------

For example we have following models:
- My Category
- My Model

And in category view, we have stat-button that display number of records
of records in this category. On click, it have to open view for My
Model, with domain like `[('category_id', '=', 42)]`.
In this case, following error will be raised:

```tracaback
Error:
Odoo Server Error

Traceback (most recent call last):
  ...
  File "/opt/odoo/custom_addons/web_view_searchpanel/models/base.py", line 60, in search_panel_select_range
    hierarchical_naming=False).search_read(model_domain, fields),
  File "/opt/odoo/odoo/odoo/models.py", line 4615, in search_read
    records = self.search(domain or [], offset=offset, limit=limit, order=order)
  File "/opt/odoo/odoo/odoo/models.py", line 1581, in search
    res = self._search(args, offset=offset, limit=limit, order=order, count=count)
  File "/opt/odoo/odoo/odoo/models.py", line 4147, in _search
    query = self._where_calc(args)
  File "/opt/odoo/odoo/odoo/models.py", line 3939, in _where_calc
    e = expression.expression(domain, self)
  File "/opt/odoo/odoo/odoo/osv/expression.py", line 673, in __init__
    self.parse()
  File "/opt/odoo/odoo/odoo/osv/expression.py", line 854, in parse
    raise ValueError("Invalid field %r in leaf %r" % (left, str(leaf)))
ValueError: Invalid field 'category_id' in leaf "<osv.ExtendedLeaf: ('category_id', '=', 26) on bureaucrat_knowledge_category (ctx: )>"
```

Diagnostics
-----------

It seems that model domain was passed to comodel, thus system cannot
find field related to model in comodel. See code
(web_view_searchpanel/models/base.py", line 60, in
search_panel_select_range)

As tested, the `search_domain` causes this bug.

But if we look at implementation of same method
(`search_panel_select_range`) in Odoo 13.0 (see
[code](https://github.com/odoo/odoo/blob/13.0/addons/web/models/models.py#L214))
then we can see, that there is only empty domain applied for search.

Solution
--------

It seems, that variable `model_domain` could be simply removed, and
we could do the search in comodel without any extra domain in this case.

So, this commit, only makes imlementation of this method look same as in
Odoo 13.0

After this commit
-----------------

Everything is working fine.
@katyukha
Copy link
Author

katyukha commented Feb 2, 2021

Hi @keshrath, @etobella,

Can you review this pull-request?

Thanks!

@katyukha
Copy link
Author

katyukha commented Feb 2, 2021

It seems that change, that added this bug was added via this commit (muk-it/muk_web@702eab7#diff-e6c9cddaa493ae1763ae87e87ddadb4f2d9108d8686b5b21c1f33b39e0ac025b)

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

I think we need to maintain the same logic than odoo

@katyukha
Copy link
Author

@etobella thank you for review.

And i have a question, what are the next steps to get this PR merged?

@etobella
Copy link
Member

Well, It would be interesting that someone checks this PR besides me.
As this is used by DMS, maybe we can ask kindly a review from @victoralmau

@victoralmau
Copy link
Member

I have reviewed this change in dms 12.0 and ran the tests and it does not seem to affect this change.
If you @etobella see the correct change from me too.
Ping @pedrobaeza to confirm it in case you know other addons use it and it can have side effects.

@pedrobaeza
Copy link
Member

The only OCA module that I know using it is DMS, but seems legit.

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

@pedrobaeza
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-1798-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c5f55a7 into OCA:12.0 Feb 12, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 5368e7a. 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.

None yet

5 participants