Chore: Enable Users to create multiple accounts (#440)

Addresses: #402
- migrations to split roles and other attributes from users table
- make changes in code to accommodate this change

Co-authored-by: Sojan Jose <sojan@pepalo.com>
Co-authored-by: Pranav Raj Sreepuram <pranavrajs@gmail.com>

.gitignore

@@ -53,3 +53,5 @@ coverage
 # ignore packages
 node_modules
 package-lock.json
+
+*.dump

.rubocop.yml

@@ -41,6 +41,9 @@ RSpec/NestedGroups:
   Max: 4
 RSpec/MessageSpies:
   Enabled: false
+Rails/BulkChangeTable:
+  Exclude:
+    - 'db/migrate/20200121190901_create_account_users.rb'
 AllCops:
   Exclude:
     - db/*

app/builders/account_builder.rb

@@ -42,18 +42,26 @@ class AccountBuilder
 
   def create_and_link_user
     password = Time.now.to_i
-    @user = @account.users.new(email: @email,
+    @user = User.new(email: @email,
                      password: password,
                      password_confirmation: password,
-                               role: User.roles['administrator'],
                      name: email_to_name(@email))
     if @user.save!
+      link_user_to_account(@user, @account)
       @user
     else
       raise UserErrors.new(errors: @user.errors)
     end
   end
 
+  def link_user_to_account(user, account)
+    AccountUser.create!(
+      account_id: account.id,
+      user_id: user.id,
+      role: AccountUser.roles['administrator']
+    )
+  end
+
   def email_to_name(email)
     name = email[/[^@]+/]
     name.split('.').map(&:capitalize).join(' ')

app/controllers/api/v1/agents_controller.rb

@@ -1,25 +1,27 @@
 class Api::V1::AgentsController < Api::BaseController
   before_action :fetch_agent, except: [:create, :index]
   before_action :check_authorization
-  before_action :build_agent, only: [:create]
+  before_action :find_user, only: [:create]
+  before_action :create_user, only: [:create]
+  before_action :save_account_user, only: [:create]
 
   def index
     @agents = agents
   end
 
   def destroy
-    @agent.destroy
+    @agent.account_user.destroy
     head :ok
   end
 
   def update
-    @agent.update!(agent_params)
-    render json: @agent
+    @agent.update!(agent_params.except(:role))
+    @agent.account_user.update!(role: agent_params[:role]) if agent_params[:role]
+    render 'api/v1/models/user.json', locals: { resource: @agent }
   end
 
   def create
-    @agent.save!
-    render json: @agent
+    render 'api/v1/models/user.json', locals: { resource: @user }
   end
 
   private
@@ -32,8 +34,23 @@ class Api::V1::AgentsController < Api::BaseController
     @agent = agents.find(params[:id])
   end
 
-  def build_agent
-    @agent = agents.new(new_agent_params)
+  def find_user
+    @user =  User.find_by(email: new_agent_params[:email])
+  end
+
+  def create_user
+    return if @user
+
+    @user = User.create!(new_agent_params.slice(:email, :name, :password, :password_confirmation))
+  end
+
+  def save_account_user
+    AccountUser.create!(
+      account_id: current_account.id,
+      user_id: @user.id,
+      role: new_agent_params[:role],
+      inviter_id: current_user.id
+    )
   end
 
   def agent_params

app/models/account.rb

@@ -13,7 +13,8 @@ class Account < ApplicationRecord
 
   validates :name, presence: true
 
-  has_many :users, dependent: :destroy
+  has_many :account_users, dependent: :destroy
+  has_many :users, through: :account_users
   has_many :inboxes, dependent: :destroy
   has_many :conversations, dependent: :destroy
   has_many :contacts, dependent: :destroy

app/models/account_user.rb

@@ -0,0 +1,48 @@
+# == Schema Information
+#
+# Table name: account_users
+#
+#  id         :bigint           not null, primary key
+#  role       :integer          default("agent")
+#  created_at :datetime         not null
+#  updated_at :datetime         not null
+#  account_id :bigint
+#  inviter_id :bigint
+#  user_id    :bigint
+#
+# Indexes
+#
+#  index_account_users_on_account_id  (account_id)
+#  index_account_users_on_user_id     (user_id)
+#  uniq_user_id_per_account_id        (account_id,user_id) UNIQUE
+#
+# Foreign Keys
+#
+#  fk_rails_...  (account_id => accounts.id)
+#  fk_rails_...  (user_id => users.id)
+#
+
+class AccountUser < ApplicationRecord
+  belongs_to :account
+  belongs_to :user
+  belongs_to :inviter, class_name: 'User', optional: true
+
+  enum role: { agent: 0, administrator: 1 }
+  accepts_nested_attributes_for :account
+
+  after_create :create_notification_setting
+  after_destroy :destroy_notification_setting
+
+  validates :user_id, uniqueness: { scope: :account_id }
+
+  def create_notification_setting
+    setting = user.notification_settings.new(account_id: account.id)
+    setting.selected_email_flags = [:conversation_assignment]
+    setting.save!
+  end
+
+  def destroy_notification_setting
+    setting = user.notification_settings.new(account_id: account.id)
+    setting.destroy!
+  end
+end

app/models/user.rb

@@ -19,28 +19,20 @@
 #  remember_created_at    :datetime
 #  reset_password_sent_at :datetime
 #  reset_password_token   :string
-#  role                   :integer          default("agent")
 #  sign_in_count          :integer          default(0), not null
 #  tokens                 :json
 #  uid                    :string           default(""), not null
 #  unconfirmed_email      :string
 #  created_at             :datetime         not null
 #  updated_at             :datetime         not null
-#  account_id             :integer          not null
-#  inviter_id             :bigint
 #
 # Indexes
 #
 #  index_users_on_email                 (email)
-#  index_users_on_inviter_id            (inviter_id)
 #  index_users_on_pubsub_token          (pubsub_token) UNIQUE
 #  index_users_on_reset_password_token  (reset_password_token) UNIQUE
 #  index_users_on_uid_and_provider      (uid,provider) UNIQUE
 #
-# Foreign Keys
-#
-#  fk_rails_...  (inviter_id => users.id) ON DELETE => nullify
-#
 
 class User < ApplicationRecord
   # Include default devise modules.
@@ -62,25 +54,23 @@ class User < ApplicationRecord
   # The validation below has been commented out as it does not
   # work because :validatable in devise overrides this.
   # validates_uniqueness_of :email, scope: :account_id
-  validates :email, :name, :account_id, presence: true
+  validates :email, :name, presence: true
 
-  enum role: [:agent, :administrator]
-
-  belongs_to :account
-  belongs_to :inviter, class_name: 'User', required: false
-  has_many :invitees, class_name: 'User', foreign_key: 'inviter_id', dependent: :nullify
+  has_many :account_users, dependent: :destroy
+  has_many :accounts, through: :account_users
+  accepts_nested_attributes_for :account_users
 
   has_many :assigned_conversations, foreign_key: 'assignee_id', class_name: 'Conversation', dependent: :nullify
   has_many :inbox_members, dependent: :destroy
   has_many :assigned_inboxes, through: :inbox_members, source: :inbox
   has_many :messages
+  has_many :invitees, through: :account_users, class_name: 'User', foreign_key: 'inviter_id', dependent: :nullify
   has_many :notification_settings, dependent: :destroy
 
   before_validation :set_password_and_uid, on: :create
 
-  accepts_nested_attributes_for :account
+  after_create :notify_creation
 
-  after_create :notify_creation, :create_notification_setting
   after_destroy :notify_deletion
 
   def send_devise_notification(notification, *args)
@@ -91,6 +81,32 @@ class User < ApplicationRecord
     self.uid = email
   end
 
+  def account_user
+    # FIXME : temporary hack to transition over to multiple accounts per user
+    # We should be fetching the current account user relationship here.
+    account_users&.first
+  end
+
+  def account
+    account_user&.account
+  end
+
+  def administrator?
+    account_user&.administrator?
+  end
+
+  def agent?
+    account_user&.agent?
+  end
+
+  def role
+    account_user&.role
+  end
+
+  def inviter
+    account_user&.inviter
+  end
+
   def serializable_hash(options = nil)
     serialized_user = super(options).merge(confirmed: confirmed?)
     serialized_user.merge(subscription: account.try(:subscription).try(:summary)) if ENV['BILLING_ENABLED']
@@ -102,7 +118,7 @@ class User < ApplicationRecord
   end
 
   def create_notification_setting
-    setting = notification_settings.new(account_id: account_id)
+    setting = notification_settings.new(account_id: account.id)
     setting.selected_email_flags = [:conversation_assignment]
     setting.save!
   end

app/views/api/v1/agents/index.json.jbuilder

@@ -1,5 +1,5 @@
 json.array! @agents do |agent|
-  json.account_id agent.account_id
+  json.account_id agent.account.id
   json.availability_status agent.availability_status
   json.confirmed agent.confirmed?
   json.email agent.email

app/views/api/v1/models/user.json.jbuilder

@@ -0,0 +1,12 @@
+json.id resource.id
+json.provider resource.provider
+json.uid resource.uid
+json.name resource.name
+json.nickname resource.nickname
+json.email resource.email
+json.account_id resource.account.id
+json.pubsub_token resource.pubsub_token
+json.role resource.role
+json.inviter_id resource.account_user.inviter_id
+json.confirmed resource.confirmed?
+json.avatar_url resource.avatar_url

app/views/api/v1/profiles/update.json.jbuilder

@@ -4,7 +4,7 @@ json.uid @user.uid
 json.name @user.name
 json.nickname @user.nickname
 json.email @user.email
-json.account_id @user.account_id
+json.account_id @user.account.id
 json.pubsub_token @user.pubsub_token
 json.role @user.role
 json.confirmed @user.confirmed?

app/views/devise/auth.json.jbuilder

@@ -5,10 +5,10 @@ json.data do
   json.name @resource.name
   json.nickname @resource.nickname
   json.email @resource.email
-  json.account_id @resource.account_id
+  json.account_id @resource.account.id
   json.pubsub_token @resource.pubsub_token
-  json.role @resource.role
-  json.inviter_id @resource.inviter_id
+  json.role @resource.account_user.role
+  json.inviter_id @resource.account_user.inviter_id
   json.confirmed @resource.confirmed?
   json.avatar_url @resource.avatar_url
 end

app/views/devise/token.json.jbuilder

@@ -7,10 +7,10 @@ json.payload do
     json.name @resource.name
     json.nickname @resource.nickname
     json.email @resource.email
-    json.account_id @resource.account_id
+    json.account_id @resource.account.id
     json.pubsub_token @resource.pubsub_token
-    json.role @resource.role
-    json.inviter_id @resource.inviter_id
+    json.role @resource.account_user.role
+    json.inviter_id @resource.account_user.inviter_id
     json.confirmed @resource.confirmed?
     json.avatar_url @resource.avatar_url
   end

db/migrate/20200121190901_create_account_users.rb

@@ -0,0 +1,42 @@
+class CreateAccountUsers < ActiveRecord::Migration[6.0]
+  def change
+    create_table :account_users do |t|
+      t.references :account, foreign_key: true, index: true
+      t.references :user, foreign_key: true, index: true
+      t.integer :role, default: 0
+      t.bigint :inviter_id
+      t.timestamps
+    end
+
+    migrate_to_account_users
+
+    remove_column :users, :account_id, :bigint
+    remove_column :users, :role, :integer
+    remove_column :users, :inviter_id, :bigint
+  end
+
+  def migrate_to_account_users
+    ::User.find_in_batches.each do |users|
+      users.each do |user|
+        account_user = ::AccountUser.find_by(account_id: user.account_id, user_id: user.id, role: user.role)
+
+        notification_setting = ::NotificationSetting.find_by(account_id: user.account_id, user_id: user.id)
+        selected_email_flags = notification_setting.selected_email_flags
+        notification_setting.destroy!
+
+        next if account_user.present?
+
+        ::AccountUser.create!(
+          account_id: user.account_id,
+          user_id: user.id,
+          role: user[:role], # since we are overriding role method, lets fetch value from attribute
+          inviter_id: user.inviter_id
+        )
+
+        updated_notification_setting = ::NotificationSetting.find_by(account_id: user.account_id, user_id: user.id)
+        updated_notification_setting.selected_email_flags = selected_email_flags
+        updated_notification_setting.save!
+      end
+    end
+  end
+end

db/migrate/20200121220431_add_uniqueness_constraint_to_account_users.rb

@@ -0,0 +1,5 @@
+class AddUniquenessConstraintToAccountUsers < ActiveRecord::Migration[6.0]
+  def change
+    add_index :account_users, [:account_id, :user_id], unique: true, name: 'uniq_user_id_per_account_id'
+  end
+end

db/schema.rb

@@ -14,6 +14,18 @@ ActiveRecord::Schema.define(version: 20_200_226_194_012) do
   # These are extensions that must be enabled in order to support this database
   enable_extension 'plpgsql'
 
+  create_table 'account_users', force: :cascade do |t|
+    t.bigint 'account_id'
+    t.bigint 'user_id'
+    t.integer 'role', default: 0
+    t.bigint 'inviter_id'
+    t.datetime 'created_at', precision: 6, null: false
+    t.datetime 'updated_at', precision: 6, null: false
+    t.index %w[account_id user_id], name: 'uniq_user_id_per_account_id', unique: true
+    t.index ['account_id'], name: 'index_account_users_on_account_id'
+    t.index ['user_id'], name: 'index_account_users_on_user_id'
+  end
+
   create_table 'accounts', id: :serial, force: :cascade do |t|
     t.string 'name', null: false
     t.datetime 'created_at', null: false
@@ -230,11 +242,9 @@ ActiveRecord::Schema.define(version: 20_200_226_194_012) do
     t.index %w[taggable_id taggable_type context], name: 'index_taggings_on_taggable_id_and_taggable_type_and_context'
     t.index %w[taggable_id taggable_type tagger_id context], name: 'taggings_idy'
     t.index ['taggable_id'], name: 'index_taggings_on_taggable_id'
-    t.index %w[taggable_type taggable_id], name: 'index_taggings_on_taggable_type_and_taggable_id'
     t.index ['taggable_type'], name: 'index_taggings_on_taggable_type'
     t.index %w[tagger_id tagger_type], name: 'index_taggings_on_tagger_id_and_tagger_type'
     t.index ['tagger_id'], name: 'index_taggings_on_tagger_id'
-    t.index %w[tagger_type tagger_id], name: 'index_taggings_on_tagger_type_and_tagger_id'
   end
 
   create_table 'tags', id: :serial, force: :cascade do |t|
@@ -271,14 +281,10 @@ ActiveRecord::Schema.define(version: 20_200_226_194_012) do
     t.string 'nickname'
     t.string 'email'
     t.json 'tokens'
-    t.integer 'account_id', null: false
     t.datetime 'created_at', null: false
     t.datetime 'updated_at', null: false
     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 %w[uid provider], name: 'index_users_on_uid_and_provider', unique: true
@@ -293,10 +299,11 @@ ActiveRecord::Schema.define(version: 20_200_226_194_012) do
     t.integer 'webhook_type', default: 0
   end
 
+  add_foreign_key 'account_users', 'accounts'
+  add_foreign_key 'account_users', 'users'
   add_foreign_key 'active_storage_attachments', 'active_storage_blobs', column: 'blob_id'
   add_foreign_key 'contact_inboxes', 'contacts'
   add_foreign_key 'contact_inboxes', 'inboxes'
   add_foreign_key 'conversations', 'contact_inboxes'
   add_foreign_key 'messages', 'contacts'
-  add_foreign_key 'users', 'users', column: 'inviter_id', on_delete: :nullify
 end

db/seeds.rb

@@ -1,9 +1,15 @@
 account = Account.create!(name: 'Acme Inc')
 
-user = User.new(name: 'John', email: 'john@acme.inc', password: '123456', account: account, role: :administrator)
+user = User.new(name: 'John', email: 'john@acme.inc', password: '123456')
 user.skip_confirmation!
 user.save!
 
+AccountUser.create!(
+  account_id: account.id,
+  user_id: user.id,
+  role: :administrator
+)
+
 web_widget = Channel::WebWidget.create!(account: account, website_name: 'Acme', website_url: 'https://acme.inc')
 
 inbox = Inbox.create!(channel: web_widget, account: account, name: 'Acme Support')

spec/factories/account_users.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :account_user do
+    account
+    user
+    role { 'agent' }
+  end
+end

spec/factories/users.rb

@@ -4,6 +4,9 @@ FactoryBot.define do
   factory :user do
     transient do
       skip_confirmation { true }
+      role { 'agent' }
+      account { nil }
+      inviter { nil }
     end
 
     provider { 'email' }
@@ -11,12 +14,11 @@ FactoryBot.define do
     name { Faker::Name.name }
     nickname { Faker::Name.first_name }
     email { nickname + '@example.com' }
-    role { 'agent' }
     password { 'password' }
-    account
 
     after(:build) do |user, evaluator|
       user.skip_confirmation! if evaluator.skip_confirmation
+      create(:account_user, user: user, account: evaluator.account, role: evaluator.role, inviter: evaluator.inviter) if evaluator.account
     end
 
     trait :with_avatar do

spec/mailers/confirmation_instructions_spec.rb

@@ -4,7 +4,8 @@ require 'rails_helper'
 
 RSpec.describe 'Confirmation Instructions', type: :mailer do
   describe :notify do
-    let(:confirmable_user) { FactoryBot.build(:user, inviter: inviter_val) }
+    let(:account) { create(:account) }
+    let(:confirmable_user) { build(:user, inviter: inviter_val, account: account) }
     let(:inviter_val) { nil }
     let(:mail) { Devise::Mailer.confirmation_instructions(confirmable_user, nil, {}) }
 
@@ -23,9 +24,7 @@ RSpec.describe 'Confirmation Instructions', type: :mailer do
     end
 
     context 'when there is an inviter' do
-      let(:inviter_val) do
-        FactoryBot.create(:user, role: :administrator, skip_confirmation: true)
-      end
+      let(:inviter_val) { create(:user, :administrator, skip_confirmation: true, account: account) }
 
       it 'refers to the inviter and their account' do
         expect(mail.body).to match(

spec/models/account_spec.rb

@@ -5,7 +5,8 @@ require 'rails_helper'
 RSpec.describe Account do
   it { is_expected.to validate_presence_of(:name) }
 
-  it { is_expected.to have_many(:users).dependent(:destroy) }
+  it { is_expected.to have_many(:users).through(:account_users) }
+  it { is_expected.to have_many(:account_users) }
   it { is_expected.to have_many(:inboxes).dependent(:destroy) }
   it { is_expected.to have_many(:conversations).dependent(:destroy) }
   it { is_expected.to have_many(:contacts).dependent(:destroy) }

spec/models/account_user_spec.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe User do
+  let!(:account_user) { create(:account_user) }
+
+  describe 'notification_settings' do
+    it 'gets created with the right default settings' do
+      expect(account_user.user.notification_settings).not_to eq(nil)
+
+      expect(account_user.user.notification_settings.first.conversation_creation?).to eq(false)
+      expect(account_user.user.notification_settings.first.conversation_assignment?).to eq(true)
+    end
+  end
+end

spec/models/user_spec.rb

@@ -8,12 +8,11 @@ RSpec.describe User do
   context 'validations' do
     it { is_expected.to validate_presence_of(:email) }
     it { is_expected.to validate_presence_of(:name) }
-    it { is_expected.to validate_presence_of(:account_id) }
   end
 
   context 'associations' do
-    it { is_expected.to belong_to(:account) }
-    it { is_expected.to belong_to(:inviter).class_name('User').required(false) }
+    it { is_expected.to have_many(:accounts).through(:account_users) }
+    it { is_expected.to have_many(:account_users) }
     it { is_expected.to have_many(:assigned_conversations).class_name('Conversation').dependent(:nullify) }
     it { is_expected.to have_many(:inbox_members).dependent(:destroy) }
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
@@ -27,13 +26,4 @@ RSpec.describe User do
     it { expect(user.pubsub_token).not_to eq(nil) }
     it { expect(user.saved_changes.keys).not_to eq('pubsub_token') }
   end
-
-  describe 'notification_settings' do
-    it 'gets created with the right default settings' do
-      expect(user.notification_settings).not_to eq(nil)
-
-      expect(user.notification_settings.first.conversation_creation?).to eq(false)
-      expect(user.notification_settings.first.conversation_assignment?).to eq(true)
-    end
-  end
 end

spec/policies/contact_policy_spec.rb

@@ -5,8 +5,10 @@ require 'rails_helper'
 RSpec.describe ContactPolicy, type: :policy do
   subject(:contact_policy) { described_class }
 
-  let(:administrator) { create(:user, :administrator) }
-  let(:agent) { create(:user) }
+  let(:account) { create(:account) }
+
+  let(:administrator) { create(:user, :administrator, account: account) }
+  let(:agent) { create(:user, account: account) }
   let(:contact) { create(:contact) }
 
   permissions :index?, :show?, :update? do

spec/policies/inbox_policy_spec.rb

@@ -5,8 +5,10 @@ require 'rails_helper'
 RSpec.describe InboxPolicy, type: :policy do
   subject(:inbox_policy) { described_class }
 
-  let(:administrator) { create(:user, :administrator) }
-  let(:agent) { create(:user) }
+  let(:account) { create(:account) }
+
+  let(:administrator) { create(:user, :administrator, account: account) }
+  let(:agent) { create(:user, account: account) }
   let(:inbox) { create(:inbox) }
 
   permissions :create?, :destroy? do

spec/policies/user_policy_spec.rb

@@ -5,9 +5,11 @@ require 'rails_helper'
 RSpec.describe UserPolicy, type: :policy do
   subject(:user_policy) { described_class }
 
-  let(:administrator) { create(:user, :administrator) }
-  let(:agent) { create(:user) }
-  let(:user) { create(:user) }
+  let(:account) { create(:account) }
+
+  let(:administrator) { create(:user, :administrator, account: account) }
+  let(:agent) { create(:user, account: account) }
+  let(:user) { create(:user, account: account) }
 
   permissions :create?, :update?, :destroy? do
     context 'when administrator' do