config_service: Handled errors in the front page

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

 <!--
   Copyright 2017 The LUCI Authors. All rights reserved.
   Use of this source code is governed under the Apache License, Version 2.0
   that can be found in the LICENSE file.
 -->
 
 <link rel="import" href="../../bower_components/app-layout/app-layout.html">
 <link rel="import" href="../../bower_components/iron-ajax/iron-ajax.html">
 <link rel="import" href="../../bower_components/paper-button/paper-button.html">
 <link rel="import" href="../../bower_components/paper-search/paper-search-bar.html">
@@ skipping 48 matching lines @@
         height: 100%;
         margin: auto;
       }
 
     </style>
 
     <iron-ajax
         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>
     
     <div class="search-bar">
       <paper-search-bar 
           query="{{query}}"
           hide-filter-button="true"></paper-search-bar>
     </div>
     
     <div class="config-list">
       <template is="dom-if" if="[[isLoading]]">
         <div class="center loading">
           <paper-spinner active></paper-spinner>
         </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 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="[[_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 is="dom-if" if="[[errorMessage]]">
+          <div class="center name">[[errorMessage]]</div>
         </template>
       </template>
     </div>
   </template>
   <script>
     Polymer({
       is: 'front-page',
 
       properties: {
         configSetList: {
           type: Array,
           value: () => []
         },
 
+        errorMessage: {
+          type: String,
+          value: null
+        },
+
         isLoading: {
           type: Boolean,
           value: true
         },
 
         query: {
           type: String,
           observer: '_updateSearchResults'
         },
 
@@ skipping 21 matching lines @@
           return lastImportAttempt;
         } else {
           return null;
         }
       },
 
       _isEmpty: function(array) {
         return array.length === 0;
       },
 
+      _not: function(b) {
+        return !b;
+      },
+
       _onGotConfigSets: function(event) {
         this.configSetList = event.detail.response.config_sets;
         this._updateSearchResults();
         this.isLoading = false;
         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() {
         // This method sorts search results by the name of the config set, that way
         // the list doesn't consist of all projects followed by all services due to
         // the path beginning with "projects/" or "services/"
         var tempResults = this.configSetList.filter(e => e.config_set.includes(this.query));
         tempResults.sort(function(a, b) {
           return this._formatName(a.config_set).localeCompare(this._formatName(b.config_set));
         }.bind(this));
         this.searchResults = tempResults;
       },
 
     });
   </script>
 </dom-module>