Prevent negative reward points and address CodeRabbit review comments

CodeRabbit issues addressed:

1. Negative points prevention in Reward_lib:
   - adjustRewardPoints(): Validate sufficient balance before deduct/adjust
   - handleCustomerChange(): Cap charge at available points, add 'insufficient' flag
   - adjustRewardDelta(): Validate sufficient points for positive adjustments

2. Sale.php fixes:
   - Add null coalescing for reward points in processPaymentType()
   - Validate giftcard payment format before accessing array index
   - Remove unused loop variables $paymentId and $line
   - Add null check for deleted customer in delete() method
   - Log warnings when insufficient points detected

3. Test coverage:
   - Add test for exact points match (hasSufficientPoints)
   - Add tests for insufficient points scenarios
   - Add tests for negative adjustment (refund)
   - Add tests for handleCustomerChange caps

All changes prevent customers from having negative reward point balances.

app/Libraries/Reward_lib.php

@@ -34,11 +34,12 @@ class Reward_lib
     /**
      * Adjusts customer reward points for a sale transaction.
      * Handles new sales, sale updates, and sale cancellations.
+     * Prevents negative balances by validating sufficient points before deduction.
      *
      * @param int|null $customerId Customer ID (null for walk-in customers)
      * @param float $rewardAmount Amount to deduct from points (for new/updated sales)
      * @param string $operation Operation type: 'deduct', 'restore', 'adjust'
-     * @return bool Success status
+     * @return bool Success status (false if insufficient points for deduct/adjust)
      */
     public function adjustRewardPoints(?int $customerId, float $rewardAmount, string $operation): bool
     {
@@ -51,6 +52,14 @@ class Reward_lib
         switch ($operation) {
             case 'deduct':
             case 'adjust':
+                if ($currentPoints < $rewardAmount) {
+                    log_message(
+                        'warning',
+                        'Reward_lib::adjustRewardPoints insufficient points customer_id=' . $customerId
+                        . ' current=' . $currentPoints . ' requested=' . $rewardAmount
+                    );
+                    return false;
+                }
                 $this->customer->update_reward_points_value($customerId, $currentPoints - $rewardAmount);
                 return true;
             case 'restore':
@@ -64,16 +73,17 @@ class Reward_lib
     /**
      * Handles reward point adjustment when customer changes on a sale.
      * Restores points to previous customer, deducts from new customer.
+     * Prevents negative balances by capping deduction at available points.
      *
      * @param int|null $previousCustomerId Previous customer ID
      * @param int|null $newCustomerId New customer ID
      * @param float $previousRewardUsed Reward points used by previous customer
      * @param float $newRewardUsed Reward points to be used by new customer
-     * @return array ['restored' => float, 'charged' => float] Amounts restored/charged
+     * @return array ['restored' => float, 'charged' => float, 'insufficient' => bool] Amounts restored/charged
      */
     public function handleCustomerChange(?int $previousCustomerId, ?int $newCustomerId, float $previousRewardUsed, float $newRewardUsed): array
     {
-        $result = ['restored' => 0.0, 'charged' => 0.0];
+        $result = ['restored' => 0.0, 'charged' => 0.0, 'insufficient' => false];
 
         if ($previousCustomerId === $newCustomerId) {
             return $result;
@@ -87,8 +97,19 @@ class Reward_lib
 
         if (!empty($newCustomerId) && $newRewardUsed != 0) {
             $newPoints = $this->customer->get_info($newCustomerId)->points ?? 0;
-            $this->customer->update_reward_points_value($newCustomerId, $newPoints - $newRewardUsed);
-            $result['charged'] = $newRewardUsed;
+            
+            if ($newPoints < $newRewardUsed) {
+                log_message(
+                    'warning',
+                    'Reward_lib::handleCustomerChange insufficient points new_customer_id=' . $newCustomerId
+                    . ' available=' . $newPoints . ' requested=' . $newRewardUsed
+                );
+                $result['insufficient'] = true;
+            }
+            
+            $actualCharged = min($newPoints, $newRewardUsed);
+            $this->customer->update_reward_points_value($newCustomerId, max(0, $newPoints - $newRewardUsed));
+            $result['charged'] = $actualCharged;
         }
 
         return $result;
@@ -96,10 +117,11 @@ class Reward_lib
 
     /**
      * Adjusts reward points delta for same customer (e.g., payment amount changed).
+     * Prevents negative balances by validating sufficient points before adjustment.
      *
      * @param int|null $customerId Customer ID
-     * @param float $rewardAdjustment Difference between new and previous reward usage
-     * @return bool Success status
+     * @param float $rewardAdjustment Difference between new and previous reward usage (positive = more used)
+     * @return bool Success status (false if insufficient points)
      */
     public function adjustRewardDelta(?int $customerId, float $rewardAdjustment): bool
     {
@@ -108,6 +130,16 @@ class Reward_lib
         }
 
         $currentPoints = $this->customer->get_info($customerId)->points ?? 0;
+        
+        if ($rewardAdjustment > 0 && $currentPoints < $rewardAdjustment) {
+            log_message(
+                'warning',
+                'Reward_lib::adjustRewardDelta insufficient points customer_id=' . $customerId
+                . ' current=' . $currentPoints . ' adjustment=' . $rewardAdjustment
+            );
+            return false;
+        }
+        
         $this->customer->update_reward_points_value($customerId, $currentPoints - $rewardAdjustment);
         return true;
     }

app/Models/Sale.php

@@ -556,7 +556,14 @@ class Sale extends Model
 
                 if (!empty($newCustomerId) && $newRewardUsed != 0) {
                     $newPoints = $customer->get_info($newCustomerId)->points ?? 0;
-                    $customer->update_reward_points_value($newCustomerId, $newPoints - $newRewardUsed);
+                    if ($newPoints < $newRewardUsed) {
+                        log_message(
+                            'warning',
+                            'Sale::update insufficient points new_customer_id=' . $newCustomerId
+                            . ' available=' . $newPoints . ' requested=' . $newRewardUsed
+                        );
+                    }
+                    $customer->update_reward_points_value($newCustomerId, max(0, $newPoints - $newRewardUsed));
                     log_message(
                         'debug',
                         'Sale::update reward charge new_customer_id=' . $newCustomerId
@@ -568,6 +575,13 @@ class Sale extends Model
                 $rewardAdjustment = $newRewardUsed - $currentRewardUsed;
                 if ($rewardAdjustment != 0 && !empty($newCustomerId)) {
                     $currentPoints = $customer->get_info($newCustomerId)->points ?? 0;
+                    if ($rewardAdjustment > 0 && $currentPoints < $rewardAdjustment) {
+                        log_message(
+                            'warning',
+                            'Sale::update insufficient points customer_id=' . $newCustomerId
+                            . ' current=' . $currentPoints . ' adjustment=' . $rewardAdjustment
+                        );
+                    }
                     $customer->update_reward_points_value($newCustomerId, $currentPoints - $rewardAdjustment);
                     log_message(
                         'debug',
@@ -649,7 +663,7 @@ class Sale extends Model
         $totalAmount = 0;
         $totalAmountUsed = 0;
 
-        foreach ($payments as $paymentId => $payment) {
+        foreach ($payments as $payment) {
             $totalAmountUsed += $this->processPaymentType(
                 $payment,
                 $customer_id,
@@ -676,7 +690,7 @@ class Sale extends Model
 
         $customer = $customer->get_info($customer_id);
 
-        foreach ($items as $line => $itemData) {
+        foreach ($items as $itemData) {
             $currentItemInfo = $item->get_info($itemData['item_id']);
 
             if ($itemData['price'] == 0.00) {
@@ -905,16 +919,21 @@ class Sale extends Model
                 . ' payment_count=' . count($payments)
             );
             if ($rewardUsed > 0) {
-                $customerId = $this->get_customer($sale_id)->person_id;
-                $customer = model(Customer::class);
-                $currentPoints = $customer->get_info($customerId)->points ?? 0;
-                $customer->update_reward_points_value($customerId, $currentPoints + $rewardUsed);
-                log_message(
-                    'debug',
-                    'Sale::delete reward restore customer_id=' . $customerId
-                    . ' current_points=' . $currentPoints
-                    . ' restored=' . $rewardUsed
-                );
+                $customerObj = $this->get_customer($sale_id);
+                if (empty($customerObj) || empty($customerObj->person_id)) {
+                    log_message('error', 'Sale::delete cannot restore rewards - no customer for sale_id=' . $sale_id);
+                } else {
+                    $customerId = $customerObj->person_id;
+                    $customer = model(Customer::class);
+                    $currentPoints = $customer->get_info($customerId)->points ?? 0;
+                    $customer->update_reward_points_value($customerId, $currentPoints + $rewardUsed);
+                    log_message(
+                        'debug',
+                        'Sale::delete reward restore customer_id=' . $customerId
+                        . ' current_points=' . $currentPoints
+                        . ' restored=' . $rewardUsed
+                    );
+                }
             }
         }
 
@@ -1551,14 +1570,26 @@ class Sale extends Model
 
         if (!empty(strstr($paymentType, lang('Sales.giftcard')))) {
             $splitPayment = explode(':', $paymentType);
-            $currentGiftcardValue = $giftcard->get_giftcard_value($splitPayment[1]);
-            $giftcard->update_giftcard_value($splitPayment[1], $currentGiftcardValue - $paymentAmount);
+            if (count($splitPayment) < 2 || empty($splitPayment[1])) {
+                log_message('error', 'Sale::processPaymentType invalid giftcard format: ' . $paymentType);
+                return 0;
+            }
+            $giftcardNumber = $splitPayment[1];
+            $currentGiftcardValue = $giftcard->get_giftcard_value($giftcardNumber);
+            $giftcard->update_giftcard_value($giftcardNumber, $currentGiftcardValue - $paymentAmount);
             return 0;
         }
 
         if ($this->isRewardPayment($paymentType)) {
-            $currentRewardsValue = $customer->get_info($customerId)->points;
-            $customer->update_reward_points_value($customerId, $currentRewardsValue - $paymentAmount);
+            $currentRewardsValue = $customer->get_info($customerId)->points ?? 0;
+            if ($currentRewardsValue < $paymentAmount) {
+                log_message(
+                    'warning',
+                    'Sale::processPaymentType insufficient points customer_id=' . $customerId
+                    . ' available=' . $currentRewardsValue . ' requested=' . $paymentAmount
+                );
+            }
+            $customer->update_reward_points_value($customerId, max(0, $currentRewardsValue - $paymentAmount));
             return floatval($paymentAmount);
         }
 

tests/Libraries/Reward_libTest.php

@@ -204,7 +204,7 @@ class Reward_libTest extends CIUnitTestCase
     public function testHandleCustomerChangeWithSameCustomerReturnsEmpty(): void
     {
         $result = $this->rewardLib->handleCustomerChange(1, 1, 50.0, 75.0);
-        $this->assertEquals(['restored' => 0.0, 'charged' => 0.0], $result);
+        $this->assertEquals(['restored' => 0.0, 'charged' => 0.0, 'insufficient' => false], $result);
     }
 
     /**
@@ -213,7 +213,7 @@ class Reward_libTest extends CIUnitTestCase
     public function testHandleCustomerChangeWithNullCustomers(): void
     {
         $result = $this->rewardLib->handleCustomerChange(null, null, 50.0, 75.0);
-        $this->assertEquals(['restored' => 0.0, 'charged' => 0.0], $result);
+        $this->assertEquals(['restored' => 0.0, 'charged' => 0.0, 'insufficient' => false], $result);
     }
 
     /**
@@ -289,4 +289,151 @@ class Reward_libTest extends CIUnitTestCase
 
         $this->rewardLib->adjustRewardPoints(1, 50, 'restore');
     }
+
+    /**
+     * Test hasSufficientPoints returns true when points exactly match required
+     */
+    public function testHasSufficientPointsReturnsTrueWhenExactMatch(): void
+    {
+        $customerModel = $this->getMockBuilder(Customer::class)
+            ->onlyMethods(['get_info'])
+            ->getMock();
+
+        $customerModel->method('get_info')
+            ->willReturn((object)['points' => 50]);
+
+        $reflection = new \ReflectionClass($this->rewardLib);
+        $property = $reflection->getProperty('customer');
+        $property->setAccessible(true);
+        $property->setValue($this->rewardLib, $customerModel);
+
+        $this->assertTrue($this->rewardLib->hasSufficientPoints(1, 50));
+    }
+
+    /**
+     * Test adjustRewardPoints returns false when insufficient points for deduct
+     */
+    public function testAdjustRewardPointsReturnsFalseWhenInsufficientPointsForDeduct(): void
+    {
+        $customerModel = $this->getMockBuilder(Customer::class)
+            ->onlyMethods(['get_info', 'update_reward_points_value'])
+            ->getMock();
+
+        $customerModel->method('get_info')
+            ->willReturn((object)['points' => 30]);
+
+        $customerModel->expects($this->never())
+            ->method('update_reward_points_value');
+
+        $reflection = new \ReflectionClass($this->rewardLib);
+        $property = $reflection->getProperty('customer');
+        $property->setAccessible(true);
+        $property->setValue($this->rewardLib, $customerModel);
+
+        $result = $this->rewardLib->adjustRewardPoints(1, 50, 'deduct');
+        $this->assertFalse($result);
+    }
+
+    /**
+     * Test adjustRewardDelta returns false when insufficient points for positive adjustment
+     */
+    public function testAdjustRewardDeltaReturnsFalseWhenInsufficientPoints(): void
+    {
+        $customerModel = $this->getMockBuilder(Customer::class)
+            ->onlyMethods(['get_info', 'update_reward_points_value'])
+            ->getMock();
+
+        $customerModel->method('get_info')
+            ->willReturn((object)['points' => 20]);
+
+        $customerModel->expects($this->never())
+            ->method('update_reward_points_value');
+
+        $reflection = new \ReflectionClass($this->rewardLib);
+        $property = $reflection->getProperty('customer');
+        $property->setAccessible(true);
+        $property->setValue($this->rewardLib, $customerModel);
+
+        $result = $this->rewardLib->adjustRewardDelta(1, 50);
+        $this->assertFalse($result);
+    }
+
+    /**
+     * Test adjustRewardDelta succeeds for negative adjustment (refund)
+     */
+    public function testAdjustRewardDeltaSucceedsForNegativeAdjustment(): void
+    {
+        $customerModel = $this->getMockBuilder(Customer::class)
+            ->onlyMethods(['get_info', 'update_reward_points_value'])
+            ->getMock();
+
+        $customerModel->method('get_info')
+            ->willReturn((object)['points' => 100]);
+
+        $customerModel->expects($this->once())
+            ->method('update_reward_points_value')
+            ->with(1, 150);
+
+        $reflection = new \ReflectionClass($this->rewardLib);
+        $property = $reflection->getProperty('customer');
+        $property->setAccessible(true);
+        $property->setValue($this->rewardLib, $customerModel);
+
+        $result = $this->rewardLib->adjustRewardDelta(1, -50);
+        $this->assertTrue($result);
+    }
+
+    /**
+     * Test handleCustomerChange caps charge at available points when insufficient
+     */
+    public function testHandleCustomerChangeCapsChargeWhenInsufficient(): void
+    {
+        $customerModel = $this->getMockBuilder(Customer::class)
+            ->onlyMethods(['get_info', 'update_reward_points_value'])
+            ->getMock();
+
+        $customerModel->method('get_info')
+            ->willReturn((object)['points' => 30]);
+
+        $customerModel->expects($this->once())
+            ->method('update_reward_points_value')
+            ->with(2, 0);
+
+        $reflection = new \ReflectionClass($this->rewardLib);
+        $property = $reflection->getProperty('customer');
+        $property->setAccessible(true);
+        $property->setValue($this->rewardLib, $customerModel);
+
+        $result = $this->rewardLib->handleCustomerChange(null, 2, 0, 50.0);
+
+        $this->assertEquals(30.0, $result['charged']);
+        $this->assertTrue($result['insufficient']);
+    }
+
+    /**
+     * Test handleCustomerChange does not charge when new customer has zero points
+     */
+    public function testHandleCustomerChangeCapsChargeAtZero(): void
+    {
+        $customerModel = $this->getMockBuilder(Customer::class)
+            ->onlyMethods(['get_info', 'update_reward_points_value'])
+            ->getMock();
+
+        $customerModel->method('get_info')
+            ->willReturn((object)['points' => 0]);
+
+        $customerModel->expects($this->once())
+            ->method('update_reward_points_value')
+            ->with(2, 0);
+
+        $reflection = new \ReflectionClass($this->rewardLib);
+        $property = $reflection->getProperty('customer');
+        $property->setAccessible(true);
+        $property->setValue($this->rewardLib, $customerModel);
+
+        $result = $this->rewardLib->handleCustomerChange(null, 2, 0, 50.0);
+
+        $this->assertEquals(0.0, $result['charged']);
+        $this->assertTrue($result['insufficient']);
+    }
 }