16.0 [IMP] maintenance_timesheet: use create_multi in account.analytic.line #390
Conversation
IT-Ideas
changed the title
IT-Ideas
force-pushed
the
16.0-maintenance_timsheet-create_multi-lst
branch
from
1bb5732 to
45bee4a
Compare
|
@pedrobaeza what do you think? |
| maintenance_request_ids = [ | ||
| vals.get("maintenance_request_id") | ||
| for vals in vals_list | ||
| if vals.get("maintenance_request_id") | ||
| ] | ||
| self._check_requests_done(maintenance_request_ids) | ||
| return super().create(vals_list) |
There was a problem hiding this comment.
Please don't complicate API with 2 methods with just an extra s (very easy to be typo error), and don't complicate logic. It's easier this way:
| maintenance_request_ids = [ | |
| vals.get("maintenance_request_id") | |
| for vals in vals_list | |
| if vals.get("maintenance_request_id") | |
| ] | |
| self._check_requests_done(maintenance_request_ids) | |
| return super().create(vals_list) | |
| for vals in vals_list: | |
| if vals.get("maintenance_request_id"): | |
| self._check_requests_done(vals["maintenance_request_id"]) | |
| return super().create(vals_list) |
There was a problem hiding this comment.
Hello @pedrobaeza , the problem to me is really that https://github.com/OCA/maintenance/pull/390/files#diff-ce85fd70c56eb1115afe99031137572f69d8b227218faad2d9119f9d5fcde50dR55 will make a distinct select for each record... The create_multi has been made in order to prevent those situations so I personally find that 'just' making a loop around the previous implementation is a bit sad...
I would rather change the name in order to avoid typo issue and keep my implementation. What name would you propose?
There was a problem hiding this comment.
@IT-Ideas We could keep the same method and document that it could be called with a single id or with a list of ids...
| maintenance_request_ids = [ | ||
| vals.get("maintenance_request_id") | ||
| for vals in vals_list | ||
| if vals.get("maintenance_request_id") | ||
| ] | ||
| self._check_requests_done(maintenance_request_ids) | ||
| return super().create(vals_list) |
There was a problem hiding this comment.
@IT-Ideas We could keep the same method and document that it could be called with a single id or with a list of ids...
| def _check_request_done(self, request_id): | ||
| """ | ||
| Editing a timesheet related to a finished request is forbidden. | ||
| """ |
There was a problem hiding this comment.
| def _check_request_done(self, request_id): | |
| """ | |
| Editing a timesheet related to a finished request is forbidden. | |
| """ | |
| def _check_request_done(self, request_id: int | list[int]): | |
| """ | |
| Editing a timesheet related to a finished request is forbidden. | |
| """ | |
| request_ids = [request_id] if isinstance(request_id, int) else request_id |
| self._check_requests_done([request_id]) | ||
|
|
||
| def _check_requests_done(self, request_ids): | ||
| """ | ||
| Editing a timesheet related to a finished request is forbidden. | ||
| """ |
There was a problem hiding this comment.
| self._check_requests_done([request_id]) | |
| def _check_requests_done(self, request_ids): | |
| """ | |
| Editing a timesheet related to a finished request is forbidden. | |
| """ |
IT-Ideas
force-pushed
the
16.0-maintenance_timsheet-create_multi-lst
branch
from
45bee4a to
55a4bf4
Compare
There was a problem hiding this comment.
As of the polymorphic approach is documented, it's the bet solution to take benefit of perf improvement without introducing duplicate code. The other option would be to keep only one method taking a list of ids as arg and update the existing call to the method accordingly...
|
/ocabot merge patch |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at 80a15c0. Thanks a lot for contributing to OCA. ❤️ |
No description provided.