From 9031c710b7a05208cb9fea5a7498ba947793b545 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma Date: Mon, 18 Aug 2025 19:26:08 +0530 Subject: [PATCH] self review changes --- .../v1/accounts/leave_records_controller.rb | 17 +++++++++---- .../app/policies/leave_record_policy.rb | 16 +++---------- .../accounts/leave_records_controller_spec.rb | 24 ++++++++++++------- 3 files changed, 30 insertions(+), 27 deletions(-) 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 9e9d621cc..a65b03172 100644 --- a/enterprise/app/controllers/api/v1/accounts/leave_records_controller.rb +++ b/enterprise/app/controllers/api/v1/accounts/leave_records_controller.rb @@ -1,7 +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, only: [:index, :create] - before_action :check_record_authorization, only: [:show, :update, :destroy, :approve, :reject] + before_action :check_authorization def index @leave_records = policy_scope(Current.account.leave_records) @@ -14,10 +13,14 @@ class Api::V1::Accounts::LeaveRecordsController < Api::V1::Accounts::EnterpriseA end def update + render json: { error: 'Cannot update non-pending leave record' }, status: :unprocessable_entity and return unless @leave_record.pending? + @leave_record.update!(permitted_params) end def destroy + render json: { error: 'Cannot delete this leave record' }, status: :unprocessable_entity and return unless @leave_record.can_be_cancelled? + @leave_record.destroy! head :ok end @@ -39,10 +42,14 @@ class Api::V1::Accounts::LeaveRecordsController < Api::V1::Accounts::EnterpriseA end def fetch_leave_record - @leave_record = policy_scope(Current.account.leave_records).find(params[:id]) + @leave_record = leave_records_scope.find(params[:id]) end - def check_record_authorization - authorize(@leave_record) + def leave_records_scope + if Current.account_user.administrator? + Current.account.leave_records + else + Current.account.leave_records.where(user: current_user) + end end end diff --git a/enterprise/app/policies/leave_record_policy.rb b/enterprise/app/policies/leave_record_policy.rb index f1f4b7375..8fd8ad0a3 100644 --- a/enterprise/app/policies/leave_record_policy.rb +++ b/enterprise/app/policies/leave_record_policy.rb @@ -4,7 +4,7 @@ class LeaveRecordPolicy < ApplicationPolicy end def show? - @account_user.administrator? || owned_by_user? + @account_user.administrator? || @account_user.agent? end def create? @@ -12,15 +12,11 @@ class LeaveRecordPolicy < ApplicationPolicy end def update? - return false unless @record.pending? - - @account_user.administrator? || owned_by_user? + @account_user.administrator? || @account_user.agent? end def destroy? - return false unless @record.can_be_cancelled? - - @account_user.administrator? || owned_by_user? + @account_user.administrator? || @account_user.agent? end def approve? @@ -50,10 +46,4 @@ class LeaveRecordPolicy < ApplicationPolicy end end end - - private - - def owned_by_user? - @record&.user_id == @account_user.user_id - end 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 220194977..b27a0ac5b 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 @@ -182,7 +182,8 @@ RSpec.describe 'LeaveRecords API', type: :request do params: update_params, headers: agent.create_new_auth_token - expect(response).to have_http_status(:not_found) + 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 @@ -193,6 +194,8 @@ RSpec.describe 'LeaveRecords API', type: :request do 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 @@ -204,6 +207,8 @@ RSpec.describe 'LeaveRecords API', type: :request do 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 @@ -221,14 +226,15 @@ RSpec.describe 'LeaveRecords API', type: :request do 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) + 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(:not_found) + expect(response).to have_http_status(:unprocessable_entity) + expect(JSON.parse(response.body)['error']).to eq('Cannot delete this leave record') end end end @@ -253,7 +259,7 @@ RSpec.describe 'LeaveRecords API', type: :request 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(:not_found) + expect(response).to have_http_status(:unauthorized) end end end @@ -278,7 +284,7 @@ RSpec.describe 'LeaveRecords API', type: :request 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(:not_found) + expect(response).to have_http_status(:unauthorized) end end end @@ -293,7 +299,7 @@ RSpec.describe 'LeaveRecords API', type: :request do 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', + 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') end @@ -312,4 +318,4 @@ RSpec.describe 'LeaveRecords API', type: :request do expect(body['approver'].keys).to include('id', 'name', 'email') end end -end \ No newline at end of file +end