-
-
Notifications
You must be signed in to change notification settings - Fork 45
[18.0][FIX] edi_core_oca: fix search logic #216
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
base: 18.0
Are you sure you want to change the base?
Conversation
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.
Can we have a test ?
To ensure that:
- final ordering is respected
- company visibility fix works
Gracias! :)
|
@ThiagoMForgeFlow gentle reminder |
Maintaining this part of code can occur a visual issue. If the result variable has 80 records (default by Odoo) and the orig_ids is less than result, Odoo only shows the orig_ids, and we lost the previous work finding next records.
2d26c4d to
8cce9bd
Compare
|
First of all, my apologies for the delayed response — I have been busy. I have just added a test that checks exactly what I mention in the PR. In this test, 100
Then, when performing a search with a limit of 80 (simulating Odoo view pagination), with only the first company active and using a non-superuser, the expected result is the first 60 records and the last 20. Keeping the line of code proposed to be removed in this PR, the actual result is only the first 60 records, because the additional 20 records (from the second pagination) are not part of |
|
@ThiagoMForgeFlow LGTM. Just one "efficiency" point: do we really need to create 100 records? Assuming we can control the |
Maintaining this part of code can occur a visual issue. If the result variable has 80 records (default by Odoo) and the orig_ids is less than result, Odoo only shows the orig_ids, and we lost the previous work finding next records.
In a multicompany environment, if you only enable one company you won't see all the records that you should. Real example:
Do the same logic proposed in 15: OCA/edi#1147
@ForgeFlow