refactor: Unify Item model save_value signature - #4510
Conversation
- Rename save_value() to saveValue() for PSR compliance - Remove second parameter (item_id) - now derived from data array - Check for item_id in data to determine insert vs update - Update all call sites in Items controller - Update test file references Part of #4459
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughController, model, and tests were updated to use a renamed upsert method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Models/Item.php (1)
446-472: RefactoredsaveValuemethod looks good overall.The method correctly derives
item_idfrom the data array and implements upsert logic. A few observations:
When performing an UPDATE (line 455), the
$dataarray may containitem_id, which is not in$allowedFields. CodeIgniter's Query Builder should filter this out, but it's cleaner to explicitly exclude the primary key from the update payload.On line 452,
exists($id, true)withignore_deleted=truemeans updates will target even soft-deleted items. Verify this is the intended behavior.♻️ Optional: Exclude primary key from update data
// If id > 0 and record exists, update it if ($id > 0 && $this->exists($id, true)) { $builder = $this->db->table('items'); $builder->where($primaryKey, $id); + $updateData = array_diff_key($data, [$primaryKey => true]); - return $builder->update($data); + return $builder->update($updateData); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 446 - 472, The saveValue method may send the primary key in the update payload and currently calls exists($id, true) which includes soft-deleted records; before updating in saveValue, remove the primary key key (use $this->primaryKey) from the $data array to ensure only allowed fields are updated (so the Query Builder doesn't receive item_id), and confirm whether using exists($id, true) (the second param that ignores deleted state) is intended—if not, change to exists($id) or pass false; also ensure the low_sell_item_id update logic still uses the inserted ID from $this->db->insertID().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/Items.php`:
- Around line 483-487: The code saves an empty item_number because it never
generates a barcode when $item['item_number'] is empty despite
$this->config['barcode_generate_if_empty'] being true; fix by generating a new
barcode value before saving: inside the same conditional that checks
isset($item['item_number']) && empty($item['item_number']) &&
$this->config['barcode_generate_if_empty'], call or implement a barcode
generator (e.g. a new helper or an existing method like
$this->item->generateBarcode or BarcodeGenerator::generate) to produce a
non-empty value, assign that value to $save_item['item_number'], then call
$this->item->saveValue($save_item) (keeping the isset($item['item_id']) guard),
and ensure the generated barcode is validated/unique before persisting.
---
Nitpick comments:
In `@app/Models/Item.php`:
- Around line 446-472: The saveValue method may send the primary key in the
update payload and currently calls exists($id, true) which includes soft-deleted
records; before updating in saveValue, remove the primary key key (use
$this->primaryKey) from the $data array to ensure only allowed fields are
updated (so the Query Builder doesn't receive item_id), and confirm whether
using exists($id, true) (the second param that ignores deleted state) is
intended—if not, change to exists($id) or pass false; also ensure the
low_sell_item_id update logic still uses the inserted ID from
$this->db->insertID().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e15e8752-0103-4c7f-9482-9199509e3d3a
📒 Files selected for processing (3)
app/Controllers/Items.phpapp/Models/Item.phptests/Controllers/ItemsCsvImportTest.php
| 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); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's barcode generation logic that should be invoked here
rg -n -C5 'barcode_generate_if_empty' --type=phpRepository: opensourcepos/opensourcepos
Length of output: 50383
🏁 Script executed:
sed -n '480,495p' app/Controllers/Items.phpRepository: opensourcepos/opensourcepos
Length of output: 666
🏁 Script executed:
# Search for barcode generation functions
rg -n 'generate.*barcode|barcode.*generate' --type=php -iRepository: opensourcepos/opensourcepos
Length of output: 18109
🏁 Script executed:
# Look for functions in Barcode_lib
rg -n 'function|public|private' app/Libraries/Barcode_lib.php | head -20Repository: opensourcepos/opensourcepos
Length of output: 841
🏁 Script executed:
# Read the generate_barcode function in Barcode_lib
sed -n '107,125p' app/Libraries/Barcode_lib.phpRepository: opensourcepos/opensourcepos
Length of output: 698
🏁 Script executed:
# Read the get_barcode_value and display_barcode functions
sed -n '95,165p' app/Libraries/Barcode_lib.phpRepository: opensourcepos/opensourcepos
Length of output: 2859
🏁 Script executed:
# Check the Item model saveValue method
rg -n 'saveValue|function saveValue' app/Models/Item.php -A 10Repository: opensourcepos/opensourcepos
Length of output: 845
🏁 Script executed:
# Look for barcode generation functions (UUID, sequence, etc.)
rg -n 'generate.*barcode|uuid|barcode.*id|barcode.*number' app/Models/Item.php -iRepository: opensourcepos/opensourcepos
Length of output: 118
🏁 Script executed:
# Get full context of getGenerateBarcodes function
sed -n '470,495p' app/Controllers/Items.phpRepository: opensourcepos/opensourcepos
Length of output: 1338
Logic error: Saving empty item_number value.
The condition on line 483 checks if $item['item_number'] is empty, but line 485 assigns that same empty value to $save_item['item_number']. The code should generate a new barcode value when the item lacks one and barcode_generate_if_empty is enabled, but the barcode generation logic is missing entirely. Currently, it only persists the empty value back to the database.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/Items.php` around lines 483 - 487, The code saves an empty
item_number because it never generates a barcode when $item['item_number'] is
empty despite $this->config['barcode_generate_if_empty'] being true; fix by
generating a new barcode value before saving: inside the same conditional that
checks isset($item['item_number']) && empty($item['item_number']) &&
$this->config['barcode_generate_if_empty'], call or implement a barcode
generator (e.g. a new helper or an existing method like
$this->item->generateBarcode or BarcodeGenerator::generate) to produce a
non-empty value, assign that value to $save_item['item_number'], then call
$this->item->saveValue($save_item) (keeping the isset($item['item_id']) guard),
and ensure the generated barcode is validated/unique before persisting.
- Unset item_id from data array before update - Cleaner approach to avoid including PK in update payload Address CodeRabbit review feedback
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Item.php`:
- Around line 462-472: Wrap the insert + low_sell_item_id update in a DB
transaction and return success only if both operations succeed: start a
transaction via $this->db->transStart()/transComplete() (or
transBegin()/transCommit()/transRollback()), perform $builder->insert($data) and
capture $this->db->insertID(), then perform the follow-up
$builder->update(['low_sell_item_id' => $data[$primaryKey]]) and check its
boolean result; on failure roll back (or rely on transStatus()) and return
false, otherwise commit and return true. Ensure you reference the same $builder,
$primaryKey and $this->db->insertID() values used currently so the update
targets the newly inserted row.
- Around line 449-453: The update branch is being chosen when non-primary unique
fields (like item_number/barcode) match because the code uses exists($id, true)
which can match by other identifiers; change the decision to a primary-key-only
check so only an explicit primary key triggers UPDATE. Replace the exists($id,
true) usage in the block that evaluates $id = $data[$primaryKey] ?? NEW_ENTRY
with a call that verifies existence strictly by primary key (e.g. add/use an
existsByPrimaryKey($id) or adjust exists() to accept a primary-only flag) and
use that instead to decide the update path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Replace exists() with direct primary key check to avoid matching by other identifiers - Wrap insert + low_sell_item_id update in transaction for atomicity - Check db insert result and rollback on failure Address CodeRabbit review feedback
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Item.php`:
- Around line 451-457: The existence check in the Item model's upsert logic
erroneously excludes soft-deleted rows by adding ->where('deleted', 0), causing
records with a valid primary key ($primaryKey / $id) to be treated as not found;
remove the ->where('deleted', 0) condition (or otherwise ensure the existence
check only filters by $primaryKey and $id using the $builder on table 'items')
so $exists correctly reflects any record with that primary key regardless of
soft-delete status, allowing the code that relies on $exists to choose UPDATE
rather than INSERT.
- Around line 449-475: The fallback insert path currently inserts
caller-supplied primary key from $data (when $id > 0 but update isn't taken);
before calling $builder->insert($data) remove the primary key from the insert
payload (e.g. unset $data[$primaryKey] or create $insertData = $data and unset
$insertData[$primaryKey]) and use that sanitized payload for the insert, keeping
the existing transaction logic ($this->db->transBegin() / insert /
commit/rollback) intact; reference $primaryKey, $data, NEW_ENTRY, and the
$builder->insert(...) call to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- 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
|
|
||
| if ($this->item->save_value($item_data, $item_id)) { | ||
| // For updates, include item_id in data array | ||
| if ($item_id !== NEW_ENTRY) { |
There was a problem hiding this comment.
It's helpful if we refactor local variables as we touch them to be PSR compliant. Not mandatory but helpful.
| $item_data['item_id'] = $item_id; | ||
| } | ||
|
|
||
| if ($this->item->saveValue($item_data)) { |
| { | ||
| $item_data = ['pic_filename' => null]; | ||
| $result = $this->item->save_value($item_data, $item_id); | ||
| $item_data = ['pic_filename' => null, 'item_id' => $item_id]; |
There was a problem hiding this comment.
Helpful but not required: refactor local variables to PSR compliant naming.
| $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); |
There was a problem hiding this comment.
Helpful but not required: refactor local variables you touch here to PSR compliant names.
| $builder->update(['low_sell_item_id' => $item_data['item_id']]); | ||
| } | ||
| $primaryKey = $this->primaryKey; | ||
| $id = $data[$primaryKey] ?? NEW_ENTRY; |
There was a problem hiding this comment.
Since we are dealing with items.item_id this variable might be better named as $itemId instead of the generic $id.
| $id = $data[$primaryKey] ?? NEW_ENTRY; | ||
|
|
||
| return true; | ||
| // If id > 0 and record exists by primary key only, update it |
There was a problem hiding this comment.
Replace this comment with // Update
| $item_data['item_id'] = $item_id; | ||
| } | ||
|
|
||
| // Insert new record with transaction for atomicity |
There was a problem hiding this comment.
Replace this comment with // Insert
| $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]; |
There was a problem hiding this comment.
Not required but good: refactor $item_id in this function to PSR compliant code
2 participants
Summary
save_value()tosaveValue()for PSR complianceitem_id) - now derived from data arrayitem_idin data to determine insert vs updateProblem
Model
save_value()functions had inconsistent signatures. Some took the primary key as an optional second parameter, others required it. Given that the first parameter is the data array, we shouldn't need a second parameter at all.Solution
This PR focuses on the
Itemmodel as a representative subset:save_value()tosaveValue()(PSR naming)item_idfrom data arrayitem_id > 0and exists → UPDATE, else → INSERTItems.phpcontrollerPart of #4459
Ultraworked with Sisyphus
Co-authored-by: Sisyphus clio-agent@sisyphuslabs.ai
Summary by CodeRabbit
Note: No user-facing behavior or public APIs changed.