config_service: change error message for error 403.

config_service: change error message for error 403.

Changed error message for error 403.

BUG=749050
Review-Url: https://codereview.chromium.org/2990713002
Committed: https://github.com/luci/luci-py/commit/d82109ce6507782d10b3f7ed00af18dc196827c8

[ayanaadylova, 2017-07-26 16:13:02]

ayanaadylova@google.com changed reviewers:
+ hinoka@chromium.org, sergeyberezin@chromium.org

[ayanaadylova, 2017-07-26 16:13:03]

PTAL

[Sergey Berezin, 2017-07-26 18:46:42]

https://codereview.chromium.org/2990713002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-set.html (right):

https://codereview.chromium.org/2990713002/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set.html:278: "Otherwise, try
again later.";
"Access denied" is probably the only thing that we can say at this point. Trying
again later will likely not work, the user needs to gain access first.

Can we detect whether the user is signed in or not from Polymer? (maybe query
the auth element?)
Ideally, the message would change depending on whether the user is actually
logged in or not, e.g. "Access denied, please sign in." vs. "Access denied,
<username> is not authorized to access this config set. Request access or sign
in as a different user."

If detecting sign-in is nontrivial, I'd just leave it with "Access denied.".

[ayanaadylova, 2017-07-26 21:48:04]

PTAL. Turns out the error was not that users were forbidden, it was that
refreshing the webpage on a config set launched an ajax request before the gapi
had loaded. This caused other issues around the app, so we fixed all issues
related to page-reloading.

[Sergey Berezin, 2017-07-26 22:35:51]

The code looks good! I believe the tests need an update too.

https://codereview.chromium.org/2990713002/diff/80001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2990713002/diff/80001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/config-set_test.html:297: "Otherwise,
try again later.");
Shouldn't the tests be updated with the new message?
You may also want to test for two different scenarios, when the user is not
signed in at all, and when signed in without access.

[ayanaadylova, 2017-07-26 22:49:07]

PTAL

https://codereview.chromium.org/2990713002/diff/80001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2990713002/diff/80001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/config-set_test.html:297: "Otherwise,
try again later.");
On 2017/07/26 22:35:50, Sergey Berezin wrote:
> Shouldn't the tests be updated with the new message?
> You may also want to test for two different scenarios, when the user is not
> signed in at all, and when signed in without access.

Done.

[Sergey Berezin, 2017-07-27 00:05:31]

LGTM + a few more nits. Thanks!

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:275:
test('displays error if fetch failed due to error 403 when not signed in',
function (done) {
nit: break the line to fit in 80 char. Or consider shortening the name, e.g.
'displays error on 403 when not signed in'.

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:296:
assert.equal(config_set.errorMessage, "Access denied, please sign in.");
nit: line break to fit in 80 char

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:301:
test('displays error if fetch failed due to error 403 when signed in', function
(done) {
nit: same as above - fit in 80 char, possibly by shortening the name.

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:302: param = {
Suggestion: consider creating a separate helper function within the suite which
creates and returns a new fake server based on the desired return code and the
error message (or assigns the server to a suite-global var).
Then each test calls this helper to get its copy of the test server. This way,
you only write the boilerplate once, and there is no need to .restore() it at
the end.

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:326:
assert.equal(config_set.errorMessage, "Access denied, some_email@google.com" +
nit: move the second arg to new line to fit in 80 char.

[ayanaadylova, 2017-07-27 16:57:29]

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:275:
test('displays error if fetch failed due to error 403 when not signed in',
function (done) {
On 2017/07/27 00:05:31, Sergey Berezin wrote:
> nit: break the line to fit in 80 char. Or consider shortening the name, e.g.
> 'displays error on 403 when not signed in'.

Done.

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:296:
assert.equal(config_set.errorMessage, "Access denied, please sign in.");
On 2017/07/27 00:05:31, Sergey Berezin wrote:
> nit: line break to fit in 80 char

Done.

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:301:
test('displays error if fetch failed due to error 403 when signed in', function
(done) {
On 2017/07/27 00:05:31, Sergey Berezin wrote:
> nit: same as above - fit in 80 char, possibly by shortening the name.

Done.

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:302: param = {
On 2017/07/27 00:05:31, Sergey Berezin wrote:
> Suggestion: consider creating a separate helper function within the suite
which
> creates and returns a new fake server based on the desired return code and the
> error message (or assigns the server to a suite-global var).
> Then each test calls this helper to get its copy of the test server. This way,
> you only write the boilerplate once, and there is no need to .restore() it at
> the end.

Done.

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:326:
assert.equal(config_set.errorMessage, "Access denied, some_email@google.com" +
On 2017/07/27 00:05:31, Sergey Berezin wrote:
> nit: move the second arg to new line to fit in 80 char.

Done.

[ayanaadylova, 2017-07-27 16:57:38]

The CQ bit was checked by ayanaadylova@google.com

[ayanaadylova, 2017-07-27 16:57:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org
Link to the patchset: https://codereview.chromium.org/2990713002/#ps140001
(title: "Nit: change tests for config-set page.")

[commit-bot: I haz the power, 2017-07-27 16:57:45]

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-07-27 17:00:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1501174658506550,
"parent_rev": "6674189717fad0cf0b8694eaa412e716487653ca", "commit_rev":
"d82109ce6507782d10b3f7ed00af18dc196827c8"}

[commit-bot: I haz the power, 2017-07-27 17:00:08]

Description was changed from

==========
config_service: change error message for error 403.

Changed error message for error 403.

BUG=749050
==========

to

==========
config_service: change error message for error 403.

Changed error message for error 403.

BUG=749050

Review-Url: https://codereview.chromium.org/2990713002
Committed:
https://github.com/luci/luci-py/commit/d82109ce6507782d10b3f7ed00af18dc196827c8
==========

[commit-bot: I haz the power, 2017-07-27 17:00:09]

Committed patchset #8 (id:140001) as
https://github.com/luci/luci-py/commit/d82109ce6507782d10b3f7ed00af18dc196827c8

[Sergey Berezin, 2017-07-27 20:42:18]

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
appengine/config_service/ui/test/config-ui/config-set_test.html:275:
test('displays error if fetch failed due to error 403 when not signed in',
function (done) {
On 2017/07/27 16:57:29, ayanaadylova wrote:
> On 2017/07/27 00:05:31, Sergey Berezin wrote:
> > nit: break the line to fit in 80 char. Or consider shortening the name, e.g.
> > 'displays error on 403 when not signed in'.
> 
> Done.

This wasn't actually done... Similarly other test names. OK for now, it's just a
nit.

[ayanaadylova, 2017-07-27 22:07:29]

On 2017/07/27 20:42:18, Sergey Berezin wrote:
>
https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
> File appengine/config_service/ui/test/config-ui/config-set_test.html (right):
> 
>
https://codereview.chromium.org/2990713002/diff/120001/appengine/config_servi...
> appengine/config_service/ui/test/config-ui/config-set_test.html:275:
> test('displays error if fetch failed due to error 403 when not signed in',
> function (done) {
> On 2017/07/27 16:57:29, ayanaadylova wrote:
> > On 2017/07/27 00:05:31, Sergey Berezin wrote:
> > > nit: break the line to fit in 80 char. Or consider shortening the name,
e.g.
> > > 'displays error on 403 when not signed in'.
> > 
> > Done.
> 
> This wasn't actually done... Similarly other test names. OK for now, it's just
a
> nit.

I made these changes from Mac book yesterday and I think I accidentally forgot
to save them. I will fix it in my next CL.

[Sergey Berezin, 2017-07-27 22:09:05]

On 2017/07/27 22:07:29, ayanaadylova wrote:
> I made these changes from Mac book yesterday and I think I accidentally forgot
> to save them. I will fix it in my next CL.

OK, no worries, thanks!