fenos / Notifynder

Easy Internal Notification management for laravel 4.* / 5.*
MIT License
432 stars 86 forks source link

Setting custom fields not wroking #277

Closed Malraoosh closed 7 years ago

Malraoosh commented 7 years ago

Hello, I tried to add custom fields to the notifications table, but when i tried to set them using setField method it didn't work.

I dig in to your code and i found that the mergeFillables in vendor/fenos/notifynder/src/Notifynder/Models/Notification.php the getCustomFillableFields didn't get merged with the getFillable .

I am using V4.1 and this is the result of getFillable and getCustomFillableFields functions

getFillable returns array:10 [ 0 => "to_id" 1 => "to_type" 2 => "from_id" 3 => "from_type" 4 => "category_id" 5 => "read" 6 => "url" 7 => "extra" 8 => "expires_at" 9 => "stack_id" ]

getCustomFillableFields returns array:4 [ 0 => "from_type" 1 => "to_type" 2 => "message_en" 3 => "message_ar" ]

and the final result mergeFillables function array:10 [ 0 => "to_id" 1 => "to_type" 2 => "from_id" 3 => "from_type" 4 => "category_id" 5 => "read" 6 => "url" 7 => "extra" 8 => "expires_at" 9 => "stack_id" ]

Gummibeer commented 7 years ago

Hey, thanks for the report - there was an error. We used array + array instead of array_merge(...).

I'm waiting for the travis results https://travis-ci.org/fenos/Notifynder/builds/237438986 and if everything is fine I will merge and tag the fix with v4.2.1. To prevent something like this I've added some more unittests that should cover this error:

Malraoosh commented 7 years ago

Tell me when it's done

Gummibeer commented 7 years ago

https://github.com/fenos/Notifynder/releases/tag/4.2.1 All tests passed - so it should work now.

Malraoosh commented 7 years ago

Thanks

Gummibeer commented 7 years ago

Can you pls give feedback if this solved your issue or not!?

Malraoosh commented 7 years ago

Yes it dose, thanks for your help 👍