Repository navigation
fix: driver and vehicle observer cleanup, index latest status, contact address filter - #391
Merged
Merged
Conversation
Cached API lists hold every relation their query eager loads, and the whereHas filters that decide membership, but their keys were only versioned by the model's own table. Declare `$apiCacheDependencies` on the cached models (Order, Driver, Vehicle, Device, DeviceEvent, Vendor, ServiceArea) so a write to an embedded table invalidates the list. The property is read by fleetbase/core-api#301; until that is released it is unused. Waypoint, FleetDriver, FleetVehicle and AssetConnection had no cache trait, so their writes bumped nothing. They now use HasApiModelCache, which only registers the invalidation hooks: they have no HasApiModelBehavior, so none of them gains a cached list.
Order writes that skip model events left the cached order lists, and the order's own cached lookups, on the old version: dispatching, attaching a route, purchase rate or transaction, setting distance and time, scheduling, bulk driver assignment and unassignment, unassigning a deleted driver's or a vehicle's orders, saving recipient instructions and recording operational alerts. Each now invalidates the orders cache for the order's company after the write. The writes themselves stay quiet.
Raw inserts, query builder updates and deletes, and quiet saves bump no cache version, so lists embedding those tables kept serving stale rows. Invalidate the written table after each of them: - waypoints: raw insert, payload waypoint removal, orchestrator resequence - payloads: current waypoint and pickup/dropoff/return writes, service stop tracking numbers - tracking numbers and statuses: raw inserts, status pointer updates, and the owner's status and tracking number columns - entities and places: raw inserts and payload reassignment - drivers: vehicle unassignment and deletes cascaded from vehicles and users - fleet memberships, trailer detach, contact and issue quiet saves, customer reassignment to a vendor - order delete and restore cascades (payloads, entities, waypoints, tracking numbers and statuses) - telematics: a sweep that changes the connection status; the sync meta it rewrites every minute does not invalidate The writes themselves stay quiet.
…d online writes The driver and vehicle track endpoints write location quietly on every ping, so cached driver, vehicle and order lists (and storefront lists that embed vehicles) showed a location up to the list TTL old. Invalidating on every ping would keep those lists from ever being served from cache, so ApiCacheThrottle invalidates a table at most once a minute per company: a ping costs one Cache::add, and a cached location is at most a minute old. The online toggle is infrequent and invalidates on every call.
…dation Walk every declared relation path through the real models and check that named tables are exactly the morphTo targets' tables, so a renamed relation fails here rather than in production (fleetbase/core-api#301 throws on unresolvable entries only in debug). Check that the newly hooked models bump their table on writes, that Order::dispatch(), raw waypoint and tracking status inserts and waypoint removal invalidate, and that throttled invalidation runs once per window. The tests read the property directly and use only released core-api APIs, since CI installs the released package.
…leting them VehicleObserver::deleted soft-deleted every driver holding the vehicle, although its comment says it unassigns them. Clear their vehicle_uuid instead.
assignVehicle cleared vehicle_uuid on every driver holding the vehicle, this driver included, then saved a model that was not dirty, so the cleared value stayed. Skip this driver when unassigning the others.
… driver CompanyUserObserver matched drivers by user alone, so removing a user from one organization deleted their driver profiles in every organization, and a status change could land on another organization's driver. Match on the membership's company too, as HandleUserRemovedFromCompany already does.
…bership goes UserObserver, CompanyUserObserver and HandleUserRemovedFromCompany deleted drivers with a query builder delete, so DriverObserver never ran: orders stayed assigned to the deleted driver and a managed login account was never released. Delete each driver through the model instead; its deleted event also invalidates the API cache, so the manual invalidation goes.
The index resource took the first loaded tracking status as the latest, but the relation has no order, so lists usually reported the oldest one (Created). Pick the newest by created_at, then id.
ContactFilter::address queried an addresses relation that Contact does not have, so filtering contacts by address threw. Search the contact's place, the address the contacts list shows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes five bugs from an automated audit of the server code. I reproduced each one with a failing test before fixing it. Each fix is its own commit.
This branch is built on #390 (
fix/api-cache-dependencies), so merge #390 first. #390 rewrites the same lines inDriver::assignVehicle,VehicleObserver,UserObserverandCompanyUserObserver. Until #390 merges, this PR's diff also shows its commits.What changed
VehicleObserver::deleted). The comment says the drivers are unassigned, but the code soft-deleted every driver holding the vehicle. Now it sets theirvehicle_uuidto null.Driver::assignVehicle). The builder update clearedvehicle_uuidon every driver holding the vehicle, including this one. The save that followed was not dirty, so nothing wrote the vehicle back. The update now skips this driver.CompanyUserObserver). It matched drivers by user only. Removing someone from company A therefore deleted their driver profile in company B too, and a status change in A could land on B's driver. It now also matches on the membership's company, asHandleUserRemovedFromCompanyalready does. This is a separate commit because item 4 depends on it.DriverObserver(UserObserver,CompanyUserObserver, and the same pattern inHandleUserRemovedFromCompany). Builder deletes fire no model events. As a result, orders stayed assigned to the deleted driver and the driver's managed login account was never released. Each driver is now deleted through the model. The model'sdeletedevent also invalidates the API cache, so fix(api-cache): invalidate cached lists when the tables they embed are written #390's manual invalidation in these helpers is removed.latest_statuswas usually the oldest status (Http/Resources/v1/Index/Order.php). It tooktrackingStatuses->first(), but the relation has no ordering, so lists generally showed "Created". It now picks the newest status bycreated_at, then byidfor statuses created in the same second.ContactFilter::address). It queried anaddressesrelation thatContactdoesn't have, so?address=returned a 500. It now searches the contact'splace, which is the address the contacts list shows.Behaviour to review
DriverObserverdoes whenever a driver is deleted (ProfileAccountManager::releaseForProfile). Staff accounts linked to a driver are untouched. A user who still has a driver or contact profile in another organization keeps their account, which item 3 makes safe.Tests
New tests run against an in-memory SQLite database:
Unit/Observers/VehicleObserverDeleteTestUnit/Models/DriverAssignVehicleTestUnit/Observers/DriverDeletionCleanupTest: registers the realDriverObserverand covers the user, membership and listener paths.Unit/Observers/FleetOpsCompanyUserAndServiceAreaObserversTestUnit/Http/Resources/IndexOrderResourceTestUnit/Http/Filter/ContactFilterAddressTestExisting tests that asserted the old behaviour were updated, because the old behaviour was the bug:
ObserverListenerSeamsTestasserted that vehicle deletion soft-deletes drivers.ControllerFilterContractsTestasserted theaddressesrelation.ObserverContractsTest,EventContractsTestandHandleUserRemovedFromCompanyTestwere changed to match the renamed or re-signed helpers.Validation
php-cs-fixer --dry-runis clean on all changed files.server/testssuite, run file by file: 2,730 tests. 13 fail, and the same 13 fail on fix(api-cache): invalidate cached lists when the tables they embed are written #390's branch without these commits (tracking page, notifications, telematics prune, position replay). None touch this change.mixedones already present acrossserver/src.Docs / API impact
There are no request or response schema changes, so neither
fleetbase/postmannorfleetbase/fleetbase.ioneeds updating:latest_statusandlatest_status_codekeep their shape and now carry the correct value.addressfilter now works as its name implies instead of returning a 500.