New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Pusher] Rewrite the whole thing #321

Open
tobscure opened this Issue Aug 29, 2015 · 25 comments

Comments

Projects
None yet
6 participants
@tobscure
Member

tobscure commented Aug 29, 2015

From @tobscure on August 27, 2015 13:51

I threw the Pusher extension together in a night without much thought, because I wanted to get the beta out the door :P

While it does the basics OK (update the discussion list, push new posts, and update the unread notifications count), it could be way better. And it's a mess and basically needs to be rewritten. Anyway, here's how I think it should be improved:

  • Extract a lot of the code into core, so that the extension is effectively telling core "hey I just found out about this new post in this discussion – whatchu gunna do about it?" and then core shows the "1 updated discussions" button or updates the post stream or whatever. This will allow us to write other extensions for different push services without duplicating/having inconsistent implementations.
  • Make it push more information about the discussion that was updated, so that we can check whether we already have the most up-to-date information, and don't have to extend addDiscussion.
  • Don't say there are updated discussions if they are in a hidden tag
  • Push to a user whenever they read notifications, so that the number of unread notifications stays in sync between browser windows.
  • Push for discussion events (e.g. discussion renaming)
  • Push for post likes
  • Show a little bubble every time a notification is received (again, this is something that would probably belong in core)
  • Push user online status (via presence channels? idk)

What needs to be done:

  • Working out a plan of attack for that first bullet point is the most important thing.
  • Reimplement the current basic features in a cleaner way.
  • Create issues for the additional features.

Copied from original issue: flarum/pusher#1

@tobscure tobscure changed the title from Rewrite the whole thing to [Pusher] Rewrite the whole thing Aug 29, 2015

@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Aug 29, 2015

Member

Also, the "new updates" notification in the discussion list should be fixed to the top.

Member

franzliedke commented Aug 29, 2015

Also, the "new updates" notification in the discussion list should be fixed to the top.

@tobscure tobscure referenced this issue Aug 29, 2015

Closed

v0.1.0 roadmap (old) #74

19 of 53 tasks complete

@tobscure tobscure added this to the 0.1.0-beta.4 milestone Sep 4, 2015

@justjavac justjavac referenced this issue Sep 7, 2015

Open

Flarum v0.1.0 开发路线图 #3

18 of 53 tasks complete
@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Nov 21, 2015

Member

It would be amazing if the core would support a generic notification method and you can add new broadcasting systems to it. By default you would add pusher that would receive Events and handles them. But you could also think of e-mail, slack, hipchat; see for instance notifymehq.

Member

luceos commented Nov 21, 2015

It would be amazing if the core would support a generic notification method and you can add new broadcasting systems to it. By default you would add pusher that would receive Events and handles them. But you could also think of e-mail, slack, hipchat; see for instance notifymehq.

@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Nov 21, 2015

Member

Hmm, well, since we already have an event system, it's somewhat easy to do it for any notification provider anyway. But yeah, there might be value in abstracting away external services like that... could very well be done by an extension, though.

Member

franzliedke commented Nov 21, 2015

Hmm, well, since we already have an event system, it's somewhat easy to do it for any notification provider anyway. But yeah, there might be value in abstracting away external services like that... could very well be done by an extension, though.

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Nov 21, 2015

Member

Also take into consideration #512

Member

luceos commented Nov 21, 2015

Also take into consideration #512

@tarunmarkose

This comment has been minimized.

Show comment
Hide comment
@tarunmarkose

tarunmarkose Dec 8, 2015

Hey guys, quick question - will the rewritten Push system allow any extension to do a push event or only the core ones?

For example, if I created a private messaging system, could it use your push extension to send out new messages as they came along? Kinda like a global push mechanism that any extension could tie into...

tarunmarkose commented Dec 8, 2015

Hey guys, quick question - will the rewritten Push system allow any extension to do a push event or only the core ones?

For example, if I created a private messaging system, could it use your push extension to send out new messages as they came along? Kinda like a global push mechanism that any extension could tie into...

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Dec 23, 2015

Member

@tarunmarkose any extension might use such an event system.

Member

luceos commented Dec 23, 2015

@tarunmarkose any extension might use such an event system.

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Dec 23, 2015

Member

@franzliedke @tobscure I'm considering picking this up, my proposal would be the following:

  • Create a Message Interface and an implementation of that for flarum/core that will specify some basic properties: sender, receiver, title, body.
  • Create a Broadcast Interface that specifies methods that are required to send Messages, eg: send(MessageInterface $message)
  • Modify the Pusher package to use this methodology, implementing the Broadcast Interface.
  • Allow extensions that have a Broadcast implementation to register themselves and make core send Messages over all registered Broadcasts. Any easy way of doing this? Maybe a seperate event, RegisterBroadcaster?

Also I agree the pusher rewrite is a seperate issue related to this, but requires this to be completed.

In hindsight, Channel (or MessageChannel) over Broadcast.

Awaiting feedback.

Member

luceos commented Dec 23, 2015

@franzliedke @tobscure I'm considering picking this up, my proposal would be the following:

  • Create a Message Interface and an implementation of that for flarum/core that will specify some basic properties: sender, receiver, title, body.
  • Create a Broadcast Interface that specifies methods that are required to send Messages, eg: send(MessageInterface $message)
  • Modify the Pusher package to use this methodology, implementing the Broadcast Interface.
  • Allow extensions that have a Broadcast implementation to register themselves and make core send Messages over all registered Broadcasts. Any easy way of doing this? Maybe a seperate event, RegisterBroadcaster?

Also I agree the pusher rewrite is a seperate issue related to this, but requires this to be completed.

In hindsight, Channel (or MessageChannel) over Broadcast.

Awaiting feedback.

@luceos luceos self-assigned this Dec 23, 2015

@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Dec 27, 2015

Member

I'd say we need separate issues for most of these.

First step: Implementing the interfaces.
Second step: A manager for broadcasts (possibly one implementation of the interface that has references to all other instances). I can take care of that.
Third step: Adapting the extension.

Member

franzliedke commented Dec 27, 2015

I'd say we need separate issues for most of these.

First step: Implementing the interfaces.
Second step: A manager for broadcasts (possibly one implementation of the interface that has references to all other instances). I can take care of that.
Third step: Adapting the extension.

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Dec 27, 2015

Member

Sounds good to me.

Member

tobscure commented Dec 27, 2015

Sounds good to me.

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Dec 28, 2015

Member

This might be great solution for the manager: https://github.com/notifymehq/notifyme @franzliedke

Member

luceos commented Dec 28, 2015

This might be great solution for the manager: https://github.com/notifymehq/notifyme @franzliedke

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Jan 3, 2016

Member

Hmm, I just had a bit of a brainstorm about how to better factor in discussion/tag permissions when pushing new posts. The currently implementation is very basic: if the post/discussion is visible to guests, push it to the public channel, otherwise don't push it at all. That's not very useful for members-only forums though!

It's a complex problem though, because Flarum's permissions are so dynamic. We could add code in the push mechanism to check which tags a discussion has and which groups are allowed to view them... but then what if a forum isn't using the tags extensions, and is using something else instead? And how would a private discussion extension fit in? Hardcoding references to extensions and their logic is bad.

A potential solution to this would be to add a couple of extensible methods to the Post model which return which groups/users that the post is visible to. Kind of like a reverse permission lookup – instead of "can this user (in these groups) view this post?" it's asking "this post: which users/groups can view it?"

By default it would just be implemented something like this:

public function groupsThatCanView() {
    return Permission::where('permission', 'viewDiscussions')->lists('group_id')->all();
}
public function usersThatCanView() {
    return []; // not visible to any users in particular
}

But then there would also be a couple of new events in there for extensions to hook into. Tags would do something like this:

$events->listen(PrepareGroupsThatCanViewPost::class, function ($event) {
    // Loop through each of the discussion's tags. If any of them are restricted,
    // then determine which groups can view that tag and return that.
    foreach ($event->post->discussion->tags as $tag) {
        if ($tag->is_restricted) {
            $event->groups = Permission::where('permission', 'tag'.$tag->id.'.viewDiscussions')->lists('group_id')->all();
        }
    }
});

And the private discussions extension would do:

$events->listen(PrepareUsersThatCanViewPost::class, function ($event) {
    // Get the IDs of all the users that are invited to this private discussion
    $event->users = $event->post->discussion->privateUsers->lists('id')->all();
});

With this information, the push mechanism could push to public/private channels depending on the groups/users that are determined to be allowed to view the post:

$groups = $event->post->groupsThatCanView();
$channels = [];

if (in_array(Group::GUEST_ID, $groups)) {
    $channels[] = 'public';
} elseif (in_array(Group::MEMBER_ID, $groups)) {
    $channels[] = 'private-members';
} else {
    foreach ($groups as $id) {
        $channels[] = 'private-group'.$id;
    }

    $users = $event->post->usersThatCanView();
    foreach ($users as $user) {
        $channels[] = 'private-user'.$id;
    }
}
Member

tobscure commented Jan 3, 2016

Hmm, I just had a bit of a brainstorm about how to better factor in discussion/tag permissions when pushing new posts. The currently implementation is very basic: if the post/discussion is visible to guests, push it to the public channel, otherwise don't push it at all. That's not very useful for members-only forums though!

It's a complex problem though, because Flarum's permissions are so dynamic. We could add code in the push mechanism to check which tags a discussion has and which groups are allowed to view them... but then what if a forum isn't using the tags extensions, and is using something else instead? And how would a private discussion extension fit in? Hardcoding references to extensions and their logic is bad.

A potential solution to this would be to add a couple of extensible methods to the Post model which return which groups/users that the post is visible to. Kind of like a reverse permission lookup – instead of "can this user (in these groups) view this post?" it's asking "this post: which users/groups can view it?"

By default it would just be implemented something like this:

public function groupsThatCanView() {
    return Permission::where('permission', 'viewDiscussions')->lists('group_id')->all();
}
public function usersThatCanView() {
    return []; // not visible to any users in particular
}

But then there would also be a couple of new events in there for extensions to hook into. Tags would do something like this:

$events->listen(PrepareGroupsThatCanViewPost::class, function ($event) {
    // Loop through each of the discussion's tags. If any of them are restricted,
    // then determine which groups can view that tag and return that.
    foreach ($event->post->discussion->tags as $tag) {
        if ($tag->is_restricted) {
            $event->groups = Permission::where('permission', 'tag'.$tag->id.'.viewDiscussions')->lists('group_id')->all();
        }
    }
});

And the private discussions extension would do:

$events->listen(PrepareUsersThatCanViewPost::class, function ($event) {
    // Get the IDs of all the users that are invited to this private discussion
    $event->users = $event->post->discussion->privateUsers->lists('id')->all();
});

With this information, the push mechanism could push to public/private channels depending on the groups/users that are determined to be allowed to view the post:

$groups = $event->post->groupsThatCanView();
$channels = [];

if (in_array(Group::GUEST_ID, $groups)) {
    $channels[] = 'public';
} elseif (in_array(Group::MEMBER_ID, $groups)) {
    $channels[] = 'private-members';
} else {
    foreach ($groups as $id) {
        $channels[] = 'private-group'.$id;
    }

    $users = $event->post->usersThatCanView();
    foreach ($users as $user) {
        $channels[] = 'private-user'.$id;
    }
}
@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Jan 4, 2016

Member

What about pushing an event on any activity, and then polling for new posts from the client once those events are received? That's obviously a very naive solution...

Member

franzliedke commented Jan 4, 2016

What about pushing an event on any activity, and then polling for new posts from the client once those events are received? That's obviously a very naive solution...

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Jan 4, 2016

Member

Polling would reduce the benefit of the websocket implementation altogether @franzliedke . Even so, your remark does make sense and is way less complicated than what @tobscure proposes. The question is, how to push all events and filter them properly by rights before broadcasting.

The solution by @tobscure is practical, even though it's a bit complicated to understand at first. It might actually work well. I do see only examples of giving access, how would one remove access through an event listener?

Member

luceos commented Jan 4, 2016

Polling would reduce the benefit of the websocket implementation altogether @franzliedke . Even so, your remark does make sense and is way less complicated than what @tobscure proposes. The question is, how to push all events and filter them properly by rights before broadcasting.

The solution by @tobscure is practical, even though it's a bit complicated to understand at first. It might actually work well. I do see only examples of giving access, how would one remove access through an event listener?

@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Jan 4, 2016

Member

Yeah, bad wording again. It's not really "polling", just loading new data exactly once when receiving an event. That would probably cause a lot of unnecessary traffic on the server, though...

Member

franzliedke commented Jan 4, 2016

Yeah, bad wording again. It's not really "polling", just loading new data exactly once when receiving an event. That would probably cause a lot of unnecessary traffic on the server, though...

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Jan 5, 2016

Member

@franzliedke What about pushing an event on any activity, and then polling for new posts from the client once those events are received? That's obviously a very naive solution...

That would be a nice solution, though it's not ideal security-wise... everyone would know exactly when and how many posts are being made across the whole forum, even in sections they don't know exist... not a huge deal but still worth mentioning. And yeah, that could mean a lot of unnecessary traffic.

I really do like the simplicity of it though, not having to implement reverse permission logic.

@luceos I do see only examples of giving access, how would one remove access through an event listener?

The PrepareGroupsThatCanViewPost event would have an array of groups passed by reference, so items in $event->groups can be modified/added/removed as needed.

Member

tobscure commented Jan 5, 2016

@franzliedke What about pushing an event on any activity, and then polling for new posts from the client once those events are received? That's obviously a very naive solution...

That would be a nice solution, though it's not ideal security-wise... everyone would know exactly when and how many posts are being made across the whole forum, even in sections they don't know exist... not a huge deal but still worth mentioning. And yeah, that could mean a lot of unnecessary traffic.

I really do like the simplicity of it though, not having to implement reverse permission logic.

@luceos I do see only examples of giving access, how would one remove access through an event listener?

The PrepareGroupsThatCanViewPost event would have an array of groups passed by reference, so items in $event->groups can be modified/added/removed as needed.

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Jan 6, 2016

Member

I'm starting to feel a lot for the reverse permission logic @tobscure . It might be useful in other areas as well in the near future.

Also I don't think it's wise to broadcast all events to everyone, who knows what information extensions will be sending along at some point.

Member

luceos commented Jan 6, 2016

I'm starting to feel a lot for the reverse permission logic @tobscure . It might be useful in other areas as well in the near future.

Also I don't think it's wise to broadcast all events to everyone, who knows what information extensions will be sending along at some point.

@franzliedke franzliedke modified the milestones: 0.1.0-beta.5, 0.1.0-beta.6 Jan 7, 2016

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Jan 13, 2016

Member

I'm removing myself from this task until I have the chance to actually start the implementation.

Member

luceos commented Jan 13, 2016

I'm removing myself from this task until I have the chance to actually start the implementation.

@luceos luceos removed their assignment Jan 13, 2016

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Jan 14, 2016

Member

Also I don't think it's wise to broadcast all events to everyone, who knows what information extensions will be sending along at some point.

Right, that's the biggest concern and I think it justifies the implementation of reverse permission logic.

Member

tobscure commented Jan 14, 2016

Also I don't think it's wise to broadcast all events to everyone, who knows what information extensions will be sending along at some point.

Right, that's the biggest concern and I think it justifies the implementation of reverse permission logic.

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Jan 14, 2016

Member

I was under the impression we already decided on that @tobscure 😆

Member

luceos commented Jan 14, 2016

I was under the impression we already decided on that @tobscure 😆

@thedumbtechguy

This comment has been minimized.

Show comment
Hide comment
@thedumbtechguy

thedumbtechguy Jan 18, 2016

There is a bug here.

$event->groups = Permission::where('permission', 'tag'.$tag->id.'.viewDiscussions')->lists('group_id')->all();

What happens when the next tag in the loop is restricted as well? Isn't $event->groups overwritten?
Sorry, I'm treating my glaucoma so I can't trust my reasoning.

thedumbtechguy commented Jan 18, 2016

There is a bug here.

$event->groups = Permission::where('permission', 'tag'.$tag->id.'.viewDiscussions')->lists('group_id')->all();

What happens when the next tag in the loop is restricted as well? Isn't $event->groups overwritten?
Sorry, I'm treating my glaucoma so I can't trust my reasoning.

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Jan 18, 2016

Member

@frostymarvelous yes so the implementation should have an array like $requiredGroup which is filled throughout the loop. Not that much of an issue.

Member

luceos commented Jan 18, 2016

@frostymarvelous yes so the implementation should have an array like $requiredGroup which is filled throughout the loop. Not that much of an issue.

@mauricederegt

This comment has been minimized.

Show comment
Hide comment
@mauricederegt

mauricederegt Feb 8, 2016

To be honest, why not keep it as a plugin? Pusher is expensive and a paid service most users will not use. Keeping this as a plugin will keep Flarum small and clean and even better: keeps the option open to use different (or own setup) pusher like services via a plugin (and this forum is all about plugins right?)

mauricederegt commented Feb 8, 2016

To be honest, why not keep it as a plugin? Pusher is expensive and a paid service most users will not use. Keeping this as a plugin will keep Flarum small and clean and even better: keeps the option open to use different (or own setup) pusher like services via a plugin (and this forum is all about plugins right?)

@thedumbtechguy

This comment has been minimized.

Show comment
Hide comment
@thedumbtechguy

thedumbtechguy Feb 8, 2016

From the original post.

'Extract a lot of the code into core, so that the extension is effectively telling core "hey I just found out about this new post in this discussion – whatchu gunna do about it?" and then core shows the "1 updated discussions" button or updates the post stream or whatever. This will allow us to write other extensions for different push services without duplicating/having inconsistent implementations.'

The aspects being included in the core is essentially a notification event. And then we can write different plugins to interact with this event to enhance the experience.

thedumbtechguy commented Feb 8, 2016

From the original post.

'Extract a lot of the code into core, so that the extension is effectively telling core "hey I just found out about this new post in this discussion – whatchu gunna do about it?" and then core shows the "1 updated discussions" button or updates the post stream or whatever. This will allow us to write other extensions for different push services without duplicating/having inconsistent implementations.'

The aspects being included in the core is essentially a notification event. And then we can write different plugins to interact with this event to enhance the experience.

@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Mar 30, 2016

Member

Note that the broadcasting implementation needs to be separated from this issue into three other issues:

  • interface design for broadcasting
  • implementation of a broadcasting manager
  • example by modifying the mentions extension and possibly pusher extension
Member

luceos commented Mar 30, 2016

Note that the broadcasting implementation needs to be separated from this issue into three other issues:

  • interface design for broadcasting
  • implementation of a broadcasting manager
  • example by modifying the mentions extension and possibly pusher extension
@luceos

This comment has been minimized.

Show comment
Hide comment
@luceos

luceos Apr 28, 2016

Member

Laravel 5.2 offers the illuminate broadcasting package. Might be interesting for this implementation:

https://github.com/illuminate/broadcasting

Member

luceos commented Apr 28, 2016

Laravel 5.2 offers the illuminate broadcasting package. Might be interesting for this implementation:

https://github.com/illuminate/broadcasting

@franzliedke franzliedke modified the milestones: 0.1.0-beta.6, 0.1.0-beta.7 May 13, 2016

@tobscure tobscure removed this from the 0.1.0-beta.7 milestone Jul 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment