Follow-ups for suggested organizations follows during onboarding (#19619)

* Documentation for /api/follows end-point

* Rename and refactor User suggester

* Clean up feature flag

* User average count should be unscoped

app/controllers/concerns/api/follows_controller.rb

@@ -3,12 +3,15 @@ module Api
     extend ActiveSupport::Concern
 
     def create
+      # API authentication might set current_user or might just set @user
+      active_user = current_user || @user
+
       user_ids.each do |user_id|
-        Users::FollowWorker.perform_async(current_user.id, user_id, "User")
+        Users::FollowWorker.perform_async(active_user.id, user_id, "User")
       end
 
       org_ids.each do |org_id|
-        Users::FollowWorker.perform_async(current_user.id, org_id, "Organization")
+        Users::FollowWorker.perform_async(active_user.id, org_id, "Organization")
       end
 
       render json: {
@@ -26,16 +29,28 @@ module Api
 
     private
 
+    # The strange params situation here is mostly due to a bug in the way
+    # rswag processes array-type params that appear in a query-string.
+    # Also, Rails' strong parameters does not work well with arrays-of-objects.
+    # So, in order to include this end-point in the swagger/open-api
+    # documentation, while maintaining compatibility with the existing
+    # infrastructure, we need to accept two kinds of inputs here -
+    # in one case, we receive a list of integer IDs,
+    # in the other case, we receive an array of objects, like {id: 123}
     def user_ids
-      return [] if params[:users].blank?
-
-      @user_ids ||= params[:users].pluck("id")
+      @user_ids ||= permitted_params[:user_ids].presence
+      @user_ids ||= params[:users].pluck("id") if params[:users].present?
+      @user_ids ||= []
     end
 
     def org_ids
-      return [] if params[:organizations].blank?
+      @org_ids ||= permitted_params[:organization_ids].presence
+      @org_ids ||= params[:organizations].pluck("id") if params[:organizations].present?
+      @org_ids ||= []
+    end
 
-      @org_ids ||= params[:organizations].pluck("id")
+    def permitted_params
+      params.permit(user_ids: [], organization_ids: [])
     end
   end
 end

app/controllers/onboardings_controller.rb

@@ -16,8 +16,7 @@ class OnboardingsController < ApplicationController
   end
 
   def users_and_organizations
-    suggested_follows = suggested_user_follows
-    suggested_follows += suggested_organization_follows if feature_flag_enabled?(:suggest_organizations)
+    suggested_follows = suggested_user_follows + suggested_organization_follows
     @suggestions = ApplicationDecorator.decorate_collection(suggested_follows)
   end
 
@@ -108,7 +107,7 @@ class OnboardingsController < ApplicationController
   end
 
   def suggested_user_follows
-    Users::SuggestRecent.call(current_user,
-                              attributes_to_select: SUGGESTED_USER_ATTRIBUTES)
+    Users::SuggestProminent.call(current_user,
+                                 attributes_to_select: SUGGESTED_USER_ATTRIBUTES)
   end
 end

app/models/user.rb

@@ -213,6 +213,13 @@ class User < ApplicationRecord
     order(updated_at: :desc).limit(active_limit)
   }
 
+  scope :above_average, lambda {
+    where(
+      articles_count: average_articles_count..,
+      comments_count: average_comments_count..,
+    )
+  }
+
   before_validation :downcase_email
 
   # make sure usernames are not empty, to be able to use the database unique index
@@ -229,6 +236,18 @@ class User < ApplicationRecord
   after_commit :subscribe_to_mailchimp_newsletter
   after_commit :bust_cache
 
+  def self.average_articles_count
+    Rails.cache.fetch("established_user_article_count", expires_in: 1.day) do
+      unscoped { where(articles_count: 1..).average(:articles_count) || average(:articles_count) } || 0.0
+    end
+  end
+
+  def self.average_comments_count
+    Rails.cache.fetch("established_user_comment_count", expires_in: 1.day) do
+      unscoped { where(comments_count: 1..).average(:comments_count) || average(:comments_count) } || 0.0
+    end
+  end
+
   def self.staff_account
     find_by(id: Settings::Community.staff_user_id)
   end

app/queries/users/suggest_prominent.rb

@@ -0,0 +1,70 @@
+module Users
+  class SuggestProminent
+    RETURNING = 50
+    RECENT_WEEKS_TOP_PRODUCERS = 3
+    USER_FOLLOWS_TAG_LIMIT = 5
+    RECENT_TOP_PRODUCER_LIMIT = 80
+    RECENT_COMMENTER_LIMIT = 30
+    MINIMUM_COMMENTS = 4
+
+    def self.call(user, attributes_to_select: [])
+      new(user, attributes_to_select: attributes_to_select).suggest
+    end
+
+    def initialize(user, attributes_to_select: [])
+      @user = user
+      @attributes_to_select = attributes_to_select
+    end
+
+    def suggest
+      users_to_follow = recently_published_articles_from_followed_tags if following_some_tags.any?
+      users_to_follow ||= recent_commenters + recent_top_producers
+
+      (users_to_follow - [user]).uniq.sample(RETURNING)
+    end
+
+    private
+
+    attr_reader :user, :attributes_to_select
+
+    def following_some_tags
+      user.decorate.cached_followed_tag_names.sample(USER_FOLLOWS_TAG_LIMIT)
+    end
+
+    def tagged_article_user_ids(num_weeks)
+      articles_from_followed_tags = Article.above_average
+        .tagged_with(following_some_tags, any: true)
+        .where(published_at: num_weeks.weeks.ago..)
+
+      articles_from_followed_tags
+        .pluck(:user_id)
+        .each_with_object(Hash.new(0)) { |id, counts| counts[id] += 1 }
+        .sort_by { |_id, count| count }
+        .map(&:first)
+    end
+
+    def recently_published_articles_from_followed_tags(num_weeks = RECENT_WEEKS_TOP_PRODUCERS)
+      users = user_finder.where(id: tagged_article_user_ids(num_weeks))
+      relation_as_array(users, limit: RECENT_TOP_PRODUCER_LIMIT)
+    end
+
+    def recent_top_producers
+      users = user_finder.above_average
+      relation_as_array(users, limit: RETURNING)
+    end
+
+    def recent_commenters(num_comments = MINIMUM_COMMENTS, limit = RECENT_COMMENTER_LIMIT)
+      commenters = user_finder.where(comments_count: num_comments..)
+      relation_as_array(commenters, limit: limit)
+    end
+
+    def relation_as_array(relation, limit:)
+      relation = relation.joins(:profile).select(attributes_to_select) if attributes_to_select.any?
+      relation.order(updated_at: :desc).limit(limit).to_a
+    end
+
+    def user_finder
+      @user_finder ||= User.includes(:profile).without_role(:suspended)
+    end
+  end
+end

app/queries/users/suggest_recent.rb

@@ -1,81 +0,0 @@
-module Users
-  class SuggestRecent
-    def self.call(user, attributes_to_select: [])
-      new(user, attributes_to_select: attributes_to_select).suggest
-    end
-
-    def initialize(user, attributes_to_select: [])
-      @user = user
-      @cached_followed_tag_names = user.decorate.cached_followed_tag_names
-      @attributes_to_select = attributes_to_select
-    end
-
-    def suggest
-      if cached_followed_tag_names.any?
-        (recent_producers(3) - [user]).sample(50).uniq
-      else
-        (recent_commenters(4, 30) + recent_top_producers - [user]).uniq.sample(50)
-      end
-    end
-
-    private
-
-    attr_reader :user, :attributes_to_select, :cached_followed_tag_names
-
-    def tagged_article_user_ids(num_weeks = 1)
-      Article.published
-        .tagged_with(cached_followed_tag_names.sample(5), any: true)
-        .where(score: article_score_average.., published_at: num_weeks.weeks.ago..)
-        .pluck(:user_id)
-        .each_with_object(Hash.new(0)) { |value, counts| counts[value] += 1 }
-        .sort_by { |_key, value| value }
-        .map(&:first)
-    end
-
-    def recent_producers(num_weeks = 1)
-      relation_as_array(
-        user_relation.where(id: tagged_article_user_ids(num_weeks)),
-        limit: 80,
-      )
-    end
-
-    def recent_top_producers
-      relation = user_relation.where(
-        articles_count: established_user_article_count..,
-        comments_count: established_user_comment_count..,
-      )
-      relation_as_array(relation, limit: 50)
-    end
-
-    def recent_commenters(num_comments = 2, limit = 8)
-      relation_as_array(user_relation.where(comments_count: num_comments + 1..), limit: limit)
-    end
-
-    def relation_as_array(relation, limit:)
-      relation = relation.joins(:profile).select(attributes_to_select) if attributes_to_select.any?
-      relation.order(updated_at: :desc).limit(limit).to_a
-    end
-
-    def established_user_article_count
-      Rails.cache.fetch("established_user_article_count", expires_in: 1.day) do
-        user_relation.where(articles_count: 1..).average(:articles_count) || User.average(:articles_count)
-      end
-    end
-
-    def established_user_comment_count
-      Rails.cache.fetch("established_user_comment_count", expires_in: 1.day) do
-        user_relation.where(comments_count: 1..).average(:comments_count) || User.average(:comments_count)
-      end
-    end
-
-    def article_score_average
-      Rails.cache.fetch("article_score_average", expires_in: 1.day) do
-        Article.where(score: 0..).average(:score) || Article.average(:score)
-      end
-    end
-
-    def user_relation
-      User.includes(:profile).without_role(:suspended)
-    end
-  end
-end

spec/models/user_spec.rb

@@ -867,4 +867,49 @@ RSpec.describe User do
       expect(Tag).to have_received(:followed_by).with(user)
     end
   end
+
+  describe ".above_average and .average_x_count" do
+    context "when there are not yet any articles with score above 0" do
+      it "works as expected" do
+        expect(described_class.average_articles_count).to be_within(0.1).of(0.0)
+        expect(described_class.average_comments_count).to be_within(0.1).of(0.0)
+        users = described_class.above_average
+        expect(users.pluck(:articles_count)).to eq([])
+        expect(users.pluck(:comments_count)).to eq([])
+      end
+    end
+
+    context "when there are users with articles_count" do
+      before do
+        create(:user, articles_count: 10)
+        create(:user, articles_count: 6)
+        create(:user, articles_count: 4)
+        create(:user, articles_count: 1)
+        # averages 5.25
+      end
+
+      it "works as expected" do
+        expect(described_class.average_articles_count).to be_within(0.1).of(5.25)
+        articles = described_class.above_average
+        expect(articles.pluck(:articles_count)).to contain_exactly(10, 6)
+      end
+    end
+
+    context "when there are users with comments_count" do
+      before do
+        create(:user, comments_count: 5)
+        create(:user, comments_count: 4)
+        create(:user, comments_count: 3)
+        create(:user, comments_count: 2)
+        # averages 3.5
+      end
+
+      it "works as expected" do
+        expect(described_class.average_comments_count).to be_within(0.1).of(3.5)
+        articles = described_class.above_average
+        # average is decimal but gets floored by the query builder
+        expect(articles.pluck(:comments_count)).to contain_exactly(5, 4, 3)
+      end
+    end
+  end
 end

spec/queries/users/suggest_prominent_spec.rb

@@ -1,6 +1,6 @@
 require "rails_helper"
 
-RSpec.describe Users::SuggestRecent, type: :service do
+RSpec.describe Users::SuggestProminent, type: :service do
   let(:user) { create(:user) }
   let(:suggester) { described_class.new(user) }
 

spec/requests/api/v1/docs/follows_spec.rb

@@ -0,0 +1,2 @@
+require "rails_helper"
+require "swagger_helper"

spec/requests/api/v1/follows_spec.rb

@@ -38,6 +38,23 @@ RSpec.describe "Api::V1::FollowsController" do
           end
         end.to change(Follow, :count).by(users_hash.size)
       end
+
+      it "can follow users or organizations" do
+        sign_in user
+        user_to_follow = create(:user)
+        org_to_follow = create(:organization)
+
+        expect do
+          sidekiq_perform_enqueued_jobs do
+            post "/api/follows",
+                 params: {
+                   users: [{ id: user_to_follow.id }],
+                   organizations: [{ id: org_to_follow.id }]
+                 },
+                 headers: headers
+          end
+        end.to change(Follow, :count).by(2)
+      end
     end
   end
 

spec/requests/onboardings_spec.rb

@@ -127,7 +127,7 @@ RSpec.describe "Onboardings" do
       before do
         allow(FeatureFlag).to receive(:enabled?).and_return(true)
         allow(Organizations::SuggestProminent).to receive(:call).and_return(suggested_orgs)
-        allow(Users::SuggestRecent).to receive(:call).and_return(suggested_users)
+        allow(Users::SuggestProminent).to receive(:call).and_return(suggested_users)
       end
 
       it "returns users first, then organizations" do