Enhancement: Move reporting metrics to postgres (#606)

Gemfile

@@ -30,6 +30,7 @@ gem 'google-cloud-storage', require: false
 gem 'mini_magick'
 
 ##-- gems for database --#
+gem 'groupdate'
 gem 'pg'
 gem 'redis'
 gem 'redis-namespace'

Gemfile.lock

@@ -209,6 +209,8 @@ GEM
       multi_json (~> 1.11)
       os (>= 0.9, < 2.0)
       signet (~> 0.12)
+    groupdate (5.0.0)
+      activesupport (>= 5)
     haikunator (1.1.0)
     hana (1.3.5)
     hashie (4.1.0)
@@ -506,6 +508,7 @@ DEPENDENCIES
   flag_shih_tzu
   foreman
   google-cloud-storage
+  groupdate
   haikunator
   hashie
   jbuilder

app/builders/v2/report_builder.rb

@@ -0,0 +1,110 @@
+class V2::ReportBuilder
+  attr_reader :account, :params
+
+  def initialize(account, params)
+    @account = account
+    @params = params
+  end
+
+  def timeseries
+    send(params[:metric])
+  end
+
+  # For backward compatible with old report
+  def build
+    timeseries.each_with_object([]) do |p, arr|
+      arr << { value: p[1], timestamp: p[0].to_time.to_i }
+    end
+  end
+
+  def summary
+    {
+      conversations_count: conversations_count.values.sum,
+      incoming_messages_count: incoming_messages_count.values.sum,
+      outgoing_messages_count: outgoing_messages_count.values.sum,
+      avg_first_response_time: avg_first_response_time_summary,
+      avg_resolution_time: avg_resolution_time_summary,
+      resolutions_count: resolutions_count.values.sum
+    }
+  end
+
+  private
+
+  def scope
+    return account if params[:type].match?('account')
+    return inbox if params[:type].match?('inbox')
+    return user if params[:type].match?('agent')
+  end
+
+  def inbox
+    @inbox ||= account.inboxes.where(id: params[:id]).first
+  end
+
+  def user
+    @user ||= account.users.where(id: params[:id]).first
+  end
+
+  def conversations_count
+    scope.conversations
+         .group_by_day(:created_at, range: range, default_value: 0)
+         .count
+  end
+
+  def incoming_messages_count
+    scope.messages.unscoped.incoming
+         .group_by_day(:created_at, range: range, default_value: 0)
+         .count
+  end
+
+  def outgoing_messages_count
+    scope.messages.unscoped.outgoing
+         .group_by_day(:created_at, range: range, default_value: 0)
+         .count
+  end
+
+  def resolutions_count
+    scope.conversations
+         .resolved
+         .group_by_day(:created_at, range: range, default_value: 0)
+         .count
+  end
+
+  def avg_first_response_time
+    scope.events
+         .where(name: 'first_response')
+         .group_by_day(:created_at, range: range, default_value: 0)
+         .average(:value)
+  end
+
+  def avg_resolution_time
+    scope.events.where(name: 'conversation_resolved')
+         .group_by_day(:created_at, range: range, default_value: 0)
+         .average(:value)
+  end
+
+  def range
+    parse_date_time(params[:since])..parse_date_time(params[:until])
+  end
+
+  # Taking average of average is not too accurate
+  # https://en.wikipedia.org/wiki/Simpson's_paradox
+  # TODO: Will optimize this later
+  def avg_resolution_time_summary
+    return 0 if avg_resolution_time.values.empty?
+
+    (avg_resolution_time.values.sum / avg_resolution_time.values.length)
+  end
+
+  def avg_first_response_time_summary
+    return 0 if avg_first_response_time.values.empty?
+
+    (avg_first_response_time.values.sum / avg_first_response_time.values.length)
+  end
+
+  def parse_date_time(datetime)
+    return datetime if datetime.is_a?(DateTime)
+    return datetime.to_datetime if datetime.is_a?(Time) || datetime.is_a?(Date)
+
+    DateTime.strptime(datetime, '%s')
+  end
+end

app/controllers/api/v2/accounts/reports_controller.rb

@@ -0,0 +1,39 @@
+class Api::V2::Accounts::ReportsController < Api::BaseController
+  def account
+    builder = V2::ReportBuilder.new(current_account, account_report_params)
+    data = builder.build
+    render json: data
+  end
+
+  def account_summary
+    render json: account_summary_metrics
+  end
+
+  private
+
+  def current_account
+    current_user.account
+  end
+
+  def account_summary_params
+    {
+      type: :account,
+      since: params[:since],
+      until: params[:until]
+    }
+  end
+
+  def account_report_params
+    {
+      metric: params[:metric],
+      type: :account,
+      since: params[:since],
+      until: params[:until]
+    }
+  end
+
+  def account_summary_metrics
+    builder = V2::ReportBuilder.new(current_account, account_summary_params)
+    builder.summary
+  end
+end

app/dispatchers/async_dispatcher.rb

@@ -1,11 +1,13 @@
 class AsyncDispatcher < BaseDispatcher
   def dispatch(event_name, timestamp, data)
     event_object = Events::Base.new(event_name, timestamp, data)
+    # TODO: Move this to worker
     publish(event_object.method_name, event_object)
   end
 
   def listeners
     listeners = [AgentBotListener.instance, EmailNotificationListener.instance, ReportingListener.instance, WebhookListener.instance]
+    listeners << EventListener.instance
     listeners << SubscriptionListener.instance if ENV['BILLING_ENABLED']
     listeners
   end

app/listeners/event_listener.rb

@@ -0,0 +1,31 @@
+class EventListener < 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(
+      name: 'conversation_resolved',
+      value: time_to_resolve,
+      account_id: conversation.account_id,
+      inbox_id: conversation.inbox_id,
+      user_id: conversation.assignee_id,
+      conversation_id: conversation.id
+    )
+    event.save
+  end
+
+  def first_reply_created(event)
+    message = extract_message_and_account(event)[0]
+    conversation = message.conversation
+    first_response_time = message.created_at.to_i - conversation.created_at.to_i
+
+    event = Event.new(
+      name: 'first_response',
+      value: first_response_time,
+      account_id: conversation.account_id,
+      inbox_id: conversation.inbox_id,
+      user_id: conversation.assignee_id
+    )
+    event.save
+  end
+end

app/models/account.rb

@@ -10,6 +10,7 @@
 
 class Account < ApplicationRecord
   include Events::Types
+  include Reportable
 
   validates :name, presence: true
 
@@ -18,6 +19,7 @@ class Account < ApplicationRecord
   has_many :users, through: :account_users
   has_many :inboxes, dependent: :destroy
   has_many :conversations, dependent: :destroy
+  has_many :messages, dependent: :destroy
   has_many :contacts, dependent: :destroy
   has_many :facebook_pages, dependent: :destroy, class_name: '::Channel::FacebookPage'
   has_many :twitter_profiles, dependent: :destroy, class_name: '::Channel::TwitterProfile'

app/models/concerns/reportable.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+module Reportable
+  extend ActiveSupport::Concern
+
+  included do
+    has_many :events, dependent: :destroy
+  end
+end

app/models/event.rb

@@ -0,0 +1,33 @@
+# == Schema Information
+#
+# Table name: events
+#
+#  id              :bigint           not null, primary key
+#  name            :string
+#  value           :float
+#  created_at      :datetime         not null
+#  updated_at      :datetime         not null
+#  account_id      :integer
+#  conversation_id :integer
+#  inbox_id        :integer
+#  user_id         :integer
+#
+# 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)
+#
+
+class Event < ApplicationRecord
+  validates :account_id, presence: true
+  validates :name, presence: true
+  validates :value, presence: true
+
+  belongs_to :account
+  belongs_to :user, optional: true
+  belongs_to :inbox, optional: true
+  belongs_to :conversation, optional: true
+end

app/models/inbox.rb

@@ -19,6 +19,8 @@
 #
 
 class Inbox < ApplicationRecord
+  include Reportable
+
   validates :account_id, presence: true
 
   belongs_to :account

app/models/message.rb

@@ -20,9 +20,12 @@
 #
 # Indexes
 #
+#  index_messages_on_account_id       (account_id)
 #  index_messages_on_contact_id       (contact_id)
 #  index_messages_on_conversation_id  (conversation_id)
+#  index_messages_on_inbox_id         (inbox_id)
 #  index_messages_on_source_id        (source_id)
+#  index_messages_on_user_id          (user_id)
 #
 # Foreign Keys
 #

app/models/user.rb

@@ -43,6 +43,7 @@ class User < ApplicationRecord
   include Events::Types
   include Pubsubable
   include Rails.application.routes.url_helpers
+  include Reportable
 
   devise :database_authenticatable,
          :registerable,

config/routes.rb

@@ -113,6 +113,19 @@ Rails.application.routes.draw do
         end
       end
     end
+    
+    namespace :v2  do
+      resources :accounts, only: [], module: :accounts do
+        resources :reports, only: [] do
+          collection do
+            get :account
+          end
+          member do
+            get :account_summary
+          end
+        end
+      end
+    end
   end
 
   namespace :twitter do

db/migrate/20200121190900_add_notification_settings.rb

@@ -1,5 +1,7 @@
 class AddNotificationSettings < ActiveRecord::Migration[6.0]
   def change
+    return if ActiveRecord::Base.connection.data_source_exists? 'notification_settings'
+
     create_table :notification_settings do |t|
       t.integer :account_id
       t.integer :user_id

db/migrate/20200310062527_create_events.rb

@@ -0,0 +1,14 @@
+class CreateEvents < ActiveRecord::Migration[6.0]
+  def change
+    create_table :events 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.timestamps
+    end
+  end
+end

db/migrate/20200310070540_add_index_to_event.rb

@@ -0,0 +1,9 @@
+class AddIndexToEvent < ActiveRecord::Migration[6.0]
+  def change
+    add_index :events, :name
+    add_index :events, :created_at
+    add_index :events, :account_id
+    add_index :events, :inbox_id
+    add_index :events, :user_id
+  end
+end

db/migrate/20200311083854_add_index_to_message.rb

@@ -0,0 +1,7 @@
+class AddIndexToMessage < ActiveRecord::Migration[6.0]
+  def change
+    add_index :messages, :account_id
+    add_index :messages, :inbox_id
+    add_index :messages, :user_id
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_03_09_213132) do
+ActiveRecord::Schema.define(version: 2020_03_11_083854) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -179,6 +179,22 @@ ActiveRecord::Schema.define(version: 2020_03_09_213132) do
     t.index ["contact_inbox_id"], name: "index_conversations_on_contact_inbox_id"
   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
@@ -213,9 +229,12 @@ ActiveRecord::Schema.define(version: 2020_03_09_213132) do
     t.integer "content_type", default: 0
     t.json "content_attributes", default: {}
     t.bigint "contact_id"
+    t.index ["account_id"], name: "index_messages_on_account_id"
     t.index ["contact_id"], name: "index_messages_on_contact_id"
     t.index ["conversation_id"], name: "index_messages_on_conversation_id"
+    t.index ["inbox_id"], name: "index_messages_on_inbox_id"
     t.index ["source_id"], name: "index_messages_on_source_id"
+    t.index ["user_id"], name: "index_messages_on_user_id"
   end
 
   create_table "notification_settings", force: :cascade do |t|

spec/builders/v2/report_builder_spec.rb

@@ -0,0 +1,133 @@
+require 'rails_helper'
+
+describe ::V2::ReportBuilder do
+  let!(:account) { create(:account) }
+  let!(:user) { create(:user, account: account) }
+  let!(:inbox) { create(:inbox, account: account) }
+  let(:inbox_member) { create(:inbox_member, user: user, inbox: inbox) }
+
+  describe '#timeseries' do
+    context 'when report type is account' do
+      before do
+        10.times do
+          conversation = create(:conversation, account: account,
+                                               inbox: inbox, assignee: user,
+                                               created_at: Time.zone.today)
+          create_list(:message, 5, message_type: 'outgoing',
+                                   account: account, inbox: inbox,
+                                   conversation: conversation, created_at: Time.zone.today + 2.hours)
+          create_list(:message, 2, message_type: 'incoming',
+                                   account: account, inbox: inbox,
+                                   conversation: conversation,
+                                   created_at: Time.zone.today + 3.hours)
+        end
+        5.times do
+          conversation = create(:conversation, account: account,
+                                               inbox: inbox, assignee: user,
+                                               created_at: (Time.zone.today - 2.days))
+          create_list(:message, 3, message_type: 'outgoing',
+                                   account: account, inbox: inbox,
+                                   conversation: conversation,
+                                   created_at: (Time.zone.today - 2.days))
+          create_list(:message, 1, message_type: 'incoming',
+                                   account: account, inbox: inbox,
+                                   conversation: conversation,
+                                   created_at: (Time.zone.today - 2.days))
+        end
+      end
+
+      it 'return conversations count' do
+        params = {
+          metric: 'conversations_count',
+          type: :account,
+          since: (Time.zone.today - 3.days).to_time.to_i.to_s,
+          until: Time.zone.today.to_time.to_i.to_s
+        }
+
+        builder = V2::ReportBuilder.new(account, params)
+        metrics = builder.timeseries
+
+        expect(metrics[Time.zone.today]).to be 10
+        expect(metrics[Time.zone.today - 2.days]).to be 5
+      end
+
+      it 'return incoming messages count' do
+        params = {
+          metric: 'incoming_messages_count',
+          type: :account,
+          since: (Time.zone.today - 3.days).to_time.to_i.to_s,
+          until: Time.zone.today.to_time.to_i.to_s
+        }
+
+        builder = V2::ReportBuilder.new(account, params)
+        metrics = builder.timeseries
+
+        expect(metrics[Time.zone.today]).to be 20
+        expect(metrics[Time.zone.today - 2.days]).to be 5
+      end
+
+      it 'return outgoing messages count' do
+        params = {
+          metric: 'outgoing_messages_count',
+          type: :account,
+          since: (Time.zone.today - 3.days).to_time.to_i.to_s,
+          until: Time.zone.today.to_time.to_i.to_s
+        }
+
+        builder = V2::ReportBuilder.new(account, params)
+        metrics = builder.timeseries
+
+        expect(metrics[Time.zone.today]).to be 50
+        expect(metrics[Time.zone.today - 2.days]).to be 15
+      end
+
+      it 'return resolutions count' do
+        params = {
+          metric: 'resolutions_count',
+          type: :account,
+          since: (Time.zone.today - 3.days).to_time.to_i.to_s,
+          until: Time.zone.today.to_time.to_i.to_s
+        }
+
+        conversations = account.conversations.where('created_at < ?', 1.day.ago)
+        conversations.each(&:resolved!)
+        builder = V2::ReportBuilder.new(account, params)
+        metrics = builder.timeseries
+
+        expect(metrics[Time.zone.today]).to be 0
+        expect(metrics[Time.zone.today - 2.days]).to be 5
+      end
+
+      it 'returns average first response time' do
+        params = {
+          metric: 'avg_first_response_time',
+          type: :account,
+          since: (Time.zone.today - 3.days).to_time.to_i.to_s,
+          until: Time.zone.today.to_time.to_i.to_s
+        }
+
+        builder = V2::ReportBuilder.new(account, params)
+        metrics = builder.timeseries
+
+        expect(metrics[Time.zone.today].to_f).to be 0.48e4
+      end
+
+      it 'returns summary' do
+        params = {
+          type: :account,
+          since: (Time.zone.today - 3.days).to_time.to_i.to_s,
+          until: Time.zone.today.to_time.to_i.to_s
+        }
+
+        builder = V2::ReportBuilder.new(account, params)
+        metrics = builder.summary
+
+        expect(metrics[:conversations_count]).to be 15
+        expect(metrics[:incoming_messages_count]).to be 25
+        expect(metrics[:outgoing_messages_count]).to be 65
+        expect(metrics[:avg_resolution_time]).to be 0
+        expect(metrics[:resolutions_count]).to be 0
+      end
+    end
+  end
+end

spec/controllers/api/v2/accounts/report_controller_spec.rb

@@ -0,0 +1,80 @@
+require 'rails_helper'
+
+RSpec.describe 'Reports API', type: :request do
+  let(:account) { create(:account) }
+  let!(:user) { create(:user, account: account) }
+  let!(:inbox) { create(:inbox, account: account) }
+  let(:inbox_member) { create(:inbox_member, user: user, inbox: inbox) }
+
+  before do
+    create_list(:conversation, 10, account: account, inbox: inbox,
+                                   assignee: user, created_at: Time.zone.today)
+  end
+
+  describe 'GET /api/v2/accounts/:account_id/reports/account' do
+    context 'when it is an unauthenticated user' do
+      it 'returns unauthorized' do
+        get "/api/v2/accounts/#{account.id}/reports/account"
+
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+
+    context 'when it is an authenticated user' do
+      let(:agent) { create(:user, account: account, role: :agent) }
+
+      it 'return timeseries metrics' do
+        params = {
+          metric: 'conversations_count',
+          type: :account,
+          since: Time.zone.today.to_time.to_i.to_s,
+          until: Time.zone.today.to_time.to_i.to_s
+        }
+
+        get "/api/v2/accounts/#{account.id}/reports/account",
+            params: params,
+            headers: agent.create_new_auth_token,
+            as: :json
+
+        expect(response).to have_http_status(:success)
+        json_response = JSON.parse(response.body)
+
+        current_day_metric = json_response.select { |x| x['timestamp'] == Time.zone.today.to_time.to_i }
+        expect(current_day_metric.length).to eq(1)
+        expect(current_day_metric[0]['value']).to eq(10)
+      end
+    end
+  end
+
+  describe 'GET /api/v2/accounts/:account_id/reports/:id/account_summary' do
+    context 'when it is an unauthenticated user' do
+      it 'returns unauthorized' do
+        get "/api/v2/accounts/#{account.id}/reports/#{account.id}/account_summary"
+
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+
+    context 'when it is an authenticated user' do
+      let(:agent) { create(:user, account: account, role: :agent) }
+
+      it 'returns summary metrics' do
+        params = {
+          type: :account,
+          since: Time.zone.today.to_time.to_i.to_s,
+          until: Time.zone.today.to_time.to_i.to_s
+        }
+
+        get "/api/v2/accounts/#{account.id}/reports/#{account.id}/account_summary",
+            params: params,
+            headers: agent.create_new_auth_token,
+            as: :json
+
+        expect(response).to have_http_status(:success)
+        json_response = JSON.parse(response.body)
+
+        expect(json_response['conversations_count']).to eq(10)
+      end
+    end
+  end
+end

spec/factories/events.rb

@@ -0,0 +1,10 @@
+FactoryBot.define do
+  factory :event do
+    name { 'MyString' }
+    value { 1.5 }
+    account_id { 1 }
+    inbox_id { 1 }
+    user_id { 1 }
+    conversation_id { 1 }
+  end
+end

spec/factories/users.rb

@@ -13,7 +13,7 @@ FactoryBot.define do
     uid { SecureRandom.uuid }
     name { Faker::Name.name }
     nickname { Faker::Name.first_name }
-    email { nickname + '@example.com' }
+    email { nickname + "@#{SecureRandom.uuid}.com" }
     password { 'password' }
 
     after(:build) do |user, evaluator|

spec/listeners/event_listener_spec.rb

@@ -0,0 +1,30 @@
+require 'rails_helper'
+describe EventListener do
+  let(:listener) { described_class.instance }
+  let!(:account) { create(:account) }
+  let!(:user) { create(:user, account: account) }
+  let!(:inbox) { create(:inbox, account: account) }
+  let!(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user) }
+  let!(:message) do
+    create(:message, message_type: 'outgoing',
+                     account: account, inbox: inbox, conversation: conversation)
+  end
+
+  describe '#conversation_resolved' do
+    it 'creates conversation_resolved event' do
+      expect(account.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
+    end
+  end
+
+  describe '#first_reply_created' do
+    it 'creates first_response event' do
+      previous_count = account.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
+    end
+  end
+end

spec/models/account_spec.rb

@@ -17,4 +17,5 @@ RSpec.describe Account do
   it { is_expected.to have_one(:subscription).dependent(:destroy) }
   it { is_expected.to have_many(:webhooks).dependent(:destroy) }
   it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
+  it { is_expected.to have_many(:events) }
 end

spec/models/event_spec.rb

@@ -0,0 +1,16 @@
+require 'rails_helper'
+
+RSpec.describe Event, type: :model do
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:account_id) }
+    it { is_expected.to validate_presence_of(:name) }
+    it { is_expected.to validate_presence_of(:value) }
+  end
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:account) }
+    it { is_expected.to belong_to(:inbox).optional }
+    it { is_expected.to belong_to(:user).optional }
+    it { is_expected.to belong_to(:conversation).optional }
+  end
+end

spec/models/inbox_spec.rb

@@ -27,6 +27,8 @@ RSpec.describe Inbox do
     it { is_expected.to have_one(:agent_bot_inbox) }
 
     it { is_expected.to have_many(:webhooks).dependent(:destroy) }
+
+    it { is_expected.to have_many(:events) }
   end
 
   describe '#add_member' do

spec/models/user_spec.rb

@@ -18,6 +18,7 @@ RSpec.describe User do
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
     it { is_expected.to have_many(:assigned_inboxes).through(:inbox_members) }
     it { is_expected.to have_many(:messages) }
+    it { is_expected.to have_many(:events) }
   end
 
   describe 'pubsub_token' do