/h

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

Wait for feature flag data before loading sidebar view #2684

Merged
merged 2 commits into from Oct 29, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@robertknight @nickstenning
@robertknight
Contributor

robertknight commented Oct 27, 2015

This fixes an issue where the app could show a private group as focused in the top bar but annotations from the public group.

This happened when:

  1. The feature flag data was loaded after the initial search for annotations had been performed.
  2. The user has groups enabled
  3. In the user's previous H session, they had a private group selected.

In this scenario, when the initial search queries the focused group, it will get Public because until the feature flag data is returned, private groups is disabled.

Once the feature flag data loads, the groups feature is enabled, the focused group implicitly changes to the user's last selected private group but the annotation search is not updated.

The approach taken here is to request features at the same time as session data on sidebar load and wait for both before loading the view and requesting annotations.

features.fetch() now returns a promise which resolves when the features are fetched or the retrieval fails. I left out the exponential retry logic for the moment. If fetching feature data fails, the client will simply behave as if all features are turned off until the cache expires. I'm happy to reconsider this if we think it is important enough to keep.

  • Update the reference to SESSION_CHANGED once #2674 is merged

@robertknight robertknight added the groups-launch label Oct 29, 2015

Show outdated Hide outdated h/static/scripts/features.js
Show outdated Hide outdated h/static/scripts/app.coffee
h/static/scripts/features.js
@@ -21,58 +21,66 @@
*/
'use strict';
var retry = require('retry');

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Oct 29, 2015

Contributor

I'm pretty sure you can now remove this from package.json.
Copy link
@nickstenning

nickstenning Oct 29, 2015

Contributor

I'm pretty sure you can now remove this from package.json.
Show outdated Hide outdated h/static/scripts/features.js

robertknight added some commits Oct 27, 2015

@robertknight
Wait for feature flag data before loading sidebar view 
Feature flags can affect the initial search query when
the app loads. eg. If the groups feature is enabled then
we will by default load annotations for the most recently
used group, otherwise we'll load public annotations.

Therefore, wait for the feature flags to be retrieved
before loading the view.

The feature flags endpoint is very fast so the behavior
was correct most of the time previously but occassionally
the app would load and fetch Public annotations before
the feature data was available which would have resulted
in fetching group annotations.

At the same time, this clears the feature flag data
when the current user changes and fixes an issue where
switching from a non-groups enabled account to a groups-enabled
account did not immediately update the UI.

Fixes #2675
70f3c8a
@robertknight
Clear features cache only when logged in user changes 
Clear the features cache only when the logged-in user changes,
rather than on any change to session data.
Loading status checks…
29df1ab
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Oct 29, 2015

Contributor

LGTM.
Contributor

nickstenning commented Oct 29, 2015

LGTM.

nickstenning added a commit that referenced this pull request Oct 29, 2015

@nickstenning
Merge pull request #2684 from hypothesis/gh2675-wait_for_features 
Wait for feature flag data before loading sidebar view
f502519

@nickstenning nickstenning merged commit f502519 into master Oct 29, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@nickstenning nickstenning deleted the gh2675-wait_for_features branch Oct 29, 2015

@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Oct 29, 2015

Contributor

This also fixes #2530.
Contributor

nickstenning commented Oct 29, 2015

This also fixes #2530.

@nickstenning nickstenning referenced this pull request Oct 29, 2015

Closed

Flush feature flags on login/logout #2530

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