self review changes

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

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

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
+end