Skip to content
This repository has been archived by the owner on Nov 7, 2020. It is now read-only.

Dynamic updating of event interface on event admin change. #425

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Dynamic updating of event interface on event admin change. #425

wants to merge 1 commit into from

Conversation

thehunmonkgroup
Copy link
Contributor

This patch refactors all event rendering functionality to enable
updating the event interface in the case where an event admin is
added or removed. Only the user who was added or removed has their
event interface updated.

We needed this for fast switching of admins in our installation.

This patch depends on #424.

@srish srish assigned srish and yourcelf and unassigned srish Aug 14, 2015
@@ -120,25 +120,34 @@ $(document).ready(function() {
var firstScriptTag = document.getElementsByTagName('script')[0];
firstScriptTag.parentNode.insertBefore(tag, firstScriptTag);

this.currentUser = new models.User(USER);
this.currentUserIsAdmin = function() {
return this.currentUser.isAdminOf(curEvent);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The this value here isn't enclosed -- won't this.currentUser be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because this.currentUserIsAdmin() is only called in the context of the initializer. I've tested it, it works. :) Did you have some other approach in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you that it works. I'm concerned about the code style of going 3 levels deep in the call stack with this and having it work -- it seems brittle; any variation in how those methods get invoked and the binding is lost.

We could either explicitly bind "this" in the function definitions (e.g. (function() {}).bind(this)), or enclose a reference to this which isn't dynamic, e.g.:

var that = this
that.currentUserIsAdmin = function() {
    return that.currentUser.isAdminOf(curEvent);
};

Just concerned about maintainability of that code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've addressed your concern by binding 'this' to the function.

@yourcelf
Copy link
Collaborator

I'd love to have a small test of this, to ensure we don't accidentally break it. The event-session-messages test is probably a good template or this: set up an event, point selenium at it, and log a regular non-admin user at it. Then, via a socket message, change the user to admin status, and observe the change in UI for the now-admin.

This patch refactors all event rendering functionality to enable
updating the event interface in the case where an event admin is
added or removed. Only the user who was added or removed has their
event interface updated.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants