HP-2828: enhance StockLocationsListTreeSelect by adding `build…
#212
Conversation
…AliasGroupTree` method, refactor `nestRackTreeChildren` to `nestTreeChildren`, and improve stock location type handling
WalkthroughRefactors stock location handling: indexes locations via ArrayHelper, decodes LocationItem objects JSON, adds alias-group tree construction in the tree-select widget, adjusts StockLocationsRepository mapping, and changes ModelGridView constructor to use property promotion for StockLocationsProvider and User. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/grid/ModelGridView.php (1)
118-125: Missing null check for location lookup.Line 120 directly accesses
$locations[$key]without verifying the key exists. WhilegetLocations()filters against available IDs (perStockLocationsProvider), race conditions or cache staleness could cause a mismatch. Consider adding a guard.🔎 Suggested defensive check:
$locations = ArrayHelper::index($this->locationsProvider->getAllLocations(), 'id'); foreach ($this->locationsProvider->getLocations() as $key) { + if (!isset($locations[$key])) { + continue; + } $location = $locations[$key];
🧹 Nitpick comments (1)
src/models/Model.php (1)
195-196: Vague TODO comment lacks actionable context.The comment
// todo: fix this!doesn't explain what needs to be fixed. Consider adding a description of the issue, a ticket reference (e.g., the HP-2828 task or a separate issue), or removing it if the fix is already tracked elsewhere.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/grid/ModelGridView.php(2 hunks)src/models/Model.php(1 hunks)src/models/VO/LocationItem.php(2 hunks)src/widgets/StockLocationsListTreeSelect.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/grid/ModelGridView.php (2)
src/helpers/StockLocationsProvider.php (2)
getAllLocations(27-30)getLocations(37-62)src/repositories/StockLocationsRepository.php (1)
getLocations(25-38)
src/widgets/StockLocationsListTreeSelect.php (1)
src/models/VO/LocationItem.php (1)
LocationItem(10-87)
🔇 Additional comments (6)
src/models/VO/LocationItem.php (2)
46-48: LGTM on the expanded label condition.The addition of
str_starts_with($this->id, 'alias_group')correctly extends the shortcut label logic to handle new alias group IDs, aligning with thebuildAliasGroupTreemethod inStockLocationsListTreeSelect.
32-32: Consider handling malformed JSON gracefully.
Json::decodethrowsInvalidArgumentExceptionif the JSON is malformed. The?? '[]'handles null/missing data, but if$data['objects']contains an invalid JSON string, this will throw. Since this is a public factory method processing external data, consider wrapping in a try-catch block or validating upstream data quality.src/widgets/StockLocationsListTreeSelect.php (3)
221-230: LGTM on the options assembly refactor.The integration of
buildAliasGroupTreealongside existing CHW and rack trees is clean. The commented-out legacy tree is appropriately marked with a TODO for future cleanup.
272-282: Good refactor: arrow function and method rename.The filter refactor to an arrow function improves readability. Renaming
nestRackTreeChildrentonestTreeChildrenbetter reflects its general-purpose nature for building hierarchical structures.
248-253: Verify whetherremoveKeysRecursively()properly handles deeply nested array keys.The nested children arrays created at lines 248-253 have string keys (
$objName) that persist through multiple nesting levels. Whilearray_values()resets the outer array's keys, nested children retain their string keys. SinceremoveKeysRecursively()is defined in the parent class (external dependency), its implementation cannot be verified in this repository. If it doesn't recursively process all nested children arrays, the resulting tree structure may have unexpected string keys when JSON encoded.src/grid/ModelGridView.php (1)
26-33: LGTM on constructor property promotion.The use of
private readonlyproperty promotion is clean and reduces boilerplate. The dependencies are properly injected and typed.
…roup handling; remove redundant null filtering in `StockLocationsRepository`, and cleanup in `Model` formatting and link construction
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/widgets/StockLocationsListTreeSelect.php (1)
233-271: Address the undefined array key issue flagged in previous review.As noted in the previous review comment, Line 254 accesses
$result[$l->type->value]['children']without ensuring the'children'key exists. This can cause an undefined array key error if no:ANYitem was processed first for that type, or if items arrive in a different order than expected.The initialization suggested in the previous review should be implemented to ensure the array structure exists before accessing it.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/models/Model.php(3 hunks)src/repositories/StockLocationsRepository.php(1 hunks)src/widgets/StockLocationsListTreeSelect.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/repositories/StockLocationsRepository.php (1)
src/models/VO/LocationItem.php (2)
LocationItem(10-87)fromArray(30-42)
src/widgets/StockLocationsListTreeSelect.php (1)
src/models/VO/LocationItem.php (1)
LocationItem(10-87)
🔇 Additional comments (6)
src/models/Model.php (2)
120-120: LGTM - Formatting improvement.The trailing comma is a good practice for multi-line arrays and doesn't change behavior.
184-202: LGTM - Standard URL generation pattern.The refactoring to use
Url::to()with a structured route array is a standard Yii2 pattern and improves readability. The parameters are appropriately structured for a search URL.src/widgets/StockLocationsListTreeSelect.php (4)
13-13: LGTM - Required import for new functionality.The
ArrayHelperimport is necessary for the newbuildAliasGroupTreemethod that usesArrayHelper::index().
46-46: Good defensive programming.Handling the empty string case explicitly prevents attempting to explode an empty value, which would result in an array with a single empty string element. Returning an empty array directly is the correct behavior.
278-281: LGTM - Cleaner arrow function syntax.The refactoring to use arrow function syntax is more concise while maintaining the same filtering logic.
288-288: LGTM - Good generalization of method name.Renaming
nestRackTreeChildrentonestTreeChildrenbetter reflects that the method is used for general tree nesting, not just racks. The rename is consistently applied throughout the file.Also applies to: 299-299, 314-314
| static fn(array $item) => LocationItem::fromArray($item), | ||
| array_filter($data, static fn(array $datum) => !in_array(null, $datum, true)) | ||
| ); | ||
| return array_map(fn(array $item) => LocationItem::fromArray($item), $data); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find LocationItem and StockLocationsRepository files
find . -type f -name "*.php" | grep -E "(LocationItem|StockLocationsRepository)" | head -20Repository: hiqdev/hipanel-module-stock
Length of output: 151
🏁 Script executed:
#!/bin/bash
# 1. First, locate the relevant files
git ls-files | grep -E "(LocationItem|StockLocationsRepository)" | head -20Repository: hiqdev/hipanel-module-stock
Length of output: 147
🏁 Script executed:
#!/bin/bash
# 2. Check the LocationItem::fromArray implementation
if git ls-files | grep -q "LocationItem"; then
files=$(git ls-files | grep -i "locationitem")
for file in $files; do
echo "=== File: $file ==="
cat -n "$file" 2>/dev/null || echo "Could not read $file"
done
fiRepository: hiqdev/hipanel-module-stock
Length of output: 3755
🏁 Script executed:
#!/bin/bash
# 3. Check StockLocationsRepository for the current code
if git ls-files | grep -q "StockLocationsRepository"; then
file=$(git ls-files | grep "StockLocationsRepository")
echo "=== File: $file ==="
wc -l "$file"
fiRepository: hiqdev/hipanel-module-stock
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Read the StockLocationsRepository file
cat -n src/repositories/StockLocationsRepository.phpRepository: hiqdev/hipanel-module-stock
Length of output: 1219
🏁 Script executed:
#!/bin/bash
# Check git history to see if there was a previous filter that was removed
cd $(git rev-parse --show-toplevel)
git log --oneline -20 src/repositories/StockLocationsRepository.phpRepository: hiqdev/hipanel-module-stock
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Get the diff of recent changes to this file
git log -p -2 src/repositories/StockLocationsRepository.php | head -100Repository: hiqdev/hipanel-module-stock
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check if enum::from() throws on null values by looking at PHP docs behavior
# Also check if there's any type that allows null in the data structure
rg -B5 -A5 'stock-locations-list' src/Repository: hiqdev/hipanel-module-stock
Length of output: 1741
🏁 Script executed:
#!/bin/bash
# Get the actual diff showing what was removed
git show 3fd9676 -- src/repositories/StockLocationsRepository.phpRepository: hiqdev/hipanel-module-stock
Length of output: 1588
🏁 Script executed:
#!/bin/bash
# Check the enums to understand their behavior
cat -n src/enums/StockLocationCategory.php src/enums/StockLocationType.php 2>/dev/null | head -50Repository: hiqdev/hipanel-module-stock
Length of output: 1583
🏁 Script executed:
#!/bin/bash
# Search for Model::perform to understand what data structure it returns
rg -A10 'function perform' src/models/Model.phpRepository: hiqdev/hipanel-module-stock
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Find and read the Model class
git ls-files | grep -E "models/Model\.php" | xargs cat -nRepository: hiqdev/hipanel-module-stock
Length of output: 9494
🏁 Script executed:
#!/bin/bash
# Check previous version before the commit to see what was removed
git show 3fd9676~1:src/repositories/StockLocationsRepository.php 2>/dev/null || echo "Previous version not found or file didn't exist"Repository: hiqdev/hipanel-module-stock
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Search for any previous usage patterns in git history that might show null filtering
git log --all --source --remotes -S "array_filter" -- "*StockLocations*" 2>/dev/null | head -20Repository: hiqdev/hipanel-module-stock
Length of output: 1091
🏁 Script executed:
#!/bin/bash
# Look for any other files that might have had this code before the refactor
git show 3fd9676~1:src/ 2>/dev/null | grep -i "location" || echo "Checking other locations..."
git log --name-status --oneline 3fd9676~1 | grep -i locationRepository: hiqdev/hipanel-module-stock
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Check if there's a commit message or PR info mentioning what was refactored
git show 3fd9676 --statRepository: hiqdev/hipanel-module-stock
Length of output: 16979
🏁 Script executed:
#!/bin/bash
# Check the parent commit to see the state before this change
git show aa66070104fb038eb0277d567b55a25ec41956e4:src/repositories/StockLocationsRepository.php 2>/dev/null | head -50Repository: hiqdev/hipanel-module-stock
Length of output: 1179
Flag the assumption that the API now guarantees non-null category and location_type values.
Removing the null filter means LocationItem::fromArray will receive all items from $data. Since the method uses StockLocationCategory::from() and StockLocationType::from() without null checks, passing null for category or location_type will throw a TypeError at runtime. The previous code's TODO comment indicates this filter was temporary until the API returned correct types. Verify that the API contract now guarantees these fields are never null before removing this filter.
Additionally, the arrow function doesn't use $this or capture parent scope variables, so marking it static fn(array $item) would clarify intent and align with the previous implementation.
2 participants
…AliasGroupTree
method, refactornestRackTreeChildrentonestTreeChildren`, and improve stock location type handlingSummary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.