mirror of https://github.com/docusealco/docuseal
CP-10294 add account groups (#19)
- Add account_groups table and model - Add account_group references to accounts, users, templates, template_folders - Make account_id nullable on users, templates, template_folders - Add controllers and specs * Consolidate account groups migrations - Replace 8 separate migrations with 2 consolidated ones - Create account groups and relationships in one migration - Make account_id columns nullable in second migration * this logic is being handled in external_auth_controller * remove unnecessary controllers * remove unnecessary routes * refactor account_group.default_template_folder * align method with Account version of this method * refactor controllers to move complex logic to service * move account/account group validation to concern * this method is not yet needed * we may implement this differently in next ticket to handle account and account group syncing for templates. * rubocop violation fixes * a few more refactors and add tests * Change external_account_group_id to integer type * Refactored external_account_group_id from string to integer in models, migrations, factories, and specs for consistency. * Merged account_id nullability changes into a single migration and removed the obsolete migrations. * Updated authentication logic to require either account or account_group presence for user activation.pull/544/head
Ryan Arakawa
3 months ago
committed by
GitHub
parent
c14f217812
commit
a1ed992ee4
@ -0,0 +1,36 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
# == Schema Information
|
||||||
|
#
|
||||||
|
# Table name: account_groups
|
||||||
|
#
|
||||||
|
# id :bigint not null, primary key
|
||||||
|
# name :string not null
|
||||||
|
# created_at :datetime not null
|
||||||
|
# updated_at :datetime not null
|
||||||
|
# external_account_group_id :integer not null
|
||||||
|
#
|
||||||
|
# Indexes
|
||||||
|
#
|
||||||
|
# index_account_groups_on_external_account_group_id (external_account_group_id) UNIQUE
|
||||||
|
#
|
||||||
|
class AccountGroup < ApplicationRecord
|
||||||
|
has_many :accounts, dependent: :nullify
|
||||||
|
has_many :users, dependent: :nullify
|
||||||
|
has_many :templates, dependent: :destroy
|
||||||
|
has_many :template_folders, dependent: :destroy
|
||||||
|
|
||||||
|
validates :external_account_group_id, presence: true, uniqueness: true
|
||||||
|
validates :name, presence: true
|
||||||
|
|
||||||
|
def self.find_or_create_by_external_id(external_id, attributes = {})
|
||||||
|
find_by(external_account_group_id: external_id) ||
|
||||||
|
create!(attributes.merge(external_account_group_id: external_id))
|
||||||
|
end
|
||||||
|
|
||||||
|
def default_template_folder
|
||||||
|
template_folders.find_by(name: TemplateFolder::DEFAULT_NAME) ||
|
||||||
|
template_folders.create!(name: TemplateFolder::DEFAULT_NAME,
|
||||||
|
author_id: users.minimum(:id))
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,19 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
module AccountGroupValidation
|
||||||
|
extend ActiveSupport::Concern
|
||||||
|
|
||||||
|
included do
|
||||||
|
validate :must_belong_to_account_or_account_group
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
|
def must_belong_to_account_or_account_group
|
||||||
|
if account.blank? && account_group.blank?
|
||||||
|
errors.add(:base, 'Must belong to either an account or account group')
|
||||||
|
elsif account.present? && account_group.present?
|
||||||
|
errors.add(:base, 'Cannot belong to both account and account group')
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,58 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class ExternalAuthService
|
||||||
|
def initialize(params)
|
||||||
|
@params = params
|
||||||
|
end
|
||||||
|
|
||||||
|
def authenticate_user
|
||||||
|
user = if @params[:account].present?
|
||||||
|
find_or_create_user_with_account
|
||||||
|
elsif @params[:account_group].present?
|
||||||
|
find_or_create_user_with_account_group
|
||||||
|
else
|
||||||
|
raise ArgumentError, 'Either account or account_group params must be provided'
|
||||||
|
end
|
||||||
|
|
||||||
|
user.access_token.token
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
|
def find_or_create_user_with_account
|
||||||
|
account = Account.find_or_create_by_external_id(
|
||||||
|
@params[:account][:external_id]&.to_i,
|
||||||
|
name: @params[:account][:name],
|
||||||
|
locale: @params[:account][:locale] || 'en-US',
|
||||||
|
timezone: @params[:account][:timezone] || 'UTC'
|
||||||
|
)
|
||||||
|
|
||||||
|
User.find_or_create_by_external_id(
|
||||||
|
account,
|
||||||
|
@params[:user][:external_id]&.to_i,
|
||||||
|
user_attributes
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
def find_or_create_user_with_account_group
|
||||||
|
account_group = AccountGroup.find_or_create_by_external_id(
|
||||||
|
@params[:account_group][:external_id],
|
||||||
|
name: @params[:account_group][:name]
|
||||||
|
)
|
||||||
|
|
||||||
|
User.find_or_create_by_external_group_id(
|
||||||
|
account_group,
|
||||||
|
@params[:user][:external_id]&.to_i,
|
||||||
|
user_attributes
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
def user_attributes
|
||||||
|
{
|
||||||
|
email: @params[:user][:email],
|
||||||
|
first_name: @params[:user][:first_name],
|
||||||
|
last_name: @params[:user][:last_name],
|
||||||
|
role: 'admin'
|
||||||
|
}
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,19 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class TemplateService
|
||||||
|
def initialize(template, user, params = {})
|
||||||
|
@template = template
|
||||||
|
@user = user
|
||||||
|
@params = params
|
||||||
|
end
|
||||||
|
|
||||||
|
def assign_ownership
|
||||||
|
if @user.account_group.present?
|
||||||
|
@template.account_group = @user.account_group
|
||||||
|
@template.folder = @user.account_group.default_template_folder
|
||||||
|
elsif @user.account.present?
|
||||||
|
@template.account = @user.account
|
||||||
|
@template.folder = TemplateFolders.find_or_create_by_name(@user, @params[:folder_name])
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,21 @@
|
|||||||
|
class CreateAccountGroupsAndAddRelationships < ActiveRecord::Migration[8.0]
|
||||||
|
def change
|
||||||
|
create_table :account_groups do |t|
|
||||||
|
t.integer :external_account_group_id, null: false
|
||||||
|
t.string :name, null: false
|
||||||
|
|
||||||
|
t.timestamps
|
||||||
|
end
|
||||||
|
add_index :account_groups, :external_account_group_id, unique: true
|
||||||
|
|
||||||
|
add_reference :accounts, :account_group, foreign_key: true
|
||||||
|
add_reference :templates, :account_group, foreign_key: true
|
||||||
|
add_reference :users, :account_group, foreign_key: true
|
||||||
|
add_reference :template_folders, :account_group, foreign_key: true
|
||||||
|
|
||||||
|
# Make account_ids nullable since records can belong to either account or account_group
|
||||||
|
change_column_null :templates, :account_id, true
|
||||||
|
change_column_null :users, :account_id, true
|
||||||
|
change_column_null :template_folders, :account_id, true
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,8 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
FactoryBot.define do
|
||||||
|
factory :account_group do
|
||||||
|
external_account_group_id { Faker::Number.unique.number(digits: 8) }
|
||||||
|
name { Faker::Company.name }
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,56 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
# == Schema Information
|
||||||
|
#
|
||||||
|
# Table name: account_groups
|
||||||
|
#
|
||||||
|
# id :bigint not null, primary key
|
||||||
|
# name :string not null
|
||||||
|
# created_at :datetime not null
|
||||||
|
# updated_at :datetime not null
|
||||||
|
# external_account_group_id :integer not null
|
||||||
|
#
|
||||||
|
# Indexes
|
||||||
|
#
|
||||||
|
# index_account_groups_on_external_account_group_id (external_account_group_id) UNIQUE
|
||||||
|
#
|
||||||
|
describe AccountGroup do
|
||||||
|
let(:account_group) { create(:account_group) }
|
||||||
|
|
||||||
|
describe 'associations' do
|
||||||
|
it 'has many accounts' do
|
||||||
|
expect(account_group).to respond_to(:accounts)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe 'validations' do
|
||||||
|
it 'validates presence of external_account_group_id' do
|
||||||
|
account_group = build(:account_group, external_account_group_id: nil)
|
||||||
|
expect(account_group).not_to be_valid
|
||||||
|
expect(account_group.errors[:external_account_group_id]).to include("can't be blank")
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'validates uniqueness of external_account_group_id' do
|
||||||
|
create(:account_group, external_account_group_id: 123)
|
||||||
|
duplicate = build(:account_group, external_account_group_id: 123)
|
||||||
|
expect(duplicate).not_to be_valid
|
||||||
|
expect(duplicate.errors[:external_account_group_id]).to include('has already been taken')
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'validates presence of name' do
|
||||||
|
account_group = build(:account_group, name: nil)
|
||||||
|
expect(account_group).not_to be_valid
|
||||||
|
expect(account_group.errors[:name]).to include("can't be blank")
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe 'when account group is destroyed' do
|
||||||
|
it 'nullifies accounts account_group_id' do
|
||||||
|
account = create(:account, account_group: account_group)
|
||||||
|
|
||||||
|
account_group.destroy
|
||||||
|
|
||||||
|
expect(account.reload.account_group).to be_nil
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,38 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
require 'rails_helper'
|
||||||
|
|
||||||
|
RSpec.describe AccountGroupValidation do
|
||||||
|
# Test with User model since it includes the concern
|
||||||
|
describe 'validation' do
|
||||||
|
context 'with account only' do
|
||||||
|
it 'is valid' do
|
||||||
|
user = build(:user, account: create(:account), account_group: nil)
|
||||||
|
expect(user).to be_valid
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with account_group only' do
|
||||||
|
it 'is valid' do
|
||||||
|
user = build(:user, account: nil, account_group: create(:account_group))
|
||||||
|
expect(user).to be_valid
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with neither account nor account_group' do
|
||||||
|
it 'is invalid' do
|
||||||
|
user = build(:user, account: nil, account_group: nil)
|
||||||
|
expect(user).not_to be_valid
|
||||||
|
expect(user.errors[:base]).to include('Must belong to either an account or account group')
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with both account and account_group' do
|
||||||
|
it 'is invalid' do
|
||||||
|
user = build(:user, account: create(:account), account_group: create(:account_group))
|
||||||
|
expect(user).not_to be_valid
|
||||||
|
expect(user.errors[:base]).to include('Cannot belong to both account and account group')
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,68 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
require 'rails_helper'
|
||||||
|
|
||||||
|
RSpec.describe ExternalAuthService do
|
||||||
|
describe '#authenticate_user' do
|
||||||
|
let(:user_params) do
|
||||||
|
{
|
||||||
|
external_id: 123,
|
||||||
|
email: 'test@example.com',
|
||||||
|
first_name: 'John',
|
||||||
|
last_name: 'Doe'
|
||||||
|
}
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with account params' do
|
||||||
|
let(:params) do
|
||||||
|
{
|
||||||
|
account: { external_id: 456, name: 'Test Account' },
|
||||||
|
user: user_params
|
||||||
|
}
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns access token for new account and user' do
|
||||||
|
token = described_class.new(params).authenticate_user
|
||||||
|
|
||||||
|
expect(token).to be_present
|
||||||
|
expect(Account.last.external_account_id).to eq(456)
|
||||||
|
expect(User.last.external_user_id).to eq(123)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns access token for existing user' do
|
||||||
|
account = create(:account, external_account_id: 456)
|
||||||
|
user = create(:user, account: account, external_user_id: 123)
|
||||||
|
|
||||||
|
token = described_class.new(params).authenticate_user
|
||||||
|
|
||||||
|
expect(token).to eq(user.access_token.token)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with account_group params' do
|
||||||
|
let(:params) do
|
||||||
|
{
|
||||||
|
account_group: { external_id: 789, name: 'Test Group' },
|
||||||
|
user: user_params
|
||||||
|
}
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns access token for new account_group and user' do
|
||||||
|
token = described_class.new(params).authenticate_user
|
||||||
|
|
||||||
|
expect(token).to be_present
|
||||||
|
expect(AccountGroup.last.external_account_group_id).to eq(789)
|
||||||
|
expect(User.last.external_user_id).to eq(123)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with invalid params' do
|
||||||
|
it 'raises error when neither account nor account_group provided' do
|
||||||
|
params = { user: user_params }
|
||||||
|
|
||||||
|
expect { described_class.new(params).authenticate_user }
|
||||||
|
.to raise_error(ArgumentError, 'Either account or account_group params must be provided')
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
@ -0,0 +1,48 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
require 'rails_helper'
|
||||||
|
|
||||||
|
RSpec.describe TemplateService do
|
||||||
|
describe '#assign_ownership' do
|
||||||
|
let(:template) { build(:template, account: nil, account_group: nil) }
|
||||||
|
let(:params) { { folder_name: 'Custom Folder' } }
|
||||||
|
|
||||||
|
context 'with account_group user' do
|
||||||
|
let(:account_group) { create(:account_group) }
|
||||||
|
let(:user) { create(:user, account: nil, account_group: account_group) }
|
||||||
|
|
||||||
|
it 'assigns account_group and default folder' do
|
||||||
|
service = described_class.new(template, user, params)
|
||||||
|
service.assign_ownership
|
||||||
|
|
||||||
|
expect(template.account_group).to eq(account_group)
|
||||||
|
expect(template.folder).to eq(account_group.default_template_folder)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with account user' do
|
||||||
|
let(:account) { create(:account) }
|
||||||
|
let(:user) { create(:user, account: account, account_group: nil) }
|
||||||
|
|
||||||
|
it 'assigns account and finds/creates folder' do
|
||||||
|
service = described_class.new(template, user, params)
|
||||||
|
service.assign_ownership
|
||||||
|
|
||||||
|
expect(template.account).to eq(account)
|
||||||
|
expect(template.folder).to be_present
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with user having neither account nor account_group' do
|
||||||
|
let(:user) { build(:user, account: nil, account_group: nil) }
|
||||||
|
|
||||||
|
it 'does not assign ownership' do
|
||||||
|
service = described_class.new(template, user, params)
|
||||||
|
service.assign_ownership
|
||||||
|
|
||||||
|
expect(template.account).to be_nil
|
||||||
|
expect(template.account_group).to be_nil
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
Loading…
Reference in new issue