j3k0 / ganomede-notifications

Long-pull notification service for Ganomede
0 stars 0 forks source link

Interaction with Invitation service #3

Closed elmigranto closed 9 years ago

elmigranto commented 9 years ago

I created this very professional drawing:

notifications-workflow

Questions

update: following isn't really meaningful and should be ignored:

Is this module only notifies clients about things (like invitations received), so they need to ask other services in order to make a decision (get list of invitations and see what was that about) or do we transfer necessary data as part of notification (like invitation <ID> received from alice for game X)?

Like when you're on your phone or Facebook you get this little red number of new things, you click it and it loads a page or app screen. Based on notification type, we load appropriate screen that knows what info to query from where.

j3k0 commented 9 years ago

Nice graph.

Generally, I think of notifications as lightweight messages that lets the client know about something that needs to be refreshed.

In the "invitations" case, a simple client will just reload its list for any invitation related event. This saves it from having to reload the list of invitations every 5 seconds to check for updates.

But as long as it stays lightweight, a bit of extra info can be added in the "data" field to allow specific client logic. For invitations, adding the full data of the invitation event seems fine, it's nothing heavy and can prove useful. More advanced client may then chose not to reload the full list but to update theirs based on notification events.

A little note concerning the example notification in your graph:

{
  "type": "invitation",
  "data": { "action": "accept" }
}

The "from" field of an invitations already allows to know it's a notification related to invitations. I'd reformulate like this:

{
  "from": "invitations/v1",
  "type": "accept",
  "data": { invitationdata... }
}
elmigranto commented 9 years ago

The "from" field of an invitations already allows to know it's a notification related to invitations.

My reasoning for that is possibility of service (or future version of invitations even) to have multiple types of notifications with same states. For example, accept invitation for 2 players game or accept challenge from a friend to beat his record. I wouldn't mix verbs with nouns here.

{ "from": "invitations/v1",
  "type": "invitation",
  "data": { "action": "created",  // someone invited you
            "invitation": "<actual invitaion fields>" // could be anything
                                                      // full invitation or its ID only
} }
j3k0 commented 9 years ago

Ok.

Notice that type doesn't have to be unique globally, client side I'm filtering based on the from field. Doing something like this:

client.notifications.listenTo("invitations/v1", function(event) {
  var n = event.notification;
  if (n.type === "invitation" && n.data.action === "created") {
    // do my stuff
  }
});

Just in case it clears things up.

elmigranto commented 9 years ago

Okay. So next thing is document full spec listing: every action and its meaning and decide on payload.

Currently invitation defined as follows:

class Invitation
  constructor: (from, data) ->
    @id = Invitation._rand() + Invitation._rand()
    @from = from
    @to = data.to
    @gameId = data.gameId
    @type = data.type

Do we want full invitation info required to take meaningful decision inside data.invitation or do we just notify client that its invite list has changed and it should reload it?

If former is the case, we'll probably need to include everything except to, so we can say something like User FROM invited you to play TYPE with Accept button accepting invitation and starting/joining game gameId.

j3k0 commented 9 years ago

Yes, let's send the whole invitation in data.invitation (with to, that won't hurt). Even if it isn't useful for now, it could be later and it's cheap too.

elmigranto commented 9 years ago

Do we want to use Dates here or timestamps as well?

elmigranto commented 9 years ago

I'm documenting specs for notifications from invitation module and wondering do you want to rename action to state since it would be more appropriate name?

j3k0 commented 9 years ago

Do we want to use Dates here or timestamps as well?

I don't recall that there are dates in the notification module. What "here" are you referring to? Anyway, we started to turn all dates with timestamps, let's stick to that. Only drawback is the harder debugging (when you have to read logs, iso-dates are easier to read)

Do you want to rename action to state?

Think of notifications as "events". If you think this way, the user will want to know "what kind of event just happened?". I think having just the below isn't explicit enough:

This isn't explicitly saying that the state changed, could as well be something else).

In my original design, the root type field was supposed to give the answer about the type of event, I personally would have done it this way:

Don't you think it's clearer to the API user that he receive this notification because an invitation got accepted?

Eventuallly, I'd say we can stick to the invitations API terminology and have:

j3k0 commented 9 years ago

Just giving my opinion, you decide!

elmigranto commented 9 years ago

I am referring to timestamps instead of dates in notification date field.

I've updated spec with your suggestion, let me know what you think.

j3k0 commented 9 years ago

Missed it sorry... Rest of the answer still the same, let's change to timestamp.

j3k0 commented 9 years ago

I've updated spec with your suggestion, let me know what you think.

Excellent, let's go for it. What's the status apart from that? Are you free for a Skype meeting tomorrow about it?