[10.0][IMP] event: Make event report inheritable. #16633
Conversation
| event_event e | ||
| def _from(self): | ||
| from_str = """ | ||
| event_event e |
There was a problem hiding this comment.
Include here FROM word
sergio-teruel
force-pushed
the
10.0-PR-event
branch
from
4c5ddcf
to
79bce5f
Compare
|
@pedrobaeza Changes done. |
| FROM | ||
| event_event e | ||
| LEFT JOIN event_registration r ON (e.id=r.event_id) | ||
| event_event e LEFT JOIN event_registration r ON (e.id=r.event_id) |
There was a problem hiding this comment.
Why changing here the 2 lines?
There was a problem hiding this comment.
Because it fit and it is short
There was a problem hiding this comment.
Yeah, but making changes here the priority is to modify the minimum number of lines, and it doesn't comply with general rule applied in other reports (one line, one table)
| tools.drop_view_if_exists(self.env.cr, 'report_event_registration') | ||
| # TOFIX this request won't select events that have no registration | ||
| def _select(self): | ||
| select_str = """SELECT |
There was a problem hiding this comment.
Don't change here the indentation
sergio-teruel
force-pushed
the
10.0-PR-event
branch
from
79bce5f
to
7b61f7d
Compare
|
@pedrobaeza changes done |
|
@sergio-teruel Thanks for the feedback. By the way I don't understand why you make a query and a subquery for the SELECT. Furthermore, I won't do it in 10.0 but in master instead. Waiting your input. |
|
@tivisse Thanks for your review, In a first commit I did not use a subquery, but in other modules that depend of event I had needed make calculation over the fields, also by this way i can add other models to query and make compute fields in the query, i think this way is more cleaner. |
|
Well I still don't understand why you make a second request to retrieve all the elements you selected in the sub request. It's not correct performance wise. IMHO you should have only one select. |
|
@tivisse are there any chance that this can land on 10.0 if we fix it? Should we pass an OPW ticket? |
|
@pedrobaeza If we remove the first query, I have no objections to merge this in 10.0 :) |
pedrobaeza
force-pushed
the
10.0-PR-event
branch
from
420c9a4
to
9ceeec6
Compare
|
Hi @tivisse, I have fixed the PR, removing the subquery. I have also checked that the |
|
As the report doesn't exist anymore in master (I suppose to use a pivot view instead), I merged (exceptionally) your PR on 10.0 at 18152f3 Have a nice day. |
|
Thank you very much! I see the new approach of using pivot view over event.event with computed fields for the quantities. Indeed it seems better. |
Description of the issue/feature this PR addresses:
This PR makes event report inheritable for other modules as sale report
Current behavior before PR:
extend event repor tis not possible
Desired behavior after PR is merged:
You can extend the event report
--
@mart-e Can you take a look?. Thanks
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr
cc @Tecnativa