config_service: Handled errors in the front page

config_service: Handled errors in the front page

The front page was not handling 500 errors so loading screen went forever.

BUG=748809
Review-Url: https://codereview.chromium.org/2985923002
Committed: https://github.com/luci/luci-py/commit/6674189717fad0cf0b8694eaa412e716487653ca

[cwpayton, 2017-07-26 16:46:22]

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

[cwpayton, 2017-07-26 16:46:23]

PTAL. Added error handling to front page as well as tests for the errors.

[Sergey Berezin, 2017-07-26 19:40:31]

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/front-page.html (right):

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/front-page.html:178: this.errorMessage
= "Access denied. Please try again later.";
I'd just leave it at "Access denied." There is no point in trying again later if
the user doesn't have access.
See also my comment in
https://codereview.chromium.org/2990713002/diff/40001/appengine/config_servic...
- perhaps we should coordinate these error messages on different pages, and
maybe even have common helper functions that respond to those.

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/front-page.html:180: this.errorMessage
= "Internal server error.";
Here you want to ask "Please try again later", since hopefully it's a transient
error.
Also, can we retry the request once automatically, say, after 500ms, and only
then report an error if it still fails? This should eliminate majority of error
pages due to 5xx codes.

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/front-page_test.html (right):

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/front-page_test.html:147:
suite('Front page ajax 500 error', function() {
nit: I'd name suite / tests something like:
suite('Front page ajax errors are handled correctly', ...
  test('500 error is retried once, fails the second time', ...)
  test('500 error is retried once and succeeds on retry', ...)
  test('403 error fails immediately with a message', ...) etc.
)

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/front-page_test.html:189:
suite('Front page ajax 500 error', function() {
This looks like an identical repeated test suite - did you mean to adapt it to
non-500 code?

BTW, it would be nice to be able to test for different error codes in the same
suite. Perhaps, write a helper function serverResponse(code, message) and call
it in the tests, instead of in setup().

[cwpayton, 2017-07-26 22:48:50]

PTAL

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/front-page.html (right):

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/front-page.html:178: this.errorMessage
= "Access denied. Please try again later.";
On 2017/07/26 19:40:30, Sergey Berezin wrote:
> I'd just leave it at "Access denied." There is no point in trying again later
if
> the user doesn't have access.
> See also my comment in
>
https://codereview.chromium.org/2990713002/diff/40001/appengine/config_servic...
> - perhaps we should coordinate these error messages on different pages, and
> maybe even have common helper functions that respond to those.

Done.

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/front-page.html:180: this.errorMessage
= "Internal server error.";
On 2017/07/26 19:40:30, Sergey Berezin wrote:
> Here you want to ask "Please try again later", since hopefully it's a
transient
> error.
> Also, can we retry the request once automatically, say, after 500ms, and only
> then report an error if it still fails? This should eliminate majority of
error
> pages due to 5xx codes.

I want to try to avoid any timeouts in the code, so is it okay if I simply tell
the user to refresh the page or try again later? See my changes for the exact
message.

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/front-page_test.html (right):

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/front-page_test.html:147:
suite('Front page ajax 500 error', function() {
On 2017/07/26 19:40:30, Sergey Berezin wrote:
> nit: I'd name suite / tests something like:
> suite('Front page ajax errors are handled correctly', ...
>   test('500 error is retried once, fails the second time', ...)
>   test('500 error is retried once and succeeds on retry', ...)
>   test('403 error fails immediately with a message', ...) etc.
> )

Done.

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/front-page_test.html:189:
suite('Front page ajax 500 error', function() {
On 2017/07/26 19:40:30, Sergey Berezin wrote:
> This looks like an identical repeated test suite - did you mean to adapt it to
> non-500 code?
> 
> BTW, it would be nice to be able to test for different error codes in the same
> suite. Perhaps, write a helper function serverResponse(code, message) and call
> it in the tests, instead of in setup().

Done.

[Sergey Berezin, 2017-07-26 23:09:13]

LGTM + a couple of suggestions. Thanks!

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/front-page.html (right):

https://codereview.chromium.org/2985923002/diff/20001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/front-page.html:180: this.errorMessage
= "Internal server error.";
On 2017/07/26 22:48:50, cwpayton wrote:
> On 2017/07/26 19:40:30, Sergey Berezin wrote:
> > Here you want to ask "Please try again later", since hopefully it's a
> transient
> > error.
> > Also, can we retry the request once automatically, say, after 500ms, and
only
> > then report an error if it still fails? This should eliminate majority of
> error
> > pages due to 5xx codes.
> 
> I want to try to avoid any timeouts in the code, so is it okay if I simply
tell
> the user to refresh the page or try again later? See my changes for the exact
> message.

Timeouts should be avoided for synchronization (e.g. waiting for request to
finish, or for time passing in tests), but they are perfectly legitimate for
retries. In particular, you don't want to hammer the server too fast if it's
already failing, thus a short delay before retrying is actually healthy.

It's OK to land this CL as is, but I'd encourage you to implement a retry logic,
maybe as a follow-up.

https://codereview.chromium.org/2985923002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/front-page_test.html (right):

https://codereview.chromium.org/2985923002/diff/40001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/front-page_test.html:158: server500 =
sinon.fakeServer.create();
Suggestion: instead of using setup(), how about 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.
Then each test calls this helper to get its local copy of the test server. This
way, you only write the boilerplate once, and there is no need to .restore() it
at the end.

[cwpayton, 2017-07-26 23:45:27]

The CQ bit was checked by cwpayton@google.com

[commit-bot: I haz the power, 2017-07-26 23:45:36]

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-26 23:48:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1501112727015180,
"parent_rev": "3737c64693c62ba160b0ab17af54f74b8dc65a0e", "commit_rev":
"6674189717fad0cf0b8694eaa412e716487653ca"}

[commit-bot: I haz the power, 2017-07-26 23:48:14]

Description was changed from

==========
config_service: Handled errors in the front page

The front page was not handling 500 errors so loading screen went forever.

BUG=748809
==========

to

==========
config_service: Handled errors in the front page

The front page was not handling 500 errors so loading screen went forever.

BUG=748809

Review-Url: https://codereview.chromium.org/2985923002
Committed:
https://github.com/luci/luci-py/commit/6674189717fad0cf0b8694eaa412e716487653ca
==========

[commit-bot: I haz the power, 2017-07-26 23:48:15]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-py/commit/6674189717fad0cf0b8694eaa412e716487653ca