From da2143b3089ec16fca449b97acbfb65acfe92197 Mon Sep 17 00:00:00 2001
From: ThibG <thib@sitedethib.com>
Date: Tue, 17 Dec 2019 13:31:56 +0100
Subject: [PATCH] Fixes featured hashtag setting page erroring out instead of
 rejecting invalid tags (#12436)

* Revert "Fix ignoring whole status because of one invalid hashtag (#11621)"

This reverts commit dff46b260b2f7d765d254c84a4b89105c7de5e97.

* Fix statuses being rejected because of invalid hashtag names

* Add spec for invalid hashtag names in statuses

* Add test for featured tags controller
---
 app/lib/activitypub/activity/create.rb        |  2 +-
 app/models/tag.rb                             |  2 +-
 .../settings/featured_tags_controller_spec.rb | 43 +++++++++++++++++++
 spec/lib/activitypub/activity/create_spec.rb  | 22 ++++++++++
 4 files changed, 67 insertions(+), 2 deletions(-)
 create mode 100644 spec/controllers/settings/featured_tags_controller_spec.rb

diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb
index 8a12a2b08b..756d5cb1c3 100644
--- a/app/lib/activitypub/activity/create.rb
+++ b/app/lib/activitypub/activity/create.rb
@@ -157,7 +157,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
     return if tag['name'].blank?
 
     Tag.find_or_create_by_names(tag['name']) do |hashtag|
-      @tags << hashtag unless @tags.include?(hashtag)
+      @tags << hashtag unless @tags.include?(hashtag) || !hashtag.valid?
     end
   rescue ActiveRecord::RecordInvalid
     nil
diff --git a/app/models/tag.rb b/app/models/tag.rb
index d3a7e1e6d4..bce76fc166 100644
--- a/app/models/tag.rb
+++ b/app/models/tag.rb
@@ -117,7 +117,7 @@ class Tag < ApplicationRecord
   class << self
     def find_or_create_by_names(name_or_names)
       Array(name_or_names).map(&method(:normalize)).uniq { |str| str.mb_chars.downcase.to_s }.map do |normalized_name|
-        tag = matching_name(normalized_name).first || create!(name: normalized_name)
+        tag = matching_name(normalized_name).first || create(name: normalized_name)
 
         yield tag if block_given?
 
diff --git a/spec/controllers/settings/featured_tags_controller_spec.rb b/spec/controllers/settings/featured_tags_controller_spec.rb
new file mode 100644
index 0000000000..33b87f9f67
--- /dev/null
+++ b/spec/controllers/settings/featured_tags_controller_spec.rb
@@ -0,0 +1,43 @@
+require 'rails_helper'
+
+describe Settings::FeaturedTagsController do
+  render_views
+
+  shared_examples 'authenticate user' do
+    it 'redirects to sign_in page' do
+      is_expected.to redirect_to new_user_session_path
+    end
+  end
+
+  describe 'POST #create' do
+    context 'when user is not sign in' do
+      subject { post :create }
+
+      it_behaves_like 'authenticate user'
+    end
+
+    context 'when user is sign in' do
+      subject { post :create, params: { featured_tag: params } }
+
+      let(:user) { Fabricate(:user, password: '12345678') }
+
+      before { sign_in user, scope: :user }
+
+      context 'when parameter is valid' do
+        let(:params) { { name: 'test' } }
+
+        it 'creates featured tag' do
+          expect { subject }.to change { user.account.featured_tags.count }.by(1)
+        end
+      end
+
+      context 'when parameter is invalid' do
+        let(:params) { { name: 'test, #foo !bleh' } }
+
+        it 'renders new' do
+          expect(subject).to render_template :index
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/activitypub/activity/create_spec.rb b/spec/lib/activitypub/activity/create_spec.rb
index b709954a30..c4efb5cc93 100644
--- a/spec/lib/activitypub/activity/create_spec.rb
+++ b/spec/lib/activitypub/activity/create_spec.rb
@@ -378,6 +378,28 @@ RSpec.describe ActivityPub::Activity::Create do
         end
       end
 
+      context 'with hashtags invalid name' do
+        let(:object_json) do
+          {
+            id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join,
+            type: 'Note',
+            content: 'Lorem ipsum',
+            tag: [
+              {
+                type: 'Hashtag',
+                href: 'http://example.com/blah',
+                name: 'foo, #eh !',
+              },
+            ],
+          }
+        end
+
+        it 'creates status' do
+          status = sender.statuses.first
+          expect(status).to_not be_nil
+        end
+      end
+
       context 'with emojis' do
         let(:object_json) do
           {