Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f1cede4
Adds a route for fetching issues with optional parameters
discorick Apr 18, 2016
dc2bce4
Begins working on the client-side session service and tests
discorick Apr 19, 2016
275b732
Implements events & handlers for browser focus blur as an injected "b…
discorick Apr 19, 2016
40c2205
Tweaks the focus logic and tests
discorick Apr 20, 2016
1952937
Adds logged_in endpoint, tweaks issues api endpoint
discorick Apr 20, 2016
9b0f528
Implements basic board syncing service (sans success/fail handlers) a…
discorick Apr 20, 2016
51aba7a
Fixes a bug related to misunderstanding the RSVP.all contract
discorick Apr 21, 2016
f072790
Uses correct method to flatten the array
discorick Apr 21, 2016
04c9148
Fixes flash sync messages from display an x on sync progress
discorick Apr 21, 2016
5829e45
Make sure messageData is always a new object instance, ensures only o…
discorick Apr 21, 2016
224d420
Implements session checking logic in the application controller
discorick Apr 21, 2016
251b281
Adds #/sync-issues route to manually sync the board
discorick Apr 21, 2016
cb862ac
Fixes problem where array proxy wasn't propagating changes to the mod…
discorick Apr 22, 2016
5e9bbc1
Removes logged_in route, throttles the board sync @30 seconds
discorick Apr 22, 2016
0edf225
Gives syncing message 1 second (to adjust for insta-syncs) shortens s…
discorick Apr 22, 2016
e43f3e1
Implements a failure message for board syncing
discorick Apr 22, 2016
8b69116
styles the progress flash message more subtley
discorick Apr 23, 2016
533e684
Use same-as-background shadow on progress message
dahlbyk Apr 25, 2016
3ac1cad
Force refresh after 24hours of no focus
discorick Apr 25, 2016
87ca4b1
Gracefully returns if no flash can be found
discorick May 3, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/controllers/api/api_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
module Api
class ApiController < ApplicationController
skip_before_action :check_account

def logged_in
if logged_in?
return render json: {success: true}
else
return render json: {success: false, status: 403}
Copy link
Copy Markdown
Member

@drusellers drusellers Apr 20, 2016

Choose a reason for hiding this comment

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

So, this is just a discussion point. you request was technically successful 200 - you are returning the correct payload etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Im rethinking this right now... maybe logged_in isn't the best option.

end
end
end
end
5 changes: 5 additions & 0 deletions app/controllers/api/issues_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ def issue
render json: api.issue(params[:number])
end

def issues
api = huboard.board(params[:user], params[:repo])
render json: api.issues(params[:label], params[:options])
end

def details
api = huboard.board(params[:user], params[:repo])
render json: api.issue(params[:number]).activities
Expand Down
2 changes: 2 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@


namespace :api do
get 'logged_in' => 'api#logged_in'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

NIT: _ in a URL are 👎 prefer -

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is that a common convention (honest question) ? Easy change to make

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, it is for me. It goes back a pretty long ways to HTML webpage dev days - imagine http://www.bob.com/this_is_awesome where the default link underlining is in play

image

vs

image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also this url is a query, for a pure RESTafarian- 💩 head - should it be ~/session?exist kind of thing? again, this is purely surface stuff.

get 'uploads/asset' => 'uploads#asset_uploader'
#Webhooks
post '/site/webhook/issue' => 'webhooks#legacy'
Expand Down Expand Up @@ -72,6 +73,7 @@

#Issues
get 'issues/:number' => 'issues#issue'
get 'issues' => 'issues#issues'
get 'issues/:number/details' => 'issues#details'
get 'issues/:number/status' => 'issues#status'
post 'issues' => 'issues#open_issue'
Expand Down
19 changes: 19 additions & 0 deletions ember-app/app/controllers/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,25 @@ var ApplicationController = Ember.Controller.extend(

//Fix the need to delay event subscriptions
subscribeDisabled: true,

//Browser Session Checker
boardSyncing: Ember.inject.service(),
checkBrowserSession: function(){
var lastFocus = this.get('browser-session.lastFocus');
var _self = this;
if(lastFocus >= _self.browserCheckInterval){
var since = new Date(new Date().getTime() - _self.browserCheckInterval);
this.validateCredentials().success((response)=>{
_self.get('boardSyncing').syncIssues(_self.get('model.board'), {since: since.toISOString()});
}).fail((error)=>{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than separately/explicitly check if the user is logged in here, it seems like we should just handle a 403 when we attempt to fetch latest. More generally, could we use jQuery.ajaxSetup (or a more Ember-y equivalent?) to do something like:

$.ajaxSetup({
  statusCode: {
    403: function() {
      alert( "Access denied! Do you need to [login|grant private access]?" );
    },
    500: function() {
      alert( "Oh noes, something went wrong! Try again?" );
    }
  }
});

(With better UX than alerts, of course.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We already have a global ajax handler picking up failed requests and transition to a "unauthorized" modal. User login may be the wrong criteria, I honestly didn't think that part of it through too much.

I would like to avoid using ajax catch-alls as much as possible, I see this feature as a way to "latch" on focus and fail before any unauthorized tasks are performed to begin with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see this feature as a way to "latch" on focus and fail before any unauthorized tasks are performed to begin with.

I'm not sure what that actually gains us, though. Server-side, we're checking logged_in? in either case, we just have to make two requests if we have an explicit login check.

I'm curious what scenarios we have (or might have) in the app that would require different experiences when a 403 or 500 are encountered?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm curious what scenarios we have (or might have) in the app that would require different experiences when a 403 or 500 are encountered?

Not entirely clear on the question. A session token can get stale, in which case all actions on the board will cause 403's. We handle this with the unauthorized modal right now, but it would be nice to let users know they need to "relogin" before they attempt to even do something that will fail

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right. My question is: is there any situation in the app where we wouldn't want to handle a 403 with a unified UX to give a chance to restore authorization?


});
}
}.observes('browser-session.lastFocus').on('init'),
validateCredentials: function(){
return Ember.$.getJSON('/api/logged_in');
},
browserCheckInterval: 3600000//One Hour
});

export default ApplicationController;
14 changes: 14 additions & 0 deletions ember-app/app/initializers/browser-session.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import BrowserSession from 'app/services/browser-session';

export function initialize(container, application){
application.register('browser-session:main', BrowserSession);
application.inject('controller', 'browser-session', 'browser-session:main');
application.inject('component', 'browser-session', 'browser-session:main');
}

export default {
name: 'browser-session',
after: 'advanceReadiness',
initialize: initialize
}

11 changes: 10 additions & 1 deletion ember-app/app/models/new/board.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,16 @@ var Board = Model.extend({
return issue.data.assignee && issue.data.assignee.login === assignee.login;
});
});
}).property("assignees.[]", "issues.@each.assignee")
}).property("assignees.[]", "issues.@each.assignee"),
fetchIssues: function(options){
var promises = this.get('repos').map((repo)=>{
return repo.fetchIssues(options);
});

return Ember.RSVP.all(promises).then((issues)=>{
return _.flatten(issues);
});
}
});

Board.reopenClass({
Expand Down
6 changes: 5 additions & 1 deletion ember-app/app/models/new/repo.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,11 @@ var Repo = Model.extend({
},
assigneesLength: function(){
return this.get("assignees.length");
}.property("assignees.[]")
}.property("assignees.[]"),
fetchIssues: function(options){
var url = `/api/${this.get('data.repo.full_name')}/issues`
return Ember.$.getJSON(url,{ options: options });
}
});

export default Repo;
64 changes: 64 additions & 0 deletions ember-app/app/services/board-syncing.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import Ember from 'ember';

var BoardSyncingService = Ember.Service.extend({

//Sync Notifier
flashMessages: Ember.inject.service(),
syncFlashNotifier: function(){
if(this.get('syncInProgress')){
this.get('flashMessages').add(this.messageData());
} else {
var message = this.messageData().message;
var flash = this.get('flashMessages.queue').find((flash)=>{
return flash.message === message;
});
Ember.set(flash.progress, 'status', false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any need to guard against flash being null here?

}
}.observes('syncInProgress'),
messageData: function(){
return {
message: 'syncing your board, please wait...',
sticky: true,
type: 'info',
progress: {
status: true,
callback: function(){
this.set('message', 'sync complete!');
this.get('flash')._setTimer('timer', 'destroyMessage', 3000);
}
}
};
},

//Issue Syncing
syncIssues: function(board, opts){
if(this.get('syncInProgress')){ return; }
var _self = this;
_self.set('syncInProgress', true);

board.fetchIssues(opts).then((issues)=>{
_self.issueSuccess(board, issues);
_self.set('syncInProgress', false);
}, (error)=>{
_self.issueFail();
_self.set('syncInProgress', false);
});
},
issueSuccess: function(board, issues){
if(!issues.length){ return; }
Ember.run.once(()=>{
board.get('issues').forEach((issue)=>{
issues.forEach((i)=>{
if(i.id === issue.get('id')){
Ember.set(issue, 'data', i);
};
});
});
});
},
issueFail: function(error){

}
});

export default BoardSyncingService;
31 changes: 31 additions & 0 deletions ember-app/app/services/browser-session.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import Ember from 'ember';

var BrowserSessionService = Ember.Service.extend(Ember.Evented, {
initEventObservers: function(){
var _self = this;
Ember.$(window).on('focus blur', (e)=>{
_self[`${e.type}Handlers`].forEach((h) => _self[h]());
});
}.on('init'),
setLastFocus: function(){
var before = this.get('lastBlur');
var now = new Date().getTime();
this.set('lastFocus', (now - before));
}.on('didFocusBrowser'),

//Focus Handlers
focusHandlers: ['sendFocusEvent'],
sendFocusEvent: function(){
this.trigger('didFocusBrowser');
},

//Blur Handlers
blurHandlers: ['updateLastBlur'],
lastBlur: new Date().getTime(),
updateLastBlur: function(){
var time = new Date().getTime();
this.set('lastBlur', time);
}
});

export default BrowserSessionService;
13 changes: 8 additions & 5 deletions ember-app/app/templates/components/flash/hb-message.hbs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
{{#if flash.sticky}}
<i class='ui-icon ui-icon-x-thin'></i>
{{#if progress}}
{{hb-spinner}}
{{/if}}

{{#unless progress}}
{{#if flash.sticky}}
<i class='ui-icon ui-icon-x-thin'></i>
{{/if}}
{{/unless}}

<div class='message-copy'>{{truncate message 50}}</div>

{{#if progress}}
{{hb-spinner}}
{{/if}}
64 changes: 64 additions & 0 deletions ember-app/tests/unit/services/board-syncing-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import Ember from 'ember';
import {
moduleFor,
test
} from 'ember-qunit';

var sut;
var fakeServer;
var fakeResponse;
moduleFor('service:board-syncing', {
setup: function(){
sut = this.subject();
}
});

test('syncs the boards issues successfuly', (assert)=>{
var issues = ['issue1', 'issue2'];
var success = $.ajax().then(()=>{return issues});
var board = { fetchIssues: sinon.stub().returns(success) };
sut.syncFlashNotifier = sinon.stub();
sut.issueSuccess = sinon.stub();

var done = assert.async();
sut.syncIssues(board, {});
setTimeout(()=>{
assert.ok(board.fetchIssues.calledWith({}));
assert.ok(sut.issueSuccess.calledWith(board, issues));
assert.ok(sut.get('syncInProgress') === false);
done();
}, 10);
});

test('fails gracefully on syncing the boards issues', (assert)=>{
var fail = $.ajax('fail');
var board = { fetchIssues: sinon.stub().returns(fail) };
sut.syncFlashNotifier = sinon.stub();
sut.issueFail = sinon.stub();

var done = assert.async();
sut.syncIssues(board, {});
setTimeout(()=>{
assert.ok(board.fetchIssues.calledWith({}));
assert.ok(sut.issueFail.called);
assert.ok(sut.get('syncInProgress') === false);
done();
}, 10);
});

test('sends a flash notifier on sync', (assert)=> {
var flash = { add: sinon.stub() };
sut.messageData = sinon.stub();
sut.set('flashMessages', flash);
sut.set('syncInProgress', true);

assert.ok(sut.get('flashMessages').add.calledWith(sut.messageData()));
});

test('clears flash when sync is finished', (assert)=> {
var flash = { queue: [sut.messageData()] };
sut.set('flashMessages', flash);
sut.set('syncInProgress', false);

assert.ok(flash.queue[0].progress.status === false);
});
65 changes: 65 additions & 0 deletions ember-app/tests/unit/services/browser-session-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import Ember from 'ember';
import {
moduleFor,
test
} from 'ember-qunit';

var sut;
moduleFor('service:browser-session', {
setup: function(){
sut = this.subject();
sut.focusHandlers = ['focusHandler1', 'focusHandler2'];
sut.get('focusHandlers').forEach((handler) => {
sut[handler] = sinon.stub();
});

sut.blurHandlers = ['blurHandler1', 'blurHandler2'];
sut.get('blurHandlers').forEach((handler) => {
sut[handler] = sinon.stub();
});
}
});

test('Runs Blur Handlers on window.blur', (assert)=>{
$(window).trigger('blur');

assert.ok(sut.blurHandler1.called);
assert.ok(sut.blurHandler2.called);
});

test('sets last focus interval', (assert)=>{
var done = assert.async();
var interval;
var interval2;

sut.trigger('didFocusBrowser');
interval = sut.get('lastFocus');

setTimeout(()=>{
sut.trigger('didFocusBrowser');
interval2 = sut.get('lastFocus');
assert.ok(interval < interval2);
done();
}, 10);
});

//Focus Handlers
test('Send ember observable didFocusBrowser event', (assert)=>{
sinon.stub(sut, 'trigger');

sut.sendFocusEvent();
assert.ok(sut.trigger.calledWith('didFocusBrowser'));
});

//Blur Handlers
test('Set lastBlur with time of last blur', (assert)=>{
var done = assert.async();
var current = sut.get('lastBlur');

setTimeout(()=>{
sut.updateLastBlur();
var updated = sut.get('lastBlur');
assert.ok((updated - current) >= 100);
done();
}, 100);
});
4 changes: 2 additions & 2 deletions lib/bridge/github/issues.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ def overridable(&blk)

class Huboard
module Issues
def issues(label = nil)
params = {direction: "asc"}
def issues(label = nil, opts={})
params = {direction: "asc"}.merge(opts)
params = params.merge(labels: label) if label

gh.issues(params).all.each{
Expand Down