From 4260441f8c95b141095e61fbe23e12852cdd8347 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Mon, 28 Feb 2022 18:16:12 +0530 Subject: [PATCH] Chore: clean up Reporting Events (#4044) Tech debt clean up Fixes #4057 Co-authored-by: Aswin Dev P S --- app/builders/v2/report_builder.rb | 8 +-- app/dispatchers/async_dispatcher.rb | 2 +- app/helpers/api/v1/reports_helper.rb | 2 - ...istener.rb => reporting_event_listener.rb} | 10 +-- app/listeners/reporting_listener.rb | 45 ------------ app/models/concerns/reportable.rb | 2 +- app/models/label.rb | 4 +- app/models/{event.rb => reporting_event.rb} | 14 ++-- app/models/team.rb | 4 +- ...13143021_rename_events_to_report_events.rb | 5 ++ db/schema.rb | 32 ++++----- lib/reports/update_account_identity.rb | 10 --- lib/reports/update_agent_identity.rb | 11 --- lib/reports/update_identity.rb | 69 ------------------- spec/builders/v2/report_builder_spec.rb | 2 +- .../{events.rb => reporting_events.rb} | 2 +- ...ec.rb => reporting_event_listener_spec.rb} | 10 +-- spec/models/account_spec.rb | 2 +- spec/models/inbox_spec.rb | 2 +- ...{event_spec.rb => reporting_event_spec.rb} | 2 +- spec/models/user_spec.rb | 2 +- 21 files changed, 54 insertions(+), 186 deletions(-) delete mode 100644 app/helpers/api/v1/reports_helper.rb rename app/listeners/{event_listener.rb => reporting_event_listener.rb} (83%) delete mode 100644 app/listeners/reporting_listener.rb rename app/models/{event.rb => reporting_event.rb} (64%) create mode 100644 db/migrate/20210513143021_rename_events_to_report_events.rb delete mode 100644 lib/reports/update_account_identity.rb delete mode 100644 lib/reports/update_agent_identity.rb delete mode 100644 lib/reports/update_identity.rb rename spec/factories/{events.rb => reporting_events.rb} (83%) rename spec/listeners/{event_listener_spec.rb => reporting_event_listener_spec.rb} (69%) rename spec/models/{event_spec.rb => reporting_event_spec.rb} (91%) diff --git a/app/builders/v2/report_builder.rb b/app/builders/v2/report_builder.rb index 7f0dd470c..1f17dbb08 100644 --- a/app/builders/v2/report_builder.rb +++ b/app/builders/v2/report_builder.rb @@ -95,15 +95,15 @@ class V2::ReportBuilder end def avg_first_response_time - (get_grouped_values scope.events.where(name: 'first_response')).average(:value) + (get_grouped_values scope.reporting_events.where(name: 'first_response')).average(:value) end def avg_resolution_time - (get_grouped_values scope.events.where(name: 'conversation_resolved')).average(:value) + (get_grouped_values scope.reporting_events.where(name: 'conversation_resolved')).average(:value) end def avg_resolution_time_summary - avg_rt = scope.events + avg_rt = scope.reporting_events .where(name: 'conversation_resolved', created_at: range) .average(:value) @@ -113,7 +113,7 @@ class V2::ReportBuilder end def avg_first_response_time_summary - avg_frt = scope.events + avg_frt = scope.reporting_events .where(name: 'first_response', created_at: range) .average(:value) diff --git a/app/dispatchers/async_dispatcher.rb b/app/dispatchers/async_dispatcher.rb index 3f95405f9..f2f238cd4 100644 --- a/app/dispatchers/async_dispatcher.rb +++ b/app/dispatchers/async_dispatcher.rb @@ -12,10 +12,10 @@ class AsyncDispatcher < BaseDispatcher [ CampaignListener.instance, CsatSurveyListener.instance, - EventListener.instance, HookListener.instance, InstallationWebhookListener.instance, NotificationListener.instance, + ReportingEventListener.instance, WebhookListener.instance, AutomationRuleListener.instance ] diff --git a/app/helpers/api/v1/reports_helper.rb b/app/helpers/api/v1/reports_helper.rb deleted file mode 100644 index 308bab3e6..000000000 --- a/app/helpers/api/v1/reports_helper.rb +++ /dev/null @@ -1,2 +0,0 @@ -module Api::V1::ReportsHelper -end diff --git a/app/listeners/event_listener.rb b/app/listeners/reporting_event_listener.rb similarity index 83% rename from app/listeners/event_listener.rb rename to app/listeners/reporting_event_listener.rb index 8e7fd0daf..05aa1db4b 100644 --- a/app/listeners/event_listener.rb +++ b/app/listeners/reporting_event_listener.rb @@ -1,9 +1,9 @@ -class EventListener < BaseListener +class ReportingEventListener < BaseListener def conversation_resolved(event) conversation = extract_conversation_and_account(event)[0] time_to_resolve = conversation.updated_at.to_i - conversation.created_at.to_i - event = Event.new( + reporting_event = ReportingEvent.new( name: 'conversation_resolved', value: time_to_resolve, account_id: conversation.account_id, @@ -11,7 +11,7 @@ class EventListener < BaseListener user_id: conversation.assignee_id, conversation_id: conversation.id ) - event.save + reporting_event.save end def first_reply_created(event) @@ -19,7 +19,7 @@ class EventListener < BaseListener conversation = message.conversation first_response_time = message.created_at.to_i - conversation.created_at.to_i - event = Event.new( + reporting_event = ReportingEvent.new( name: 'first_response', value: first_response_time, account_id: conversation.account_id, @@ -27,6 +27,6 @@ class EventListener < BaseListener user_id: conversation.assignee_id, conversation_id: conversation.id ) - event.save + reporting_event.save end end diff --git a/app/listeners/reporting_listener.rb b/app/listeners/reporting_listener.rb deleted file mode 100644 index 407dc47bc..000000000 --- a/app/listeners/reporting_listener.rb +++ /dev/null @@ -1,45 +0,0 @@ -class ReportingListener < BaseListener - def conversation_created(event) - conversation, account = extract_conversation_and_account(event) - timestamp = event.timestamp - - ::Reports::UpdateAccountIdentity.new(account, timestamp).incr_conversations_count - end - - def conversation_resolved(event) - conversation, account = extract_conversation_and_account(event) - timestamp = event.timestamp - - time_to_resolve = conversation.updated_at.to_i - conversation.created_at.to_i - - if conversation.assignee.present? - agent = conversation.assignee - ::Reports::UpdateAgentIdentity.new(account, agent, timestamp).update_avg_resolution_time(time_to_resolve) - ::Reports::UpdateAgentIdentity.new(account, agent, timestamp).incr_resolutions_count - end - - ::Reports::UpdateAccountIdentity.new(account, timestamp).update_avg_resolution_time(time_to_resolve) - ::Reports::UpdateAccountIdentity.new(account, timestamp).incr_resolutions_count - end - - def first_reply_created(event) - message, account = extract_message_and_account(event) - timestamp = event.timestamp - - conversation = message.conversation - agent = conversation.assignee - first_response_time = message.created_at.to_i - conversation.created_at.to_i - ::Reports::UpdateAgentIdentity.new(account, agent, timestamp).update_avg_first_response_time(first_response_time) if agent.present? - ::Reports::UpdateAccountIdentity.new(account, timestamp).update_avg_first_response_time(first_response_time) - end - - def message_created(event) - message, account = extract_message_and_account(event) - timestamp = event.timestamp - - return unless message.reportable? - - identity = ::Reports::UpdateAccountIdentity.new(account, timestamp) - message.outgoing? ? identity.incr_outgoing_messages_count : identity.incr_incoming_messages_count - end -end diff --git a/app/models/concerns/reportable.rb b/app/models/concerns/reportable.rb index 9da1e93dc..23ff69fd2 100644 --- a/app/models/concerns/reportable.rb +++ b/app/models/concerns/reportable.rb @@ -4,6 +4,6 @@ module Reportable extend ActiveSupport::Concern included do - has_many :events, dependent: :destroy_async + has_many :reporting_events, dependent: :destroy end end diff --git a/app/models/label.rb b/app/models/label.rb index 9b551141d..650ea6f8e 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -39,8 +39,8 @@ class Label < ApplicationRecord account.messages.where(conversation_id: conversations.pluck(:id)) end - def events - account.events.where(conversation_id: conversations.pluck(:id)) + def reporting_events + account.reporting_events.where(conversation_id: conversations.pluck(:id)) end private diff --git a/app/models/event.rb b/app/models/reporting_event.rb similarity index 64% rename from app/models/event.rb rename to app/models/reporting_event.rb index 7cb49c576..288b15504 100644 --- a/app/models/event.rb +++ b/app/models/reporting_event.rb @@ -1,6 +1,6 @@ # == Schema Information # -# Table name: events +# Table name: reporting_events # # id :bigint not null, primary key # name :string @@ -14,14 +14,14 @@ # # Indexes # -# index_events_on_account_id (account_id) -# index_events_on_created_at (created_at) -# index_events_on_inbox_id (inbox_id) -# index_events_on_name (name) -# index_events_on_user_id (user_id) +# index_reporting_events_on_account_id (account_id) +# index_reporting_events_on_created_at (created_at) +# index_reporting_events_on_inbox_id (inbox_id) +# index_reporting_events_on_name (name) +# index_reporting_events_on_user_id (user_id) # -class Event < ApplicationRecord +class ReportingEvent < ApplicationRecord validates :account_id, presence: true validates :name, presence: true validates :value, presence: true diff --git a/app/models/team.rb b/app/models/team.rb index 74ce2aedf..22e9b3fc6 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -45,7 +45,7 @@ class Team < ApplicationRecord account.messages.where(conversation_id: conversations.pluck(:id)) end - def events - account.events.where(conversation_id: conversations.pluck(:id)) + def reporting_events + account.reporting_events.where(conversation_id: conversations.pluck(:id)) end end diff --git a/db/migrate/20210513143021_rename_events_to_report_events.rb b/db/migrate/20210513143021_rename_events_to_report_events.rb new file mode 100644 index 000000000..9241ff7b2 --- /dev/null +++ b/db/migrate/20210513143021_rename_events_to_report_events.rb @@ -0,0 +1,5 @@ +class RenameEventsToReportEvents < ActiveRecord::Migration[6.0] + def change + rename_table :events, :reporting_events + end +end diff --git a/db/schema.rb b/db/schema.rb index 8c37b7897..2f3334b74 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -434,22 +434,6 @@ ActiveRecord::Schema.define(version: 2022_02_18_120357) do t.index ["name", "account_id"], name: "index_email_templates_on_name_and_account_id", unique: true end - create_table "events", force: :cascade do |t| - t.string "name" - t.float "value" - t.integer "account_id" - t.integer "inbox_id" - t.integer "user_id" - t.integer "conversation_id" - t.datetime "created_at", precision: 6, null: false - t.datetime "updated_at", precision: 6, null: false - t.index ["account_id"], name: "index_events_on_account_id" - t.index ["created_at"], name: "index_events_on_created_at" - t.index ["inbox_id"], name: "index_events_on_inbox_id" - t.index ["name"], name: "index_events_on_name" - t.index ["user_id"], name: "index_events_on_user_id" - end - create_table "inbox_members", id: :serial, force: :cascade do |t| t.integer "user_id", null: false t.integer "inbox_id", null: false @@ -664,6 +648,22 @@ ActiveRecord::Schema.define(version: 2022_02_18_120357) do t.datetime "updated_at", precision: 6, null: false end + create_table "reporting_events", force: :cascade do |t| + t.string "name" + t.float "value" + t.integer "account_id" + t.integer "inbox_id" + t.integer "user_id" + t.integer "conversation_id" + t.datetime "created_at", precision: 6, null: false + t.datetime "updated_at", precision: 6, null: false + t.index ["account_id"], name: "index_reporting_events_on_account_id" + t.index ["created_at"], name: "index_reporting_events_on_created_at" + t.index ["inbox_id"], name: "index_reporting_events_on_inbox_id" + t.index ["name"], name: "index_reporting_events_on_name" + t.index ["user_id"], name: "index_reporting_events_on_user_id" + end + create_table "taggings", id: :serial, force: :cascade do |t| t.integer "tag_id" t.string "taggable_type" diff --git a/lib/reports/update_account_identity.rb b/lib/reports/update_account_identity.rb deleted file mode 100644 index 5a19f439f..000000000 --- a/lib/reports/update_account_identity.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -class Reports::UpdateAccountIdentity < Reports::UpdateIdentity - attr_reader :account - - def initialize(account, timestamp = Time.now) - super(account, timestamp) - @identity = ::AccountIdentity.new(account.id) - end -end diff --git a/lib/reports/update_agent_identity.rb b/lib/reports/update_agent_identity.rb deleted file mode 100644 index e40d8353e..000000000 --- a/lib/reports/update_agent_identity.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -class Reports::UpdateAgentIdentity < Reports::UpdateIdentity - attr_reader :agent - - def initialize(account, agent, timestamp = Time.now) - super(account, timestamp) - @agent = agent - @identity = ::AgentIdentity.new(agent.id, tags: { account_id: account.id }) - end -end diff --git a/lib/reports/update_identity.rb b/lib/reports/update_identity.rb deleted file mode 100644 index 0849936f5..000000000 --- a/lib/reports/update_identity.rb +++ /dev/null @@ -1,69 +0,0 @@ -# frozen_string_literal: true - -class Reports::UpdateIdentity - attr_reader :account, :identity - attr_accessor :timestamp - - def initialize(account, timestamp = Time.now) - @account = account - @timestamp = timestamp - end - - def incr_conversations_count(step = 1) - update_conversations_count(:incr, step) - end - - def decr_conversations_count(step = 1) - update_conversations_count(:decr, step) - end - - def incr_incoming_messages_count(step = 1) - update_incoming_messages_count(:incr, step) - end - - def decr_incoming_messages_count(step = 1) - update_incoming_messages_count(:decr, step) - end - - def incr_outgoing_messages_count(step = 1) - update_outgoing_messages_count(:incr, step) - end - - def decr_outgoing_messages_count(step = 1) - update_outgoing_messages_count(:decr, step) - end - - def incr_resolutions_count(step = 1) - update_resolutions_count(:incr, step) - end - - def decr_resolutions_count(step = 1) - update_resolutions_count(:decr, step) - end - - def update_avg_first_response_time(response_time) - identity.avg_first_response_time.set(response_time, timestamp) - end - - def update_avg_resolution_time(response_time) - identity.avg_resolution_time.set(response_time, timestamp) - end - - private - - def update_conversations_count(method, step) - identity.conversations_count.send(method, step, timestamp) - end - - def update_incoming_messages_count(method, step) - identity.incoming_messages_count.send(method, step, timestamp) - end - - def update_outgoing_messages_count(method, step) - identity.outgoing_messages_count.send(method, step, timestamp) - end - - def update_resolutions_count(method, step) - identity.resolutions_count.send(method, step, timestamp) - end -end diff --git a/spec/builders/v2/report_builder_spec.rb b/spec/builders/v2/report_builder_spec.rb index 16fe18f49..c2c9977b3 100644 --- a/spec/builders/v2/report_builder_spec.rb +++ b/spec/builders/v2/report_builder_spec.rb @@ -227,7 +227,7 @@ describe ::V2::ReportBuilder do end it 'returns average first response time' do - label_2.events.update(value: 1.5) + label_2.reporting_events.update(value: 1.5) params = { metric: 'avg_first_response_time', diff --git a/spec/factories/events.rb b/spec/factories/reporting_events.rb similarity index 83% rename from spec/factories/events.rb rename to spec/factories/reporting_events.rb index 936e56ece..c779c01f6 100644 --- a/spec/factories/events.rb +++ b/spec/factories/reporting_events.rb @@ -1,5 +1,5 @@ FactoryBot.define do - factory :event do + factory :reporting_event do name { 'MyString' } value { 1.5 } account_id { 1 } diff --git a/spec/listeners/event_listener_spec.rb b/spec/listeners/reporting_event_listener_spec.rb similarity index 69% rename from spec/listeners/event_listener_spec.rb rename to spec/listeners/reporting_event_listener_spec.rb index a62da7682..399815014 100644 --- a/spec/listeners/event_listener_spec.rb +++ b/spec/listeners/reporting_event_listener_spec.rb @@ -1,5 +1,5 @@ require 'rails_helper' -describe EventListener do +describe ReportingEventListener do let(:listener) { described_class.instance } let!(:account) { create(:account) } let!(:user) { create(:user, account: account) } @@ -12,19 +12,19 @@ describe EventListener do describe '#conversation_resolved' do it 'creates conversation_resolved event' do - expect(account.events.where(name: 'conversation_resolved').count).to be 0 + expect(account.reporting_events.where(name: 'conversation_resolved').count).to be 0 event = Events::Base.new('conversation.resolved', Time.zone.now, conversation: conversation) listener.conversation_resolved(event) - expect(account.events.where(name: 'conversation_resolved').count).to be 1 + expect(account.reporting_events.where(name: 'conversation_resolved').count).to be 1 end end describe '#first_reply_created' do it 'creates first_response event' do - previous_count = account.events.where(name: 'first_response').count + previous_count = account.reporting_events.where(name: 'first_response').count event = Events::Base.new('first.reply.created', Time.zone.now, message: message) listener.first_reply_created(event) - expect(account.events.where(name: 'first_response').count).to eql previous_count + 1 + expect(account.reporting_events.where(name: 'first_response').count).to eql previous_count + 1 end end end diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index fc3787ab4..bd835865b 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Account do it { is_expected.to have_many(:web_widgets).class_name('::Channel::WebWidget').dependent(:destroy_async) } it { is_expected.to have_many(:webhooks).dependent(:destroy_async) } it { is_expected.to have_many(:notification_settings).dependent(:destroy_async) } - it { is_expected.to have_many(:events) } + it { is_expected.to have_many(:reporting_events) } it { is_expected.to have_many(:kbase_portals).dependent(:destroy_async) } it { is_expected.to have_many(:kbase_categories).dependent(:destroy_async) } it { is_expected.to have_many(:teams).dependent(:destroy_async) } diff --git a/spec/models/inbox_spec.rb b/spec/models/inbox_spec.rb index ba94540d8..2da7145cd 100644 --- a/spec/models/inbox_spec.rb +++ b/spec/models/inbox_spec.rb @@ -29,7 +29,7 @@ RSpec.describe Inbox do it { is_expected.to have_many(:webhooks).dependent(:destroy_async) } - it { is_expected.to have_many(:events) } + it { is_expected.to have_many(:reporting_events) } it { is_expected.to have_many(:hooks) } end diff --git a/spec/models/event_spec.rb b/spec/models/reporting_event_spec.rb similarity index 91% rename from spec/models/event_spec.rb rename to spec/models/reporting_event_spec.rb index 359f6c3db..553a4e360 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/reporting_event_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -RSpec.describe Event, type: :model do +RSpec.describe ReportingEvent, type: :model do describe 'validations' do it { is_expected.to validate_presence_of(:account_id) } it { is_expected.to validate_presence_of(:name) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e9923cd38..92015edcc 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -19,7 +19,7 @@ RSpec.describe User do it { is_expected.to have_many(:inbox_members).dependent(:destroy_async) } it { is_expected.to have_many(:notification_settings).dependent(:destroy_async) } it { is_expected.to have_many(:messages) } - it { is_expected.to have_many(:events) } + it { is_expected.to have_many(:reporting_events) } it { is_expected.to have_many(:teams) } end