From f0e9c0cfef261597282fe421148166c939237fd8 Mon Sep 17 00:00:00 2001
From: Anna Buianova <lightallloy@gmail.com>
Date: Wed, 20 Mar 2019 19:51:09 +0300
Subject: [PATCH] Extract sending new follower notification to a separate job
 (#2030)

---
 Gemfile                                       |   1 +
 Gemfile.lock                                  |  27 ++++
 app/jobs/notifications/new_follower_job.rb    |   9 ++
 app/models/notification.rb                    |  42 +-----
 app/services/notifications.rb                 |  14 ++
 .../notifications/new_follower/follow_data.rb |  16 ++
 .../notifications/new_follower/send.rb        |  60 ++++++++
 package.json                                  |   2 +-
 .../notifications/new_follower_job_spec.rb    |  24 +++
 spec/models/notification_spec.rb              |   8 +-
 .../notifications/new_follower/send_spec.rb   | 140 ++++++++++++++++++
 11 files changed, 304 insertions(+), 39 deletions(-)
 create mode 100644 app/jobs/notifications/new_follower_job.rb
 create mode 100644 app/services/notifications.rb
 create mode 100644 app/services/notifications/new_follower/follow_data.rb
 create mode 100644 app/services/notifications/new_follower/send.rb
 create mode 100644 spec/jobs/notifications/new_follower_job_spec.rb
 create mode 100644 spec/services/notifications/new_follower/send_spec.rb

diff --git a/Gemfile b/Gemfile
index 383ae040e..c78f17144 100644
--- a/Gemfile
+++ b/Gemfile
@@ -38,6 +38,7 @@ gem "delayed_job_active_record", "~> 4.1"
 gem "delayed_job_web", "~> 1.4"
 gem "devise", "~> 4.6"
 gem "draper", "~> 3.0"
+gem "dry-struct", "~> 0.6"
 gem "email_validator", "~> 2.0"
 gem "emoji_regex", "~> 1.0"
 gem "envied", "~> 0.9"
diff --git a/Gemfile.lock b/Gemfile.lock
index a96c8dc7c..9b110e014 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -269,6 +269,32 @@ GEM
       activemodel-serializers-xml (~> 1.0)
       activesupport (~> 5.0)
       request_store (~> 1.0)
+    dry-configurable (0.8.2)
+      concurrent-ruby (~> 1.0)
+      dry-core (~> 0.4, >= 0.4.7)
+    dry-container (0.7.0)
+      concurrent-ruby (~> 1.0)
+      dry-configurable (~> 0.1, >= 0.1.3)
+    dry-core (0.4.7)
+      concurrent-ruby (~> 1.0)
+    dry-equalizer (0.2.2)
+    dry-inflector (0.1.2)
+    dry-logic (0.5.0)
+      dry-container (~> 0.2, >= 0.2.6)
+      dry-core (~> 0.2)
+      dry-equalizer (~> 0.2)
+    dry-struct (0.6.0)
+      dry-core (~> 0.4, >= 0.4.3)
+      dry-equalizer (~> 0.2)
+      dry-types (~> 0.13)
+      ice_nine (~> 0.11)
+    dry-types (0.14.0)
+      concurrent-ruby (~> 1.0)
+      dry-container (~> 0.3)
+      dry-core (~> 0.4, >= 0.4.4)
+      dry-equalizer (~> 0.2)
+      dry-inflector (~> 0.1, >= 0.1.2)
+      dry-logic (~> 0.5, >= 0.5)
     em-websocket (0.5.1)
       eventmachine (>= 0.12.9)
       http_parser.rb (~> 0.6.0)
@@ -981,6 +1007,7 @@ DEPENDENCIES
   derailed_benchmarks (~> 1.3)
   devise (~> 4.6)
   draper (~> 3.0)
+  dry-struct (~> 0.6)
   email_validator (~> 2.0)
   emoji_regex (~> 1.0)
   envied (~> 0.9)
diff --git a/app/jobs/notifications/new_follower_job.rb b/app/jobs/notifications/new_follower_job.rb
new file mode 100644
index 000000000..510b25aff
--- /dev/null
+++ b/app/jobs/notifications/new_follower_job.rb
@@ -0,0 +1,9 @@
+module Notifications
+  class NewFollowerJob < ApplicationJob
+    queue_as :send_new_follower_notification
+
+    def perform(follow_data, is_read = false, service = Notifications::NewFollower::Send)
+      service.call(follow_data, is_read)
+    end
+  end
+end
diff --git a/app/models/notification.rb b/app/models/notification.rb
index 2b421f886..aca243554 100644
--- a/app/models/notification.rb
+++ b/app/models/notification.rb
@@ -14,31 +14,14 @@ class Notification < ApplicationRecord
     def send_new_follower_notification(follow, is_read = false)
       return unless Follow.need_new_follower_notification_for?(follow.followable_type)
 
-      recent_follows = Follow.where(followable_type: follow.followable_type, followable_id: follow.followable_id).where("created_at > ?", 24.hours.ago).order("created_at DESC")
-
-      notification_params = { action: "Follow" }
-      if follow.followable_type == "User"
-        notification_params[:user_id] = follow.followable_id
-      elsif follow.followable_type == "Organization"
-        notification_params[:organization_id] = follow.followable_id
-      end
-
-      followers = User.where(id: recent_follows.pluck(:follower_id))
-      aggregated_siblings = followers.map { |follower| user_data(follower) }
-      if aggregated_siblings.size.zero?
-        Notification.find_or_create_by(notification_params).destroy
-      else
-        json_data = { user: user_data(follow.follower), aggregated_siblings: aggregated_siblings }
-        notification = Notification.find_or_create_by(notification_params)
-        notification.notifiable_id = recent_follows.first.id
-        notification.notifiable_type = "Follow"
-        notification.json_data = json_data
-        notification.notified_at = Time.current
-        notification.read = is_read
-        notification.save!
-      end
+      follow_data = follow.attributes.slice("follower_id", "followable_id", "followable_type").symbolize_keys
+      Notifications::NewFollowerJob.perform_later(follow_data, is_read)
+    end
+
+    def send_new_follower_notification_without_delay(follow, is_read = false)
+      follow_data = follow.attributes.slice("follower_id", "followable_id", "followable_type").symbolize_keys
+      Notifications::NewFollowerJob.perform_now(follow_data, is_read)
     end
-    handle_asynchronously :send_new_follower_notification
 
     def send_to_followers(notifiable, action = nil)
       # for now, arguments are always: notifiable = article, action = "Published"
@@ -322,16 +305,7 @@ class Notification < ApplicationRecord
     private
 
     def user_data(user)
-      {
-        id: user.id,
-        class: { name: "User" },
-        name: user.name,
-        username: user.username,
-        path: user.path,
-        profile_image_90: user.profile_image_90,
-        comments_count: user.comments_count,
-        created_at: user.created_at
-      }
+      Notifications.user_data(user)
     end
 
     def organization_data(organization)
diff --git a/app/services/notifications.rb b/app/services/notifications.rb
new file mode 100644
index 000000000..052fb0902
--- /dev/null
+++ b/app/services/notifications.rb
@@ -0,0 +1,14 @@
+module Notifications
+  def self.user_data(user)
+    {
+      id: user.id,
+      class: { name: "User" },
+      name: user.name,
+      username: user.username,
+      path: user.path,
+      profile_image_90: user.profile_image_90,
+      comments_count: user.comments_count,
+      created_at: user.created_at
+    }
+  end
+end
diff --git a/app/services/notifications/new_follower/follow_data.rb b/app/services/notifications/new_follower/follow_data.rb
new file mode 100644
index 000000000..a21f2c00f
--- /dev/null
+++ b/app/services/notifications/new_follower/follow_data.rb
@@ -0,0 +1,16 @@
+require "dry-types"
+require "dry-struct"
+
+module Notifications
+  module NewFollower
+    module Types
+      include Dry::Types.module
+    end
+
+    class FollowData < Dry::Struct
+      attribute :followable_id, Types::Strict::Integer
+      attribute :followable_type, Types::Strict::String.enum("User", "Organization")
+      attribute :follower_id, Types::Strict::Integer
+    end
+  end
+end
diff --git a/app/services/notifications/new_follower/send.rb b/app/services/notifications/new_follower/send.rb
new file mode 100644
index 000000000..f4683cacc
--- /dev/null
+++ b/app/services/notifications/new_follower/send.rb
@@ -0,0 +1,60 @@
+# send notifications about the new followers
+module Notifications
+  module NewFollower
+    class Send
+      # @param follow_data [Hash]
+      #   * :followable_id [Integer]
+      #   * :followable_type [String] - "User" or "Organization"
+      #   * :follower_id [Integer] - user id
+      def initialize(follow_data, is_read = false)
+        follow_data = follow_data.is_a?(FollowData) ? follow_data : FollowData.new(follow_data)
+        @followable_id = follow_data.followable_id # fetch(:followable_id)
+        @followable_type = follow_data.followable_type # fetch(:followable_type)
+        @follower_id = follow_data.follower_id # fetch(:follower_id)
+        @is_read = is_read
+      end
+
+      delegate :user_data, to: Notifications
+
+      def self.call(*args)
+        new(*args).call
+      end
+
+      def call
+        recent_follows = Follow.where(followable_type: followable_type, followable_id: followable_id).
+          where("created_at > ?", 24.hours.ago).order("created_at DESC")
+
+        notification_params = { action: "Follow" }
+        if followable_type == "User"
+          notification_params[:user_id] = followable_id
+        elsif followable_type == "Organization"
+          notification_params[:organization_id] = followable_id
+        end
+
+        followers = User.where(id: recent_follows.pluck(:follower_id))
+        aggregated_siblings = followers.map { |f| user_data(f) }
+        if aggregated_siblings.size.zero?
+          notification = Notification.find_by(notification_params)&.destroy
+        else
+          json_data = { user: user_data(follower), aggregated_siblings: aggregated_siblings }
+          notification = Notification.find_or_create_by(notification_params)
+          notification.notifiable_id = recent_follows.first.id
+          notification.notifiable_type = "Follow"
+          notification.json_data = json_data
+          notification.notified_at = Time.current
+          notification.read = is_read
+          notification.save!
+        end
+        notification
+      end
+
+      private
+
+      attr_reader :followable_id, :followable_type, :follower_id, :is_read
+
+      def follower
+        User.find(follower_id)
+      end
+    end
+  end
+end
diff --git a/package.json b/package.json
index d759e54a5..765936ee4 100644
--- a/package.json
+++ b/package.json
@@ -26,7 +26,7 @@
       "git add"
     ],
     "{app,spec}/**/*.rb": [
-      "bin/rubocop --require rubocop-rspec --safe-auto-correct",
+      "bundle exec rubocop --require rubocop-rspec --safe-auto-correct",
       "git add"
     ],
     "app/**/*.html.erb": [
diff --git a/spec/jobs/notifications/new_follower_job_spec.rb b/spec/jobs/notifications/new_follower_job_spec.rb
new file mode 100644
index 000000000..09dac140b
--- /dev/null
+++ b/spec/jobs/notifications/new_follower_job_spec.rb
@@ -0,0 +1,24 @@
+require "rails_helper"
+
+RSpec.describe Notifications::NewFollowerJob, type: :job do
+  let(:follow_data) { { followable_type: "User", followable_id: 1, follower_id: 2 } }
+
+  describe "#perform_later" do
+    it "enqueues the job" do
+      ActiveJob::Base.queue_adapter = :test
+      expect do
+        described_class.perform_later(follow_data, true)
+      end.to have_enqueued_job.with(follow_data, true).on_queue("send_new_follower_notification")
+    end
+  end
+
+  describe "#perform_now" do
+    it "calls the service" do
+      new_follower_service = double
+      allow(new_follower_service).to receive(:call)
+
+      described_class.perform_now(follow_data, false, new_follower_service)
+      expect(new_follower_service).to have_received(:call).with(follow_data, false).once
+    end
+  end
+end
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 30f679dae..b68698f8e 100644
--- a/spec/models/notification_spec.rb
+++ b/spec/models/notification_spec.rb
@@ -29,9 +29,8 @@ RSpec.describe Notification, type: :model do
 
   describe "#send_new_follower_notification" do
     before do
-      run_background_jobs_immediately do
-        Notification.send_new_follower_notification(follow_instance)
-      end
+      ActiveJob::Base.queue_adapter = :inline
+      Notification.send_new_follower_notification(follow_instance)
     end
 
     it "sets the notifiable_at column upon creation" do
@@ -71,7 +70,8 @@ RSpec.describe Notification, type: :model do
     context "when a user unfollows another user" do
       it "destroys the follow notification" do
         follow_instance = user.stop_following(user2)
-        run_background_jobs_immediately { Notification.send_new_follower_notification(follow_instance) }
+        ActiveJob::Base.queue_adapter = :inline
+        Notification.send_new_follower_notification(follow_instance)
         expect(Notification.count).to eq 0
       end
     end
diff --git a/spec/services/notifications/new_follower/send_spec.rb b/spec/services/notifications/new_follower/send_spec.rb
new file mode 100644
index 000000000..1bb902cf8
--- /dev/null
+++ b/spec/services/notifications/new_follower/send_spec.rb
@@ -0,0 +1,140 @@
+require "rails_helper"
+
+RSpec.describe Notifications::NewFollower::Send, type: :service do
+  let(:user)            { create(:user) }
+  let(:user2)           { create(:user) }
+  let(:user3)           { create(:user) }
+  let(:organization)    { create(:organization) }
+  let(:article)         { create(:article, user_id: user.id, page_views_count: 4000, positive_reactions_count: 70) }
+  let!(:follow)         { user.follow(user2) }
+
+  def follow_data(follow)
+    {
+      followable_id: follow.followable_id,
+      followable_type: follow.followable_type,
+      follower_id: follow.follower_id
+    }
+  end
+
+  context "when trying to pass tag follow data" do
+    it "raises an exception" do
+      tag = create(:tag)
+      tag_follow = user.follow(tag)
+      expect do
+        described_class.call(follow_data(tag_follow))
+      end.to raise_error(Dry::Struct::Error)
+    end
+  end
+
+  context "when user follows another user" do
+    it "creates a notification" do
+      expect do
+        described_class.call(follow_data(follow))
+      end.to change(Notification, :count).by(1)
+    end
+
+    it "creates a notification with data" do
+      notification = described_class.call(follow_data(follow))
+      expect(notification.notifiable).to eq(follow)
+      expect(notification.notified_at).not_to be_nil
+      expect(notification.read).to be_falsey
+      expect(notification.json_data["user"]["id"]).to eq(user.id)
+    end
+
+    it "creates a read notification" do
+      notification = described_class.call(follow_data(follow), true)
+      expect(notification.read).to be_truthy
+    end
+
+    context "when destroyed follow" do
+      let(:unfollow) { user.stop_following(user2) }
+
+      it "does not create a notification" do
+        expect do
+          described_class.call(follow_data(unfollow))
+        end.not_to change(Notification, :count)
+      end
+
+      it "destroys notification if it exists" do
+        create(:notification, action: "Follow", user: user2, notifiable: follow, notified_at: Time.now - 1.year)
+        expect do
+          described_class.call(follow_data(unfollow))
+        end.to change(Notification, :count).by(-1)
+      end
+
+      it "destroys the correct notification" do
+        notification = create(:notification, action: "Follow", user: user2, notifiable: follow, notified_at: Time.now - 1.year)
+        described_class.call(follow_data(unfollow))
+        expect(Notification.where(id: notification.id)).not_to exist
+      end
+    end
+  end
+
+  context "when 2 user follows another user" do
+    let!(:follow2) { user3.follow user2 }
+    let(:follower_data) do
+      {
+        "id" => user3.id,
+        "class" => { "name" => "User" },
+        "name" => user3.name,
+        "username" => user3.username,
+        "path" => user3.path,
+        "profile_image_90" => user3.profile_image_90,
+        "comments_count" => user3.comments_count
+      }
+    end
+
+    it "creates a notification" do
+      expect do
+        described_class.call(follow_data(follow2))
+      end.to change(Notification, :count).by(1)
+    end
+
+    it "creates a notification with data" do
+      notification = described_class.call(follow_data(follow2))
+      expect(notification.notifiable).to eq(follow2)
+      expect(notification.notified_at).not_to be_nil
+      expect(notification.json_data["aggregated_siblings"].map { |j| j["id"] }.sort).to eq([user.id, user3.id].sort)
+    end
+
+    it "creates a notification with user data" do
+      notification = described_class.call(follow_data(follow2))
+      expect(notification.json_data["user"]["name"]).to eq(user3.name)
+      expect(notification.json_data["user"]["username"]).to eq(user3.username)
+      expect(notification.json_data["user"]["id"]).to eq(user3.id)
+      expect(notification.json_data["user"]["class"]).to eq("name" => "User")
+    end
+
+    context "when notification exists" do
+      before do
+        create(:notification, user: user2, action: "Follow", notifiable: follow, notified_at: Time.now - 1.year)
+      end
+
+      it "does not create a notification" do
+        expect do
+          described_class.call(follow_data(follow2))
+        end.not_to change(Notification, :count)
+      end
+
+      it "updates a notification" do
+        notification = described_class.call(follow_data(follow2))
+        expect(notification.notifiable).to eq(follow2)
+        expect(notification.notified_at).to be >= Time.now - 1.day
+      end
+    end
+
+    context "when destroyed follow and notification exists" do
+      let(:unfollow) { user.stop_following(user2) }
+
+      before do
+        create(:notification, action: "Follow", user: user2, notifiable: follow, notified_at: Time.now - 1.year)
+      end
+
+      it "does not destroy a notification" do
+        expect do
+          described_class.call(follow_data(unfollow))
+        end.not_to change(Notification, :count)
+      end
+    end
+  end
+end