Merge pull request #4455 from tgxworld/fix_category_logo_url

FIX: Form CDN URL client side instead.

app/assets/javascripts/discourse/components/category-logo-link.js.es6

@@ -1,12 +0,0 @@
-export default Em.Component.extend({
-  tagName: 'a',
-  attributeBindings: ['href'],
-  href: function() {
-    return Discourse.getURL('/c/') + Discourse.Category.slugFor(this.get('category'));
-  }.property(),
-
-  render(buffer) {
-    const categoryLogo = this.get('category.logo_url');
-    buffer.push(`<img class="category-logo" src='${categoryLogo}'/>`);
-  }
-});

app/assets/javascripts/discourse/components/category-title-link.js.es6

@@ -1,17 +1,10 @@
-import { iconHTML } from 'discourse/helpers/fa-icon';
+import computed from 'ember-addons/ember-computed-decorators';
 
 export default Em.Component.extend({
   tagName: 'h3',
 
-  render(buffer) {
-    const category = this.get('category');
-    const categoryUrl = Discourse.getURL('/c/') + Discourse.Category.slugFor(category);
-    const categoryName = Handlebars.Utils.escapeExpression(category.get('name'));
-
-    if (category.get('read_restricted')) { buffer.push(iconHTML('lock')); }
-
-    buffer.push(`<a href='${categoryUrl}'>`);
-    buffer.push(`<span class='category-name'>${categoryName}</span>`);
-    buffer.push(`</a>`);
+  @computed("category.name")
+  categoryName(name) {
+    return Handlebars.Utils.escapeExpression(name);
   }
 });

app/assets/javascripts/discourse/templates/components/categories_only.hbs

@@ -15,9 +15,6 @@
           <td class="category" style={{border-color c.color}}>
             <div>
               {{category-title-link category=c}}
-              {{#if c.logo_url}}
-                {{category-logo-link category=c}}
-              {{/if}}
               <div class="category-description">
                 {{{c.description_excerpt}}}
               </div>

app/assets/javascripts/discourse/templates/components/category-title-link.hbs

@@ -0,0 +1,11 @@
+<a href={{category.url}}>
+  {{#if category.read_restricted}}
+    {{fa-icon 'lock'}}
+  {{/if}}
+
+  <span class="category-name">{{categoryName}}</span>
+
+  {{#if category.logo_url}}
+    <div>{{cdn-img src=category.logo_url class="category-logo"}}</div>
+  {{/if}}
+</a>

app/controllers/categories_controller.rb

@@ -132,7 +132,7 @@ class CategoriesController < ApplicationController
 
       render_serialized(@category, CategorySerializer)
     else
-      render_json_error(@category) unless @category.save
+      return render_json_error(@category) unless @category.save
     end
   end
 
@@ -151,10 +151,6 @@ class CategoriesController < ApplicationController
 
       old_permissions = cat.permissions_params
 
-      # remove asset host & cdn from both logo_url and background_url (never trust the client)
-      category_params[:logo_url] = fix_upload_url(category_params[:logo_url])
-      category_params[:background_url] = fix_upload_url(category_params[:background_url])
-
       if result = cat.update_attributes(category_params)
         Scheduler::Defer.later "Log staff action change category settings" do
           @staff_action_logger.log_category_settings_change(@category, category_params, old_permissions)
@@ -262,24 +258,4 @@ class CategoriesController < ApplicationController
       params[:include_topics] ||
       SiteSetting.desktop_category_page_style == "categories_with_featured_topics".freeze
     end
-
-    def fix_upload_url(url)
-      return if url.blank?
-
-      if Discourse.asset_host.present?
-        asset_host = UrlHelper.schemaless(Discourse.asset_host)
-        url.sub!(/^(https?:)?#{Regexp.escape(asset_host)}/, "")
-      end
-
-      if SiteSetting.enable_s3_uploads? && SiteSetting.s3_cdn_url.present?
-        s3_cdn_url = UrlHelper.schemaless(SiteSetting.s3_cdn_url)
-        url.sub!(/^(https?:)?#{Regexp.escape(s3_cdn_url)}/, Discourse.store.absolute_base_url)
-      end
-
-      base_url = UrlHelper.schemaless(Discourse.base_url_no_prefix)
-      url.sub!(/^(https?:)?#{Regexp.escape(base_url)}/, "")
-
-      url
-    end
-
 end

app/serializers/basic_category_serializer.rb

@@ -40,14 +40,4 @@ class BasicCategorySerializer < ApplicationSerializer
   def notification_level
     object.notification_level
   end
-
-  def logo_url
-    url = object.logo_url
-    url.present? && UrlHelper.is_local(url) ? UrlHelper.schemaless(UrlHelper.absolute(url)) : url
-  end
-
-  def background_url
-    url = object.background_url
-    url.present? && UrlHelper.is_local(url) ? UrlHelper.schemaless(UrlHelper.absolute(url)) : url
-  end
 end

lib/validators/upload_url_validator.rb

@@ -3,7 +3,7 @@ class UploadUrlValidator < ActiveModel::EachValidator
     if value.present?
       uri = URI.parse(value) rescue nil
 
-      unless uri && Discourse.store.has_been_uploaded?(value)
+      unless uri && Upload.exists?(url: value)
         record.errors[attribute] << (options[:message] || I18n.t('errors.messages.invalid'))
       end
     end

spec/controllers/categories_controller_spec.rb

@@ -230,26 +230,6 @@ describe CategoriesController do
 
           expect(UserHistory.count).to eq(5) # 2 + 3 (bootstrap mode)
         end
-
-        it "fix both logo and background urls" do
-          Discourse.stubs(:asset_host).returns("http://foo.bar")
-          Discourse.stubs(:base_url_no_prefix).returns("http://bar.foo")
-
-          upload = build(:upload)
-
-          xhr :put, :update, id: @category.id,
-                             name: @category.name,
-                             color: @category.color,
-                             text_color: @category.text_color,
-                             slug: @category.slug,
-                             logo_url: UrlHelper.schemaless(UrlHelper.absolute(upload.url)),
-                             background_url: UrlHelper.absolute_without_cdn(upload.url)
-
-          @category.reload
-          expect(@category.logo_url).to eq(upload.url)
-          expect(@category.background_url).to eq(upload.url)
-        end
-
       end
     end
 

spec/models/category_spec.rb

@@ -14,20 +14,19 @@ describe Category do
 
   context "url validation" do
     let(:user) { Fabricate(:user) }
+
     let(:upload) { Fabricate(:upload) }
 
     it "ensures logo_url is valid" do
       expect(Fabricate.build(:category, user: user, logo_url: "---%")).not_to be_valid
       expect(Fabricate.build(:category, user: user, logo_url: "http://example.com/made-up.jpg")).not_to be_valid
       expect(Fabricate.build(:category, user: user, logo_url: upload.url)).to be_valid
-      expect(Fabricate.build(:category, user: user, logo_url: UrlHelper.schemaless(UrlHelper.absolute(upload.url)))).to be_valid
     end
 
     it "ensures background_url is valid" do
       expect(Fabricate.build(:category, user: user, background_url: ";test")).not_to be_valid
       expect(Fabricate.build(:category, user: user, background_url: "http://example.com/no.jpg")).not_to be_valid
       expect(Fabricate.build(:category, user: user, background_url: upload.url)).to be_valid
-      expect(Fabricate.build(:category, user: user, background_url: UrlHelper.schemaless(UrlHelper.absolute(upload.url)))).to be_valid
     end
   end