FIX: Gracefully handle GitHub API errors in plugin config (#70)

FIX: Gracefully handle GitHub API errors in plugin config (#70)

Previously the plugin config page was not very helpful in guiding the user if there were errors with their GitHub token or if their token did not have the correct permissions set up.

This PR:

  • Pops up an AJAX error if there is an issue with the token on the server instead of just doing a 500 error
  • Adds a warning about what the Configure Webhooks button will do
  • Disables Configure Webhooks if other errors are encountered
  • Adds acceptance tests
diff --git a/app/controllers/discourse_code_review/repos_controller.rb b/app/controllers/discourse_code_review/repos_controller.rb
index e8ad1ce..ab49cbd 100644
--- a/app/controllers/discourse_code_review/repos_controller.rb
+++ b/app/controllers/discourse_code_review/repos_controller.rb
@@ -6,11 +6,12 @@ module DiscourseCodeReview
     before_action :set_repo, only: [:has_configured_webhook, :configure_webhook]
 
     def index
-      render_json_dump(
-        client
-          .organization_repositories(organization)
-          .map(&:name)
-      )
+      repository_names = client.organization_repositories(organization).map(&:name)
+      render_json_dump(repository_names)
+    rescue Octokit::Unauthorized
+      render json: failed_json.merge(
+        error: I18n.t("discourse_code_review.bad_github_credentials_error")
+      ), status: 401
     end
 
     def has_configured_webhook
@@ -20,10 +21,13 @@ module DiscourseCodeReview
       has_configured_webhook &&= hook[:events].to_set == webhook_events.to_set
       has_configured_webhook &&= hook[:config][:url] == webhook_config[:url]
       has_configured_webhook &&= hook[:config][:content_type] == webhook_config[:content_type]
-
       render_json_dump(
         has_configured_webhook: has_configured_webhook
       )
+    rescue Octokit::NotFound
+      render json: failed_json.merge(
+        error: I18n.t("discourse_code_review.bad_github_permissions_error")
+      ), status: 400
     end
 
     def configure_webhook
diff --git a/assets/javascripts/discourse/controllers/admin-plugins-code-review.js.es6 b/assets/javascripts/discourse/controllers/admin-plugins-code-review.js.es6
index 43809c0..6bb00ce 100644
--- a/assets/javascripts/discourse/controllers/admin-plugins-code-review.js.es6
+++ b/assets/javascripts/discourse/controllers/admin-plugins-code-review.js.es6
@@ -1,4 +1,7 @@
 import { ajax } from "discourse/lib/ajax";
+import { Promise } from "rsvp";
+import discourseComputed from "discourse-common/utils/decorators";
+import { popupAjaxError } from "discourse/lib/ajax-error";
 
 const prefix = "/admin/plugins/code-review";
 
@@ -8,41 +11,93 @@ export default Ember.Controller.extend({
 
     const organizations = Ember.A([]);
     this.set("organizations", organizations);
+    this.set("loading", true);
 
     ajax(`${prefix}/organizations.json`).then((orgNames) => {
+      let promises = [];
       for (const orgName of orgNames) {
         let organization = Ember.Object.create({
           name: orgName,
           repos: Ember.A([]),
         });
         organizations.pushObject(organization);
-
-        ajax(`${prefix}/organizations/${orgName}/repos.json`).then(
-          (repoNames) => {
-            for (const repoName of repoNames) {
-              let repo = Ember.Object.create({
-                name: repoName,
-                hasConfiguredWebhook: null,
-                receivedWebhookState: false,
-              });
-              organization.repos.pushObject(repo);
-
-              ajax(
-                `${prefix}/organizations/${orgName}/repos/${repoName}/has-configured-webhook.json`
-              ).then((response) => {
-                repo.set("receivedWebhookState", true);
-                repo.set(
-                  "hasConfiguredWebhook",
-                  response["has_configured_webhook"]
-                );
-              });
-            }
-          }
-        );
+        promises.push(this.loadOrganizationRepos(organization));
       }
+
+      Promise.all(promises)
+        .then(() => this.set("loading", false))
+        .catch(() => {
+          this.set("organizationReposLoadFailed", true);
+        });
     });
   },
 
+  loadOrganizationRepos(organization) {
+    return ajax(`${prefix}/organizations/${organization.name}/repos.json`)
+      .then((repoNames) => {
+        for (const repoName of repoNames) {
+          let repo = Ember.Object.create({
+            name: repoName,
+            hasConfiguredWebhook: null,
+            receivedWebhookState: false,
+          });
+          organization.repos.pushObject(repo);
+        }
+
+        // No point continuing doing requests for the webhooks if there
+        // is an error with the first request, the token permissions must be fixed first;
+        this.hasConfiguredWebhook(organization.name, organization.repos[0])
+          .then(() => {
+            Promise.all(
+              this.loadWebhookConfiguration(
+                organization.name,
+                organization.repos
+              )
+            ).then(() => this.set("loading", false));
+          })
+          .catch((response) => {
+            this.setProperties({ loading: false, loadError: true });
+            popupAjaxError(response);
+          });
+      })
+      .catch((response) => {
+        this.setProperties({ loading: false, loadError: true });
+        popupAjaxError(response);
+      });
+  },
+
+  loadWebhookConfiguration(orgName, repos) {
+    let promises = [];
+    for (let repo of repos) {
+      promises.push(this.hasConfiguredWebhook(orgName, repo));
+    }
+    return promises;
+  },
+
+  hasConfiguredWebhook(orgName, repo) {
+    if (repo.receivedWebhookState) {
+      return Promise.resolve(true);
+    }
+
+    return ajax(
+      `${prefix}/organizations/${orgName}/repos/${repo.name}/has-configured-webhook.json`
+    )
+      .then((response) => {
+        repo.set("receivedWebhookState", true);
+        repo.set("hasConfiguredWebhook", response["has_configured_webhook"]);
+      })
+      .catch(popupAjaxError);
+  },
+
+  @discourseComputed("loadError")
+  configureWebhooksTitle(loadError) {
+    if (!loadError) {
+      return "";
+    }
+
+    return "code_review.webhooks_load_error";
+  },
+
   actions: {
     configureWebhook(organization, repo) {
       if (repo.hasConfiguredWebhook === false) {
diff --git a/assets/javascripts/discourse/templates/admin/plugins-code-review.hbs b/assets/javascripts/discourse/templates/admin/plugins-code-review.hbs
index 9984e10..2112079 100644
--- a/assets/javascripts/discourse/templates/admin/plugins-code-review.hbs
+++ b/assets/javascripts/discourse/templates/admin/plugins-code-review.hbs
@@ -1,37 +1,49 @@
 <h1>{{i18n "code_review.github_webhooks"}}</h1>
 
-{{d-button
-  action=(action "configureWebhooks")
-  label="code_review.configure_webhooks"
-  class="code-review-configure-webhooks-button"
-}}
+{{#if this.organizations}}
+  <div class="alert alert-warning">
+    {{html-safe (i18n "code_review.configure_webhooks_warning") }}
+  </div>
 
-<div class="code-review-webhook-tree">
-  {{#each this.organizations as |organization|}}
-    <div class="code-review-webhook-org">
-      <h2>{{organization.name}}</h2>
+  {{d-button
+    action=(action "configureWebhooks")
+    label="code_review.configure_webhooks"
+    class="code-review-configure-webhooks-button"
+    disabled=loadError
+    title=configureWebhooksTitle
+  }}
+{{else}}
+  {{html-safe (i18n "code_review.no_organizations_configured") }}
+{{/if}}
 
-      {{#each organization.repos as |repo|}}
-        <div class="code-review-webhook-repo">
-          <h3>{{repo.name}}</h3>
+{{#conditional-loading-spinner condition=loading}}
+  <div class="code-review-webhook-tree">
+    {{#each this.organizations as |organization|}}
+      <div class="code-review-webhook-org">
+        <h2>{{organization.name}}</h2>
 
-          {{#if repo.receivedWebhookState}}
-            {{#if repo.hasConfiguredWebhook}}
-              <div class="code-review-webhook-configured">
-                {{d-icon "check"}}
-              </div>
-            {{else}}
-              <div class="code-review-webhook-not-configured">
-                {{d-icon "times"}}
-                {{d-button
-                  action=(action "configureWebhook" organization repo)
-                  label="code_review.configure_webhook"
-                }}
-              </div>
+        {{#each organization.repos as |repo|}}

[... diff too long, it was truncated ...]

GitHub sha: 40423c19f45fd22240c530b958913d4e95a5f7ad

This commit appears in #70 which was approved by tgxworld. It was merged by martin.