Closed renatomassaro closed 6 years ago
Reviewed 105 of 116 files at r1, 15 of 15 files at r2. Review status: :shipit: complete! all files reviewed, all discussions resolved
lib/event/notificable/notificable.ex, line 16 at r2 (raw file):
| Server.id # TODO: Rename this method
Rename and add docs
lib/notification/event/notification.ex, line 151 at r2 (raw file):
def whom_to_publish(event), do: %{account: event.account_id}
Extra line
lib/notification/model/notification.ex, line 242 at r2 (raw file):
def validate_id(_, _), do: :error
Extra line
Comments from Reviewable
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/HackerExperience/Helix/pulls/406.
Reviewed 7 of 7 files at r3, 1 of 1 files at r4. Review status: :shipit: complete! all files reviewed, all discussions resolved
Comments from Reviewable
This PR introduces the in-game notification system. There are 4 types of notifications:
FileDownloadedEvent
ServerPasswordAcquiredEvent
ClanMemberJoinEvent
,ClanWarStartedEvent
ChatMessageEvent
For this PR we'll add Server and Account notifications. Clan and Chat notifications are out of scope for now.
Notificable
system was renamed toPublishable
, meaning that outgoing events arepublications
, i.e. they are published to the client. Referring to that feature asnotification
is wrong from now on.Notificable
macros, used to register notifications off of existing events.NotificationAddedEvent
, used to publish to the Client that a notification has been addednotification.read
andnotification.read.all
requestsNotificationReadEvent
This change is![Reviewable](https://reviewable.io/review_button.svg)