send_milestone_notification in own job (#2804)

- that calls a service
  - spec for job + move spec to service
  - minor refactoring

app/jobs/notifications/milestone_job.rb

@@ -0,0 +1,12 @@
+module Notifications
+  class MilestoneJob < ApplicationJob
+    queue_as :send_milestone_notification
+
+    def perform(milestone_hash, service = Notifications::Milestone::Send)
+      article = Article.find_by(id: milestone_hash[:article_id])
+      return unless article
+
+      service.call(milestone_hash[:type], article)
+    end
+  end
+end

app/labor/article_analytics_fetcher.rb

@@ -19,11 +19,11 @@ class ArticleAnalyticsFetcher
       page_views_obj = pageviews.to_h
       chunk.each do |article|
         article.update_columns(previous_positive_reactions_count: article.positive_reactions_count)
-        Notification.send_milestone_notification(type: "Reaction", article: article)
+        Notification.send_milestone_notification(type: "Reaction", article_id: article.id)
         next if article.page_views_count > page_views_obj[article.id].to_i
 
         article.update_columns(page_views_count: page_views_obj[article.id].to_i)
-        Notification.send_milestone_notification(type: "View", article: article)
+        Notification.send_milestone_notification(type: "View", article_id: article.id)
       end
     end
   end

app/models/notification.rb

@@ -29,7 +29,7 @@ class Notification < ApplicationRecord
       # for now, arguments are always: notifiable = article, action = "Published"
       json_data = {
         user: user_data(notifiable.user),
-        article: article_data(notifiable)
+        article: Notifications.article_data(notifiable)
       }
       followers = if notifiable.organization_id
                     json_data[:organization] = organization_data(notifiable.organization)
@@ -130,29 +130,12 @@ class Notification < ApplicationRecord
     end
 
     def send_milestone_notification(milestone_hash)
-      milestone_hash[:next_milestone] = next_milestone(milestone_hash)
-      return unless should_send_milestone?(milestone_hash)
-
-      json_data = { article: article_data(milestone_hash[:article]), gif_id: RandomGif.new.random_id }
-
-      Notification.create!(
-        user_id: milestone_hash[:article].user_id,
-        notifiable_id: milestone_hash[:article].id,
-        notifiable_type: "Article",
-        json_data: json_data,
-        action: "Milestone::#{milestone_hash[:type]}::#{milestone_hash[:next_milestone]}",
-      )
-      return unless milestone_hash[:article].organization_id
-
-      Notification.create!(
-        organization_id: milestone_hash[:article].organization_id,
-        notifiable_id: milestone_hash[:article].id,
-        notifiable_type: "Article",
-        json_data: json_data,
-        action: "Milestone::#{milestone_hash[:type]}::#{milestone_hash[:next_milestone]}",
-      )
+      Notifications::MilestoneJob.perform_later(milestone_hash)
+    end
+
+    def send_milestone_notification_without_delay(milestone_hash)
+      Notifications::MilestoneJob.perform_now(milestone_hash)
     end
-    handle_asynchronously :send_milestone_notification
 
     def remove_all(notifiable_hash)
       Notification.where(
@@ -193,6 +176,10 @@ class Notification < ApplicationRecord
       Notifications.comment_data(comment)
     end
 
+    def article_data(article)
+      Notifications.article_data(article)
+    end
+
     def reaction_notification_attributes(reaction, receiver)
       reactable_data = {
         reactable_id: reaction.reactable_id,
@@ -206,45 +193,6 @@ class Notification < ApplicationRecord
     def organization_data(organization)
       Notifications.organization_data(organization)
     end
-
-    def article_data(article)
-      Notifications.article_data(article)
-    end
-
-    def should_send_milestone?(milestone_hash)
-      return if milestone_hash[:article].published_at < Time.zone.local(2019, 2, 25)
-
-      last_milestone_notification = Notification.find_by(
-        user_id: milestone_hash[:article].user_id,
-        notifiable_type: "Article",
-        notifiable_id: milestone_hash[:article].id,
-        action: "Milestone::#{milestone_hash[:type]}::#{milestone_hash[:next_milestone]}",
-      )
-
-      if milestone_hash[:type] == "View"
-        last_milestone_notification.blank? && milestone_hash[:article].page_views_count > milestone_hash[:next_milestone]
-      elsif milestone_hash[:type] == "Reaction"
-        last_milestone_notification.blank? && milestone_hash[:article].positive_reactions_count > milestone_hash[:next_milestone]
-      end
-    end
-
-    def next_milestone(milestone_hash)
-      case milestone_hash[:type]
-      when "View"
-        milestones = [1024, 2048, 4096, 8192, 16_384, 32_768, 65_536, 131_072, 262_144, 524_288, 1_048_576]
-        milestone_count = milestone_hash[:article].page_views_count
-      when "Reaction"
-        milestones = [64, 128, 256, 512, 1024, 2048, 4096, 8192]
-        milestone_count = milestone_hash[:article].positive_reactions_count
-      end
-
-      closest_number = milestones.min_by { |num| (milestone_count - num).abs }
-      if milestone_count > closest_number
-        closest_number
-      else
-        milestones[milestones.index(closest_number) - 1]
-      end
-    end
   end
 
   # instance methods

app/services/notifications/milestone/send.rb

@@ -0,0 +1,84 @@
+module Notifications
+  module Milestone
+    ARTICLE_FINAL_PUBLICATION_TIME_FOR_MILESTONE = Time.zone.local(2019, 2, 25)
+
+    class Send
+      # @param type [String] - "View" or "Reaction"
+      # @param article [Object] - ActiveRecord Article object
+      def initialize(type, article)
+        @type = type
+        @article = article
+        @next_milestone = next_milestone
+      end
+
+      def self.call(*args)
+        new(*args).call
+      end
+
+      def call
+        return unless should_send_milestone?
+
+        create_notification(user_id: article.user_id)
+        return unless article.organization_id
+
+        create_notification(organization_id: article.organization_id)
+      end
+
+      private
+
+      attr_reader :type, :article
+
+      def create_notification(hash_id)
+        Notification.create!({
+          notifiable_id: article.id,
+          notifiable_type: "Article",
+          json_data: json_data,
+          action: "Milestone::#{type}::#{@next_milestone}"
+        }.merge(hash_id))
+      end
+
+      def json_data
+        { article: Notifications.article_data(article), gif_id: RandomGif.new.random_id }
+      end
+
+      def article_published_behind_time?
+        article.published_at < ARTICLE_FINAL_PUBLICATION_TIME_FOR_MILESTONE
+      end
+
+      def should_send_milestone?
+        return if article_published_behind_time?
+
+        last_milestone_notification = Notification.find_by(
+          user_id: article.user_id,
+          notifiable_type: "Article",
+          notifiable_id: article.id,
+          action: "Milestone::#{type}::#{@next_milestone}",
+        )
+
+        if type == "View"
+          last_milestone_notification.blank? && article.page_views_count > @next_milestone
+        elsif type == "Reaction"
+          last_milestone_notification.blank? && article.positive_reactions_count > @next_milestone
+        end
+      end
+
+      def next_milestone
+        case type
+        when "View"
+          milestones = [1024, 2048, 4096, 8192, 16_384, 32_768, 65_536, 131_072, 262_144, 524_288, 1_048_576]
+          milestone_count = article.page_views_count
+        when "Reaction"
+          milestones = [64, 128, 256, 512, 1024, 2048, 4096, 8192]
+          milestone_count = article.positive_reactions_count
+        end
+
+        closest_number = milestones.min_by { |num| (milestone_count - num).abs }
+        if milestone_count > closest_number
+          closest_number
+        else
+          milestones[milestones.index(closest_number) - 1]
+        end
+      end
+    end
+  end
+end

spec/jobs/notifications/milestone_job_spec.rb

@@ -0,0 +1,39 @@
+require "rails_helper"
+
+RSpec.describe Notifications::MilestoneJob do
+  include_examples "#enqueues_job", "send_milestone_notification", {}
+
+  describe "#perform_now" do
+    let(:milestone_hash) do
+      { type: "Reaction", article_id: 456 }
+    end
+    let(:milestone_service) { double }
+    let(:article) { double }
+
+    before do
+      allow(milestone_service).to receive(:call)
+    end
+
+    specify "fetch article id only if :article_id key in hash " do
+      allow(Article).to receive(:find_by)
+      described_class.perform_now(milestone_hash, milestone_service)
+      expect(Article).to have_received(:find_by).with(id: 456)
+    end
+
+    describe "When it does not find article from id" do
+      it "does not call the service" do
+        allow(Article).to receive(:find_by)
+        described_class.perform_now(milestone_hash, milestone_service)
+        expect(milestone_service).not_to have_received(:call)
+      end
+    end
+
+    describe "When finds article from id" do
+      it "calls the service" do
+        allow(Article).to receive(:find_by).and_return(article)
+        described_class.perform_now(milestone_hash, milestone_service)
+        expect(milestone_service).to have_received(:call).with("Reaction", article)
+      end
+    end
+  end
+end

spec/labor/article_analytics_fetcher_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe ArticleAnalyticsFetcher do
 
       it "sends send_milestone_notification for Reaction and View" do
         %w[Reaction View].each do |type|
-          expect(Notification).to have_received(:send_milestone_notification).with(type: type, article: article)
+          expect(Notification).to have_received(:send_milestone_notification).with(type: type, article_id: article.id)
         end
       end
 

spec/models/notification_spec.rb

@@ -301,66 +301,6 @@ RSpec.describe Notification, type: :model do
     end
   end
 
-  describe "#send_milestone_notification" do
-    # milestones = [64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768, 65536, 131072, 262144]
-    let(:view_milestone_hash) { { type: "View", article: article } }
-    let(:reaction_milestone_hash) { { type: "Reaction", article: article } }
-
-    context "when a user has never received a milestone notification" do
-      it "sends the appropriate level view milestone notification" do
-        Notification.send_milestone_notification_without_delay(view_milestone_hash)
-        expect(user.notifications.first.action).to include "2048"
-      end
-
-      it "sends the appropriate level reaction milestone notification" do
-        Notification.send_milestone_notification_without_delay(reaction_milestone_hash)
-        expect(user.notifications.first.action).to include "64"
-      end
-    end
-
-    context "when a user has received a milestone notification before" do
-      def mock_previous_view_milestone_notification
-        Notification.send_milestone_notification_without_delay(view_milestone_hash)
-        article.update_column(:page_views_count, 9001)
-        Notification.send_milestone_notification_without_delay(view_milestone_hash)
-      end
-
-      def mock_previous_reaction_milestone_notification
-        Notification.send_milestone_notification_without_delay(reaction_milestone_hash)
-        article.update_column(:positive_reactions_count, 150)
-        Notification.send_milestone_notification_without_delay(reaction_milestone_hash)
-      end
-
-      it "sends the appropriate level view milestone notification" do
-        mock_previous_view_milestone_notification
-        expect(user.notifications.second.action).to include "8192"
-      end
-
-      it "sends the appropriate level reaction milestone notification" do
-        mock_previous_reaction_milestone_notification
-        expect(user.notifications.last.action).to include "128"
-      end
-
-      it "adds an additional view milestone notification" do
-        mock_previous_view_milestone_notification
-        expect(user.notifications.count).to eq 2
-      end
-
-      it "does not the same view milestone notification if called again" do
-        mock_previous_view_milestone_notification
-        Notification.send_milestone_notification_without_delay(view_milestone_hash)
-        expect(user.notifications.count).to eq 2
-      end
-
-      it "does not send a view milestone notification again if the latest number of views is not past the next milestone" do
-        mock_previous_view_milestone_notification
-        article.update_column(:page_views_count, rand(9002..16_383))
-        Notification.send_milestone_notification_without_delay(view_milestone_hash)
-        expect(user.notifications.count).to eq 2
-      end
-    end
-  end
-
   describe "#update_notifications" do
     context "when there are notifications to update" do
       before do

spec/services/notifications/milestone/send_spec.rb

@@ -0,0 +1,87 @@
+require "rails_helper"
+
+RSpec.describe Notifications::Milestone::Send do
+  let(:user) { create(:user) }
+  let(:article) { create(:article, user_id: user.id, page_views_count: 4000, positive_reactions_count: 70) }
+
+  context "when a user has never received a milestone notification" do
+    it "sends the appropriate level view milestone notification" do
+      send_milestone_notification_view
+      expect(user.notifications.first.action).to include "2048"
+    end
+
+    it "sends the appropriate level reaction milestone notification" do
+      send_milestone_notification_reaction
+      expect(user.notifications.first.action).to include "64"
+    end
+  end
+
+  context "when a user has received a milestone notification before" do
+    def mock_previous_view_milestone_notification
+      send_milestone_notification_view
+      article.update_column(:page_views_count, 9001)
+      send_milestone_notification_view
+    end
+
+    def mock_previous_reaction_milestone_notification
+      send_milestone_notification_reaction
+      article.update_column(:positive_reactions_count, 150)
+      send_milestone_notification_reaction
+    end
+
+    describe "When send view type milestone notification" do
+      before do
+        mock_previous_view_milestone_notification
+      end
+
+      it "sends the appropriate level view milestone notification" do
+        expect(user.notifications.second.action).to include "8192"
+      end
+
+      it "adds an additional view milestone notification" do
+        expect(user.notifications.count).to eq 2
+      end
+
+      it "does not the same view milestone notification if called again" do
+        send_milestone_notification_view
+        expect(user.notifications.count).to eq 2
+      end
+
+      it "does not send a view milestone notification again if the latest number of views is not past the next milestone" do
+        article.update_column(:page_views_count, rand(9002..16_383))
+        send_milestone_notification_view
+        expect(user.notifications.count).to eq 2
+      end
+
+      it "checks notification json data", :aggregate_failures do
+        expect(user.notifications.where(notifiable_type: "Article").first.json_data["article"]["class"]["name"]).to eq "Article"
+        expect(user.notifications.where(notifiable_id: article.id).first.json_data["article"]["id"]).to eq article.id
+        expect(user.notifications.where(notifiable_id: article.id).first.json_data["article"]["title"]).to eq article.title
+      end
+    end
+
+    it "sends the appropriate level reaction milestone notification" do
+      mock_previous_reaction_milestone_notification
+      expect(user.notifications.last.action).to include "128"
+    end
+
+    context "when an article is related to an organization" do
+      let(:organization) { create(:organization) }
+
+      before { article.organization = organization }
+
+      it "creates another notification related to organization" do
+        coll = Notification.where(organization_id: article.organization_id)
+        expect { send_milestone_notification_reaction }.to change { coll.count }.by(1)
+      end
+    end
+  end
+end
+
+def send_milestone_notification_view
+  described_class.call("View", article)
+end
+
+def send_milestone_notification_reaction
+  described_class.call("Reaction", article)
+end