From d883d5d58e24a566065195522de6fc555279abb6 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma Date: Wed, 20 Aug 2025 16:10:00 +0530 Subject: [PATCH] self review changes --- db/schema.rb | 5 - .../v1/accounts/leave_records_controller.rb | 2 +- enterprise/app/models/captain/document.rb | 5 + .../app/models/enterprise/concerns/user.rb | 1 - enterprise/app/models/leave_record.rb | 18 +- .../app/policies/leave_record_policy.rb | 4 +- .../api/v1/models/_leave_record.json.jbuilder | 12 +- .../accounts/leave_records_controller_spec.rb | 399 +++++------------- spec/enterprise/models/leave_record_spec.rb | 361 +++++----------- .../policies/leave_record_policy_spec.rb | 222 +++------- 10 files changed, 299 insertions(+), 730 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 3d62e5416..aa669a892 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -320,14 +320,9 @@ ActiveRecord::Schema[7.1].define(version: 2025_08_20_083315) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "status", default: 0, null: false - t.string "content_type" - t.bigint "file_size" - t.jsonb "metadata", default: {} t.index ["account_id"], name: "index_captain_documents_on_account_id" t.index ["assistant_id", "external_link"], name: "index_captain_documents_on_assistant_id_and_external_link", unique: true t.index ["assistant_id"], name: "index_captain_documents_on_assistant_id" - t.index ["content_type"], name: "index_captain_documents_on_content_type" - t.index ["metadata"], name: "index_captain_documents_on_metadata", using: :gin t.index ["status"], name: "index_captain_documents_on_status" end diff --git a/enterprise/app/controllers/api/v1/accounts/leave_records_controller.rb b/enterprise/app/controllers/api/v1/accounts/leave_records_controller.rb index b2e724516..13506d1b4 100644 --- a/enterprise/app/controllers/api/v1/accounts/leave_records_controller.rb +++ b/enterprise/app/controllers/api/v1/accounts/leave_records_controller.rb @@ -1,6 +1,6 @@ class Api::V1::Accounts::LeaveRecordsController < Api::V1::Accounts::EnterpriseAccountsController - before_action :fetch_leave_record, only: [:show, :update, :destroy, :approve, :reject] before_action :check_authorization + before_action :fetch_leave_record, only: [:show, :update, :destroy, :approve, :reject] before_action :ensure_pending_status, only: [:update] before_action :ensure_can_be_cancelled, only: [:destroy] diff --git a/enterprise/app/models/captain/document.rb b/enterprise/app/models/captain/document.rb index d2a02f5b5..445062c1e 100644 --- a/enterprise/app/models/captain/document.rb +++ b/enterprise/app/models/captain/document.rb @@ -4,7 +4,10 @@ # # id :bigint not null, primary key # content :text +# content_type :string # external_link :string not null +# file_size :bigint +# metadata :jsonb # name :string # status :integer default("in_progress"), not null # created_at :datetime not null @@ -17,6 +20,8 @@ # index_captain_documents_on_account_id (account_id) # index_captain_documents_on_assistant_id (assistant_id) # index_captain_documents_on_assistant_id_and_external_link (assistant_id,external_link) UNIQUE +# index_captain_documents_on_content_type (content_type) +# index_captain_documents_on_metadata (metadata) USING gin # index_captain_documents_on_status (status) # class Captain::Document < ApplicationRecord diff --git a/enterprise/app/models/enterprise/concerns/user.rb b/enterprise/app/models/enterprise/concerns/user.rb index 1721ee315..ba4c6b876 100644 --- a/enterprise/app/models/enterprise/concerns/user.rb +++ b/enterprise/app/models/enterprise/concerns/user.rb @@ -7,7 +7,6 @@ module Enterprise::Concerns::User has_many :captain_responses, class_name: 'Captain::AssistantResponse', dependent: :nullify, as: :documentable has_many :copilot_threads, dependent: :destroy_async has_many :leave_records, dependent: :destroy_async - has_many :approved_leave_records, class_name: 'LeaveRecord', foreign_key: 'approved_by_id', dependent: :nullify, inverse_of: :approver end def ensure_installation_pricing_plan_quantity diff --git a/enterprise/app/models/leave_record.rb b/enterprise/app/models/leave_record.rb index efb83eeaf..40dc06d96 100644 --- a/enterprise/app/models/leave_record.rb +++ b/enterprise/app/models/leave_record.rb @@ -25,7 +25,7 @@ class LeaveRecord < ApplicationRecord belongs_to :account belongs_to :user - belongs_to :approver, class_name: 'User', foreign_key: 'approved_by_id', optional: true, inverse_of: :approved_leave_records + belongs_to :approved_by, class_name: 'User', optional: true enum leave_type: { annual: 0, @@ -50,7 +50,7 @@ class LeaveRecord < ApplicationRecord validates :leave_type, :status, presence: true validate :end_date_after_start_date validate :future_dates_for_pending_leaves - validate :approver_is_admin + validate :approved_by_is_admin scope :for_account, ->(account_id) { where(account_id: account_id) } scope :for_user, ->(user_id) { where(user_id: user_id) } @@ -58,12 +58,12 @@ class LeaveRecord < ApplicationRecord scope :by_leave_type, ->(leave_type) { where(leave_type: leave_type) } scope :in_date_range, ->(start_date, end_date) { where('start_date <= ? AND end_date >= ?', end_date, start_date) } - def approve!(approver) - update!(status: :approved, approved_by_id: approver.id, approved_at: Time.current) + def approve!(approved_by_user) + update!(status: :approved, approved_by_id: approved_by_user.id, approved_at: Time.current) end - def reject!(approver) - update!(status: :rejected, approved_by_id: approver.id, approved_at: Time.current) + def reject!(approved_by_user) + update!(status: :rejected, approved_by_id: approved_by_user.id, approved_at: Time.current) end def duration_in_days @@ -96,10 +96,10 @@ class LeaveRecord < ApplicationRecord errors.add(:start_date, 'must be in the future') if start_date && start_date <= Date.current end - def approver_is_admin - return unless approved_by_id && approver + def approved_by_is_admin + return unless approved_by_id && approved_by - account_user = account.account_users.find_by(user: approver) + account_user = account.account_users.find_by(user: approved_by) errors.add(:approved_by, 'must be an administrator') unless account_user&.administrator? end end diff --git a/enterprise/app/policies/leave_record_policy.rb b/enterprise/app/policies/leave_record_policy.rb index 8fd8ad0a3..8ad897db2 100644 --- a/enterprise/app/policies/leave_record_policy.rb +++ b/enterprise/app/policies/leave_record_policy.rb @@ -40,9 +40,9 @@ class LeaveRecordPolicy < ApplicationPolicy def resolve if @account_user.administrator? - scope.includes(:user, :approver) + scope.includes(:user, :approved_by) else - scope.where(user: @user).includes(:user, :approver) + scope.where(user: @user).includes(:user, :approved_by) end end end diff --git a/enterprise/app/views/api/v1/models/_leave_record.json.jbuilder b/enterprise/app/views/api/v1/models/_leave_record.json.jbuilder index 80e427196..bcc7b9881 100644 --- a/enterprise/app/views/api/v1/models/_leave_record.json.jbuilder +++ b/enterprise/app/views/api/v1/models/_leave_record.json.jbuilder @@ -15,12 +15,12 @@ json.user do json.email leave_record.user.email end -if leave_record.approver.present? - json.approver do - json.id leave_record.approver.id - json.name leave_record.approver.name - json.email leave_record.approver.email +if leave_record.approved_by.present? + json.approved_by do + json.id leave_record.approved_by.id + json.name leave_record.approved_by.name + json.email leave_record.approved_by.email end else - json.approver nil + json.approved_by nil end diff --git a/spec/enterprise/controllers/api/v1/accounts/leave_records_controller_spec.rb b/spec/enterprise/controllers/api/v1/accounts/leave_records_controller_spec.rb index 0d55ce2c7..c10278eb0 100644 --- a/spec/enterprise/controllers/api/v1/accounts/leave_records_controller_spec.rb +++ b/spec/enterprise/controllers/api/v1/accounts/leave_records_controller_spec.rb @@ -2,322 +2,149 @@ require 'rails_helper' RSpec.describe 'LeaveRecords API', type: :request do let(:account) { create(:account) } - let(:administrator) { create(:user, account: account, role: :administrator) } + let(:admin) { create(:user, account: account, role: :administrator) } let(:agent) { create(:user, account: account, role: :agent) } - let(:other_user) { create(:user, account: account, role: :agent) } + let(:other_agent) { create(:user, account: account, role: :agent) } - describe 'GET #index' do - before do - create(:leave_record, account: account, user: agent) - create(:leave_record, account: account, user: other_user) + describe 'GET /api/v1/accounts/:account_id/leave_records' do + let!(:agent_leave) { create(:leave_record, account: account, user: agent) } + let!(:other_leave) { create(:leave_record, account: account, user: other_agent) } + + it 'allows admins to see all leave records' do + get "/api/v1/accounts/#{account.id}/leave_records", + headers: admin.create_new_auth_token + + expect(response).to have_http_status(:success) + expect(json_response.length).to eq(2) end - context 'when authenticated as administrator' do - it 'returns all leave records in the account' do - get "/api/v1/accounts/#{account.id}/leave_records", - headers: administrator.create_new_auth_token + it 'allows agents to see only their own leave records' do + get "/api/v1/accounts/#{account.id}/leave_records", + headers: agent.create_new_auth_token - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body.length).to eq(2) - end - end - - context 'when authenticated as agent' do - it 'returns only own leave records' do - get "/api/v1/accounts/#{account.id}/leave_records", - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body.length).to eq(1) - expect(body[0]['user']['id']).to eq(agent.id) - end - end - - context 'when unauthenticated' do - it 'returns unauthorized' do - get "/api/v1/accounts/#{account.id}/leave_records" - - expect(response).to have_http_status(:unauthorized) - end + expect(response).to have_http_status(:success) + expect(json_response.length).to eq(1) + expect(json_response.first['user']['id']).to eq(agent.id) end end - describe 'GET #show' do - let(:leave_record) { create(:leave_record, account: account, user: agent) } - let(:other_leave_record) { create(:leave_record, account: account, user: other_user) } - - context 'when authenticated as administrator' do - it 'shows any leave record in the account' do - get "/api/v1/accounts/#{account.id}/leave_records/#{other_leave_record.id}", - headers: administrator.create_new_auth_token - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body['id']).to eq(other_leave_record.id) - expect(body['user']['id']).to eq(other_user.id) - end - end - - context 'when authenticated as agent' do - it 'shows own leave record' do - get "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}", - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body['id']).to eq(leave_record.id) - expect(body['user']['id']).to eq(agent.id) - end - - it 'denies access to other user leave record' do - get "/api/v1/accounts/#{account.id}/leave_records/#{other_leave_record.id}", - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:not_found) - end - end - end - - describe 'POST #create' do + describe 'POST /api/v1/accounts/:account_id/leave_records' do let(:valid_params) do { leave_record: { start_date: 1.week.from_now.to_date, end_date: 2.weeks.from_now.to_date, leave_type: 'annual', - reason: 'Vacation time' + reason: 'Family vacation' } } end - context 'when authenticated as agent' do - it 'creates a leave record for the current user' do - expect do - post "/api/v1/accounts/#{account.id}/leave_records", - params: valid_params, - headers: agent.create_new_auth_token - end.to change(LeaveRecord, :count).by(1) - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body['user']['id']).to eq(agent.id) - expect(body['leave_type']).to eq('annual') - expect(body['status']).to eq('pending') - end - - it 'returns validation errors for invalid data' do - invalid_params = valid_params.deep_merge( - leave_record: { start_date: nil } - ) - + it 'creates a leave record for the current user' do + expect do post "/api/v1/accounts/#{account.id}/leave_records", - params: invalid_params, + params: valid_params, + headers: agent.create_new_auth_token + end.to change(LeaveRecord, :count).by(1) + + expect(response).to have_http_status(:success) + expect(json_response['user']['id']).to eq(agent.id) + expect(json_response['status']).to eq('pending') + end + + it 'fails with invalid params' do + post "/api/v1/accounts/#{account.id}/leave_records", + params: { leave_record: { start_date: nil } }, + headers: agent.create_new_auth_token + + expect(response).to have_http_status(:unprocessable_entity) + end + end + + describe 'PATCH /api/v1/accounts/:account_id/leave_records/:id' do + let(:leave_record) { create(:leave_record, account: account, user: agent, status: :pending) } + let(:update_params) { { leave_record: { reason: 'Updated reason' } } } + + it 'allows updating pending leave records' do + patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}", + params: update_params, + headers: agent.create_new_auth_token + + expect(response).to have_http_status(:success) + expect(json_response['reason']).to eq('Updated reason') + end + + it 'prevents updating approved leave records' do + leave_record.update!(status: :approved) + + patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}", + params: update_params, + headers: agent.create_new_auth_token + + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response['error']).to eq(I18n.t('errors.leave_records.cannot_update_non_pending')) + end + end + + describe 'DELETE /api/v1/accounts/:account_id/leave_records/:id' do + let!(:leave_record) { create(:leave_record, account: account, user: agent, status: :pending) } + + it 'allows deleting cancellable leave records' do + expect do + delete "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}", + headers: agent.create_new_auth_token + end.to change(LeaveRecord, :count).by(-1) + + expect(response).to have_http_status(:ok) + end + + it 'prevents deleting non-cancellable leave records' do + leave_record.update!(status: :approved, start_date: Date.current, end_date: Date.current + 2.days) + + delete "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}", headers: agent.create_new_auth_token - expect(response).to have_http_status(:unprocessable_entity) - end - end - - context 'when authenticated as administrator' do - it 'creates a leave record for the current user' do - expect do - post "/api/v1/accounts/#{account.id}/leave_records", - params: valid_params, - headers: administrator.create_new_auth_token - end.to change(LeaveRecord, :count).by(1) - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body['user']['id']).to eq(administrator.id) - end + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response['error']).to eq(I18n.t('errors.leave_records.cannot_delete')) end end - describe 'PUT #update' do - let(:leave_record) { create(:leave_record, account: account, user: agent, status: :pending) } - let(:other_leave_record) { create(:leave_record, account: account, user: other_user, status: :pending) } - let(:update_params) do - { - leave_record: { - reason: 'Updated reason' - } - } - end - - context 'when authenticated as administrator' do - it 'updates any pending leave record in the account' do - put "/api/v1/accounts/#{account.id}/leave_records/#{other_leave_record.id}", - params: update_params, - headers: administrator.create_new_auth_token - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body['reason']).to eq('Updated reason') - end - end - - context 'when authenticated as agent' do - it 'updates own pending leave record' do - put "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}", - params: update_params, - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body['reason']).to eq('Updated reason') - end - - it 'denies updating other user leave record' do - put "/api/v1/accounts/#{account.id}/leave_records/#{other_leave_record.id}", - params: update_params, - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:not_found) - end - - it 'denies updating approved leave record' do - approved_leave_record = create(:leave_record, account: account, user: agent, status: :approved) - - put "/api/v1/accounts/#{account.id}/leave_records/#{approved_leave_record.id}", - params: update_params, - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:unprocessable_entity) - expect(JSON.parse(response.body)['error']).to eq('Cannot update non-pending leave record') - end - end - end - - describe 'DELETE #destroy' do - let(:leave_record) { create(:leave_record, account: account, user: agent, status: :pending) } - let(:other_leave_record) { create(:leave_record, account: account, user: other_user, status: :pending) } - - context 'when authenticated as administrator' do - it 'deletes any cancellable leave record in the account' do - other_leave_record # Force creation before the test - - expect do - delete "/api/v1/accounts/#{account.id}/leave_records/#{other_leave_record.id}", - headers: administrator.create_new_auth_token - end.to change(LeaveRecord, :count).by(-1) - - expect(response).to have_http_status(:ok) - end - end - - context 'when authenticated as agent' do - it 'deletes own cancellable leave record' do - leave_record # Force creation before the test - - expect do - delete "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}", - headers: agent.create_new_auth_token - end.to change(LeaveRecord, :count).by(-1) - - expect(response).to have_http_status(:ok) - end - - it 'denies deleting other user leave record' do - delete "/api/v1/accounts/#{account.id}/leave_records/#{other_leave_record.id}", - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:not_found) - end - - it 'denies deleting non-cancellable leave record' do - non_cancellable_leave_record = create(:leave_record, account: account, user: agent, - status: :approved, - start_date: Date.current, - end_date: Date.current + 2.days) - - delete "/api/v1/accounts/#{account.id}/leave_records/#{non_cancellable_leave_record.id}", - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:unprocessable_entity) - expect(JSON.parse(response.body)['error']).to eq('Cannot delete this leave record') - end - end - end - - describe 'PATCH #approve' do + describe 'PATCH /api/v1/accounts/:account_id/leave_records/:id/approve' do let(:leave_record) { create(:leave_record, account: account, user: agent, status: :pending) } - context 'when authenticated as administrator' do - it 'approves the leave record' do - patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}/approve", - headers: administrator.create_new_auth_token - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body['status']).to eq('approved') - expect(body['approver']['id']).to eq(administrator.id) - end - end - - context 'when authenticated as agent' do - it 'denies approval' do - patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}/approve", - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:unauthorized) - end - end - end - - describe 'PATCH #reject' do - let(:leave_record) { create(:leave_record, account: account, user: agent, status: :pending) } - - context 'when authenticated as administrator' do - it 'rejects the leave record' do - patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}/reject", - headers: administrator.create_new_auth_token - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - expect(body['status']).to eq('rejected') - expect(body['approver']['id']).to eq(administrator.id) - end - end - - context 'when authenticated as agent' do - it 'denies rejection' do - patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}/reject", - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:unauthorized) - end - end - end - - describe 'response format' do - let(:leave_record) { create(:leave_record, account: account, user: agent) } - - it 'returns leave record data in correct format' do - get "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}", - headers: agent.create_new_auth_token + it 'allows admins to approve leave records' do + patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}/approve", + headers: admin.create_new_auth_token expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - - expect(body.keys).to include('id', 'start_date', 'end_date', 'leave_type', 'status', - 'reason', 'duration_in_days', 'created_at', 'updated_at', 'user') - expect(body['user'].keys).to include('id', 'name', 'email') + expect(json_response['status']).to eq('approved') + expect(json_response['approved_by']['id']).to eq(admin.id) end - it 'includes approver data when present' do - approved_leave_record = create(:leave_record, account: account, user: agent, status: :approved) - approved_leave_record.update!(approver: administrator, approved_at: Time.current) + it 'denies agents from approving leave records' do + patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}/approve", + headers: agent.create_new_auth_token - get "/api/v1/accounts/#{account.id}/leave_records/#{approved_leave_record.id}", - headers: agent.create_new_auth_token - - expect(response).to have_http_status(:success) - body = JSON.parse(response.body) - - expect(body['approver']).not_to be_nil - expect(body['approver'].keys).to include('id', 'name', 'email') + expect(response).to have_http_status(:unauthorized) end end -end + + describe 'PATCH /api/v1/accounts/:account_id/leave_records/:id/reject' do + let(:leave_record) { create(:leave_record, account: account, user: agent, status: :pending) } + + it 'allows admins to reject leave records' do + patch "/api/v1/accounts/#{account.id}/leave_records/#{leave_record.id}/reject", + headers: admin.create_new_auth_token + + expect(response).to have_http_status(:success) + expect(json_response['status']).to eq('rejected') + expect(json_response['approved_by']['id']).to eq(admin.id) + end + end + + private + + def json_response + JSON.parse(response.body) + end +end \ No newline at end of file diff --git a/spec/enterprise/models/leave_record_spec.rb b/spec/enterprise/models/leave_record_spec.rb index febe26c9c..46f5a7396 100644 --- a/spec/enterprise/models/leave_record_spec.rb +++ b/spec/enterprise/models/leave_record_spec.rb @@ -3,285 +3,128 @@ require 'rails_helper' RSpec.describe LeaveRecord, type: :model do let(:account) { create(:account) } let(:user) { create(:user, account: account) } - let(:approver) { create(:user, :administrator, account: account) } + let(:admin) { create(:user, account: account, role: :administrator) } + + describe 'associations' do + it { is_expected.to belong_to(:account) } + it { is_expected.to belong_to(:user) } + it { is_expected.to belong_to(:approved_by).class_name('User').optional } + end + + describe 'validations' do + subject { build(:leave_record, account: account, user: user) } + + it { is_expected.to validate_presence_of(:start_date) } + it { is_expected.to validate_presence_of(:end_date) } + it { is_expected.to validate_presence_of(:leave_type) } + it { is_expected.to validate_presence_of(:status) } + + it 'validates end_date is after start_date' do + leave = build(:leave_record, account: account, user: user, + start_date: Date.current + 2.days, + end_date: Date.current) + expect(leave).not_to be_valid + expect(leave.errors[:end_date]).to include('must be after start date') + end + + it 'validates future dates for pending leaves' do + leave = build(:leave_record, account: account, user: user, + status: :pending, + start_date: Date.current) + expect(leave).not_to be_valid + expect(leave.errors[:start_date]).to include('must be in the future') + end + + it 'validates approved_by is admin when present' do + agent = create(:user, account: account, role: :agent) + leave = build(:leave_record, account: account, user: user, + approved_by: agent, + status: :approved) + expect(leave).not_to be_valid + expect(leave.errors[:approved_by]).to include('must be an administrator') + end + end describe '#duration_in_days' do - it 'calculates duration correctly for single day' do - leave_record = create(:leave_record, start_date: Date.current, end_date: Date.current, status: :approved) - expect(leave_record.duration_in_days).to eq(1) - end - - it 'calculates duration correctly for multiple days' do - leave_record = create(:leave_record, start_date: Date.current, end_date: Date.current + 4.days, status: :approved) - expect(leave_record.duration_in_days).to eq(5) - end - - it 'returns 0 when start_date is nil' do - leave_record = build(:leave_record, start_date: nil, end_date: Date.current) - expect(leave_record.duration_in_days).to eq(0) - end - - it 'returns 0 when end_date is nil' do - leave_record = build(:leave_record, start_date: Date.current, end_date: nil) - expect(leave_record.duration_in_days).to eq(0) + it 'calculates correct duration' do + leave = build(:leave_record, + start_date: Date.current + 1.day, + end_date: Date.current + 5.days) + expect(leave.duration_in_days).to eq(5) end end describe '#can_be_cancelled?' do - context 'when leave record is pending' do - it 'returns true' do - leave_record = create(:leave_record, status: :pending) - expect(leave_record.can_be_cancelled?).to be true - end + it 'returns true for pending leaves' do + leave = build(:leave_record, status: :pending) + expect(leave.can_be_cancelled?).to be true end - context 'when leave record is approved' do - it 'returns true for future leaves' do - leave_record = create(:leave_record, status: :approved, - start_date: 1.week.from_now.to_date, - end_date: 2.weeks.from_now.to_date) - expect(leave_record.can_be_cancelled?).to be true - end - - it 'returns false for current leaves' do - leave_record = create(:leave_record, status: :approved, - start_date: Date.current, - end_date: Date.current + 1.day) - expect(leave_record.can_be_cancelled?).to be false - end - - it 'returns false for past leaves' do - leave_record = create(:leave_record, status: :approved, - start_date: 1.week.ago.to_date, - end_date: 3.days.ago.to_date) - expect(leave_record.can_be_cancelled?).to be false - end + it 'returns true for approved future leaves' do + leave = build(:leave_record, status: :approved, + start_date: Date.current + 1.day) + expect(leave.can_be_cancelled?).to be true end - context 'when leave record is rejected' do - it 'returns false' do - leave_record = create(:leave_record, status: :rejected) - expect(leave_record.can_be_cancelled?).to be false - end + it 'returns false for approved ongoing leaves' do + leave = build(:leave_record, status: :approved, + start_date: Date.current) + expect(leave.can_be_cancelled?).to be false end - context 'when leave record is cancelled' do - it 'returns false' do - leave_record = create(:leave_record, status: :cancelled) - expect(leave_record.can_be_cancelled?).to be false + it 'returns false for rejected leaves' do + leave = build(:leave_record, status: :rejected) + expect(leave.can_be_cancelled?).to be false + end + end + + describe '#approve!' do + let(:leave) { create(:leave_record, account: account, user: user, status: :pending) } + + it 'approves the leave and sets approved_by' do + freeze_time do + leave.approve!(admin) + + expect(leave.status).to eq('approved') + expect(leave.approved_by).to eq(admin) + expect(leave.approved_at).to eq(Time.current) + end + end + end + + describe '#reject!' do + let(:leave) { create(:leave_record, account: account, user: user, status: :pending) } + + it 'rejects the leave and sets approved_by' do + freeze_time do + leave.reject!(admin) + + expect(leave.status).to eq('rejected') + expect(leave.approved_by).to eq(admin) + expect(leave.approved_at).to eq(Time.current) end end end describe '#overlaps_with?' do - let(:base_leave_record) do - create(:leave_record, start_date: Date.current + 10.days, - end_date: Date.current + 15.days) + let(:leave1) do + build(:leave_record, + start_date: Date.current + 5.days, + end_date: Date.current + 10.days) end - it 'returns false for non-LeaveRecord objects' do - expect(base_leave_record.overlaps_with?('not a leave record')).to be false + it 'detects overlapping leaves' do + leave2 = build(:leave_record, + start_date: Date.current + 7.days, + end_date: Date.current + 12.days) + expect(leave1.overlaps_with?(leave2)).to be true end - it 'detects overlapping leave records - same dates' do - overlapping_leave_record = build(:leave_record, start_date: Date.current + 10.days, - end_date: Date.current + 15.days) - expect(base_leave_record.overlaps_with?(overlapping_leave_record)).to be true - end - - it 'detects overlapping leave records - partial overlap start' do - overlapping_leave_record = build(:leave_record, start_date: Date.current + 8.days, - end_date: Date.current + 12.days) - expect(base_leave_record.overlaps_with?(overlapping_leave_record)).to be true - end - - it 'detects overlapping leave records - partial overlap end' do - overlapping_leave_record = build(:leave_record, start_date: Date.current + 13.days, - end_date: Date.current + 18.days) - expect(base_leave_record.overlaps_with?(overlapping_leave_record)).to be true - end - - it 'detects overlapping leave records - contained within' do - overlapping_leave_record = build(:leave_record, start_date: Date.current + 12.days, - end_date: Date.current + 13.days) - expect(base_leave_record.overlaps_with?(overlapping_leave_record)).to be true - end - - it 'detects overlapping leave records - contains other' do - overlapping_leave_record = build(:leave_record, start_date: Date.current + 8.days, - end_date: Date.current + 18.days) - expect(base_leave_record.overlaps_with?(overlapping_leave_record)).to be true - end - - it 'returns false for non-overlapping leave records - before' do - non_overlapping_leave_record = build(:leave_record, start_date: Date.current + 5.days, - end_date: Date.current + 9.days) - expect(base_leave_record.overlaps_with?(non_overlapping_leave_record)).to be false - end - - it 'returns false for non-overlapping leave records - after' do - non_overlapping_leave_record = build(:leave_record, start_date: Date.current + 16.days, - end_date: Date.current + 20.days) - expect(base_leave_record.overlaps_with?(non_overlapping_leave_record)).to be false - end - - it 'returns false for adjacent leave records - ending where other starts' do - adjacent_leave_record = build(:leave_record, start_date: Date.current + 16.days, - end_date: Date.current + 20.days) - expect(base_leave_record.overlaps_with?(adjacent_leave_record)).to be false - end - - it 'returns false for adjacent leave records - starting where other ends' do - adjacent_leave_record = build(:leave_record, start_date: Date.current + 5.days, - end_date: Date.current + 9.days) - expect(base_leave_record.overlaps_with?(adjacent_leave_record)).to be false + it 'returns false for non-overlapping leaves' do + leave2 = build(:leave_record, + start_date: Date.current + 11.days, + end_date: Date.current + 15.days) + expect(leave1.overlaps_with?(leave2)).to be false end end - - describe '#approve!' do - let(:leave_record) { create(:leave_record, account: account, status: :pending) } - - before do - # Ensure approver has administrator role in the account - account_user = approver.account_users.find_by(account: account) - account_user&.update!(role: :administrator) - end - - it 'updates status to approved' do - leave_record.approve!(approver) - expect(leave_record.status).to eq('approved') - end - - it 'sets approver' do - leave_record.approve!(approver) - expect(leave_record.approver).to eq(approver) - end - - it 'sets approved_at timestamp' do - freeze_time = Time.current - allow(Time).to receive(:current).and_return(freeze_time) - - leave_record.approve!(approver) - expect(leave_record.approved_at.to_i).to eq(freeze_time.to_i) - end - - it 'raises error if approver is not admin' do - non_admin = create(:user, account: account, role: :agent) - - expect do - leave_record.approve!(non_admin) - end.to raise_error(ActiveRecord::RecordInvalid, /must be an administrator/) - end - end - - describe '#reject!' do - let(:leave_record) { create(:leave_record, account: account, status: :pending) } - - before do - # Ensure approver has administrator role in the account - account_user = approver.account_users.find_by(account: account) - account_user&.update!(role: :administrator) - end - - it 'updates status to rejected' do - leave_record.reject!(approver) - expect(leave_record.status).to eq('rejected') - end - - it 'sets approver' do - leave_record.reject!(approver) - expect(leave_record.approver).to eq(approver) - end - - it 'sets approved_at timestamp' do - freeze_time = Time.current - allow(Time).to receive(:current).and_return(freeze_time) - - leave_record.reject!(approver) - expect(leave_record.approved_at.to_i).to eq(freeze_time.to_i) - end - - it 'raises error if approver is not admin' do - non_admin = create(:user, account: account, role: :agent) - - expect do - leave_record.reject!(non_admin) - end.to raise_error(ActiveRecord::RecordInvalid, /must be an administrator/) - end - end - - describe 'custom validations' do - describe '#end_date_after_start_date' do - it 'is valid when end_date is after start_date' do - leave_record = build(:leave_record, start_date: 1.day.from_now.to_date, end_date: 2.days.from_now.to_date) - expect(leave_record).to be_valid - end - - it 'is valid when end_date equals start_date' do - leave_record = build(:leave_record, start_date: 1.day.from_now.to_date, end_date: 1.day.from_now.to_date) - expect(leave_record).to be_valid - end - - it 'is invalid when end_date is before start_date' do - leave_record = build(:leave_record, start_date: 2.days.from_now.to_date, end_date: 1.day.from_now.to_date) - expect(leave_record).not_to be_valid - expect(leave_record.errors[:end_date]).to include('must be after start date') - end - - it 'skips validation when dates are nil' do - leave_record = build(:leave_record, start_date: nil, end_date: nil) - leave_record.valid? - expect(leave_record.errors[:end_date]).not_to include('must be after start date') - end - end - - describe '#future_dates_for_pending_leaves' do - it 'is valid for future start_date on pending leave record' do - leave_record = build(:leave_record, status: :pending, start_date: Date.current + 1.day) - expect(leave_record).to be_valid - end - - it 'is invalid for current start_date on pending leave record' do - leave_record = build(:leave_record, status: :pending, start_date: Date.current) - expect(leave_record).not_to be_valid - expect(leave_record.errors[:start_date]).to include('must be in the future') - end - - it 'is invalid for past start_date on pending leave record' do - leave_record = build(:leave_record, status: :pending, start_date: Date.current - 1.day) - expect(leave_record).not_to be_valid - expect(leave_record.errors[:start_date]).to include('must be in the future') - end - - it 'skips validation for non-pending leave records' do - leave_record = build(:leave_record, status: :approved, start_date: Date.current - 1.day) - leave_record.valid? - expect(leave_record.errors[:start_date]).not_to include('must be in the future') - end - end - - describe '#approver_is_admin' do - let(:admin_user) { create(:user, :administrator, account: account) } - let(:agent_user) { create(:user, account: account, role: :agent) } - - # Users are already associated with the account via factory - - it 'is valid with admin approver' do - leave_record = build(:leave_record, account: account, approver: admin_user, status: :approved) - expect(leave_record).to be_valid - end - - it 'is invalid with non-admin approver' do - leave_record = build(:leave_record, account: account, approver: agent_user, status: :approved) - expect(leave_record).not_to be_valid - expect(leave_record.errors[:approved_by]).to include('must be an administrator') - end - - it 'skips validation when approver is nil' do - leave_record = build(:leave_record, account: account, approver: nil) - leave_record.valid? - expect(leave_record.errors[:approved_by]).not_to include('must be an administrator') - end - end - end -end +end \ No newline at end of file diff --git a/spec/enterprise/policies/leave_record_policy_spec.rb b/spec/enterprise/policies/leave_record_policy_spec.rb index a0402ed58..45b1023da 100644 --- a/spec/enterprise/policies/leave_record_policy_spec.rb +++ b/spec/enterprise/policies/leave_record_policy_spec.rb @@ -2,174 +2,74 @@ require 'rails_helper' RSpec.describe LeaveRecordPolicy, type: :policy do let(:account) { create(:account) } - let(:administrator) { create(:user, account: account, role: :administrator) } + let(:admin) { create(:user, account: account, role: :administrator) } let(:agent) { create(:user, account: account, role: :agent) } + let(:leave_record) { create(:leave_record, account: account, user: agent) } - let(:admin_context) { { user: administrator, account: account, account_user: administrator.account_users.find_by(account: account) } } - let(:agent_context) { { user: agent, account: account, account_user: agent.account_users.find_by(account: account) } } + describe 'permissions' do + context 'for administrators' do + let(:policy) { described_class.new(pundit_context(admin), leave_record) } - describe 'index?' do - it 'allows administrators to list leave records' do - policy = described_class.new(admin_context, LeaveRecord) - expect(policy.index?).to be true - end - - it 'allows agents to list leave records' do - policy = described_class.new(agent_context, LeaveRecord) - expect(policy.index?).to be true - end - end - - describe 'show?' do - let(:leave_record) { create(:leave_record, account: account, user: agent) } - - it 'allows administrators to view any leave record' do - policy = described_class.new(admin_context, leave_record) - expect(policy.show?).to be true - end - - it 'allows agents to view leave records' do - policy = described_class.new(agent_context, leave_record) - expect(policy.show?).to be true - end - end - - describe 'create?' do - it 'allows administrators to create leave records' do - policy = described_class.new(admin_context, LeaveRecord) - expect(policy.create?).to be true - end - - it 'allows agents to create leave records' do - policy = described_class.new(agent_context, LeaveRecord) - expect(policy.create?).to be true - end - end - - describe 'update?' do - let(:leave_record) { create(:leave_record, account: account, user: agent) } - - it 'allows administrators to access update action' do - policy = described_class.new(admin_context, leave_record) - expect(policy.update?).to be true - end - - it 'allows agents to access update action' do - policy = described_class.new(agent_context, leave_record) - expect(policy.update?).to be true - end - end - - describe 'destroy?' do - let(:leave_record) { create(:leave_record, account: account, user: agent) } - - it 'allows administrators to access destroy action' do - policy = described_class.new(admin_context, leave_record) - expect(policy.destroy?).to be true - end - - it 'allows agents to access destroy action' do - policy = described_class.new(agent_context, leave_record) - expect(policy.destroy?).to be true - end - end - - describe 'approve?' do - let(:leave_record) { create(:leave_record, account: account, user: agent) } - - it 'allows only administrators to approve leave records' do - policy = described_class.new(admin_context, leave_record) - expect(policy.approve?).to be true - end - - it 'denies agents from approving leave records' do - policy = described_class.new(agent_context, leave_record) - expect(policy.approve?).to be false - end - end - - describe 'reject?' do - let(:leave_record) { create(:leave_record, account: account, user: agent) } - - it 'allows only administrators to reject leave records' do - policy = described_class.new(admin_context, leave_record) - expect(policy.reject?).to be true - end - - it 'denies agents from rejecting leave records' do - policy = described_class.new(agent_context, leave_record) - expect(policy.reject?).to be false - end - end - - describe 'Scope' do - let(:other_agent) { create(:user, account: account, role: :agent) } - - describe '#resolve' do - before do - create(:leave_record, account: account, user: administrator) - create(:leave_record, account: account, user: agent) - create(:leave_record, account: account, user: other_agent) - end - - context 'when user is administrator' do - it 'returns all leave records in the account' do - scope = LeaveRecordPolicy::Scope.new(admin_context, account.leave_records) - result = scope.resolve - - expect(result.count).to eq(3) - expect(result.map(&:user_id)).to contain_exactly( - administrator.id, - agent.id, - other_agent.id - ) - end - - it 'includes user and approver associations for performance' do - scope = LeaveRecordPolicy::Scope.new(admin_context, account.leave_records) - result = scope.resolve - - expect(result.includes_values).to include(:user, :approver) - end - end - - context 'when user is agent' do - it 'returns only the agent\'s own leave records' do - scope = LeaveRecordPolicy::Scope.new(agent_context, account.leave_records) - result = scope.resolve - - expect(result.count).to eq(1) - expect(result.first.user_id).to eq(agent.id) - end - - it 'does not return other agents\' leave records' do - scope = LeaveRecordPolicy::Scope.new(agent_context, account.leave_records) - result = scope.resolve - - user_ids = result.map(&:user_id) - expect(user_ids).not_to include(other_agent.id) - expect(user_ids).not_to include(administrator.id) - end - - it 'includes user and approver associations for performance' do - scope = LeaveRecordPolicy::Scope.new(agent_context, account.leave_records) - result = scope.resolve - - expect(result.includes_values).to include(:user, :approver) - end + it 'permits all actions' do + expect(policy.index?).to be true + expect(policy.show?).to be true + expect(policy.create?).to be true + expect(policy.update?).to be true + expect(policy.destroy?).to be true + expect(policy.approve?).to be true + expect(policy.reject?).to be true end end - describe 'initialization' do - it 'properly sets all context attributes' do - scope = LeaveRecordPolicy::Scope.new(admin_context, account.leave_records) + context 'for agents' do + let(:policy) { described_class.new(pundit_context(agent), leave_record) } - expect(scope.user_context).to eq(admin_context) - expect(scope.user).to eq(administrator) - expect(scope.account).to eq(account) - expect(scope.account_user).to eq(administrator.account_users.find_by(account: account)) - expect(scope.scope).to eq(account.leave_records) + it 'permits basic actions' do + expect(policy.index?).to be true + expect(policy.show?).to be true + expect(policy.create?).to be true + expect(policy.update?).to be true + expect(policy.destroy?).to be true + end + + it 'denies approval actions' do + expect(policy.approve?).to be false + expect(policy.reject?).to be false end end end -end + + describe 'scope' do + let!(:agent_leave) { create(:leave_record, account: account, user: agent) } + let!(:other_agent) { create(:user, account: account, role: :agent) } + let!(:other_leave) { create(:leave_record, account: account, user: other_agent) } + + it 'returns all leaves for administrators' do + scope = described_class::Scope.new(pundit_context(admin), LeaveRecord).resolve + expect(scope).to include(agent_leave, other_leave) + expect(scope.count).to eq(2) + end + + it 'returns only own leaves for agents' do + scope = described_class::Scope.new(pundit_context(agent), LeaveRecord).resolve + expect(scope).to include(agent_leave) + expect(scope).not_to include(other_leave) + expect(scope.count).to eq(1) + end + + it 'includes associations for performance' do + scope = described_class::Scope.new(pundit_context(admin), LeaveRecord).resolve + expect(scope.includes_values).to include(:user, :approved_by) + end + end + + private + + def pundit_context(user) + { + user: user, + account: account, + account_user: user.account_users.find_by(account: account) + } + end +end \ No newline at end of file