From c7d81e2682f2e11270f3d148f2878ad9ca0f37bc Mon Sep 17 00:00:00 2001 From: OsamaSayegh Date: Sat, 8 Sep 2018 16:24:11 +0300 Subject: [PATCH] FIX/FEATURE: don't blow up when can't reach theme's repo, show problem themes on dashboard --- .../admin-customize-themes-show.js.es6 | 13 ++++---- .../javascripts/admin/models/theme.js.es6 | 7 +++++ .../admin-customize-themes-index.js.es6 | 2 +- .../admin/templates/customize-themes-show.hbs | 12 +++++++- .../stylesheets/common/admin/customize.scss | 11 +++++++ app/models/admin_dashboard_data.rb | 19 ++++++++++-- app/models/remote_theme.rb | 30 ++++++++++++++----- app/serializers/theme_serializer.rb | 2 +- config/locales/client.en.yml | 1 + config/locales/server.en.yml | 1 + ...13_add_last_error_text_to_remote_themes.rb | 5 ++++ lib/theme_store/git_importer.rb | 17 ++++++++--- spec/models/admin_dashboard_data_spec.rb | 15 ++++++++++ spec/models/remote_theme_spec.rb | 26 ++++++++++++++++ 14 files changed, 139 insertions(+), 22 deletions(-) create mode 100644 db/migrate/20180907075713_add_last_error_text_to_remote_themes.rb diff --git a/app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6 b/app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6 index 47b59317f1a..8ee6b5d46c3 100644 --- a/app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6 +++ b/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 diff --git a/app/assets/javascripts/admin/models/theme.js.es6 b/app/assets/javascripts/admin/models/theme.js.es6 index 413d9a93c61..da881abf9bc 100644 --- a/app/assets/javascripts/admin/models/theme.js.es6 +++ b/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}`; }, diff --git a/app/assets/javascripts/admin/routes/admin-customize-themes-index.js.es6 b/app/assets/javascripts/admin/routes/admin-customize-themes-index.js.es6 index e29ef8659e7..d74a4778009 100644 --- a/app/assets/javascripts/admin/routes/admin-customize-themes-index.js.es6 +++ b/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); diff --git a/app/assets/javascripts/admin/templates/customize-themes-show.hbs b/app/assets/javascripts/admin/templates/customize-themes-show.hbs index 9cb796793a5..fcddcfa165c 100644 --- a/app/assets/javascripts/admin/templates/customize-themes-show.hbs +++ b/app/assets/javascripts/admin/templates/customize-themes-show.hbs @@ -87,10 +87,20 @@ {{/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}} + {{#if showRemoteError}} +
+ {{d-icon "exclamation-triangle"}} {{I18n "admin.customize.theme.repo_unreachable"}} +
+
+ {{model.remoteError}} +
+ {{/if}} {{/if}} diff --git a/app/assets/stylesheets/common/admin/customize.scss b/app/assets/stylesheets/common/admin/customize.scss index beefd8de9af..41bee9e411d 100644 --- a/app/assets/stylesheets/common/admin/customize.scss +++ b/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 { diff --git a/app/models/admin_dashboard_data.rb b/app/models/admin_dashboard_data.rb index fffdd0e0360..8295934a729 100644 --- a/app/models/admin_dashboard_data.rb +++ b/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| "
  • #{CGI.escapeHTML(name)}
    • " end.join("\n") - message = I18n.t("dashboard.out_of_date_themes") + message = I18n.t(i18n_key) message += "" message end diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb index 32e53fbec17..cdf1080df95 100644 --- a/app/models/remote_theme.rb +++ b/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 # diff --git a/app/serializers/theme_serializer.rb b/app/serializers/theme_serializer.rb index d8e6f5e6626..0e8291cea77 100644 --- a/app/serializers/theme_serializer.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index c9508a605be..808cc1cc9a4 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 761176b97f1..a7cdca724a1 100644 --- a/config/locales/server.en.yml +++ b/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 POP3 settings." force_https_warning: "Your website is using SSL. But `force_https` 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 ■■■■" diff --git a/db/migrate/20180907075713_add_last_error_text_to_remote_themes.rb b/db/migrate/20180907075713_add_last_error_text_to_remote_themes.rb new file mode 100644 index 00000000000..b03498eede9 --- /dev/null +++ b/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 diff --git a/lib/theme_store/git_importer.rb b/lib/theme_store/git_importer.rb index 761aebbf3f8..9238c4228b7 100644 --- a/lib/theme_store/git_importer.rb +++ b/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 diff --git a/spec/models/admin_dashboard_data_spec.rb b/spec/models/admin_dashboard_data_spec.rb index 2f4db5fa470..6d81f623dd5 100644 --- a/spec/models/admin_dashboard_data_spec.rb +++ b/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") + "" + ) + + remote.update!(last_error_text: nil) + expect(dashboard_data.out_of_date_themes).to eq(nil) + end + end end diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index ab5eb010a97..95950f19c59 100644 --- a/spec/models/remote_theme_spec.rb +++ b/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