fix(update-event-handler): persist attributes column on update#1197
Open
saimonhowlader wants to merge 5 commits into
Open
fix(update-event-handler): persist attributes column on update#1197saimonhowlader wants to merge 5 commits into
saimonhowlader wants to merge 5 commits into
Conversation
…ventsDev#1169) (HiEventsDev#1176) Co-authored-by: Manas Kumar <141910018+manaskumar3003@users.noreply.github.com>
UpdateEventHandler::updateEventAttributes() was silently dropping the `attributes` JSONB column on every event update. Although EventRules validates `attributes.*` and UpdateEventDTO accepts it, the handler's updateWhere() payload omitted the field entirely. CreateEventHandler correctly persists it via setAttributes(); only the UPDATE path was broken. Fix: add 'attributes' to the updateWhere() payload, falling back to the existing event's attributes when the update omits the field. This matches the partial-update pattern already used for category, timezone, and currency in the same handler, and guards against an incomplete client payload nulling out previously-saved values. Tests: - testPersistsAttributesWhenUpdatingEvent: confirms a non-empty attributes payload is persisted via the repository. - testPreservesExistingAttributesWhenUpdateOmitsThem: confirms attributes=null on the DTO does NOT null out existing attributes. Refs: discovered in v1.9.0-beta; verified bug reproduces on current main.
…tes array) Two iterations on the original fix commit (9e2ea5e), surfaced by running phpunit locally for the first time: 1. UpdateEventHandler::updateEventAttributes — change the attributes mapping from `$eventData->attributes?->toArray()` to an explicit `->map(fn(AttributesDTO $a) => $a->toArray())->all()`. BaseDTO does NOT implement Illuminate\Contracts\Support\Arrayable, so Collection::toArray() leaves AttributesDTO instances un-converted at the array layer. The original code accidentally produced correct DB JSON via PHP's implicit object→json_encode behavior, but the in-memory shape that gets passed to the repository's updateWhere() is `[AttributesDTO obj, ...]` not `[['name' => ..., 'value' => ..., 'is_public' => ...], ...]`. The explicit map makes the conversion intent obvious to reviewers and removes reliance on implicit serialization. 2. UpdateEventHandlerTest::setUp — add `Queue::fake()`. UpdateEventHandler::getUpdateEvent dispatches DispatchEventWebhookJob, which under the default sync queue tries to actually run the webhook logic, hits the real EventRepository, and fails with `Connection refused` in the unit-test environment. `Queue::fake()` short-circuits the dispatch without needing a live Postgres. Verified locally: `vendor/bin/phpunit tests/Unit/Services/Application/Handlers/Event/UpdateEventHandlerTest.php` → OK (2 tests, 11 assertions). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Author
|
I have read the CLA Document and I hereby sign the CLA |
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
3 participants
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.
Summary
UpdateEventHandler::updateEventAttributes()silently drops theattributesJSONB column on every event update.EventRulesvalidatesattributes.*andUpdateEventDTOaccepts the field, but the handler'supdateWhere()payload omitted it entirely — so the SQL UPDATE never touched the column.CreateEventHandlerpersistsattributescorrectly viasetAttributes(); only the UPDATE path was broken.Fix
Add
attributesto theupdateWhere()payload. When the update payload omits attributes ($eventData->attributes === null), fall back to the existing event's stored attributes. This matches the partial-update pattern already used forcategory,timezone, andcurrencyin the same handler.The explicit
->map(...)->all()(rather than?->toArray()) is intentional:BaseDTOdoes not implementIlluminate\Contracts\Support\Arrayable, soCollection::toArray()would leaveAttributesDTOinstances un-converted at the array layer. Production today produces correct DB JSON only via PHP's implicit object→json_encodeserialization — this PR removes that implicit dependency.Tests
testPersistsAttributesWhenUpdatingEvent— non-empty attributes payload is persisted in the expected scalar-array shape.testPreservesExistingAttributesWhenUpdateOmitsThem—attributes=nulldoes NOT null out existing attributes (regression guard).Queue::fake()insetUpto prevent the handler'sDispatchEventWebhookJob::dispatch()from hitting a live DB in the unit-test environment.Verified locally:
vendor/bin/phpunit tests/Unit/Services/Application/Handlers/Event/UpdateEventHandlerTest.php→OK (2 tests, 11 assertions).Repro
attributesis empty.Discovered against
v1.9.0-beta; reproduces on currentdevelop.