-
Notifications
You must be signed in to change notification settings - Fork 269
[15.0][FIX] fieldservice: Fixed expected singleton on mass edit #1405
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: 15.0
Are you sure you want to change the base?
Conversation
peluko00
left a comment
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.
LGTM!
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.
Code reviwed LGTM
mpascuall
left a comment
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.
LGTM!
63f9208 to
cc6f232
Compare
fieldservice/models/fsm_order.py
Outdated
| self._calc_scheduled_dates(vals) | ||
| for record in self: | ||
| if vals.get("stage_id", False) and vals.get("is_button", False): | ||
| vals["is_button"] = False |
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.
get(x, False) is redundant, as the default result for a missing key is already a falsy value.
Does this if need to be inside the loop? It doesn't make any use of the iterated record.
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.
Hi,
I do not change the write logic, just solved an error when you try to mass update records that was throwing an expected singleton error.
I just added for record in self, and change the self._calc_scheduled_dates(vals) for record._calc_scheduled_dates(vals) to avoid that.
If you want, I can do this too, but it wasn't the main goal.
|
This PR has the |
ivantodorovich
left a comment
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.
Hello @BernatObrador .
I think this will not work correctly.
The issue you're trying to solve is that _calc_scheduled_dates is sometimes reading a record field (e.g.: self.scheduled_duration), and of course that doesn't work when self is a recordset.
However, _calc_scheduled_dates is modifying the vals dictionary. Meaning each record could potentially have a different dictionary.
IMO the solution is to get rid of _calc_scheduled_dates, and solve with computed fields and/or inverses.
Maybe even some onchanges.
I see 3 things that this method is doing:
- Computing the
scheduled_durationwhen bothscheduled_date_startandscheduled_date_endare set --> solution: makescheduled_durationcomputed - Modifying the
scheduled_date_startwhen onlyscheduled_date_endis changed, to respect the duration (shift the interval) --> IMO this behavior can be dropped, as we don't see this thing in other modules like the corecalendarfor example, it doesn't make sense from a UX point of view IMO - Modifying the
scheduled_date_endwhenscheduled_durationis modified andscheduled_date_startis set, to respect the duration (shift the interval) --> IMO this behavior can be dropped, as we don't see this thing in other modules like the corecalendarfor example, it doesn't make sense from a UX point of view IMO
For the last two, besides dropping this behavior, I would suggest switching the form widget to use daterange, and so align the UX to other Odoo core modules
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
cc6f232 to
a4a3d30
Compare
|
@ivantodorovich Could you please review the latest changes? I’ve moved the logic from _calc_scheduled_dates into separate onchange methods, and I also updated the view to use the date range widget as you suggested. Thanks! |
…scheduled_dates are a date_range)
a4a3d30 to
37daf01
Compare
Prevent "Expected Singleton" error when applying mass edit actions on FSM Orders.
Issue:
Mass editing multiple FSM Orders was causing a ValueError: Expected singleton due to the incorrect use of self in write, where self was assumed to be a singleton but was actually a recordset.
cc https://github.com/APSL HT02030
@miquelalzanillas @lbarry-apsl @mpascuall @peluko00 @javierobcn @ppyczko please review