fix(backend): serve the notifications the client already polls - #19
Conversation
The Flutter client ships with `notifications: true`, so its notifications screen and bell poll `api/v1/notifications` from the first sign-in, while the backend left `Features::notifications()` commented out and every poll was a 404. Enabling the feature alone was not enough. This app runs on integer keys, and the published notifications migration gave the table an auto-incrementing id, while Laravel's database channel always writes the notification's own UUID there: every `$user->notify()` failed at insert and the screen could only ever be empty. The table's key is now a UUID in both modes; the morph columns still follow `use_uuids`. The same fix goes into magic-starter-laravel's stub. An existing local database keeps the old table; `php artisan migrate:fresh` rebuilds it. Nothing could have written to it through the channel.
The fixed create migration only reaches a fresh database; its hasTable guard leaves a table an earlier migrate built with an auto-incrementing id, which the database channel still cannot write to. The rekey migration, copied by hand from magic-starter-laravel's unreleased stub (this app resolves 0.0.10, which does not ship it), rebuilds that table with a UUID key and carries any rows over; on a correct table it does nothing, and a run that stops part-way can be re-run. Against a copy of a real local database it turned the integer table into the UUID one with both indexes, and a notify() then listed over the API. add_sms_registered_at_to_users_table is the third migration the installer publishes with the notifications feature; it was never copied here, so the first OneSignal SMS registration would have failed on an unknown column.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Looks correct: I found no defects that should block the merge. The one gap is that no test runs the new rekey migration's rebuild path. I read all five changed files. The create stub now always writes In It assigns new ids to existing rows on purpose. That is safe because, as its comment says, those ids never came from the channel. The empty Minor
Tests
CI
|
The rekey is a hand-copied stub that drops and rebuilds a table, and under RefreshDatabase the create migration already builds a UUID table, so CI never ran its rebuild path. The test puts back the integer-keyed shape, inserts a row, runs up(), and checks the row survives under a UUID with both indexes and that the database channel writes again; forcing the auto-increment check to false turns it red. A second test pins the sms_registered_at column. DatabaseOnlyNotification moves to its own file, since a second test now uses it and PSR-4 cannot find a class declared inside another test's file.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Looks correct: the new commits fix the test gap from my last review and add no defects. What changed since my last review: the commits after
The new rekey test:
The test passes, and the The new column test: a second test asserts that The one path CI still does not run is the PostgreSQL primary-key rename in Tests
CI
|
1 participant
What changed
The backend enables
Features::notifications(), soapi/v1/notificationsandapi/v1/notification-preferencesexist. The notifications table is also keyed by UUID so the database channel can write to it.Why
The Flutter client ships with
notifications: true, so its notifications screen and bell poll these routes from the first sign-in, and every poll was a 404. Enabling the feature alone was not enough. This app runs on integer keys, and the published notifications migration gave the table an auto-incrementingid, while Laravel'sdatabasechannel always writes a UUID there, so everynotify()failed at insert. The package side is fluttersdk/magic-starter-laravel#44.$table->uuid('id')->primary()(fresh databases).rekey_notifications_table_by_uuidis copied by hand from that PR's stub, because this app resolves 0.0.10, which does not ship it. It repairs a database that already migrated the integer table.add_sms_registered_at_to_users_table, the third migration the installer publishes with the notifications feature, was never copied here; without it the first OneSignal SMS registration would fail on an unknown column.Evidence
bin/check backendgreen (pint + PHP suite). NewNotificationRoutesTest: before the change 4/4 routes answered 404, andtest_a_database_notification_reaches_the_listfailed withdatatype mismatchonce the feature was on and before the key fix.migraterebuilt the integer table with a UUID key and both indexes;$user->notify()then answeredGET /notifications(data.0.data.title),/unread-count({"data":{"count":1}}),POST /{uuid}/read(200, count back to 0) and/notification-preferences;/notificationsat 1440 and 390 widths, and delete through the confirm dialog issuedDELETE /notifications/{uuid}and emptied the table, with 0 exceptions.Contract check
MigrationHelper::usesUuids()exists there).Existing local databases: run
php artisan migrate, since the rekey repairs the table in place.