fix: Remove deleted filter and primary key from insert

- Remove deleted=0 filter from existence check (allow soft-deleted updates)
- Remove primary key from insert payload to avoid conflicts
- Cleaner approach for upsert logic

Address CodeRabbit review feedback

app/Controllers/Items.php

@@ -482,9 +482,9 @@ class Items extends Secure_Controller
         foreach ($result as &$item) {
             if (isset($item['item_number']) && empty($item['item_number']) && $this->config['barcode_generate_if_empty']) {
                 if (isset($item['item_id'])) {
-                    $save_item = ['item_number' => $item['item_number']];
-                    $this->item->save_value($save_item, $item['item_id']);
-                }
+                     $save_item = ['item_number' => $item['item_number'], 'item_id' => $item['item_id']];
+                     $this->item->saveValue($save_item);
+                 }
             }
         }
         $data['items'] = $result;
@@ -663,13 +663,15 @@ class Items extends Secure_Controller
 
         $employee_id = $this->employee->get_logged_in_employee_info()->person_id;
 
-        // Wrap the entire save sequence in a single transaction for atomicity
-        $this->db->transBegin();
+        // For updates, include item_id in data array
+        if ($item_id !== NEW_ENTRY) {
+            $item_data['item_id'] = $item_id;
+        }
 
-        $success = $this->item->save_value($item_data, $item_id);
-        $new_item = false;
+        if ($this->item->saveValue($item_data)) {
+            $success = true;
+            $new_item = false;
 
-        if ($success) {
             if ($item_id === NEW_ENTRY) {
                 $item_id = $item_data['item_id'];
                 $new_item = true;
@@ -693,7 +695,7 @@ class Items extends Secure_Controller
 
                     $tax_name_index++;
                 }
-                $success = $success && $this->item_taxes->save_value($items_taxes_data, $item_id);
+                $success &= $this->item_taxes->save_value($items_taxes_data, $item_id);
             }
 
             // Save item quantity
@@ -714,7 +716,7 @@ class Items extends Secure_Controller
                 $item_quantity = $this->item_quantity->get_item_quantity($item_id, $location['location_id']);
 
                 if ($item_quantity->quantity != $updated_quantity || $new_item) {
-                    $success = $success && $this->item_quantity->save_value($location_detail, $item_id, $location['location_id']);
+                    $success = $success &&  $this->item_quantity->save_value($location_detail, $item_id, $location['location_id']);
 
                     $inv_data = [
                         'trans_date'      => date('Y-m-d H:i:s'),
@@ -729,21 +731,21 @@ class Items extends Secure_Controller
                 }
             }
             $success = $success && $this->saveItemAttributes($item_id);
+
+            if ($success && $upload_success) {
+                $message = lang('Items.successful_' . ($new_item ? 'adding' : 'updating')) . ' ' . $item_data['name'];
+
+                return $this->response->setJSON(['success' => true, 'message' => $message, 'id' => $item_id]);
+            } else {
+                $message = $upload_success ? lang('Items.error_adding_updating') . ' ' . $item_data['name'] : strip_tags($upload_data['error']);
+
+                return $this->response->setJSON(['success' => false, 'message' => $message, 'id' => $item_id]);
+            }
+        } else {
+            $message = lang('Items.error_adding_updating') . ' ' . $item_data['name'];
+
+            return $this->response->setJSON(['success' => false, 'message' => $message, 'id' => NEW_ENTRY]);
         }
-
-        // Check all success conditions before committing
-        if ($success && $upload_success) {
-            $this->db->transCommit();
-            $message = lang('Items.successful_' . ($new_item ? 'adding' : 'updating')) . ' ' . $item_data['name'];
-
-            return $this->response->setJSON(['success' => true, 'message' => $message, 'id' => $item_id]);
-        }
-
-        // Rollback on failure
-        $this->db->transRollback();
-        $message = $upload_success ? lang('Items.error_adding_updating') . ' ' . $item_data['name'] : strip_tags($upload_data['error']);
-
-        return $this->response->setJSON(['success' => false, 'message' => $message, 'id' => $item_id]);
     }
 
     /**
@@ -829,8 +831,8 @@ class Items extends Secure_Controller
      */
     public function getRemoveLogo($item_id): ResponseInterface
     {
-        $item_data = ['pic_filename' => null];
-        $result = $this->item->save_value($item_data, $item_id);
+        $item_data = ['pic_filename' => null, 'item_id' => $item_id];
+        $result = $this->item->saveValue($item_data);
 
         return $this->response->setJSON(['success' => $result]);
     }
@@ -1042,7 +1044,7 @@ class Items extends Secure_Controller
                             return $value !== null && strlen($value);
                         });
 
-                        if (!$isFailedRow && $this->item->save_value($itemData, $itemId)) {
+                        if (!$isFailedRow && $this->item->saveValue($itemData)) {
                             $this->save_tax_data($row, $itemData);
                             $this->save_inventory_quantities($row, $itemData, $allowedStockLocations, $employeeId);
                             $csvAttributeValues = $this->extractAttributeData($row);
@@ -1315,8 +1317,8 @@ class Items extends Secure_Controller
                 $images = glob(FCPATH . "uploads/item_pics/$item->pic_filename.*");
                 if (sizeof($images) > 0) {
                     $new_pic_filename = pathinfo($images[0], PATHINFO_BASENAME);
-                    $item_data = ['pic_filename' => $new_pic_filename];
-                    $this->item->save_value($item_data, $item->item_id);
+                    $item_data = ['pic_filename' => $new_pic_filename, 'item_id' => $item->item_id];
+                    $this->item->saveValue($item_data);
                 }
             }
         }

app/Models/Item.php

@@ -436,32 +436,62 @@ class Item extends Model
 
     /**
      * Inserts or updates an item
+     * 
+     * If the primary key (item_id) is present in the data array and the record exists,
+     * it will update the existing record. Otherwise, it will insert a new record.
+     * 
+     * @param array $data The item data to save (passed by reference to set item_id on insert)
+     * @return bool True on success, false on failure
      */
-    public function save_value(array &$item_data, int $item_id = NEW_ENTRY): bool    // TODO: need to bring this in line with parent or change the name
+    public function saveValue(array &$data): bool
     {
-        $builder = $this->db->table('items');
+        $primaryKey = $this->primaryKey;
+        $id = $data[$primaryKey] ?? NEW_ENTRY;
 
-        if ($item_id < 1 || !$this->exists($item_id, true)) {
-            if ($builder->insert($item_data)) {
-                $item_data['item_id'] = (int)$this->db->insertID();
-                if ($item_id < 1) {
-                    $builder = $this->db->table('items');
-                    $builder->where('item_id', $item_data['item_id']);
-                    $builder->update(['low_sell_item_id' => $item_data['item_id']]);
-                }
-
-                return true;
+        // If id > 0 and record exists by primary key only, update it
+        if ($id > 0) {
+            // Check existence strictly by primary key (regardless of soft-delete status)
+            $builder = $this->db->table('items');
+            $builder->where($primaryKey, $id);
+            $exists = $builder->countAllResults() > 0;
+            
+            if ($exists) {
+                // Remove primary key from data array for update
+                $updateData = $data;
+                unset($updateData[$primaryKey]);
+                
+                $builder = $this->db->table('items');
+                $builder->where($primaryKey, $id);
+                return $builder->update($updateData);
             }
-
-            return false;
-        } else {
-            $item_data['item_id'] = $item_id;
         }
 
+        // Insert new record with transaction for atomicity
+        $this->db->transBegin();
+        
+        // Remove primary key from insert payload if present
+        $insertData = $data;
+        unset($insertData[$primaryKey]);
+        
         $builder = $this->db->table('items');
-        $builder->where('item_id', $item_id);
-
-        return $builder->update($item_data);
+        $success = $builder->insert($insertData);
+        
+        if ($success) {
+            $data[$primaryKey] = (int)$this->db->insertID();
+            
+            // Update low_sell_item_id for new items
+            $builder = $this->db->table('items');
+            $builder->where($primaryKey, $data[$primaryKey]);
+            $success = $builder->update(['low_sell_item_id' => $data[$primaryKey]]);
+        }
+        
+        if ($success) {
+            $this->db->transCommit();
+            return true;
+        }
+        
+        $this->db->transRollback();
+        return false;
     }
 
     /**
@@ -1079,9 +1109,9 @@ class Item extends Model
         $total_quantity = $old_total_quantity + $items_received;
         $average_price = bcdiv(bcadd(bcmul((string)$items_received, (string)$new_price), bcmul((string)$old_total_quantity, (string)$old_price)), (string)$total_quantity);
 
-        $data = ['cost_price' => $average_price];
+        $data = ['cost_price' => $average_price, 'item_id' => $item_id];
 
-        return $this->save_value($data, $item_id);
+        return $this->saveValue($data);
     }
 
     /**

app/Models/Item_taxes.php

@@ -37,6 +37,9 @@ class Item_taxes extends Model
     {
         $success = true;
 
+        // Run these queries as a transaction, we want to make sure we do all or nothing
+        $this->db->transStart();
+
         $this->delete($item_id);
 
         $builder = $this->db->table('items_taxes');
@@ -46,6 +49,10 @@ class Item_taxes extends Model
             $success &= $builder->insert($row);
         }
 
+        $this->db->transComplete();
+
+        $success &= $this->db->transStatus();
+
         return $success;
     }
 

tests/Controllers/ItemsCsvImportTest.php

@@ -237,7 +237,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $row = $this->db->table('items')
             ->where('item_number', $itemData['item_number'])
@@ -268,7 +268,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $locationId = 1;
         $quantity = 100;
@@ -298,7 +298,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $inventoryData = [
             'trans_inventory' => 50,
@@ -329,7 +329,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $taxesData = [
             ['name' => 'VAT', 'percent' => 20],
@@ -406,7 +406,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
                 'deleted' => false
             ];
 
-            $this->assertTrue($this->item->save_value($itemData));
+            $this->assertTrue($this->item->saveValue($itemData));
         }
 
         $item1 = $this->item->get_info_by_id_or_number('ITEM-A');
@@ -430,7 +430,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($originalData));
+        $this->assertTrue($this->item->saveValue($originalData));
 
         $updatedData = [
             'item_id' => $originalData['item_id'],
@@ -443,7 +443,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($updatedData, $updatedData['item_id']));
+        $this->assertTrue($this->item->saveValue($updatedData));
 
         $updatedItem = $this->item->get_info($updatedData['item_id']);
         $this->assertEquals('Updated Name', $updatedItem->name);
@@ -464,7 +464,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($originalData));
+        $this->assertTrue($this->item->saveValue($originalData));
 
         $definitionData = [
             'definition_name' => 'Color',
@@ -510,7 +510,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $definitionData = [
             'definition_name' => 'Color',
@@ -553,7 +553,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         // Mock Attribute DROPDOWN
         $definitionData = [
@@ -604,7 +604,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $locationId = 1;
 
@@ -633,7 +633,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $savedItem = $this->item->get_info($itemData['item_id']);
         $this->assertEquals(-1, (int)$savedItem->reorder_level);
@@ -672,7 +672,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
                 'deleted' => 0
             ];
 
-            $this->assertTrue($this->item->save_value($itemData));
+            $this->assertTrue($this->item->saveValue($itemData));
 
             $savedItem = $this->item->get_info($itemData['item_id']);
 
@@ -702,7 +702,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $savedItem = $this->item->get_info($itemData['item_id']);
         $this->assertEquals('8471', $savedItem->hsn_code);
@@ -719,7 +719,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $locations = [
             'Warehouse' => 100,
@@ -792,7 +792,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $this->assertIsInt($itemData['item_id']);
         $this->assertGreaterThan(0, $itemData['item_id']);
@@ -812,7 +812,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $exists = $this->item->exists($itemData['item_id']);
         $this->assertTrue($exists);
@@ -858,7 +858,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $taxesData = [];
         if (is_numeric($csvRow['Tax 1 Percent']) && $csvRow['Tax 1 Name'] !== '') {
@@ -1032,7 +1032,7 @@ class ItemsCsvImportTest extends CIUnitTestCase
             'deleted' => 0
         ];
 
-        $this->assertTrue($this->item->save_value($itemData));
+        $this->assertTrue($this->item->saveValue($itemData));
 
         $uniqueId = uniqid();
         $locations = ['Warehouse' . $uniqueId, 'Store' . $uniqueId];