diff --git a/app/Libraries/Reward_lib.php b/app/Libraries/Reward_lib.php index 5016679cb..45250ec29 100644 --- a/app/Libraries/Reward_lib.php +++ b/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; } diff --git a/app/Models/Sale.php b/app/Models/Sale.php index 2ddf983ac..b19c9e8de 100644 --- a/app/Models/Sale.php +++ b/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); } diff --git a/tests/Libraries/Reward_libTest.php b/tests/Libraries/Reward_libTest.php index 740994f52..1fe537bb6 100644 --- a/tests/Libraries/Reward_libTest.php +++ b/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']); + } } \ No newline at end of file