From 923cf73c6ef4ecc83be13dbd074402d99979c158 Mon Sep 17 00:00:00 2001
From: Neil Lalonde <neillalonde@gmail.com>
Date: Thu, 15 Dec 2016 17:46:43 -0500
Subject: [PATCH] Topic Featured Links: move data from custom fields to topics
 and categories tables. Invert behaviour of topic_featured_link_allowed
 checkbox. Fix a bug with invalid topic records due to changing that category
 checkbox.

---
 .../discourse/models/category.js.es6          | 15 ++--------
 .../routes/discovery-categories.js.es6        |  3 +-
 .../components/edit-category-settings.hbs     |  2 +-
 app/controllers/categories_controller.rb      |  1 +
 app/jobs/onceoff/migrate_featured_links.rb    | 28 +++++++++++++++++++
 app/models/topic.rb                           | 12 ++------
 app/models/topic_list.rb                      |  2 --
 app/serializers/category_serializer.rb        |  3 +-
 config/locales/client.en.yml                  |  2 +-
 ...1215201907_migrate_featured_link_fields.rb |  6 ++++
 lib/discourse_featured_link.rb                | 27 ------------------
 lib/guardian/category_guardian.rb             |  3 +-
 lib/guardian/topic_guardian.rb                |  6 ++--
 lib/post_creator.rb                           |  1 -
 spec/components/guardian_spec.rb              | 18 ++++++++----
 .../fabricators/embeddable_host_fabricator.rb |  2 +-
 spec/models/topic_spec.rb                     | 17 +++++++++--
 17 files changed, 77 insertions(+), 71 deletions(-)
 create mode 100644 app/jobs/onceoff/migrate_featured_links.rb
 create mode 100644 db/migrate/20161215201907_migrate_featured_link_fields.rb
 delete mode 100644 lib/discourse_featured_link.rb

diff --git a/app/assets/javascripts/discourse/models/category.js.es6 b/app/assets/javascripts/discourse/models/category.js.es6
index a8657ad16ab..680f8b4ccfe 100644
--- a/app/assets/javascripts/discourse/models/category.js.es6
+++ b/app/assets/javascripts/discourse/models/category.js.es6
@@ -100,7 +100,8 @@ const Category = RestModel.extend({
         allowed_tags: this.get('allowed_tags'),
         allowed_tag_groups: this.get('allowed_tag_groups'),
         sort_order: this.get('sort_order'),
-        sort_ascending: this.get('sort_ascending')
+        sort_ascending: this.get('sort_ascending'),
+        topic_featured_link_allowed: this.get('topic_featured_link_allowed')
       },
       type: id ? 'PUT' : 'POST'
     });
@@ -169,18 +170,6 @@ const Category = RestModel.extend({
   @computed("id")
   isUncategorizedCategory(id) {
     return id === Discourse.Site.currentProp("uncategorized_category_id");
-  },
-
-  @computed('custom_fields.topic_featured_link_allowed')
-  topicFeaturedLinkAllowed: {
-    get(allowed) {
-      return allowed === "true";
-    },
-    set(value) {
-      value = value ? "true" : "false";
-      this.set("custom_fields.topic_featured_link_allowed", value);
-      return value;
-    }
   }
 });
 
diff --git a/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 b/app/assets/javascripts/discourse/routes/discovery-categories.js.es6
index ad7118fcc0e..9f4c76ee7d5 100644
--- a/app/assets/javascripts/discourse/routes/discovery-categories.js.es6
+++ b/app/assets/javascripts/discourse/routes/discovery-categories.js.es6
@@ -114,7 +114,8 @@ const DiscoveryCategoriesRoute = Discourse.Route.extend(OpenComposer, {
       const model = this.store.createRecord('category', {
         color: "AB9364", text_color: "FFFFFF", group_permissions: [{group_name: everyoneName, permission_type: 1}],
         available_groups: groups.map(g => g.name),
-        allow_badges: true
+        allow_badges: true,
+        topic_featured_link_allowed: true
       });
 
       showModal("edit-category", { model });
diff --git a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs
index 2900ab3af8f..c7cf75bb509 100644
--- a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs
+++ b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs
@@ -23,7 +23,7 @@
   <section class='field'>
     <div class="allowed-topic-featured-link-category">
       <label class="checkbox-label">
-        {{input type="checkbox" checked=category.topicFeaturedLinkAllowed}}
+        {{input type="checkbox" checked=category.topic_featured_link_allowed}}
         {{i18n 'category.topic_featured_link_allowed'}}
       </label>
     </div>
diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb
index 5887ec71971..15b653b91d2 100644
--- a/app/controllers/categories_controller.rb
+++ b/app/controllers/categories_controller.rb
@@ -240,6 +240,7 @@ class CategoriesController < ApplicationController
                         :topic_template,
                         :sort_order,
                         :sort_ascending,
+                        :topic_featured_link_allowed,
                         :custom_fields => [params[:custom_fields].try(:keys)],
                         :permissions => [*p.try(:keys)],
                         :allowed_tags => [],
diff --git a/app/jobs/onceoff/migrate_featured_links.rb b/app/jobs/onceoff/migrate_featured_links.rb
new file mode 100644
index 00000000000..6d08826dfc1
--- /dev/null
+++ b/app/jobs/onceoff/migrate_featured_links.rb
@@ -0,0 +1,28 @@
+module Jobs
+
+  class MigrateFeaturedLinks < Jobs::Onceoff
+
+    def execute_onceoff(args)
+      TopicCustomField.where(name: "featured_link").find_each do |tcf|
+        if tcf.value.present?
+          Topic.where(id: tcf.topic_id).update_all(featured_link: tcf.value)
+        end
+      end
+
+      # Plugin behaviour: only categories explicitly allowed to have featured links can have them.
+      # All others implicitly DO NOT allow them.
+      # If no categories were explicitly allowed to have them, then all implicitly DID allow them.
+
+      allowed = CategoryCustomField.where(name: "topic_featured_link_allowed").where(value: "true").pluck(:category_id)
+
+      if !allowed.empty?
+        # all others are not allowed
+        Category.where.not(id: allowed).update_all(topic_featured_link_allowed: false)
+      else
+        not_allowed = CategoryCustomField.where(name: "topic_featured_link_allowed").where.not(value: "true").pluck(:category_id)
+        Category.where(id: not_allowed).update_all(topic_featured_link_allowed: false)
+      end
+    end
+  end
+
+end
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 577c9315a01..1cb5b301296 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -7,7 +7,6 @@ require_dependency 'text_cleaner'
 require_dependency 'archetype'
 require_dependency 'html_prettify'
 require_dependency 'discourse_tagging'
-require_dependency 'discourse_featured_link'
 
 class Topic < ActiveRecord::Base
   include ActionView::Helpers::SanitizeHelper
@@ -76,11 +75,12 @@ class Topic < ActiveRecord::Base
 
   validates :featured_link, allow_nil: true, format: URI::regexp(%w(http https))
   validate if: :featured_link do
-    errors.add(:featured_link, :invalid_category) unless Guardian.new.can_edit_featured_link?(category_id)
+    errors.add(:featured_link, :invalid_category) unless !featured_link_changed? || Guardian.new.can_edit_featured_link?(category_id)
   end
 
   before_validation do
     self.title = TextCleaner.clean_title(TextSentinel.title_sentinel(title).text) if errors[:title].empty?
+    self.featured_link.strip! if self.featured_link
   end
 
   belongs_to :category
@@ -383,14 +383,6 @@ class Topic < ActiveRecord::Base
     featured_topic_ids ? topics.where("topics.id NOT IN (?)", featured_topic_ids) : topics
   end
 
-  def featured_link
-    custom_fields[DiscourseFeaturedLink::CUSTOM_FIELD_NAME]
-  end
-
-  def featured_link=(link)
-    custom_fields[DiscourseFeaturedLink::CUSTOM_FIELD_NAME] = link.strip
-  end
-
   def meta_data=(data)
     custom_fields.replace(data)
   end
diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb
index f758a358afb..e0b3fd8353b 100644
--- a/app/models/topic_list.rb
+++ b/app/models/topic_list.rb
@@ -1,5 +1,4 @@
 require_dependency 'avatar_lookup'
-require_dependency 'discourse_featured_link'
 
 class TopicList
   include ActiveModel::Serialization
@@ -28,7 +27,6 @@ class TopicList
     end
 
     preloaded_custom_fields << DiscourseTagging::TAGS_FIELD_NAME if SiteSetting.tagging_enabled
-    preloaded_custom_fields << DiscourseFeaturedLink::CUSTOM_FIELD_NAME if SiteSetting.topic_featured_link_enabled
   end
 
   def tags
diff --git a/app/serializers/category_serializer.rb b/app/serializers/category_serializer.rb
index 5e487a570ed..b3086fce5d6 100644
--- a/app/serializers/category_serializer.rb
+++ b/app/serializers/category_serializer.rb
@@ -15,7 +15,8 @@ class CategorySerializer < BasicCategorySerializer
              :allow_badges,
              :custom_fields,
              :allowed_tags,
-             :allowed_tag_groups
+             :allowed_tag_groups,
+             :topic_featured_link_allowed
 
   def group_permissions
     @group_permissions ||= begin
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 8f28a51a7c7..cfc18909205 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -1909,7 +1909,7 @@ en:
       tags_allowed_tag_groups: "Tag groups that can only be used in this category:"
       tags_placeholder: "(Optional) list of allowed tags"
       tag_groups_placeholder: "(Optional) list of allowed tag groups"
-      topic_featured_link_allowed: "Restricts editing the topic featured link in this category. Require site setting topic_featured_link_enabled is checked."
+      topic_featured_link_allowed: "Allow featured links in this category"
       delete: 'Delete Category'
       create: 'New Category'
       create_long: 'Create a new category'
diff --git a/db/migrate/20161215201907_migrate_featured_link_fields.rb b/db/migrate/20161215201907_migrate_featured_link_fields.rb
new file mode 100644
index 00000000000..44820a912e9
--- /dev/null
+++ b/db/migrate/20161215201907_migrate_featured_link_fields.rb
@@ -0,0 +1,6 @@
+class MigrateFeaturedLinkFields < ActiveRecord::Migration
+  def change
+    add_column :topics, :featured_link, :string
+    add_column :categories, :topic_featured_link_allowed, :boolean, default: true
+  end
+end
diff --git a/lib/discourse_featured_link.rb b/lib/discourse_featured_link.rb
deleted file mode 100644
index 304383e9234..00000000000
--- a/lib/discourse_featured_link.rb
+++ /dev/null
@@ -1,27 +0,0 @@
-module DiscourseFeaturedLink
-  CUSTOM_FIELD_NAME = 'featured_link'.freeze
-
-  AdminDashboardData::GLOBAL_REPORTS << CUSTOM_FIELD_NAME
-
-  Report.add_report(CUSTOM_FIELD_NAME) do |report|
-    report.data = []
-    link_topics = TopicCustomField.where(name: CUSTOM_FIELD_NAME)
-    link_topics = link_topics.joins(:topic).where("topics.category_id = ?", report.category_id) if report.category_id
-    link_topics.where("topic_custom_fields.created_at >= ?", report.start_date)
-               .where("topic_custom_fields.created_at <= ?", report.end_date)
-               .group("DATE(topic_custom_fields.created_at)")
-               .order("DATE(topic_custom_fields.created_at)")
-               .count
-               .each { |date, count| report.data << { x: date, y: count } }
-    report.total = link_topics.count
-    report.prev30Days = link_topics.where("topic_custom_fields.created_at >= ?", report.start_date - 30.days)
-                                   .where("topic_custom_fields.created_at <= ?", report.start_date)
-                                   .count
-  end
-
-  def self.cache_onebox_link(link)
-    # If the link is pasted swiftly, onebox may not have time to cache it
-    Oneboxer.onebox(link, invalidate_oneboxes: false)
-    link
-  end
-end
diff --git a/lib/guardian/category_guardian.rb b/lib/guardian/category_guardian.rb
index ccebde7dbf1..7e67066e892 100644
--- a/lib/guardian/category_guardian.rb
+++ b/lib/guardian/category_guardian.rb
@@ -70,7 +70,6 @@ module CategoryGuardian
   end
 
   def topic_featured_link_allowed_category_ids
-    @topic_featured_link_allowed_category_ids = CategoryCustomField.where(name: "topic_featured_link_allowed", value: "true")
-                                                                   .pluck(:category_id)
+    @topic_featured_link_allowed_category_ids = Category.where(topic_featured_link_allowed: true).pluck(:id)
   end
 end
diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb
index b107fc7e669..df65db4fe0c 100644
--- a/lib/guardian/topic_guardian.rb
+++ b/lib/guardian/topic_guardian.rb
@@ -106,8 +106,8 @@ module TopicGuardian
   end
 
   def can_edit_featured_link?(category_id)
-    SiteSetting.topic_featured_link_enabled &&
-        (topic_featured_link_allowed_category_ids.empty? || # no per category restrictions
-        category_id && topic_featured_link_allowed_category_ids.include?(category_id.to_i)) # category restriction exists
+    return false unless SiteSetting.topic_featured_link_enabled
+    return !Category.where(topic_featured_link_allowed: false).exists? unless category_id # uncategorized
+    Category.where(id: category_id, topic_featured_link_allowed: true).exists?
   end
 end
diff --git a/lib/post_creator.rb b/lib/post_creator.rb
index 8797a48f8f1..98597091c55 100644
--- a/lib/post_creator.rb
+++ b/lib/post_creator.rb
@@ -5,7 +5,6 @@ require_dependency 'topic_creator'
 require_dependency 'post_jobs_enqueuer'
 require_dependency 'distributed_mutex'
 require_dependency 'has_errors'
-require_dependency 'discourse_featured_link'
 
 class PostCreator
   include HasErrors
diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb
index bf51aadfe4d..5461c599822 100644
--- a/spec/components/guardian_spec.rb
+++ b/spec/components/guardian_spec.rb
@@ -2285,20 +2285,28 @@ describe Guardian do
     before { SiteSetting.topic_featured_link_enabled = true }
     let(:guardian) { Guardian.new }
 
-    it 'returns true if no category restricts editing link' do
-      expect(guardian.can_edit_featured_link?(nil)).to eq(true)
-      expect(guardian.can_edit_featured_link?(5)).to eq(true)
+    context "uncategorized" do
+      let!(:link_category) { Fabricate(:link_category) }
+
+      it "allows featured links if no categories forbid it" do
+        expect(guardian.can_edit_featured_link?(nil)).to eq(true)
+      end
+
+      it "forbids featured links if a category forbids it" do
+        Fabricate(:category, topic_featured_link_allowed: false)
+        expect(guardian.can_edit_featured_link?(nil)).to eq(false)
+      end
     end
 
     context 'when exist' do
-      let!(:category) { Fabricate(:category) }
+      let!(:category) { Fabricate(:category, topic_featured_link_allowed: false) }
       let!(:link_category) { Fabricate(:link_category) }
 
       it 'returns true if the category is listed' do
         expect(guardian.can_edit_featured_link?(link_category.id)).to eq(true)
       end
 
-      it 'returns false if the category is not listed' do
+      it 'returns false if the category does not allow it' do
         expect(guardian.can_edit_featured_link?(category.id)).to eq(false)
       end
     end
diff --git a/spec/fabricators/embeddable_host_fabricator.rb b/spec/fabricators/embeddable_host_fabricator.rb
index 9f589d389e0..582619f23bb 100644
--- a/spec/fabricators/embeddable_host_fabricator.rb
+++ b/spec/fabricators/embeddable_host_fabricator.rb
@@ -27,5 +27,5 @@ Fabricator(:private_category, from: :category) do
 end
 
 Fabricator(:link_category, from: :category) do
-  before_validation { |category, transients| category.custom_fields['topic_featured_link_allowed'] = 'true' }
+  before_validation { |category, transients| category.topic_featured_link_allowed = true }
 end
diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb
index 0f35c714483..400b0535f85 100644
--- a/spec/models/topic_spec.rb
+++ b/spec/models/topic_spec.rb
@@ -1755,7 +1755,7 @@ describe Topic do
       topic.featured_link = '  https://github.com/discourse/discourse'
 
       expect(topic.save).to be_truthy
-      expect(topic.custom_fields['featured_link']).to eq('https://github.com/discourse/discourse')
+      expect(topic.featured_link).to eq('https://github.com/discourse/discourse')
     end
 
     context 'when category restricts present' do
@@ -1766,13 +1766,24 @@ describe Topic do
       it 'can save the featured link if it belongs to that category' do
         link_topic.featured_link = 'https://github.com/discourse/discourse'
         expect(link_topic.save).to be_truthy
-        expect(link_topic.custom_fields['featured_link']).to eq('https://github.com/discourse/discourse')
+        expect(link_topic.featured_link).to eq('https://github.com/discourse/discourse')
       end
 
-      it 'can not save the featured link if it belongs to that category' do
+      it 'can not save the featured link if category does not allow it' do
+        topic.category = Fabricate(:category, topic_featured_link_allowed: false)
         topic.featured_link = 'https://github.com/discourse/discourse'
         expect(topic.save).to be_falsey
       end
+
+      it 'if category changes to disallow it, topic remains valid' do
+        t = Fabricate(:topic, category: link_category, featured_link: "https://github.com/discourse/discourse")
+
+        link_category.topic_featured_link_allowed = false
+        link_category.save!
+        t.reload
+
+        expect(t.valid?).to eq(true)
+      end
     end
   end
 end