config_service: Handled errors in the front page

appengine/config_service/ui/src/config-ui/front-page.html

Index: appengine/config_service/ui/src/config-ui/front-page.html
diff --git a/appengine/config_service/ui/src/config-ui/front-page.html b/appengine/config_service/ui/src/config-ui/front-page.html
index 85fbb95c3dba80d96103335fd69f122ebf4ed1f0..2d774a4aad730a28da85a73450b19c5f4e4f1d5c 100644
--- a/appengine/config_service/ui/src/config-ui/front-page.html
+++ b/appengine/config_service/ui/src/config-ui/front-page.html
@@ -66,6 +66,7 @@
         id="requestConfigs"
         url="/_ah/api/config/v1/config-sets?include_last_import_attempt=true"
         handle-as="json"
+        on-error="_onRequestError"
         on-response="_onGotConfigSets"
         headers="[[auth_headers]]">
     </iron-ajax>
@@ -83,19 +84,24 @@
         </div>
       </template>
       <template is="dom-if" if="[[_not(isLoading)]]">
-        <template is="dom-if" if="[[_isEmpty(searchResults)]]">
-          <div class="center name">No config sets found.</div>
-        </template>
-        <template is="dom-if" if="[[_not(_isEmpty(searchResults))]]">
-          <template is="dom-repeat" items="[[searchResults]]" as="config">
-            <div class="center config-card">
-              <config-set-card
-                name="[[config.config_set]]" 
-                last-import-attempt="[[_getLastImportAttempt(config.last_import_attempt)]]">
-              </config-set-card>
-            </div>
+        <template is="dom-if" if="[[_not(errorMessage)]]">
+          <template is="dom-if" if="[[_isEmpty(searchResults)]]">
+            <div class="center name">No config sets found.</div>
+          </template>
+          <template is="dom-if" if="[[_not(_isEmpty(searchResults))]]">
+            <template is="dom-repeat" items="[[searchResults]]" as="config">
+              <div class="center config-card">
+                <config-set-card
+                  name="[[config.config_set]]" 
+                  last-import-attempt="[[_getLastImportAttempt(config.last_import_attempt)]]">
+                </config-set-card>
+              </div>
+            </template>
           </template>
         </template>
+        <template is="dom-if" if="[[errorMessage]]">
+          <div class="center name">[[errorMessage]]</div>
+        </template>
       </template>
     </div>
   </template>
@@ -109,6 +115,11 @@
           value: () => []
         },
 
+        errorMessage: {
+          type: String,
+          value: null
+        },
+
         isLoading: {
           type: Boolean,
           value: true
@@ -150,6 +161,10 @@
         return array.length === 0;
       },
 
+      _not: function(b) {
+        return !b;
+      },
+
       _onGotConfigSets: function(event) {
         this.configSetList = event.detail.response.config_sets;
         this._updateSearchResults();
@@ -157,8 +172,17 @@
         this.fire('processedConfigSets');
       },
 
-      _not: function(b) {
-        return !b;
+      _onRequestError: function(event) {
+        var error = parseInt(event.detail.error.message.match(/\d+/g));
+        if (error === 403) {
+          this.errorMessage = "Access denied. Please try again later.";

[Sergey Berezin, 2017/07/26 19:40:30]

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.

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

Done.

+        } else if (500 <= error && error < 600) {
+          this.errorMessage = "Internal server error.";

[Sergey Berezin, 2017/07/26 19:40:30]

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.

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

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.

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

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.

+        } else {
+          this.errorMessage = "Error occured. Please try again later.";
+        }
+        this.isLoading = false;
+        this.fire('fetchError');
       },
 
       _updateSearchResults: function() {