[19.0][MIG] fieldservice_route: Migration to 19.0 - #1552
Conversation
|
/ocabot migration fieldservice_route |
|
@max3903 are you able to include the previous commit history from earlier versions? |
[IMP] fieldservice_route fieldservice_route 12.0.1.1.0
[FIX] fieldservice_route
fieldservice_route 12.0.2.3.0
- Add a lower security group whose members only have access to their own field service orders. - Add related partner to location and worker forms to easily check the relationship. fieldservice_route 12.0.2.4.0
Translated using Weblate (Turkish) Currently translated at 7.6% (5 of 65 strings) Translation: field-service-14.0/field-service-14.0-fieldservice_route Translate-URL: https://translation.odoo-community.org/projects/field-service-14-0/field-service-14-0-fieldservice_route/tr/ Translated using Weblate (Turkish) Currently translated at 100.0% (65 of 65 strings) Translation: field-service-14.0/field-service-14.0-fieldservice_route Translate-URL: https://translation.odoo-community.org/projects/field-service-14-0/field-service-14-0-fieldservice_route/tr/
Translated using Weblate (Spanish (Argentina)) Currently translated at 100.0% (65 of 65 strings) Translation: field-service-14.0/field-service-14.0-fieldservice_route Translate-URL: https://translation.odoo-community.org/projects/field-service-14-0/field-service-14-0-fieldservice_route/es_AR/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: field-service-15.0/field-service-15.0-fieldservice_route Translate-URL: https://translation.odoo-community.org/projects/field-service-15-0/field-service-15-0-fieldservice_route/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: field-service-16.0/field-service-16.0-fieldservice_route Translate-URL: https://translation.odoo-community.org/projects/field-service-16-0/field-service-16-0-fieldservice_route/
e8009d8 to
ab60113
Compare
ab60113 to
7dc212a
Compare
Sadiq-OSI
left a comment
There was a problem hiding this comment.
The latest update fixes the module import error and allows the tests to run. However, both Odoo and OCB checks remain failing, with a confirmed day-route assignment failure. I also found issues with double scheduled-date calculation, inconsistent direct day-route assignments, and planned-start calculation when the default team is used. Please address the inline comments and ensure both test jobs pass.
| vals.update({"fsm_route_id": location.fsm_route_id.id}) | ||
|
|
||
| if vals.get("person_id") and vals.get("scheduled_date_start"): | ||
| vals = self._manage_fsm_route(vals) |
There was a problem hiding this comment.
An explicitly provided dayroute_id is overwritten here by _manage_fsm_route(). This is confirmed by the current CI failure: test_dayroute_order_count provides a day route, but that route keeps order_count = 0. Please preserve an explicit day-route selection and make both Odoo and OCB test jobs pass.
| "scheduled_duration", | ||
| "scheduled_date_start", | ||
| } & write_vals.keys(): | ||
| self._calc_scheduled_dates(write_vals) |
There was a problem hiding this comment.
This calculates the scheduled dates here, but the inherited fsm.order.write() calculates them again. For example, setting a two-hour order starting Tuesday at 00:30 to duration 0 can select Tuesday’s day route during the first calculation, then store Monday 22:30 during the second calculation. Please calculate the dates only once and add a regression test for zero duration across a date boundary.
|
|
||
| if "dayroute_id" in write_vals: | ||
| dayroutes_to_check |= self.dayroute_id | ||
| res = super().write(write_vals) |
There was a problem hiding this comment.
Direct assignment accepts any day route without checking its date, route, or worker. The added test_write_direct_dayroute_id_clears_old_dayroute demonstrates this by assigning an order to a day route seven days later while leaving the order’s scheduled date unchanged. Please validate compatibility or synchronize the order fields when moving it directly.
| team = ( | ||
| self.env["fsm.team"].browse(vals["team_id"]) | ||
| if vals.get("team_id") | ||
| else self.env["fsm.team"] |
There was a problem hiding this comment.
When team_id is omitted, this calculates the planned start with an empty team, so _planned_start_from_date() falls back to env.company before super().create() applies the actual default team. Because date_start_planned is explicitly supplied, that value can remain based on the wrong company calendar. Please calculate it after defaults are applied, or resolve the actual default team first, and test creation without an explicit team_id.
7dc212a to
64edb37
Compare
Sadiq-OSI
left a comment
There was a problem hiding this comment.
The module is mostly in good shape and the main test/pre-commit checks are passing, but I found a few issues that should be addressed.
The main concern is that direct dayroute_id writes can attach an order to a day route with a different date or route. I also noticed a multi-company defaulting issue where day routes use env.user.company_id instead of the active company, portal users are granted more ir.sequence read access than needed, and remaining capacity may become stale when a route’s max_order changes.
There are also failing Codecov checks. They seem coverage-related rather than functional test failures, but adding focused tests for these cases would likely help with both the review issues and the coverage gates and will also pass those codecov checks that are currently failing.
| self._unlink_empty_dayroutes(dayroutes_to_check) | ||
| return res | ||
|
|
||
| if "dayroute_id" in write_vals: |
There was a problem hiding this comment.
This direct dayroute_id branch accepts the new day route without checking that its date and route match the order. A user could attach a Monday order to a Tuesday day route, leaving the order schedule and day route inconsistent. Can we validate the target day route or synchronize the order schedule when dayroute_id is written directly?
|
|
||
| def _default_team_id(self): | ||
| teams = self.env["fsm.team"].search( | ||
| [("company_id", "in", (self.env.user.company_id.id, False))], |
There was a problem hiding this comment.
This uses env.user.company_id instead of env.company. In multi-company use, a user working in another active company could get a day route defaulted to the wrong company’s team/calendar. Can this follow the base FSM default pattern and search using the active company?
| access_fsm_route_dayroute_user,fsm.route.dayroute.user,model_fsm_route_dayroute,fieldservice.group_fsm_user,1,1,0,0 | ||
| access_fsm_route_dayroute_dispatcher,fsm.route.dayroute.dispatcher,model_fsm_route_dayroute,fieldservice.group_fsm_dispatcher,1,1,1,1 | ||
| access_fsm_route_dayroute_portal,access.fsm.route.dayroute.portal,model_fsm_route_dayroute,base.group_portal,1,0,0,0 | ||
| access_ir_sequence_portal,access.ir.sequence.portal,base.model_ir_sequence,base.group_portal,1,0,0,0 |
There was a problem hiding this comment.
This grants portal users read access to ir.sequence, which exposes internal sequence configuration beyond what is needed to view their own day routes. Can we remove this access unless there is a specific portal flow that requires it?
| fallback = tzinfo.localize(datetime.combine(route_date, time(8, 0))) | ||
| return fallback.astimezone(utc).replace(tzinfo=None) | ||
|
|
||
| @api.depends("route_id", "order_ids") |
There was a problem hiding this comment.
order_remaining is computed from rec.max_order, but the dependency list does not include max_order or route_id.max_order. If the route capacity changes, existing day routes can keep stale remaining capacity and the assignment domain may reuse or skip routes incorrectly. Can we add the missing dependency?
c7f0542 to
1408740
Compare
Sadiq-OSI
left a comment
There was a problem hiding this comment.
The earlier portal security, route-domain, cleanup, calendar, multi-company, capacity, and CI test issues are largely addressed. I found four remaining consistency cases in fsm.order:
- route worker changes do not recompute the stored order assignee;
- route-day selection uses the UTC date instead of the worker's local date;
- explicit day routes are not validated or synchronized during create;
- combined direct writes can preserve person or date values that conflict with the selected day route.
Please address the inline comments and add focused regression tests before merging. The Odoo, OCB, and pre-commit checks pass, while codecov/project remains failing.
| readonly=False, | ||
| ) | ||
|
|
||
| @api.depends("fsm_route_id") |
There was a problem hiding this comment.
Changing fsm.route.fsm_person_id does not invalidate this stored compute because the dependency only watches fsm_route_id. The day route already depends on route_id.fsm_person_id, so changing a route worker leaves the order assigned to the old person while its day route moves to the new person.
Could you include fsm_route_id.fsm_person_id in the dependency and add a regression test for changing a route's worker?
| vals.get("scheduled_date_start"), DEFAULT_SERVER_DATETIME_FORMAT | ||
| ).date() | ||
| elif isinstance(vals.get("scheduled_date_start"), datetime): | ||
| date = vals.get("scheduled_date_start").date() |
There was a problem hiding this comment.
scheduled_date_start is stored as a UTC-naive datetime, so calling .date() here selects the UTC day rather than the worker's local route day. I reproduced a Toronto order scheduled Tuesday at 23:30 being treated as Wednesday and rejected by a Tuesday-only route.
Could you convert the scheduled start to the worker or route timezone before extracting the date and add a boundary regression test?
| if ( | ||
| vals.get("person_id") | ||
| and vals.get("scheduled_date_start") | ||
| and "dayroute_id" not in vals |
There was a problem hiding this comment.
Skipping _manage_fsm_route() preserves an explicitly provided dayroute_id, but the create path never calls _prepare_vals_from_dayroute(). Creating an order from a day route can therefore leave scheduled_date_start empty, and API creates can accept a day route whose route, date, or person does not match the order.
Could you validate and synchronize explicit day routes during create and add regression tests?
| ) | ||
| if dayroute.person_id and "person_id" not in write_vals: | ||
| write_vals["person_id"] = dayroute.person_id.id | ||
| if dayroute.date and "scheduled_date_start" not in write_vals: |
There was a problem hiding this comment.
This leaves a remaining gap in the earlier direct-assignment review. _prepare_vals_from_dayroute() copies the day-route worker and date only when those fields are absent. A write that supplies dayroute_id together with another person_id or scheduled_date_start succeeds and leaves the order inconsistent with the selected day route.
Could you either make the day route authoritative or reject conflicting values, with a combined-write regression test?
1408740 to
a6e1643
Compare
| local = local.replace( | ||
| year=dayroute.date.year, | ||
| month=dayroute.date.month, | ||
| day=dayroute.date.day, | ||
| ) | ||
| return local.astimezone(utc).replace(tzinfo=None) |
There was a problem hiding this comment.
local.replace() keeps the timezone offset from the original date. For example, moving a Toronto appointment at 10:00 AM from July to January results in 9:00 AM because the July UTC offset is retained. Can you re-localize the local time using the target date so the displayed appointment time remains 10:00 AM? Please add a regression test that moves an order across a daylight-saving boundary.
| if not dayroute: | ||
| return write_vals | ||
| order_route = self._order_route_for_dayroute_write(write_vals) | ||
| if dayroute.route_id and order_route and dayroute.route_id != order_route: |
There was a problem hiding this comment.
This validation only rejects a mismatch when both the order and day route have a route. A Route A order can still be assigned to a day route with no route, leaving the records inconsistent. Could we compare the route values even when one side is empty and reject one-sided mismatches? Please add regression tests for both routed-order -> route-less-day-route and route-less-order -> routed-day-route.
| readonly=False, | ||
| ) | ||
|
|
||
| @api.depends("fsm_route_id", "fsm_route_id.fsm_person_id") |
There was a problem hiding this comment.
Changing the route worker now recomputes the order’s person_id, but it does not re-evaluate the existing day-route date using the new worker’s timezone. For example, changing from a Toronto worker to a Los Angeles worker can make the appointment fall on the previous local day while it remains attached to the original day route. Could we synchronize or reassign the day route when the route worker changes and add a timezone-boundary regression test?
| new_dayroute = self.env["fsm.route.dayroute"].browse( | ||
| write_vals.get("dayroute_id") or [] | ||
| ) | ||
| write_vals = self._prepare_vals_from_dayroute(new_dayroute, write_vals) |
There was a problem hiding this comment.
When dayroute_id is written together with scheduled_duration or scheduled_date_end, the end time is calculated from the old start date before this code moves the start to the new day route. I reproduced an order being stored with a January 22 start, January 15 end, and a duration of -166 hours. Could you calculate the end and duration after establishing the new day-route start, and add a regression test for this combined write?
| current_start = ( | ||
| write_vals.get("scheduled_date_start") or self.scheduled_date_start | ||
| ) | ||
| write_vals["scheduled_date_start"] = self._scheduled_start_on_dayroute_date( |
There was a problem hiding this comment.
Changing dayroute_id updates scheduled_date_start, but it does not correctly update scheduled_date_end. I reproduced a two-hour 09:00–11:00 order becoming 09:00–09:00 while still storing a two-hour duration. Could you keep the start, end, and duration synchronized and add a regression test for moving an existing order to another day route?
| ("person_id", "=", values["person_id"]), | ||
| ("date", "=", values["date"]), | ||
| ("route_id", "=", values.get("route_id") or False), | ||
| ("order_remaining", ">", 0), |
There was a problem hiding this comment.
This capacity condition excludes the order’s current day route when that route is full. As a result, simply changing the order’s duration creates a replacement day route and deletes the original one. Could you preserve the current matching day route because the order already occupies one of its slots, and add a test using max_order = 1?
a6e1643 to
7a2a49f
Compare
| access_fsm_route_dispatcher,fsm.route.dispatcher,model_fsm_route,fieldservice.group_fsm_dispatcher,1,1,1,1 | ||
| access_fsm_route_day_user,fsm.route.day.user,model_fsm_route_day,fieldservice.group_fsm_user,1,0,0,0 | ||
| access_fsm_route_day_dispatcher,fsm.route.day.dispatcher,model_fsm_route_day,fieldservice.group_fsm_manager,1,1,0,0 | ||
| access_fsm_route_dayroute_user,fsm.route.dayroute.user,model_fsm_route_dayroute,fieldservice.group_fsm_user,1,1,0,0 |
There was a problem hiding this comment.
Regular Field Service users can reschedule orders, but the automatic day-route creation fails with an AccessError because they don’t have create access. Please update the permissions and add a test for this workflow.
| dayroute, | ||
| person=dayroute.person_id, | ||
| ) | ||
| write_vals = self._sync_scheduled_end_from_start(write_vals) |
There was a problem hiding this comment.
Creating an order with a day route, start time, and end time resets the end time to the start and sets the duration to zero. Please calculate the duration from the supplied start and end before synchronizing the dates, and add a regression test.
| [ | ||
| ("fsm_route_id", "in", self.ids), | ||
| ("scheduled_date_start", "!=", False), | ||
| ("dayroute_id", "!=", False), |
There was a problem hiding this comment.
When a worker is added to a previously unstaffed route, scheduled orders without a day route are skipped by this filter. The worker is assigned, but the orders remain outside any day route. Please include these orders in the reassignment.
| return self.fsm_route_id.id | ||
|
|
||
| def _get_person_id_for_dayroute(self, vals, route_id): | ||
| if vals.get("person_id"): |
There was a problem hiding this comment.
Setting person_id to false falls back to the previous worker, so the order remains attached to that worker’s day route after being unassigned. Please handle an explicit false value separately and clear the obsolete day route.
7a2a49f to
59e3024
Compare
| access_fsm_route_dispatcher,fsm.route.dispatcher,model_fsm_route,fieldservice.group_fsm_dispatcher,1,1,1,1 | ||
| access_fsm_route_day_user,fsm.route.day.user,model_fsm_route_day,fieldservice.group_fsm_user,1,0,0,0 | ||
| access_fsm_route_day_dispatcher,fsm.route.day.dispatcher,model_fsm_route_day,fieldservice.group_fsm_manager,1,1,0,0 | ||
| access_fsm_route_dayroute_user,fsm.route.dayroute.user,model_fsm_route_dayroute,fieldservice.group_fsm_user,1,1,1,0 |
There was a problem hiding this comment.
The create access fixes the original error, but regular Field Service users still get an AccessError when rescheduling an order that already has a day route. The cleanup tries to delete the old empty day route, but these users don’t have unlink access. Could yopu handle that cleanup with the appropriate permissions and add a test for rescheduling an already routed order?
| if not self.scheduled_date_start or not self.person_id: | ||
| return |
There was a problem hiding this comment.
Clearing the worker from a route still leaves its scheduled orders attached to the existing day route. This returns early when the order no longer has a worker, so the old day-route assignment is never cleared. Could you clean it up here and add a test for removing a route’s worker?
| class FSMLocation(models.Model): | ||
| _inherit = "fsm.location" | ||
|
|
||
| fsm_route_id = fields.Many2one(comodel_name="fsm.route", string="Route") |
There was a problem hiding this comment.
Changing a location’s route updates the scheduled order’s route and worker, but its day route still belongs to the previous route. Could you reassign existing scheduled orders when the location route changes and add a regression test for this case?
| def _unlink_empty_dayroutes(self, dayroutes): | ||
| dayroutes.filtered( | ||
| lambda dayroute: dayroute and not dayroute.order_ids | ||
| ).unlink() |
There was a problem hiding this comment.
When a regular Field Service user reschedules an order that already has a day route, this cleanup tries to delete the old day route. Those users do not have delete access, so the reschedule fails with an AccessError. Could you handle this automatic cleanup with the required permissions and add a test for this workflow?
69af211 to
7b98bf0
Compare
| route = self.env["fsm.route"].browse(route_id) | ||
| if route.fsm_person_id: | ||
| return route.fsm_person_id.id | ||
| return self.person_id.id or self.fsm_route_id.fsm_person_id.id |
There was a problem hiding this comment.
When an order is moved to a location with an unstaffed route, it keeps the previous worker and creates a stale day route. Please clear the day route in this case and add a regression test.
| ) | ||
| tzinfo = timezone(tz_name) | ||
| day_start = tzinfo.localize(datetime.combine(route_date, time.min)) | ||
| day_end = day_start + relativedelta(days=1) |
There was a problem hiding this comment.
Adding one day here keeps the old UTC offset across DST changes, which can exclude valid working hours. Please build the end time from the next local midnight and add tests for both DST transitions.
| def _get_person_id_for_dayroute(self, vals, route_id): | ||
| if "person_id" in vals: | ||
| return vals.get("person_id") or False | ||
| if route_id: |
There was a problem hiding this comment.
When an order is manually assigned to someone other than the route’s default worker, rescheduling it moves it to the default worker’s day route while keeping the manually assigned worker on the order. Could you preserve the order’s current worker unless the route or location changes, and add a regression test for this case?
Sadiq-OSI
left a comment
There was a problem hiding this comment.
Updating my review status to Request changes for the issues noted earlier
7b98bf0 to
9ff1e75
Compare
- Bump version to 19.0.1.0.0 and update translations for 19.0 - Update tests to use FSMCommon without demo data dependencies - Update self.env._() translation calls for Odoo 19.0 - Plan day route start from worker timezone and working calendar - Add unit tests for route, dayroute, order assignment, and constraints - Restrict portal day route access with a record rule - Include route_id in day route lookup domain - Clean up empty day routes after order write - Recompute planned start when worker calendars change - Add demo data with one XML file per model (route, location, dayroute, order) - Update copyright and author to Gray Matter Logic - Fix fsm.order write() bulk updates, unscheduling, and location changes - Restrict portal day route access to read-only - Fix route-less day route reuse, scheduled_date_end reassignment, and direct dayroute_id cleanup - Complete calendar dependencies and use team company calendar fallback - Keep dayroute assignment consistent across DST, capacity, and worker timezone changes - Allow FSM users to create day routes; preserve create start/end duration; staff unstaffed routes; clear dayroute on unassign - Sudo empty dayroute cleanup; clear dayroutes when route unstaffed; reassign on location route change - Harden multi-create dayroute test against UTC midnight boundaries - Clear dayroute on unstaffed location move; keep manual worker on reschedule; fix DST day-end search range - Add unit tests covering remaining fsm.order dayroute edge cases for coverage Squashed commits (preserved on backup/19.0-mig-fieldservice_route-pre-coverage-tests-amend): - 9ff1e75 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-july27-review-amend): - 7b98bf0 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-midnight-test-fix-amend): - 69af211 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-july24-review-amend): - 59e3024 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-july23-review-amend): - 7a2a49f [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-july22-review-amend): - a6e1643 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-july14-review-amend): - 1408740 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-ci-fix-amend): - ab60113 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-july3-amend): - e8009d8 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-write-fix-amend): - b05d0a5 [MIG] fieldservice_route: Migration to 19.0 Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-credits-squash): - f2f5a63 [MIG] fieldservice_route: Migration to 19.0 - 05dc45a [UPD] fieldservice_route: update copyright and author to Gray Matter Logic Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-demo-squash): - 16cf058 [MIG] fieldservice_route: Migration to 19.0 - 3f71187 [IMP] fieldservice_route: add demo data Earlier history (preserved on backup/19.0-mig-fieldservice_route-pre-squash): - 19c1171 [MIG] fieldservice_route: Migration to 19.0 - 4ed9bc2 [UPD] Update fieldservice_route.pot - b4d3842 [IMP] fieldservice_route: plan day route start from worker schedule - e892796 [IMP] fieldservice_route: add unit tests for route and dayroute logic - 957be85 [IMP] fieldservice_route: expand unit tests to reach 98% coverage - 9635b1a [FIX] fieldservice_route: address PR review on security and routing
9ff1e75 to
c07eb31
Compare
Migration of
fieldservice_routeto Odoo 19.0 following OCA maintainer-tools guidelines.Changes applied
19.0.1.0.0es,es_AR,es_CL,it,pt_BR,pt_PT,tr)FSMCommonwithout demo data dependenciesself.env._()translation calls for Odoo 19.0Checklist
Made with Cursor