FIX/FEATURE: don't blow up when can't reach theme's repo, show problem themes on dashboard

app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6

@@ -1,7 +1,4 @@
-import {
-  default as computed,
-  observes
-} from "ember-addons/ember-computed-decorators";
+import { default as computed } from "ember-addons/ember-computed-decorators";
 import { url } from "discourse/lib/computed";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import showModal from "discourse/lib/show-modal";
@@ -27,7 +24,7 @@ export default Ember.Controller.extend({
   },
 
   @computed("model.editedFields")
-  editedFieldsFormatted(fields) {
+  editedFieldsFormatted() {
     const descriptions = [];
     ["common", "desktop", "mobile"].forEach(target => {
       const fields = this.editedFieldsForTarget(target);
@@ -96,6 +93,12 @@ export default Ember.Controller.extend({
   hasSettings(settings) {
     return settings.length > 0;
   },
+
+  @computed("model.remoteError", "updatingRemote")
+  showRemoteError(errorMessage, updating) {
+    return errorMessage && !updating;
+  },
+
   editedFieldsForTarget(target) {
     return this.get("model.editedFields").filter(
       field => field.target === target

app/assets/javascripts/admin/models/theme.js.es6

@@ -54,6 +54,13 @@ const Theme = RestModel.extend({
     );
   },
 
+  @computed("remote_theme.last_error_text")
+  remoteError(errorText) {
+    if (errorText && errorText.length > 0) {
+      return errorText;
+    }
+  },
+
   getKey(field) {
     return `${field.target} ${field.name}`;
   },

app/assets/javascripts/admin/routes/admin-customize-themes-index.js.es6

@@ -17,7 +17,7 @@ const externalResources = [
 ];
 
 export default Ember.Route.extend({
-  setupController(controller, model) {
+  setupController(controller) {
     this._super(...arguments);
     this.controllerFor("adminCustomizeThemes").set("editingTheme", false);
     controller.set("externalResources", externalResources);

app/assets/javascripts/admin/templates/customize-themes-show.hbs

@@ -87,10 +87,20 @@
               </a>
             {{/if}}
           {{else}}
-            {{i18n 'admin.customize.theme.up_to_date'}} {{format-date model.remote_theme.updated_at leaveAgo="true"}}
+            {{#unless showRemoteError}}
+              {{i18n 'admin.customize.theme.up_to_date'}} {{format-date model.remote_theme.updated_at leaveAgo="true"}}
+            {{/unless}}
           {{/if}}
         {{/if}}
       </span>
+      {{#if showRemoteError}}
+        <div class="error-message">
+          {{d-icon "exclamation-triangle"}} {{I18n "admin.customize.theme.repo_unreachable"}}
+        </div>
+        <div class="raw-error">
+          <code>{{model.remoteError}}</code>
+        </div>
+      {{/if}}
     {{/if}}
   </div>
 

app/assets/stylesheets/common/admin/customize.scss

@@ -88,6 +88,17 @@
   .admin-container {
     padding: 0;
   }
+  .error-message {
+    margin-top: 5px;
+    margin-bottom: 5px;
+    .fa {
+      color: $danger;
+    }
+  }
+  .raw-error {
+    background-color: $primary-very-low;
+    padding: 5px;
+  }
 }
 
 .admin-customize {

app/models/admin_dashboard_data.rb

@@ -102,7 +102,7 @@ class AdminDashboardData
                       :failing_emails_check,
                       :subfolder_ends_in_slash_check,
                       :pop3_polling_configuration, :email_polling_errored_recently,
-                      :out_of_date_themes
+                      :out_of_date_themes, :unreachable_themes
 
     add_problem_check do
       sidekiq_check || queue_size_check
@@ -252,11 +252,24 @@ class AdminDashboardData
     old_themes = RemoteTheme.out_of_date_themes
     return unless old_themes.present?
 
-    html = old_themes.map do |name, id|
+    themes_html_format(old_themes, "dashboard.out_of_date_themes")
+  end
+
+  def unreachable_themes
+    themes = RemoteTheme.unreachable_themes
+    return unless themes.present?
+
+    themes_html_format(themes, "dashboard.unreachable_themes")
+  end
+
+  private
+
+  def themes_html_format(themes, i18n_key)
+    html = themes.map do |name, id|
       "<li><a href=\"/admin/customize/themes/#{id}\">#{CGI.escapeHTML(name)}</a></li>"
     end.join("\n")
 
-    message = I18n.t("dashboard.out_of_date_themes")
+    message = I18n.t(i18n_key)
     message += "<ul>#{html}</ul>"
     message
   end

app/models/remote_theme.rb

@@ -10,6 +10,9 @@ class RemoteTheme < ActiveRecord::Base
   GITHUB_SSH_REGEXP = /^git@github\.com:/
 
   has_one :theme
+  scope :joined_remotes, -> {
+    joins("JOIN themes ON themes.remote_theme_id = remote_themes.id").where.not(remote_url: "")
+  }
 
   def self.update_tgz_theme(filename, user: Discourse.system_user)
     importer = ThemeStore::TgzImporter.new(filename)
@@ -61,17 +64,24 @@ class RemoteTheme < ActiveRecord::Base
   end
 
   def self.out_of_date_themes
-    self.joins("JOIN themes ON themes.remote_theme_id = remote_themes.id")
-      .where.not(remote_url: "")
-      .where("commits_behind > 0 OR remote_version <> local_version")
+    self.joined_remotes.where("commits_behind > 0 OR remote_version <> local_version")
       .pluck("themes.name", "themes.id")
   end
 
+  def self.unreachable_themes
+    self.joined_remotes.where("last_error_text IS NOT NULL").pluck("themes.name", "themes.id")
+  end
+
   def update_remote_version
     importer = ThemeStore::GitImporter.new(remote_url, private_key: private_key)
-    importer.import!
-    self.updated_at = Time.zone.now
-    self.remote_version, self.commits_behind = importer.commits_since(local_version)
+    begin
+      importer.import!
+    rescue ThemeStore::GitImporter::ImportFailed => err
+      self.last_error_text = err.message
+    else
+      self.updated_at = Time.zone.now
+      self.remote_version, self.commits_behind = importer.commits_since(local_version)
+    end
   end
 
   def update_from_remote(importer = nil, skip_update: false)
@@ -81,7 +91,12 @@ class RemoteTheme < ActiveRecord::Base
     unless importer
       cleanup = true
       importer = ThemeStore::GitImporter.new(remote_url, private_key: private_key)
-      importer.import!
+      begin
+        importer.import!
+      rescue ThemeStore::GitImporter::ImportFailed => err
+        self.last_error_text = err.message
+        return self
+      end
     end
 
     theme_info = JSON.parse(importer["about.json"])
@@ -225,4 +240,5 @@ end
 #  created_at        :datetime         not null
 #  updated_at        :datetime         not null
 #  private_key       :text
+#  last_error_text   :text
 #

app/serializers/theme_serializer.rb

@@ -47,7 +47,7 @@ end
 class RemoteThemeSerializer < ApplicationSerializer
   attributes :id, :remote_url, :remote_version, :local_version, :about_url,
              :license_url, :commits_behind, :remote_updated_at, :updated_at,
-             :github_diff_link
+             :github_diff_link, :last_error_text
 
   # wow, AMS has some pretty nutty logic where it tries to find the path here
   # from action dispatch, tell it not to

config/locales/client.en.yml

@@ -3287,6 +3287,7 @@ en:
             one: "Theme is 1 commit behind!"
             other: "Theme is {{count}} commits behind!"
           compare_commits: "(See new commits)"
+          repo_unreachable: "Couldn't contact the Git repository of this theme. Error message:"
           scss:
             text: "CSS"
             title: "Enter custom CSS, we accept all valid CSS and SCSS styles"

config/locales/server.en.yml

@@ -1118,6 +1118,7 @@ en:
     poll_pop3_auth_error: "Connection to the POP3 server is failing with an authentication error. Please check your <a href='/admin/site_settings/category/email'>POP3 settings</a>."
     force_https_warning: "Your website is using SSL. But `<a href='/admin/site_settings/category/all_results?filter=force_https'>force_https</a>` is not yet enabled in your site settings."
     out_of_date_themes: "Updates are available for the following themes:"
+    unreachable_themes: "We were unable to check for updates on the following themes:"
 
   site_settings:
     censored_words: "Words that will be automatically replaced with &#9632;&#9632;&#9632;&#9632;"

db/migrate/20180907075713_add_last_error_text_to_remote_themes.rb

@@ -0,0 +1,5 @@
+class AddLastErrorTextToRemoteThemes < ActiveRecord::Migration[5.2]
+  def change
+    add_column :remote_themes, :last_error_text, :text
+  end
+end

lib/theme_store/git_importer.rb

@@ -2,6 +2,7 @@ module ThemeStore; end
 
 class ThemeStore::GitImporter
 
+  class ImportFailed < StandardError; end
   attr_reader :url
 
   def initialize(url, private_key: nil)
@@ -65,7 +66,11 @@ class ThemeStore::GitImporter
   protected
 
   def import_public!
-    Discourse::Utils.execute_command("git", "clone", @url, @temp_folder)
+    begin
+      Discourse::Utils.execute_command("git", "clone", @url, @temp_folder)
+    rescue => err
+      raise ImportFailed.new(err.message)
+    end
   end
 
   def import_private!
@@ -77,9 +82,13 @@ class ThemeStore::GitImporter
       FileUtils.chmod(0600, 'id_rsa')
     end
 
-    Discourse::Utils.execute_command({
-      'GIT_SSH_COMMAND' => "ssh -i #{ssh_folder}/id_rsa -o StrictHostKeyChecking=no"
-    }, "git", "clone", @url, @temp_folder)
+    begin
+      Discourse::Utils.execute_command({
+        'GIT_SSH_COMMAND' => "ssh -i #{ssh_folder}/id_rsa -o StrictHostKeyChecking=no"
+      }, "git", "clone", @url, @temp_folder)
+    rescue => err
+      raise ImportFailed.new(err.message)
+    end
   ensure
     FileUtils.rm_rf ssh_folder
   end

spec/models/admin_dashboard_data_spec.rb

@@ -351,4 +351,19 @@ describe AdminDashboardData do
       expect(dashboard_data.out_of_date_themes).to eq(nil)
     end
   end
+
+  describe '#unreachable_themes' do
+    let(:remote) { RemoteTheme.create!(remote_url: "https://github.com/org/testtheme", last_error_text: "can't reach repo :'(") }
+    let!(:theme) { Fabricate(:theme, remote_theme: remote, name: "Test< Theme") }
+
+    it "outputs correctly formatted html" do
+      dashboard_data = described_class.new
+      expect(dashboard_data.unreachable_themes).to eq(
+        I18n.t("dashboard.unreachable_themes") + "<ul><li><a href=\"/admin/customize/themes/#{theme.id}\">Test&lt; Theme</a></li></ul>"
+      )
+
+      remote.update!(last_error_text: nil)
+      expect(dashboard_data.out_of_date_themes).to eq(nil)
+    end
+  end
 end

spec/models/remote_theme_spec.rb

@@ -187,6 +187,20 @@ describe RemoteTheme do
     end
   end
 
+  context ".joined_remotes" do
+    it "finds records that are associated with themes" do
+      github_repo
+      gitlab_repo
+      expect(RemoteTheme.joined_remotes).to eq([])
+
+      Fabricate(:theme, remote_theme: github_repo)
+      expect(RemoteTheme.joined_remotes).to eq([github_repo])
+
+      Fabricate(:theme, remote_theme: gitlab_repo)
+      expect(RemoteTheme.joined_remotes).to contain_exactly(github_repo, gitlab_repo)
+    end
+  end
+
   context ".out_of_date_themes" do
     let(:remote) { RemoteTheme.create!(remote_url: "https://github.com/org/testtheme") }
     let!(:theme) { Fabricate(:theme, remote_theme: remote) }
@@ -199,4 +213,16 @@ describe RemoteTheme do
       expect(described_class.out_of_date_themes).to eq([])
     end
   end
+
+  context ".unreachable_themes" do
+    let(:remote) { RemoteTheme.create!(remote_url: "https://github.com/org/testtheme", last_error_text: "can't contact this repo :(") }
+    let!(:theme) { Fabricate(:theme, remote_theme: remote) }
+
+    it "finds out of date themes" do
+      expect(described_class.unreachable_themes).to eq([[theme.name, theme.id]])
+
+      remote.update!(last_error_text: nil)
+      expect(described_class.unreachable_themes).to eq([])
+    end
+  end
 end