From 07ea9694a3906368809e62d4617cd622068248bb Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Fri, 2 Feb 2024 16:10:45 +0530 Subject: [PATCH] feat: new accounts controller for signup+onboarding (#8804) * feat: add v2 accounts controller * feat: allow empty account and user name * feat: ensure and is present for v1 signup * test: remove validation checks * chore: apply suggestions * chore: revert en.yml formatting * chore: line at EOF * fix: routes --------- Co-authored-by: Muhsin Keloth --- app/builders/account_builder.rb | 16 ++- app/controllers/api/v1/accounts_controller.rb | 13 +++ app/controllers/api/v2/accounts_controller.rb | 48 ++++++++ app/models/account.rb | 1 - app/models/user.rb | 3 +- config/locales/en.yml | 1 + config/routes.rb | 22 ++-- lib/custom_exceptions/account.rb | 6 + .../api/v1/accounts_controller_spec.rb | 5 +- .../api/v2/accounts_controller_spec.rb | 104 ++++++++++++++++++ spec/models/account_spec.rb | 1 - spec/models/user_spec.rb | 2 - 12 files changed, 200 insertions(+), 22 deletions(-) create mode 100644 app/controllers/api/v2/accounts_controller.rb create mode 100644 spec/controllers/api/v2/accounts_controller_spec.rb diff --git a/app/builders/account_builder.rb b/app/builders/account_builder.rb index 3e2ac9d6e..677041449 100644 --- a/app/builders/account_builder.rb +++ b/app/builders/account_builder.rb @@ -2,7 +2,7 @@ class AccountBuilder include CustomExceptions::Account - pattr_initialize [:account_name!, :email!, :confirmed, :user, :user_full_name, :user_password, :super_admin, :locale] + pattr_initialize [:account_name, :email!, :confirmed, :user, :user_full_name, :user_password, :super_admin, :locale] def perform if @user.nil? @@ -21,6 +21,16 @@ class AccountBuilder private + def user_full_name + # the empty string ensures that not-null constraint is not violated + @user_full_name || '' + end + + def account_name + # the empty string ensures that not-null constraint is not violated + @account_name || '' + end + def validate_email address = ValidEmail2::Address.new(@email) if address.valid? # && !address.disposable? @@ -39,7 +49,7 @@ class AccountBuilder end def create_account - @account = Account.create!(name: @account_name, locale: I18n.locale) + @account = Account.create!(name: account_name, locale: I18n.locale) Current.account = @account end @@ -64,7 +74,7 @@ class AccountBuilder @user = User.new(email: @email, password: user_password, password_confirmation: user_password, - name: @user_full_name) + name: user_full_name) @user.type = 'SuperAdmin' if @super_admin @user.confirm if @confirmed @user.save! diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index ef0e0c777..b1d31cfaf 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -5,11 +5,13 @@ class Api::V1::AccountsController < Api::BaseController skip_before_action :authenticate_user!, :set_current_user, :handle_with_exception, only: [:create], raise: false before_action :check_signup_enabled, only: [:create] + before_action :ensure_account_name, only: [:create] before_action :validate_captcha, only: [:create] before_action :fetch_account, except: [:create] before_action :check_authorization, except: [:create] rescue_from CustomExceptions::Account::InvalidEmail, + CustomExceptions::Account::InvalidParams, CustomExceptions::Account::UserExists, CustomExceptions::Account::UserErrors, with: :render_error_response @@ -53,6 +55,17 @@ class Api::V1::AccountsController < Api::BaseController private + def ensure_account_name + # ensure that account_name and user_full_name is present + # this is becuase the account builder and the models validations are not triggered + # this change is to align the behaviour with the v2 accounts controller + # since these values are not required directly there + return if account_params[:account_name].present? + return if account_params[:user_full_name].present? + + raise CustomExceptions::Account::InvalidParams.new({}) + end + def get_cache_keys { label: fetch_value_for_key(params[:id], Label.name.underscore), diff --git a/app/controllers/api/v2/accounts_controller.rb b/app/controllers/api/v2/accounts_controller.rb new file mode 100644 index 000000000..e38863c07 --- /dev/null +++ b/app/controllers/api/v2/accounts_controller.rb @@ -0,0 +1,48 @@ +class Api::V2::AccountsController < Api::BaseController + include AuthHelper + + skip_before_action :authenticate_user!, :set_current_user, :handle_with_exception, + only: [:create], raise: false + before_action :check_signup_enabled, only: [:create] + before_action :validate_captcha, only: [:create] + before_action :fetch_account, except: [:create] + before_action :check_authorization, except: [:create] + + rescue_from CustomExceptions::Account::InvalidEmail, + CustomExceptions::Account::UserExists, + CustomExceptions::Account::UserErrors, + with: :render_error_response + + def create + @user, @account = AccountBuilder.new( + email: account_params[:email], + user_password: account_params[:password], + user: current_user + ).perform + if @user + send_auth_headers(@user) + render 'api/v1/accounts/create', format: :json, locals: { resource: @user } + else + render_error_response(CustomExceptions::Account::SignupFailed.new({})) + end + end + + private + + def fetch_account + @account = current_user.accounts.find(params[:id]) + @current_account_user = @account.account_users.find_by(user_id: current_user.id) + end + + def account_params + params.permit(:account_name, :email, :name, :password, :locale, :domain, :support_email, :auto_resolve_duration, :user_full_name) + end + + def check_signup_enabled + raise ActionController::RoutingError, 'Not Found' if GlobalConfigService.load('ENABLE_ACCOUNT_SIGNUP', 'false') == 'false' + end + + def validate_captcha + raise ActionController::InvalidAuthenticityToken, 'Invalid Captcha' unless ChatwootCaptcha.new(params[:h_captcha_client_response]).valid? + end +end diff --git a/app/models/account.rb b/app/models/account.rb index c1a5773bb..38c3cf53b 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -32,7 +32,6 @@ class Account < ApplicationRecord check_for_column: false }.freeze - validates :name, presence: true validates :auto_resolve_duration, numericality: { greater_than_or_equal_to: 1, less_than_or_equal_to: 999, allow_nil: true } validates :domain, length: { maximum: 100 } diff --git a/app/models/user.rb b/app/models/user.rb index 2870983a4..77471e693 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -68,8 +68,7 @@ class User < ApplicationRecord # work because :validatable in devise overrides this. # validates_uniqueness_of :email, scope: :account_id - validates :email, :name, presence: true - validates_length_of :name, minimum: 1, maximum: 255 + validates :email, presence: true has_many :account_users, dependent: :destroy_async has_many :accounts, through: :account_users diff --git a/config/locales/en.yml b/config/locales/en.yml index e6629ae8d..da4f35032 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -45,6 +45,7 @@ en: disposable_email: We do not allow disposable emails invalid_email: You have entered an invalid email email_already_exists: "You have already signed up for an account with %{email}" + invalid_params: 'Invalid, please check the signup paramters and try again' failed: Signup failed data_import: data_type: diff --git a/config/routes.rb b/config/routes.rb index 70b19f504..0c4b0459b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -288,16 +288,18 @@ Rails.application.routes.draw do end namespace :v2 do - resources :accounts, only: [], module: :accounts do - resources :reports, only: [:index] do - collection do - get :summary - get :agents - get :inboxes - get :labels - get :teams - get :conversations - get :conversation_traffic + resources :accounts, only: [:create] do + scope module: :accounts do + resources :reports, only: [:index] do + collection do + get :summary + get :agents + get :inboxes + get :labels + get :teams + get :conversations + get :conversation_traffic + end end end end diff --git a/lib/custom_exceptions/account.rb b/lib/custom_exceptions/account.rb index bd77c5a78..efacc9e6e 100644 --- a/lib/custom_exceptions/account.rb +++ b/lib/custom_exceptions/account.rb @@ -17,6 +17,12 @@ module CustomExceptions::Account end end + class InvalidParams < CustomExceptions::Base + def message + I18n.t 'errors.signup.invalid_params' + end + end + class UserErrors < CustomExceptions::Base def message @data[:errors].full_messages.join(',') diff --git a/spec/controllers/api/v1/accounts_controller_spec.rb b/spec/controllers/api/v1/accounts_controller_spec.rb index ce5f9623c..060e241a7 100644 --- a/spec/controllers/api/v1/accounts_controller_spec.rb +++ b/spec/controllers/api/v1/accounts_controller_spec.rb @@ -61,10 +61,9 @@ RSpec.describe 'Accounts API', type: :request do params: params, as: :json - expect(AccountBuilder).to have_received(:new).with(params.merge(user_password: params[:password])) - expect(account_builder).to have_received(:perform) + expect(AccountBuilder).not_to have_received(:new) expect(response).to have_http_status(:forbidden) - expect(response.body).to eq({ message: I18n.t('errors.signup.failed') }.to_json) + expect(response.body).to eq({ message: I18n.t('errors.signup.invalid_params') }.to_json) end end end diff --git a/spec/controllers/api/v2/accounts_controller_spec.rb b/spec/controllers/api/v2/accounts_controller_spec.rb new file mode 100644 index 000000000..e0181a282 --- /dev/null +++ b/spec/controllers/api/v2/accounts_controller_spec.rb @@ -0,0 +1,104 @@ +require 'rails_helper' + +RSpec.describe 'Accounts API', type: :request do + describe 'POST /api/v2/accounts' do + let(:email) { Faker::Internet.email } + + context 'when posting to accounts with correct parameters' do + let(:account_builder) { double } + let(:account) { create(:account) } + let(:user) { create(:user, email: email, account: account) } + + before do + allow(AccountBuilder).to receive(:new).and_return(account_builder) + end + + it 'calls account builder' do + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true' do + allow(account_builder).to receive(:perform).and_return([user, account]) + + params = { email: email, user: nil, password: 'Password1!' } + + post api_v2_accounts_url, + params: params, + as: :json + + expect(AccountBuilder).to have_received(:new).with(params.except(:password).merge(user_password: params[:password])) + expect(account_builder).to have_received(:perform) + expect(response.headers.keys).to include('access-token', 'token-type', 'client', 'expiry', 'uid') + expect(response.body).to include('en') + end + end + + it 'calls ChatwootCaptcha' do + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true' do + captcha = double + allow(account_builder).to receive(:perform).and_return([user, account]) + allow(ChatwootCaptcha).to receive(:new).and_return(captcha) + allow(captcha).to receive(:valid?).and_return(true) + + params = { email: email, user: nil, password: 'Password1!', h_captcha_client_response: '123' } + + post api_v2_accounts_url, + params: params, + as: :json + + expect(ChatwootCaptcha).to have_received(:new).with('123') + expect(response.headers.keys).to include('access-token', 'token-type', 'client', 'expiry', 'uid') + expect(response.body).to include('en') + end + end + + it 'renders error response on invalid params' do + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true' do + allow(account_builder).to receive(:perform).and_return(nil) + + params = { email: nil, user: nil } + + post api_v2_accounts_url, + params: params, + as: :json + + expect(AccountBuilder).to have_received(:new).with(params.merge(user_password: params[:password])) + expect(account_builder).to have_received(:perform) + expect(response).to have_http_status(:forbidden) + expect(response.body).to eq({ message: I18n.t('errors.signup.failed') }.to_json) + end + end + end + + context 'when ENABLE_ACCOUNT_SIGNUP env variable is set to false' do + it 'responds 404 on requests' do + params = { email: email } + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'false' do + post api_v2_accounts_url, + params: params, + as: :json + + expect(response).to have_http_status(:not_found) + end + end + end + + context 'when ENABLE_ACCOUNT_SIGNUP env variable is set to api_only' do + let(:account_builder) { double } + let(:account) { create(:account) } + let(:user) { create(:user, email: email, account: account) } + + it 'does not respond 404 on requests' do + allow(AccountBuilder).to receive(:new).and_return(account_builder) + allow(account_builder).to receive(:perform).and_return([user, account]) + + params = { email: email, user: nil, password: 'Password1!' } + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'api_only' do + post api_v2_accounts_url, + params: params, + as: :json + + expect(AccountBuilder).to have_received(:new).with(params.except(:password).merge(user_password: params[:password])) + expect(response).to have_http_status(:success) + end + end + end + end +end diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 86a7e6a5d..8f9ba7d2c 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -3,7 +3,6 @@ require 'rails_helper' RSpec.describe Account do - it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_numericality_of(:auto_resolve_duration).is_greater_than_or_equal_to(1) } it { is_expected.to validate_numericality_of(:auto_resolve_duration).is_less_than_or_equal_to(999) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f38b3db0b..5dd3fc3c3 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -9,8 +9,6 @@ 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_length_of(:name).is_at_least(1) } end context 'associations' do