[EMB-110] Add waffle feature flagging #117

hmoco · 2018-03-13T20:05:53Z

Purpose

Allow ember-osf-web to consume waffle switches, polls, and flags for client-side feature-flagging.

Summary of Changes

Add ember-feature-flags for feature flag management, and attach a _setWaffle method to populate flags based on the currentUser (or lack of a user), since some can be based on the user session.

Side Effects / Testing Notes

No QA able to be done here. For developers, future uses would look exactly like the documentation on ember-feature-flag suggests. The features service is now included, and we can toggle template or controller logic by injecting it or using the helper feature-flags.

Ticket

https://openscience.atlassian.net/browse/EMB-110

Reviewer Checklist

brianjgeiger

Since this is for the current functionality, could you add in support for the Dashboard and Support pages? They'll be ember_<page-name>_page, so ember_dashboard_page and ember_support_page based on https://github.com/CenterForOpenScience/osf.io/pull/8208/files#diff-7139e1d97ed3c74b51c423786cd04d66R7

coveralls · 2018-03-13T22:15:48Z

Coverage remained the same at 21.122% when pulling df2e5e1 on hmoco:feature/EMB-110_waffle into 0460519 on CenterForOpenScience:develop.

…r-osf-web into feature/EMB-110_waffle

aaxelb · 2018-03-20T15:27:22Z

app/mixins/waffle-route.ts

+
+export default Mixin.create({
+    features: service(),
+    currentUser: service(),


For TS to understand, we have to repeat the name of the service, e.g. service('features')

(Until we upgrade to using ember-decorators, that is)

aaxelb · 2018-03-20T15:28:02Z

app/services/current-user.js

        });
    }),
+
+    _setWaflle: task(function* () {


Typo: Waflle

aaxelb · 2018-03-20T15:29:11Z

app/services/current-user.js

+
+    _setWaflle: task(function* () {
+        const url = `${config.OSF.apiUrl}/v2/_waffle/`;
+        const { data } = yield $.ajax(url, 'GET');


I don't think this will be a logged-in request; should probably use authenticatedAjax

What do you mean by not a logged-in request?

Like including the cookie, so the API knows what user to get the waffle for. But I just realized that's only a concern for cross-origin ajax; it'll do the right thing by default for same-origin. In case we ever use this code in other apps, though, we should probably use authenticatedAjax from ember-osf-web/utils/ajax-helpers for all our ajax needs.

aaxelb · 2018-03-20T15:41:55Z

app/mixins/waffle-route.ts

+        return this.get('currentUser._setWaflle').perform().then(() => this._super(...args));
+    },
+    actions: {
+        didTransition(...args) {


This should ideally go in willTransition, so we can reload the page as soon as possible after the user clicks. didTransition waits for all the route hooks to resolve, which can take a while.

Actually, would it be possible to do this check in the router willTransition, instead of the route? You could get the target route from the transition object, and maybe have route-to-flag map in the config? It might be nicer to have all this in a central place, rather than a mixin in every route.

Will try it! willTransititon was the ideal place for it, but it would've required it to go on the origin route, rather than the destination that cared about the flag, which seemed wrong. Did not think about the router's willTransition though.

aaxelb · 2018-03-20T16:06:38Z

app/services/current-user.js

     */
    user: Ember.computed('currentUserId', function() {
        const ObjectPromiseProxy = Ember.ObjectProxy.extend(Ember.PromiseProxyMixin);
+        this.get('_setWaflle').perform();


This sort of thing shouldn't go in a computed property, since it might not run if nothing is currently depending on user.

Instead, maybe subscribe to the session's authenticationSucceeded and invalidationSucceeded events in the constructor?

this.get('session').on('authenticationSucceeded', this, function() { this.get('_setWaffle').perform(); });

aaxelb · 2018-03-20T16:10:27Z

app/mixins/waffle-route.ts

+    features: service(),
+    currentUser: service(),
+    beforeModel(...args) {
+        return this.get('currentUser._setWaflle').perform().then(() => this._super(...args));


Calling _super inside a callback makes me wary, since beforeModel will actually return without having called _super.

…henticatedAJAX

…r-osf-web into feature/EMB-110_waffle

aaxelb · 2018-03-21T18:10:15Z

app/services/current-user.js


+    constructor() {
+        this._super(...arguments);
+        this.get('session').on('authenticationSucceeded', this, function() {


Could be simplified to .on('authenticationSucceeded', () => {

(correcting myself from when I suggested this, sorry)

aaxelb · 2018-03-21T18:14:49Z

app/services/current-user.js

    }),
+
+    _setWaffle: task(function* () {
+        if (this.get('features').isEnabled('_loaded')) {


Seems like we'd want to rewaffle when the user logs in/out. Maybe also add a reload argument (default false) to this task?

(or just delete this check if you do the waffleCheck thing above)

aaxelb · 2018-03-21T18:32:05Z

app/router.ts


+    willTransition(oldInfo, newInfo, transition) {
+        const to = transition.targetName;
+        const flag = `ember_${to}_page`;


I don't think this pattern will hold for all pages. It might be better to map route name (transition.targetName) to flag name in the config, like

featureFlags: { 'dashboard': 'ember_dashboard_page', 'guid-user': 'ember_user_profile_page', 'guid-user.settings': 'ember_user_settings_page', ... },

And we can map new feature-flagged routes as we add them.

aaxelb · 2018-03-21T19:10:19Z

app/router.ts

+        const to = transition.targetName;
+        const flag = `ember_${to}_page`;
+        if (flag in config.featureFlags) {
+            this.get('currentUser._setWaffle').perform().then(() => {


An issue with this approach is the .then() callback will run sometime later -- ideally, when waffle is already loaded we hit the reload() synchronously, before willTransition returns. Maybe something like

if (currentUser.get('waffleLoaded')) { // check flag, reload() } else { currentUser.doTheWaffle().then(() => { // check flag, reload() }); }

or you could wrap that in a method on current-user:

// current-user.ts waffleCheck(flag: string, waffleBack: (enabled: boolean) => void) { if (this.get('waffleLoaded')) { waffleBack(this.get('features').isEnabled(flag)); } else { this.get('_setWaffle').perform().then(() => { waffleBack(this.get('features').isEnabled(flag)); }); } } // router.ts currentUser.waffleCheck(flag, enabled => { if (!enabled) { window.location.reload(); } });

It's also nice that that keeps the task internal. I'm not a fan of exposing tasks if we can avoid it; better to leave them implementation details.

aaxelb · 2018-03-21T19:16:27Z

config/environment.js

+        featureFlags: { // default flags (whether they be switches, flags, or polls) go here with default value.
+            ember_support_page: true,
+            ember_dashboard_page: true,
+            _loaded: false,


I think this makes more sense as a property on current-user, since that's the only place that uses it.

…r-osf-web into feature/EMB-110_waffle

…llow for willTransititon to be triggered

jamescdavis · 2018-03-23T14:33:08Z

config/environment.js

+        featureFlags: { // default flags (whether they be switches, flags, or polls) go here with default value.
+            support: 'ember_support_page',
+            dashboard: 'ember_dashboard_page',
+            _loaded: false,


this seems to not be used

jamescdavis · 2018-03-23T14:34:35Z

config/environment.js

        },
+        featureFlags: { // default flags (whether they be switches, flags, or polls) go here with default value.
+            support: 'ember_support_page',
+            dashboard: 'ember_dashboard_page',


can we move these under routes?, e.g.

featureFlags: { routes: { support: 'ember_support_page', dashboard: 'ember_dashboard_page', } }

jamescdavis · 2018-03-23T14:44:18Z

app/services/current-user.ts

+            method: 'GET',
+        });
+        for (const flag of data) {
+            const { name } = flag.attributes;


why not destructure active as well?

jamescdavis · 2018-03-23T14:47:09Z

app/router.ts

    rootURL: config.rootURL,

+    willTransition(oldInfo, newInfo, transition) {
+        const to = transition.targetName;


Can we call this targetRouteName (or just use transition.targetName directly) to make it clear that this is a route name?

aaxelb · 2018-03-23T15:32:25Z

app/services/current-user.ts

+
+    getWaffle(this: CurrentUserService, feature: string) {
+        if (this.get('waffleLoaded')) {
+            return Promise.resolve(this.get('features').isEnabled(feature));


I like this in general, it's a better interface than I proposed, but I think Promise.resolve().then(...) will still be async. willTransition will return and the transition will continue a bit before reload is called.

It's probably not a big problem in practice, but I worry about having visual cues of a transition before loading the legacy page, and want to minimize the delay to the extent we can. I'm not sure, does that seem like a reasonable worry?

aaxelb · 2018-03-23T15:38:40Z

app/dashboard/route.ts

+        await this._super(transition);

        if (!this.get('session.isAuthenticated')) {
+            transition.abort();


Out of curiosity, do you have a sense of how much time it adds to start a whole new transition?

aaxelb · 2018-03-23T15:47:14Z

app/services/current-user.ts

    }
+
+    getWaffle(this: CurrentUserService, feature: string) {
+        if (this.get('waffleLoaded')) {


Might be worth checking setWaffle.isRunning, and return setWaffle.last.then(...) if so. Don't want to restart the task if we don't need to.

…affle

jamescdavis

Nice work!

Out of date

Add waffle feature flagging

3b4c84d

brianjgeiger previously requested changes Mar 13, 2018

View reviewed changes

hmoco added 2 commits March 20, 2018 10:15

Merge branch 'develop' of http://github.com/CenterForOpenScience/embe…

23b37c0

…r-osf-web into feature/EMB-110_waffle

Add waffle-route mixin for switch activated routes

b8ecfc8

aaxelb requested changes Mar 20, 2018

View reviewed changes

hmoco force-pushed the feature/EMB-110_waffle branch 2 times, most recently from 685bba1 to 1feb3a9 Compare March 20, 2018 18:35

Use router rather than route mixin to handle waffle checking, use aut…

caa68e5

…henticatedAJAX

hmoco force-pushed the feature/EMB-110_waffle branch from 1feb3a9 to caa68e5 Compare March 20, 2018 18:38

hmoco added 2 commits March 21, 2018 10:47

Add service:features to unit tests that now need it

da84221

Merge branch 'develop' of http://github.com/CenterForOpenScience/embe…

af828fb

…r-osf-web into feature/EMB-110_waffle

aaxelb previously requested changes Mar 21, 2018

View reviewed changes

hmoco added 2 commits March 22, 2018 10:46

Use getWaffle to grab waffle settings without exposing task

514179c

Merge branch 'develop' of http://github.com/CenterForOpenScience/embe…

97f5162

…r-osf-web into feature/EMB-110_waffle

hmoco force-pushed the feature/EMB-110_waffle branch from 12ac4fb to 8d3203d Compare March 22, 2018 17:54

Abort transitions when calling transitionTo inside a transition, to a…

095bccb

…llow for willTransititon to be triggered

hmoco force-pushed the feature/EMB-110_waffle branch from 8d3203d to 095bccb Compare March 22, 2018 18:40

jamescdavis reviewed Mar 23, 2018

View reviewed changes

aaxelb reviewed Mar 23, 2018

View reviewed changes

Use routes key for featureFlags, cleanups, account for isRunning setW…

df2e5e1

…affle

jamescdavis approved these changes Mar 26, 2018

View reviewed changes

jamescdavis merged commit 6bc1d63 into CenterForOpenScience:develop Mar 26, 2018

jamescdavis added this to the 0.3.0 milestone May 7, 2019

[EMB-110] Add waffle feature flagging #117

[EMB-110] Add waffle feature flagging #117

Uh oh!

Conversation

hmoco commented Mar 13, 2018

Purpose

Summary of Changes

Side Effects / Testing Notes

Ticket

Reviewer Checklist

Uh oh!

brianjgeiger left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 13, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

jamescdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Mar 13, 2018 •

edited

Loading