Merge lp://staging/~rharding/juju-gui/global-ff into lp://staging/juju-gui/experimental

Reviewers: mp+165438_code.launchpad.net,

Message:
Please take a look.

Description:
Move the featureFlag processor into window scope.

- In order to check for feature flags the processing must be done before
the
app starts
- Adds some tests to verify we parse the url correctly with our
regex/etc.
- Update the app.js to not deal with feature flags. It's done in
index.html.
- Update the simulateEvents to be triggered by the feature flag existing
vs
the custom route.

https://code.launchpad.net/~rharding/juju-gui/global-ff/+merge/165438

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9682046/

Affected files:
   A [revision details]
   M app/app.js
   M app/index.html
   M test/index.html
   A test/test_feature_flags.js
   M test/test_routing.js
   M test/test_startup.js.bottom

Please take a look.

https://codereview.appspot.com/9682046/

Thanks for this, I think this is moving in the right direction but I did
have a few comments/concerns.

https://codereview.appspot.com/9682046/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode238
app/index.html:238: /**
What do you think about moving this into its own script and including
that directly? Sure its another request but it will help curb the
tendency to keep growing index.html.

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode343
app/index.html:343: GlobalConfig.flags || {}
Does passing flags in this way somewhat conflate flags with config? If
GlobalConfig.flags has any values hasn't that moved past the
'experimental feature flag' view of the world and promoted them to
config?

https://codereview.appspot.com/9682046/diff/3001/test/test_startup.js.bottom
File test/test_startup.js.bottom (right):

https://codereview.appspot.com/9682046/diff/3001/test/test_startup.js.bottom#newcode83
test/test_startup.js.bottom:83: assert.deepEqual(window.flags, {});
If a test prior to this sets a flag I think we'll have an issue. This
seems possible to me.

https://codereview.appspot.com/9682046/

https://codereview.appspot.com/9682046/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode238
app/index.html:238: /**
On 2013/05/23 17:18:50, bcsaller wrote:
> What do you think about moving this into its own script and including
that
> directly? Sure its another request but it will help curb the tendency
to keep
> growing index.html.

FWIW, this was the approach we discussed in a pre-imp. I see your point
and don't disagree, but I'd rather not have this possibility block Rick
from landing the branch. OTOH, if he'd like to make that change, great.

https://codereview.appspot.com/9682046/

LGTM Thanks there is a trivial request if you so choose.

https://codereview.appspot.com/9682046/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9682046/diff/1/app/index.html#newcode284
app/index.html:284: // Sometimes it matches a trailing / as a second
item.
This should be able to be handled in the regex - maybe someone better at
them than me can help with the solution :)

https://codereview.appspot.com/9682046/diff/1/app/index.html#newcode342
app/index.html:342: window.location.href,
you should be able to use window.location.pathname here

https://codereview.appspot.com/9682046/

Responses below.

https://codereview.appspot.com/9682046/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode238
app/index.html:238: /**
On 2013/05/23 17:18:50, bcsaller wrote:
> What do you think about moving this into its own script and including
that
> directly? Sure its another request but it will help curb the tendency
to keep
> growing index.html.

This branch goes with the guidance of Jeff and pre-imp with Gary.

Ideally I'd prefer if this could be brought in as a YUI module and just
required in the main YUI block in index.html. It could provide an actual
api for getting/setting flags and be more flexible. This is a basic
'move' the current code.

https://codereview.appspot.com/9682046/diff/3001/app/index.html#newcode343
app/index.html:343: GlobalConfig.flags || {}
On 2013/05/23 17:18:50, bcsaller wrote:
> Does passing flags in this way somewhat conflate flags with config? If
> GlobalConfig.flags has any values hasn't that moved past the
'experimental
> feature flag' view of the world and promoted them to config?

One of the goals I'll have in a follow up is to work with the Gui charm
to allow for setting flags in the config. So yes, but it's intended. For
instance, our staging server for testing things with the browser would
want to deploy the charm and set the feature flag globally from the
charm config into the application for all users so that the url doesn't
need to be tweaked for each call.

Feature flags are meant to be deprecated and removed. I think it's a bit
different from config. In looking at the current config I think if we
get to the next step of allowing feature flags to be defined by the
config then things like simulateEvents could move to being standardized
as feature flags vs a combination of feature flag or config.

https://codereview.appspot.com/9682046/diff/3001/test/test_startup.js.bottom
File test/test_startup.js.bottom (right):

https://codereview.appspot.com/9682046/diff/3001/test/test_startup.js.bottom#newcode83
test/test_startup.js.bottom:83: assert.deepEqual(window.flags, {});
On 2013/05/23 17:18:50, bcsaller wrote:
> If a test prior to this sets a flag I think we'll have an issue. This
seems
> possible to me.

Well, if another test fails to clean up after itself it can't be the
fault of this code. This test is a good hint and should fail if someone
doesn't clean up after themselves before this test runs.

https://codereview.appspot.com/9682046/

LGTM

Thanks for the follow up.

https://codereview.appspot.com/9682046/

*** Submitted:

Move the featureFlag processor into window scope.

- In order to check for feature flags the processing must be done before
the
app starts
- Adds some tests to verify we parse the url correctly with our
regex/etc.
- Update the app.js to not deal with feature flags. It's done in
index.html.
- Update the simulateEvents to be triggered by the feature flag existing
vs
the custom route.

R=bcsaller, gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/9682046

https://codereview.appspot.com/9682046/

No approved revision specified.

No approved revision specified.