notifications system #4942
notifications system #4942
Conversation
IvanIvanoff
force-pushed
the
notifications-system
branch
from
eaadd13 to
9b76642
Compare
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }, | ||
| else: %{} | ||
| ) | ||
| |> dbg() |
There was a problem hiding this comment.
Bug: Debug statements left in production code
Two dbg() calls are left in the create_notification function. These debug statements will output diagnostic information to logs in production, which is unintended and could clutter logs or leak internal data structures.
Additional Locations (1)
| # | ||
| defp followers_user_ids(user_id) do | ||
| [user_id] ++ Sanbase.Accounts.UserFollower.followed_by_user_ids(user_id) | ||
| end |
There was a problem hiding this comment.
Bug: Notifications sent to wrong users due to inverted query
The followers_user_ids/1 function calls followed_by_user_ids(user_id) which returns users that the author follows, not users who follow the author. When an author publishes an insight or creates a watchlist, notifications should go to the author's followers. The correct function to use would be one that queries where uf.user_id == ^user_id (like the existing followers_of/1), not where uf.follower_id == ^user_id.
|
|
||
| defp maybe_emit_event({:ok, watchlist}, :update_watchlist, data) do | ||
| emit_event({:ok, watchlist}, :update_watchlist, data) | ||
| end |
There was a problem hiding this comment.
Bug: Missing error clause in maybe_emit_event causes crash
The maybe_emit_event/3 function only handles the {:ok, watchlist} pattern. When called after by_id/2 in remove_user_list_items/2 and add_user_list_items/2, if by_id returns an error tuple, the function will crash with a FunctionClauseError. The existing maybe_create_event/3 follows the pattern of including a fallback clause defp maybe_create_event(error_result, _, _), do: error_result which is missing here.
IvanIvanoff
force-pushed
the
notifications-system
branch
from
27438ab to
c51d6c3
Compare
| @type t :: %__MODULE__{ | ||
| id: pos_integer(), | ||
| type: String.t(), | ||
| title: String.t(), |
There was a problem hiding this comment.
| title: String.t(), | |
| title: String.t() | nil, |
There was a problem hiding this comment.
since it is in @optional_fields
| @type t :: %__MODULE__{ | ||
| id: pos_integer(), | ||
| type: String.t(), | ||
| title: String.t(), |
There was a problem hiding this comment.
since it is in @optional_fields
| field(:is_deleted, :boolean, default: false) | ||
|
|
||
| # Virtual field populated when we join the user reads | ||
| field(:read_at, :utc_datetime, virtual: true) |
There was a problem hiding this comment.
we should probably add to the type definition as well
Changes
Ticket
Checklist:
Note
Introduces in-app notifications with persistence/read tracking and EventBus subscriber that notifies followers on insight publish and watchlist create/update; adds watchlist visibility timestamp and emits granular update events.
Sanbase.AppNotificationswithcreate_notification/1,list_notifications_for_user/2, andmark_notification_as_read/2.Sanbase.AppNotifications.NotificationandNotificationUserRead(withread_atvirtual on Notification when joined).sanbase_notificationsandsanbase_notification_user_readswith FKs and unique index.Sanbase.EventBus.AppNotificationsSubscriber; listens to events and inserts notifications forpublish_insight,create_watchlist, andupdate_watchlist(public only), including JSONchanged_fieldsand list item deltas.Sanbase.EventBus; extend validation to accept:update_watchlist.user_lists.is_public_updated_at; auto-update whenis_publicchanges.:update_watchlistevents with change metadata; include counts when items are added/removed.by_id/2to support optional preloads; addmaybe_preload/2.Sanbase.Accounts.UserFollower.followed_by_user_ids/1helper for follower targeting.Written by Cursor Bugbot for commit 27438ab. This will update automatically on new commits. Configure here.