MD Settings: Prevent direct navigation to disabled Site Settings UI.

MD Settings: Prevent direct navigation to disabled Site Settings UI.

BUG=699421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2741343010
Cr-Commit-Position: refs/heads/master@{#457303}
Committed: https://chromium.googlesource.com/chromium/src/+/480869caaa149d728cbc2381476dbb5a153dc652

[dpapad, 2017-03-15 01:08:33]

Description was changed from

==========
MD Settings: Prevent direct navigation to disabled siteSettings UI.

BUG=699421
==========

to

==========
MD Settings: Prevent direct navigation to disabled siteSettings UI.

BUG=699421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-03-15 01:08:36]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 01:09:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 01:25:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 01:25:31]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2017-03-15 02:29:47]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 02:30:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-03-15 02:56:40]

dpapad@chromium.org changed reviewers:
+ dschuyler@chromium.org

[commit-bot: I haz the power, 2017-03-15 03:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 03:59:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dpapad, 2017-03-15 07:40:09]

Description was changed from

==========
MD Settings: Prevent direct navigation to disabled siteSettings UI.

BUG=699421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Prevent direct navigation to disabled Site Settings UI.

BUG=699421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-03-16 00:38:28]

On 2017/03/15 02:56:40, dpapad wrote:

The change to prevent navigation to /all seems reasonable.
Can you tell me more about the test changes? I was hoping to keep it under test
so that it doesn't code-rot too badly before working on it again.

[dpapad, 2017-03-16 00:49:02]

On 2017/03/16 at 00:38:28, dschuyler wrote:
> On 2017/03/15 02:56:40, dpapad wrote:
> 
> The change to prevent navigation to /all seems reasonable.
> Can you tell me more about the test changes? I was hoping to keep it under
test so that it doesn't code-rot too badly before working on it again.

The site settings code is still being tested. I am just changing how you "flip"
the boolean to true during tests. Before my CL, it was OK to do
testElement.enableSiteSettings_, because only |testElement| depended on
"enableSiteSettings". With my change, that info needs to be propagated to the
Router too, otherwise the test is failing. 

 commandLineSwitches: [{
    switchName: 'enable-site-settings',
  }],

achieves changing it in a way that is both visible by the UI code and Router. I
hope that makes sense.

[dschuyler, 2017-03-16 00:55:10]

On 2017/03/16 00:49:02, dpapad wrote:
> On 2017/03/16 at 00:38:28, dschuyler wrote:
> > On 2017/03/15 02:56:40, dpapad wrote:
> > 
> > The change to prevent navigation to /all seems reasonable.
> > Can you tell me more about the test changes? I was hoping to keep it under
> test so that it doesn't code-rot too badly before working on it again.
> 
> The site settings code is still being tested. I am just changing how you
"flip"
> the boolean to true during tests. Before my CL, it was OK to do
> testElement.enableSiteSettings_, because only |testElement| depended on
> "enableSiteSettings". With my change, that info needs to be propagated to the
> Router too, otherwise the test is failing. 
> 
>  commandLineSwitches: [{
>     switchName: 'enable-site-settings',
>   }],
> 
> achieves changing it in a way that is both visible by the UI code and Router.
I
> hope that makes sense.

Ah, I didn't understand that switchName: 'enable-site-settings', was actually
setting the value. Got it now.

[dschuyler, 2017-03-16 00:55:20]

lgtm

[dpapad, 2017-03-16 01:02:45]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-03-16 01:03:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 01:39:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489626165370400,
"parent_rev": "cd31831bb240ce19d3e3af6f2b95f8ad11c96e5c", "commit_rev":
"480869caaa149d728cbc2381476dbb5a153dc652"}

[commit-bot: I haz the power, 2017-03-16 01:40:20]

Description was changed from

==========
MD Settings: Prevent direct navigation to disabled Site Settings UI.

BUG=699421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Prevent direct navigation to disabled Site Settings UI.

BUG=699421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2741343010
Cr-Commit-Position: refs/heads/master@{#457303}
Committed:
https://chromium.googlesource.com/chromium/src/+/480869caaa149d728cbc2381476d...
==========

[commit-bot: I haz the power, 2017-03-16 01:40:25]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/480869caaa149d728cbc2381476d...