Allow forcing tags to be reported as trending

Using a comma-separated list in the evironment variable
`ALWAYS_TRENDING_TAGS`. The report bypasses the need to have marked
the tags set to always trend tags as approved. (It does not mark them
as approved.)

* Adjust code to better satisfy upstream rubocop

This edits upstream code (as modified for this commit)

* Refactor to get always-trending tags from environment

And for simpler method stubbing in testing.

The original form of the commit forced a single particular tag to
trend. @kibigo correctly suggested to handle this generically.

* [DRAFT, TODO] Add tests for new always-trending tags code

* Refactor Api::V1::Trends::TagsController for testing

* Add tests for always-trending tags

* Adopt @kibigo's suggestions

As per
https://gitlab.com/kibicat/mastodon/-/merge_requests/7#note_931604764
and
https://gitlab.com/kibicat/mastodon/-/merge_requests/7#note_931607104

diff --git a/app/controllers/api/v1/trends/tags_controller.rb b/app/controllers/api/v1/trends/tags_controller.rb
index 38003f599afb990d713c76216e24b4055848c53a..77342667829da1c0d73350c34ebff0bfffa7676c 100644 (file)
--- a/app/controllers/api/v1/trends/tags_controller.rb
+++ b/app/controllers/api/v1/trends/tags_controller.rb
@@ -13,14 +13,40 @@ class Api::V1::Trends::TagsController < Api::BaseController
 
   private
 
+  # Retrieve a comma-separated list of tags from the environment variable
+  # `ALWAYS_TRENDING_TAGS`, which will always be reported as trending by
+  # {#index}.
+  #
+  # `ALWAYS_TRENDING_TAGS` ought to match something like
+  # `/^(?:[^[:space]]+,)*[^[:space]]+,?$/i`, but we do not (yet?) enforce
+  # this. `ALWAYS_TRENDING_TAGS=Lune,Yuki,Baron,` is parsed as
+  # `['Lune', 'Yuki', 'Baron']`, the same as without the final comma.
+  #
+  # @return [ActiveRecord::Relation] The tags that will always be trending
+  def always_trending
+    # TODO: do we need to sanitize ALWAYS_TRENDING_TAGS?
+    # TODO: should we log when ALWAYS_TRENDING_TAGS includes a tag that does not exist?
+    # TODO: can we get the empty Relation without searching for (what we hope is) an impossible id?
+    ENV['ALWAYS_TRENDING_TAGS'].to_s.split(',')
+                                    .reduce(Tag.none) { |relation, tag_name| relation.or(Tag.where(name: tag_name)) }
+  end
+
+  # Determine the tags that will be reported as trending, overriding the
+  # `limit` param if {#always_trending} renders it necessary.
+  #
+  # Note that having too many tags always trending will render {#index}
+  # completely deterministic (as per {#BaseController::limit_param})!
+  # TODO: is that desirable? should we log a warning in that case?
   def set_tags
-    @tags = begin
-      if Setting.trends
-        Trends.tags.query.allowed.offset(offset_param).limit(limit_param(DEFAULT_TAGS_LIMIT))
-      else
-        []
-      end
-    end
+    @tags = if !Setting.trends
+              []
+            else
+              guaranteed_tags              = always_trending
+              # TODO: how does the query handle negative limits? is this necessary?
+              limit_considering_guaranteed = [0, limit_param(DEFAULT_TAGS_LIMIT) - guaranteed_tags.size].max
+
+              guaranteed_tags | Trends.tags.query.allowed.offset(offset_param).limit(limit_considering_guaranteed)
+            end
   end
 
   def insert_pagination_headers

diff --git a/spec/controllers/api/v1/trends/tags_controller_spec.rb b/spec/controllers/api/v1/trends/tags_controller_spec.rb
index d29551c56c0c1b4f7a623d71713a5d87340c74e0..1b26dd289c1391da4a3867fad663c4724de300bd 100644 (file)
--- a/spec/controllers/api/v1/trends/tags_controller_spec.rb
+++ b/spec/controllers/api/v1/trends/tags_controller_spec.rb
@@ -5,17 +5,57 @@ require 'rails_helper'
 RSpec.describe Api::V1::Trends::TagsController, type: :controller do
   render_views
 
+  # TODO: return "actually" trending tags that are not the always trending tags
+  # TODO: this is WET
   describe 'GET #index' do
-    before do
-      Fabricate.times(10, :tag).each do |tag|
-        10.times { |i| Trends.tags.add(tag, i) }
-      end
+    let(:forced_tags) { Fabricate.times(10, :tag) }
 
+    it 'returns http success' do
       get :index
+      expect(response).to have_http_status(200)
     end
 
-    it 'returns http success' do
-      expect(response).to have_http_status(200)
+    context 'with always trending tags' do
+      before do
+        forced_tags_relation = forced_tags.reduce(Tag.where(id: -1)) { |relation, forced_tag| relation.or(Tag.where(name: forced_tag.name)) }
+        allow(controller).to receive(:always_trending).and_return(forced_tags_relation)
+      end
+
+      context 'with fewer tags always trending than the limit parameter' do
+        before do
+          get :index, params: { limit: 11 }
+        end
+
+        it 'returns http success' do
+          expect(response).to have_http_status(200)
+        end
+
+        it 'respects the limit parameter' do
+          expect(response).to have_http_status(200)
+          expect(JSON.parse(response.body).size).to be <= 11
+        end
+
+        it 'responds with all always trending tags' do
+          response_tags = JSON.parse(response.body)
+          expect(forced_tags.all? { |forced_tag| response_tags.any? { |response_tag| forced_tag.name == response_tag['name'] } }).to be true
+        end
+      end
+
+      context 'with more tags always trending than the limit parameter' do
+        before do
+          get :index, params: { limit: 2 }
+        end
+
+        it 'overrides the limit parameter' do
+          expect(response).to have_http_status(200)
+          expect(JSON.parse(response.body).size).to eq(forced_tags.size)
+        end
+
+        it 'responds with only always trending tags' do
+          response_tags = JSON.parse(response.body)
+          expect(response_tags.all? { |response_tag| forced_tags.any? { |forced_tag| response_tag['name'] == forced_tag.name } }).to be true
+        end
+      end
     end
   end
 end