Initial ideas on how to make this a little more robust

[chrisjones-brack3t]

Initial ideas on how to make this a little more robust

I haven't included any tests nor is this complete. This is more for presenting a different direction for the mixins. I've pulled some ideas and code from my own project django-braces.

• The pusher_event_name has it's own get_pusher_event_name method. This adds a couple of benefits.
  • You can check that the name was set and that is of type string or unicode. If not, you can raise a nice, easy to read error.
  • You can override the method and programmatically generate the name if you need to.
• _set_pusher method checks that the settings exist and gives a nice error message if settings are missing. You also have the option to override this method if for some reason you needed to switch accounts. Probably could take this further and check if DEBUG=True and have settings for a production account and a dev account. (probably remove the leading _ if it's meant to be overridden).
• _set_pusher_channel is pretty straight forward. Just setup the channel. Could override this as well to programmatically setup a channel based on some data, probably the instance (probably remove the leading _ if it's meant to be overridden).
• get_pusher_payload again is mainly there to be overridable. Gives you a simple place to step in, override and send whatever data makes sense to your application. I also added a quick check about the user being anonymous because I was getting back a blank username. Probably wouldn't make sense to have this in a production app, it's more for my sanity.
• send_pusher_notification is straight forward. Again, easily overridable.
• Added some PEP8 love.

I removed the render_to_response method from the mixin itself and moved that to the PusherDetailMixin.

PusherUpdateMixin overrides the form_valid method.

PusherDeleteMixin overrides the delete method.

This isn't complete by any means but it does work. If you like the direction I was headed with this, I can flesh it out some more and obviously add docs and tests.

[aaronbassett]

Hey @chrisjones-brack3t I love this approach, thanks for taking the time to put this PR together.

• We tend to repeat the same lines in each of our mixins, what do you think about wrapping these in their own method; _trigger_pusher_event()?
• I think you're right to move the sending of the Pusher event from render_to_response on the Delete and Update views. But I still think there is a case for knowing when some has begun viewing the update form, not just when the form has saved successfully. Perhaps we could namespace the pusher_event_name. I'm not sure of the best way to achieve this, but we could send an event name of update:start in the get method and update:complete in the form_valid. Thoughts?

[leggetter]

I was thinking about pre and post update events. How about simply updating and updated?

[aaronbassett]

What if you want to subscribe to all events related to an update action? With namespacing you could just check if an event occurs within a particular namespace rather than check for each individual event name.

if(event.split(":")[0] === "update") { . . . }

vs

if(event === "updating" || event === "updated" || event === "update-failed") { . . . }

[chrisjones-brack3t]

@aaronbassett I agree about repeating the same lines. I had planned to handle that, just didn't get to it. Wanted to make sure my approach was worth putting more time into.

I think having multiple notifications triggered per view means the main PusherMixin needs some changes. Not entirely sure how to approach that yet. In prep for that, I went for a singleton approach with the main pusher connection on the mixin. Something like:

def _set_pusher(self):
    if hasattr(self, 'pusher') and isinstance(self.pusher, Pusher):
        return
    ...

My thought was having mixins for each type of event. So you'd have an UpdateSuccessMixin/UpdateFailedMixin which triggers a notification in the form_valid/form_invalid methods. You could also have an UpdateExpectedMixin which triggers a notification on the get method. I still wouldn't use render_to_response here because if the post has validation errors, I believe it will call render_to_response again. You could then inherit as many of these smaller mixins as you want and you have more options.

class SomeUpdateView(UpdateSuccessMixin, UpdateFailedMixin,
                     UpdateExpectedMixin, UpdateView):

If that feels like too much, you could also provide an UpdateAllTheThingsMixin which is basically a convenience mixin that combines all 3.

class UpdateAllTheThingsMixin(UpdateSuccessMixin, UpdateFailedMixin,
                              UpdateExpectedMixin):
    pass

On the subscription side of things, at least for the use cases I was thinking about, I think it makes sense to use Pusher's bind_all event. I think if you are looking at a detail view, update form view or delete view, it makes sense to receive notifications about any event that pertains to that object. For example, I'm editing a blog post and a user visits the detail view. It would be handy to know someone is looking at an object that I am about to change. Maybe I'm updating a blog post and another user is going to delete the post. Just a thought.

[robslotboom]

I think you're right to move the sending of the Pusher event from render_to_response on the Delete and Update views. But I still think there is a case for knowing when some has begun viewing the update form, not just when the form has saved successfully. Perhaps we could namespace the pusher_event_name. I'm not sure of the best way to achieve this, but we could send an event name of update:start in the get method and update:complete in the form_valid. Thoughts?

Hi Folks!

In an application I build, the user is only informed after some real change. I ended up using form.has_changed() for this.

Cheers,

Robert

[aaronbassett]

@chrisjones-brack3t :+1: on having mixins for each event type, I don't think we even need the UpdateAllTheThingsMixin, explicit is better than implicit, and all that.

As for subscribing to events on the client-side I agree with the bind_all. It might be an idea to allow users to specify a Javascript object as the callback which contains methods for each event type they want to handle. Sort of like the http_method_names and the dispatch method in CBVs. This would also close #3. I'll have a look at the client-side code after DjangoCon.

@robslotboom that's a great idea, no sense in notifying users of a form submission where no data has been changed.

[chrisjones-brack3t]

Okay, I took one more stab at this and refactored a lot.

I removed the leading underscores for all methods on PusherMixin except for _object_to_json_serializable. Every one of these methods could be overridden so it didn't seem to make sense to mark them as "private".

• set_pusher uses a singleton approach. When using multiple mixins on a single view there just isn't a need to recreate the Pusher object.
• I simplified the pusher notification method and it's now all wrapped up neatly in send_pusher_notification. That method also accepts an event name since we could now have multiple events for a single view.
• I created all new mixins that cover the basics... I think. PusherViewedMixin, PusherUpdatePendingMixin, PusherUpdateSucceededMixin, PusherUpdateFailedMixin, PusherDeletePendingMixin, PusherDeleteSucceededMixin. Naming things is always the hardest so I'm open to better suggestions.
• The PusherDeleteMixin is an odd beast. I'm not sure I like the current implementation and I'm not 100% positive it won't have some adverse side effect. Check the docstring notes for an explanation.
• I updated the pusher js lib to version 3.0. I also made it conform to PEP8 because my CDO (OCD in alphabetical order) can't stand linter errors.
• I rewrote the pusherable_subscribe template tag to use bind_all. A side effect of bind_all is that you also receive all pusher: namespaced events which right now I'm ignoring with that if statement. My thought behind this is that all we do is subscribe to the channel for this model instance and pass any event/data combo off the the pusherable_notify function which will be defined by the end user. It's up the user to figure out how they want to respond to those events which could vary heavily. I was thinking about possibly including a simple Bootstrap example to show how to create notifications with Bootstrap's dismissible alerts component.

I'm not sure how widely used this package is yet. I realize my changes are not backwards compatible. Do you care about backwards incompatibility at this point?

[leggetter]

Sorry for the radio silence. I'm still catching up from DjangoCon EU!

So, to summarise from a simple usability point of view. It's possible to send an event through Pusher when somebody:

• is in a viewing - viewed
• is updating - update_pending
• has updated - update_succeeded
• tried to update, but it failed - update_failed
• is in the delete view - delete_pending
• has deleted - delete_succeeded

I'd maybe suggested viewing rather than viewed since the user may still be viewing after the event is triggered.

pending is a state that feels it should have a follow-on event and that's not always the case e.g. delete_pending may not be succeeded by a delete_succeeded. The user might just not do anything. However, I can't think of an alternative suggestion so I'd be happy to stick with what's in this PR.

The other main thing I'd really like to see is Pusher's evented pub/sub paradigm shine through (as per #3). However, I'll leave that for a separate discussion in that thread.

@aaronbassett @chrisjones-brack3t - it seems that @kennethlove has raised a few valid questions. Once those have been responded to and a solution agreed upon I'd like to try out the updated package on an app to see how it feels when using it.

[decentral1se]

Bump!

[pusher-ci]

@chrisjones-brack3t: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[pusher-ci]

@chrisjones-brack3t: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[pusher-ci]

@chrisjones-brack3t: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.