Syncs board after a period of inactivity by discorick · Pull Request #278 · huboard/huboard-web

discorick · 2016-04-20T18:04:12Z

Syncs the boards issues after a period of inactivity (currently 1 hour)

Implements a clientside BrowserSessionService which currently tracks browser focus and blurs and events on them using Ember.Evented, this is injected into all controllers and components
Implements BoardSyncService which currently can sync the boards issues

TODO:

Implement an Ember route to force a sync (good for testing)
- Example: https://huboard.com/huboard/huboard#/sync-issues
Test on Firefox
Test on IE (on windows 10 huboard is broken :( )
Test on Chrome

~~Implement failure handlers (i.e issue count >= 100)~~ I Don't think this is going to be necessary with highly frequent syncs
~~If the logged in validation fails, throw unauthorized (maybe a seperate PR)~~

Should Fix #226 & alleviate huboard/huboard#214

drusellers · 2016-04-20T18:06:04Z

config/routes.rb



  namespace :api do
+    get 'logged_in' => 'api#logged_in'


NIT: _ in a URL are 👎 prefer -

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

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

vs

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.

discorick · 2016-04-20T22:09:03Z

ember-app/app/services/board-syncing.js

+  flashMessages: Ember.inject.service(),
+  syncFlashNotifier: function(){
+    if(this.get('syncInProgress')){
+      this.get('flashMessages').add(this.messageData);


It occurs to me that this.messageData should be cloned lest we modify the original messageData object later.

…rowser-session" service

…nd model methods for fetching issues

…ne sync can run at a time

dahlbyk · 2016-04-21T19:05:20Z

ember-app/app/controllers/application.js

+      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)=>{


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.)

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.

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?

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

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?

dahlbyk · 2016-04-21T19:17:29Z

Grey out board during sync ?

Is there some way to make it read-only?

discorick · 2016-04-21T19:23:29Z

Grey out board during sync ?

Is there some way to make it read-only?

We have a CSS overlay (like on issue fullscreen) that I think would be a good touch.

dahlbyk · 2016-04-21T19:25:48Z

Grey out board during sync ?

Is there some way to make it read-only?

We have a CSS overlay (like on issue fullscreen) that I think would be a good touch.

Well I was thinking read-only but otherwise functional. Let me click on issues to see detail (and edit from there, since we always* hit GitHub for latest), but don't let me rearrange stuff in column view.

* right?

discorick · 2016-04-21T19:32:08Z

right?

Only in partial, see: #90 (I need to rewrite this to take advantage of #276 which should actually let us start keeping issues completely in-sync)

Let me click on issues to see detail (and edit ...

I am nervous about pushing the limits of the Ember run loop here. If you happen to make an edit on an issue that is being synced within the bounds of the same run loop, Ember will throw either a deprecation or possibly worse an exception.

dahlbyk · 2016-04-22T01:20:25Z

If you happen to make an edit on an issue that is being synced within the bounds of the same run loop, Ember will throw either a deprecation or possibly worse an exception.

Hrm... would we be able to compare last-modified timestamps before actually updating an issue from the sync payload? Strictly speaking, the same condition seems possible from an incoming event while the sync is happening.

I'm content with completely disabling the board while syncing, but I do think it would be preferable for the sync to less intrusive. My 2¢.

…els underlying data

…uccess message lifecycle

discorick · 2016-04-22T17:01:01Z

After 30 Seconds of no focus:

On Error:

Bulk Issue Move (makes me happy):

dahlbyk · 2016-04-22T19:45:26Z

May just be me, but the sync message is almost too distracting. Once you know there's a sync process, the notification really isn't worth drawing the eye as much as the purple box does. Maybe...

discorick · 2016-04-22T19:57:17Z

I am not sure, thats an interesting thought... Probably outside the scope of this PR though (its flash message styling)?

EDIT: @cwramsey seems to like the subtle sync message better as well.

dahlbyk · 2016-04-22T20:35:31Z

Probably outside the scope of this PR though (its flash message styling)?

Eh, I would rather see the change here if there is agreement that subtler is better.

drusellers · 2016-04-22T21:09:22Z

May just be me, but the sync message is almost too distracting. Once you know there's a sync process, the notification really isn't worth drawing the eye as much as the purple box does.

👍

discorick · 2016-04-23T00:32:42Z

@dahlbyk @cwramsey @drusellers what do you think?

cwramsey · 2016-04-23T12:47:05Z

It does look better, but I think just setting the background to transparent and nullifying any border attributes would get it out of the way even more. You could even set the X button to opacity: 0; though maybe not in case the sync somehow takes forever if github is not being very fast.

dahlbyk · 2016-04-25T02:24:24Z

@discorick did raise a good point that simple transparency won't work when the board has been scrolled (at least until we're able to fix the header):

I just pushed up a commit to try out using a semi-transparent drop shadow that matches the background:

But not sure if the shadow is really necessary?

I also noticed that the header has a subtle gradient from F3F3F3 to F9F9F9, which makes the non-transparent background not an exact match. I'm wondering if such a subtle gradient is worth keeping altogether?

discorick · 2016-04-25T15:49:55Z

I can live with no shadow, the reason I left it is like you mentioned - the gradient on the navbar makes it look funny without any additional definition.

discorick · 2016-04-25T16:35:34Z

Before we merge this, I have been thinking about thresholds over which a sync would be impractical and a "refresh" recommended toast should be sent. Does 24 hours of no board focus seem reasonable?

cwramsey · 2016-04-25T20:40:24Z

@discorick imo, 24 hours seems like a lot. I'd set it at like 2 hours maybe. I don't have any proof as to why that's better, but a lot can happen in that timeframe and if it's not automatically caught up (like if the connection is lost after sleeping your computer or something) a sync would be good then.

discorick · 2016-04-25T21:15:51Z

@cwramsey the 24 hour interval is for forced refresh, thats the point where we say "sorry you need to hit the refresh button now"

cwramsey · 2016-04-25T21:18:01Z

dahlbyk · 2016-04-29T14:37:41Z

ember-app/app/services/board-syncing.js

+      var flash = this.get('flashMessages.queue').find((flash)=>{
+        return flash.identifier === 'sync-message';
+      });
+      Ember.set(flash.progress, 'status', false);


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

dahlbyk temporarily deployed to huboard-rails-pr-278 April 20, 2016 18:04 Inactive

drusellers reviewed Apr 20, 2016
View reviewed changes

discorick mentioned this pull request Apr 20, 2016

Implements Sticky and Progress flashes #267

Merged

dahlbyk temporarily deployed to huboard-rails-pr-278 April 20, 2016 21:50 Inactive

discorick reviewed Apr 20, 2016
View reviewed changes

discorick added 11 commits April 21, 2016 10:03

Adds a route for fetching issues with optional parameters

f1cede4

Begins working on the client-side session service and tests

dc2bce4

Implements events & handlers for browser focus blur as an injected "b…

275b732

…rowser-session" service

Tweaks the focus logic and tests

40c2205

Adds logged_in endpoint, tweaks issues api endpoint

1952937

Implements basic board syncing service (sans success/fail handlers) a…

9b0f528

…nd model methods for fetching issues

Fixes a bug related to misunderstanding the RSVP.all contract

51aba7a

Uses correct method to flatten the array

f072790

Fixes flash sync messages from display an x on sync progress

04c9148

Make sure messageData is always a new object instance, ensures only o…

5829e45

…ne sync can run at a time

Implements session checking logic in the application controller

224d420

discorick force-pushed the discorick/refreshing_stale_issues branch from de2f10f to 224d420 Compare April 21, 2016 17:45

dahlbyk temporarily deployed to huboard-rails-pr-278 April 21, 2016 17:45 Inactive

dahlbyk reviewed Apr 21, 2016
View reviewed changes

Adds #/sync-issues route to manually sync the board

251b281

discorick temporarily deployed to huboard-rails-pr-278 April 21, 2016 20:02 Inactive

Fixes problem where array proxy wasn't propagating changes to the mod…

cb862ac

…els underlying data

discorick temporarily deployed to huboard-rails-pr-278 April 22, 2016 15:38 Inactive

Removes logged_in route, throttles the board sync @30 seconds

5e9bbc1

Gives syncing message 1 second (to adjust for insta-syncs) shortens s…

0edf225

…uccess message lifecycle

discorick temporarily deployed to huboard-rails-pr-278 April 22, 2016 16:47 Inactive

Implements a failure message for board syncing

e43f3e1

discorick temporarily deployed to huboard-rails-pr-278 April 22, 2016 19:45 Inactive

styles the progress flash message more subtley

8b69116

discorick temporarily deployed to huboard-rails-pr-278 April 23, 2016 00:19 Inactive

Use same-as-background shadow on progress message

533e684

dahlbyk temporarily deployed to huboard-rails-pr-278 April 25, 2016 02:05 Inactive

Force refresh after 24hours of no focus

3ac1cad

discorick temporarily deployed to huboard-rails-pr-278 April 25, 2016 19:34 Inactive

discorick changed the title ~~[WIP] Syncs board after a period of inactivity~~ Syncs board after a period of inactivity Apr 25, 2016

dahlbyk reviewed Apr 29, 2016
View reviewed changes

Gracefully returns if no flash can be found

87ca4b1

discorick deployed to huboard-rails-pr-278 May 3, 2016 20:19 View deployment

dahlbyk merged commit 115ead2 into master May 3, 2016

dahlbyk deleted the discorick/refreshing_stale_issues branch May 3, 2016 20:38

dahlbyk mentioned this pull request May 13, 2016

Enterprise Update #298

Merged

Comments

Conversation

discorick commented Apr 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dahlbyk commented Apr 21, 2016

Uh oh!

discorick commented Apr 21, 2016

Uh oh!

dahlbyk commented Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

discorick commented Apr 21, 2016

Uh oh!

dahlbyk commented Apr 22, 2016

Uh oh!

discorick commented Apr 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dahlbyk commented Apr 22, 2016

Uh oh!

discorick commented Apr 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dahlbyk commented Apr 22, 2016

Uh oh!

drusellers commented Apr 22, 2016

Uh oh!

discorick commented Apr 23, 2016

Uh oh!

cwramsey commented Apr 23, 2016

Uh oh!

dahlbyk commented Apr 25, 2016

Uh oh!

discorick commented Apr 25, 2016

Uh oh!

discorick commented Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwramsey commented Apr 25, 2016

Uh oh!

discorick commented Apr 25, 2016

Uh oh!

cwramsey commented Apr 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

discorick commented Apr 20, 2016 •

edited

Loading

dahlbyk commented Apr 21, 2016 •

edited

Loading

discorick commented Apr 22, 2016 •

edited

Loading

discorick commented Apr 22, 2016 •

edited

Loading

discorick commented Apr 25, 2016 •

edited

Loading