Repository navigation
fix(notifications): resolve only registered notifiables from saved notification settings - #304
Merged
Merged
Conversation
…ic property or class A subject with a side-effect dispatch() method is named as dynamic:dispatch, directly and through a resolveDynamicNotifiable() that loads the property like fleetops Order does, and a class whose constructor records a call is named as a definition. The existing dynamic and class tests now register what they use.
…ttings resolveNotifiable() read the definition from the company's notification_settings, which saveSettings stores as given. dynamic:<property> reached any public no-argument method on the subject, through resolveDynamicNotifiable() or the Eloquent relation getter, so dynamic:dispatch ran Order::dispatch() whenever a notification fired; a class definition was built with app(). Definitions not registered with registerNotifiable() are now skipped.
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
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.
What
NotificationRegistry::resolveNotifiable()takes each notifiabledefinitionfrom the company's savednotification_settings.NotificationController::saveSettingsstores those settings after checking only that the input is an array, so whoever can save notification settings chooses the string.dynamic:<property>: the registry calls$subject->resolveDynamicNotifiable($property)or$subject->{$property}.Order::resolveDynamicNotifiable()ends in$this->load($property), which calls the method on a fresh instance.dynamic:dispatchrunsOrder::dispatch()every time an order notification fires, and that saves and queuesOrderDispatched. Any public method with no required arguments is reachable, for examplesaveInstance. The exception also stops the remaining recipients from being notified.app($definition), so the container builds any class named in the settings before theinstanceof Modelcheck.resolveNotifiable()now returnsnullunless the definition is registered withNotificationRegistry::registerNotifiable(). That covers the core defaults (User,Group,Role,Company) and whatever extensions register; fleetops registersContact,Driver,Vendor,Fleet,dynamic:customer,dynamic:driveranddynamic:facilitator. A leading\is ignored. Non-string definitions are skipped too; before, they crashed inStr::startsWith(). A new public helper,NotificationRegistry::isRegisteredNotifiable(), does the check.Why registration and not a relation check
getNotifiablesForCompany()listsstatic::$notifiables), so every definition saved through the console is already registered. Settings saved through the UI keep working.dynamic:companyordynamic:createdBy. It also depends on relation methods declaring aRelationreturn type, which fleetops models don't consistently do yet.Fleetbase\Support\RequestRelations::relationMethod()(branchfix/request-relation-allowlist) is not merged or pushed, so it can't be reused here.The existing tests resolved unregistered
dynamic:assignee,dynamic:dispatcherand unregistered fixture classes. They now register what they use, which is what an extension has to do.Compatibility
dynamic:name or notifiable class without registering it will stop notifying that recipient. Fleetops registers everything it uses, and nothing else in the Fleetbase packages callsregisterNotifiable()or definesresolveDynamicNotifiable().Validation
does not read an unregistered dynamic property from the subject:dispatch()ran on the subject, thenLogicException: ...::dispatch must return a relationship instance.does not pass an unregistered dynamic property to the subject resolver: an Order-styleload()calleddispatch()on a new instance, thenCall to a member function addEagerConstraints() on null.does not build an unregistered notifiable class:TypeErrorfromStr::startsWith()on a non-string definition. The class definition in the same test reachedapp().still resolves a registered dynamic property through the subject resolverpassed before and after.recognises only registered notifiable definitionscoversisRegisteredNotifiable().pest tests/Unit/Support/NotificationRegistryTest.php: 22 passed (58 assertions). Full suite: 2068 passed (12782 assertions; 46 deprecated, 8 warnings).php-cs-fixer --dry-runis clean on the changed files.phpstan(level max) reports nothing in the new code; the file's existing findings are unchanged.Docs and API reference
notifications/save-settingsendpoint is not in the collections, and its request and response shapes are unchanged. The StorefrontNotificationsfolder covers customer notification preferences, which is a different feature.needs-docs):content/docs/extension-development/recipes/registering-notifications.mdx, section 2, should say that dispatch only resolves registered notifiables, so everydynamic:name an extension'sresolveDynamicNotifiable()handles must be passed toregisterNotifiable().content/docs/fleet-ops/settings/notifications.mdxalready describes the options as registered notifiables, so it needs no change. This repo has noneeds-docslabel.Follow-up (not in this PR)
where($primaryKey, $key)with no company constraint. Settings could name a registered model (for example aUser) from another company. Scoping that needs care, because users belong to companies throughcompany_users.saveSettingscould drop unregistered definitions when settings are saved, not only when they're resolved.