From 3988777718689c38fc2a6dae4625a2d5395f6cb7 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Thu, 17 Oct 2019 03:18:07 +0530 Subject: [PATCH] Refactoring the code for pub sub (#155) - We were using the attribute name 'channel' to store pubsub tokens, which was confusing. - switched to faker from ffaker - spec for contact.rb --- Gemfile | 2 +- Gemfile.lock | 5 +-- app/javascript/dashboard/api/auth.js | 4 +-- app/javascript/dashboard/helper/pusher.js | 2 +- app/listeners/pusher_listener.rb | 14 ++++---- app/models/contact.rb | 14 +++----- app/models/user.rb | 10 ++---- config/application.rb | 20 +++--------- ...rename_channel_attribute_name_in_models.rb | 9 ++++++ db/schema.rb | 8 +++-- spec/factories/contacts.rb | 1 - spec/factories/users.rb | 4 +-- spec/models/contact_spec.rb | 32 +++++++++++++++++++ spec/models/user_spec.rb | 16 ++++++++++ 14 files changed, 89 insertions(+), 52 deletions(-) create mode 100644 db/migrate/20191016211109_rename_channel_attribute_name_in_models.rb create mode 100644 spec/models/contact_spec.rb diff --git a/Gemfile b/Gemfile index 330005435..fa90c347e 100644 --- a/Gemfile +++ b/Gemfile @@ -63,7 +63,7 @@ end group :development, :test do gem 'byebug', platform: :mri gem 'factory_bot_rails' - gem 'ffaker' + gem 'faker' gem 'listen' gem 'pry-rails' gem 'rspec-rails', '~> 3.8' diff --git a/Gemfile.lock b/Gemfile.lock index 2e3ebcb8c..79a2f5fdc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -186,9 +186,10 @@ GEM factory_bot_rails (5.1.1) factory_bot (~> 5.1.0) railties (>= 4.2.0) + faker (2.1.2) + i18n (>= 0.8) faraday (0.16.2) multipart-post (>= 1.2, < 3) - ffaker (2.13.0) ffi (1.11.1) figaro (1.1.1) thor (~> 0.14) @@ -435,7 +436,7 @@ DEPENDENCIES devise_token_auth! facebook-messenger (~> 0.11.1) factory_bot_rails - ffaker + faker figaro foreman hashie diff --git a/app/javascript/dashboard/api/auth.js b/app/javascript/dashboard/api/auth.js index 3ea00b2ca..5e5a44af8 100644 --- a/app/javascript/dashboard/api/auth.js +++ b/app/javascript/dashboard/api/auth.js @@ -108,9 +108,9 @@ export default { } return false; }, - getChannel() { + getPubSubToken() { if (this.isLoggedIn()) { - return Cookies.getJSON('user').channel; + return Cookies.getJSON('user').pubsub_token; } return null; }, diff --git a/app/javascript/dashboard/helper/pusher.js b/app/javascript/dashboard/helper/pusher.js index ed577fa22..934de92ed 100644 --- a/app/javascript/dashboard/helper/pusher.js +++ b/app/javascript/dashboard/helper/pusher.js @@ -63,7 +63,7 @@ export default { const pusher = new VuePusher(CONSTANTS.PUSHER.token, options); // Add to global Obj if (AuthAPI.isLoggedIn()) { - pusher.subscribe(AuthAPI.getChannel()); + pusher.subscribe(AuthAPI.getPubSubToken()); return pusher.pusher; } return null; diff --git a/app/listeners/pusher_listener.rb b/app/listeners/pusher_listener.rb index 90a5db3e6..5f6504d0c 100644 --- a/app/listeners/pusher_listener.rb +++ b/app/listeners/pusher_listener.rb @@ -3,45 +3,45 @@ class PusherListener < BaseListener def conversation_created(event) conversation, account, timestamp = extract_conversation_and_account(event) - members = conversation.inbox.members.pluck(:channel) + members = conversation.inbox.members.pluck(:pubsub_token) Pusher.trigger(members, CONVERSATION_CREATED , conversation.push_event_data) if members.present? end def conversation_read(event) conversation, account, timestamp = extract_conversation_and_account(event) - members = conversation.inbox.members.pluck(:channel) + members = conversation.inbox.members.pluck(:pubsub_token) Pusher.trigger(members, CONVERSATION_READ , conversation.push_event_data) if members.present? end def message_created(event) message, account, timestamp = extract_message_and_account(event) conversation = message.conversation - members = conversation.inbox.members.pluck(:channel) + members = conversation.inbox.members.pluck(:pubsub_token) Pusher.trigger(members, MESSAGE_CREATED , message.push_event_data) if members.present? end def conversation_reopened(event) conversation, account, timestamp = extract_conversation_and_account(event) - members = conversation.inbox.members.pluck(:channel) + members = conversation.inbox.members.pluck(:pubsub_token) Pusher.trigger(members, CONVERSATION_REOPENED, conversation.push_event_data) if members.present? end def conversation_lock_toggle(event) conversation, account, timestamp = extract_conversation_and_account(event) - members = conversation.inbox.members.pluck(:channel) + members = conversation.inbox.members.pluck(:pubsub_token) Pusher.trigger(members, CONVERSATION_LOCK_TOGGLE, conversation.lock_event_data) if members.present? end def assignee_changed(event) conversation, account, timestamp = extract_conversation_and_account(event) - members = conversation.inbox.members.pluck(:channel) + members = conversation.inbox.members.pluck(:pubsub_token) Pusher.trigger(members, ASSIGNEE_CHANGED, conversation.push_event_data) if members.present? end private - def push(channel, data) + def push(pubsub_token, data) # Enqueue sidekiq job to push event to corresponding channel end end diff --git a/app/models/contact.rb b/app/models/contact.rb index b46c6f2a1..128b8b954 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -1,5 +1,7 @@ class Contact < ApplicationRecord - + + #Used by the pusher/PubSub Service we use for real time communications + has_secure_token :pubsub_token validates :account_id, presence: true validates :inbox_id, presence: true @@ -8,21 +10,13 @@ class Contact < ApplicationRecord has_many :conversations, dependent: :destroy, foreign_key: :sender_id mount_uploader :avatar, AvatarUploader - before_create :set_channel - def push_event_data { id: id, name: name, thumbnail: avatar.thumb.url, channel: inbox.try(:channel).try(:name), - chat_channel: chat_channel + pubsub_token: pubsub_token } end - - def set_channel - begin - self.chat_channel = SecureRandom.hex - end while self.class.exists?(chat_channel: chat_channel) - end end diff --git a/app/models/user.rb b/app/models/user.rb index 48b9a7d6e..6aa0023ee 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -11,6 +11,9 @@ class User < ApplicationRecord :validatable, :confirmable + #Used by the pusher/PubSub Service we use for real time communications + has_secure_token :pubsub_token + validates_uniqueness_of :email, scope: :account_id validates :email, presence: true validates :name, presence: true @@ -26,7 +29,6 @@ class User < ApplicationRecord has_many :assigned_inboxes, through: :inbox_members, source: :inbox has_many :messages - before_create :set_channel before_validation :set_password_and_uid, on: :create accepts_nested_attributes_for :account @@ -42,12 +44,6 @@ class User < ApplicationRecord super(options).merge(confirmed: confirmed?, subscription: account.try(:subscription).try(:summary) ) end - def set_channel - begin - self.channel = SecureRandom.hex - end while self.class.exists?(channel: channel) - end - def notify_creation Rails.configuration.dispatcher.dispatch(AGENT_ADDED, Time.zone.now, account: self.account) end diff --git a/config/application.rb b/config/application.rb index 3290a81be..7715a74d4 100644 --- a/config/application.rb +++ b/config/application.rb @@ -1,19 +1,8 @@ +# frozen_string_literal: true + require_relative 'boot' -require "rails" -# Pick the frameworks you want: -require "active_model/railtie" -require "active_job/railtie" -require "active_record/railtie" -require "active_storage/engine" -require "action_controller/railtie" -require "action_mailer/railtie" -require "action_mailbox/engine" -require "action_text/engine" -require "action_view/railtie" -# require "action_cable/engine" -require "sprockets/railtie" -require "rails/test_unit/railtie" +require 'rails/all' # Require the gems listed in Gemfile, including any gems # you've limited to :test, :development, or :production. @@ -23,8 +12,7 @@ module Chatwoot class Application < Rails::Application # Initialize configuration defaults for originally generated Rails version. config.load_defaults 5.0 - config.paths.add File.join('app', 'bot'), glob: File.join('**', '*.rb') - config.autoload_paths += Dir[Rails.root.join('app', 'bot', '*')] + config.autoload_paths << Rails.root.join('lib') config.eager_load_paths << Rails.root.join('lib') diff --git a/db/migrate/20191016211109_rename_channel_attribute_name_in_models.rb b/db/migrate/20191016211109_rename_channel_attribute_name_in_models.rb new file mode 100644 index 000000000..6e1881843 --- /dev/null +++ b/db/migrate/20191016211109_rename_channel_attribute_name_in_models.rb @@ -0,0 +1,9 @@ +class RenameChannelAttributeNameInModels < ActiveRecord::Migration[6.1] + def change + rename_column :users, :channel, :pubsub_token + rename_column :contacts, :chat_channel, :pubsub_token + + add_index :users, :pubsub_token, unique: true + add_index :contacts, :pubsub_token, unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index dc475d830..448e0b895 100644 --- a/db/schema.rb +++ b/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: 2019_10_14_051743) do +ActiveRecord::Schema.define(version: 2019_10_16_211109) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -67,8 +67,9 @@ ActiveRecord::Schema.define(version: 2019_10_14_051743) do t.integer "inbox_id", null: false t.bigserial "source_id", null: false t.string "avatar" - t.string "chat_channel" + t.string "pubsub_token" t.index ["account_id"], name: "index_contacts_on_account_id" + t.index ["pubsub_token"], name: "index_contacts_on_pubsub_token", unique: true end create_table "conversations", id: :serial, force: :cascade do |t| @@ -203,11 +204,12 @@ ActiveRecord::Schema.define(version: 2019_10_14_051743) do t.integer "account_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "channel" + t.string "pubsub_token" t.integer "role", default: 0 t.bigint "inviter_id" t.index ["email"], name: "index_users_on_email" t.index ["inviter_id"], name: "index_users_on_inviter_id" + t.index ["pubsub_token"], name: "index_users_on_pubsub_token", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true t.index ["uid", "provider"], name: "index_users_on_uid_and_provider", unique: true end diff --git a/spec/factories/contacts.rb b/spec/factories/contacts.rb index b74b1f245..c21a7a998 100644 --- a/spec/factories/contacts.rb +++ b/spec/factories/contacts.rb @@ -6,7 +6,6 @@ FactoryBot.define do sequence(:email) { |n| "widget-#{n}@example.com" } phone_number { "+123456789011" } source_id { rand(100) } - chat_channel { "chat_channel" } account inbox end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index abcf67d86..33b5dd3a4 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -8,8 +8,8 @@ FactoryBot.define do provider { 'email' } uid { SecureRandom.uuid } - name { FFaker::Name.name } - nickname { FFaker::InternetSE.user_name_from_name(name) } + name { Faker::Name.name } + nickname { Faker::Name.first_name } email { nickname + '@example.com' } role { 'agent' } password { "password" } diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb new file mode 100644 index 000000000..a91294c4d --- /dev/null +++ b/spec/models/contact_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Contact do + context 'validations' do + it { is_expected.to validate_presence_of(:account_id) } + it { is_expected.to validate_presence_of(:inbox_id) } + end + + context 'associations' do + it { is_expected.to belong_to(:account) } + it { is_expected.to belong_to(:inbox) } + it { is_expected.to have_many(:conversations).dependent(:destroy) } + end + + describe 'pubsub_token' do + let(:user) { create(:user) } + + it 'gets created on object create' do + obj = user + expect(obj.pubsub_token).not_to eq(nil) + end + + it 'does not get updated on object update' do + obj = user + old_token = obj.pubsub_token + obj.update(name: 'test') + expect(obj.pubsub_token).to eq(old_token) + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7ad48c509..c95b6bd8a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -21,4 +21,20 @@ RSpec.describe User do it { is_expected.to have_many(:assigned_inboxes).through(:inbox_members) } it { is_expected.to have_many(:messages) } end + + describe 'pubsub_token' do + let(:user) { create(:user) } + + it 'gets created on object create' do + obj = user + expect(obj.pubsub_token).not_to eq(nil) + end + + it 'does not get updated on object update' do + obj = user + old_token = obj.pubsub_token + obj.update(name: 'test') + expect(obj.pubsub_token).to eq(old_token) + end + end end