fix: Fix block update bug #245
Conversation
…ava into feat/develop
WalkthroughA new method was added to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
base/src/main/java/com/tinyengine/it/mapper/BlockGroupBlockMapper.java(1 hunks)base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java(2 hunks)base/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java(3 hunks)
🔇 Additional comments (6)
base/src/main/java/com/tinyengine/it/mapper/BlockGroupBlockMapper.java (2)
97-97: LGTM: Javadoc correctionThe return type documentation has been correctly updated from "list" to "Integer" to match the actual method signature.
102-108: LGTM: Well-implemented deletion methodThe new
deleteByBlockIdmethod follows established patterns in the codebase:
- Proper SQL parameterization prevents injection vulnerabilities
- Clear method naming and documentation
- Consistent return type with similar methods
- Appropriate use of
@Deleteannotationbase/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java (3)
30-30: LGTM: Proper import additionThe import for
BlockGroupBlockMapperis correctly added to support the new mock dependency.
73-74: LGTM: Appropriate mock setupThe
@Mockannotation forBlockGroupBlockMapperfollows the established pattern in the test class.
117-117: LGTM: Correct test stub additionThe mock stub for
blockGroupBlockMapper.deleteByBlockId(123)properly supports the updated service implementation and maintains test coverage.base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java (1)
175-175: LGTM: Improved authorization checkThe authorization logic has been appropriately updated to check
blockResult.getOccupierBy()against the current user instead of comparing app IDs. This provides more granular and accurate access control based on actual block ownership.
base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base/src/main/java/com/tinyengine/it/controller/BlockController.java (1)
56-59: Remove unused import.The
Collectorsimport on line 59 is not used in the visible code and should be removed to keep imports clean.import java.util.Arrays; -import java.util.stream.Collectors;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
base/src/main/java/com/tinyengine/it/controller/BlockController.java(2 hunks)base/src/main/java/com/tinyengine/it/mapper/BlockMapper.java(1 hunks)base/src/main/java/com/tinyengine/it/model/dto/NotGroupDto.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
base/src/main/java/com/tinyengine/it/controller/BlockController.java (1)
base/src/main/java/com/tinyengine/it/common/utils/JsonUtils.java (1)
JsonUtils(45-349)
🔇 Additional comments (5)
base/src/main/java/com/tinyengine/it/model/dto/NotGroupDto.java (2)
34-36: LGTM! Addition of name field enhances filtering capabilities.The new
namefield with proper schema annotation allows filtering by block name, which aligns with the coordinated changes in the controller and mapper layers.
42-42: LGTM! Field type change aligns with JSON encoding approach.The change from
String[]toStringfor thetagsfield is consistent with the controller's new approach of JSON-encoding tag arrays before setting them on the DTO.base/src/main/java/com/tinyengine/it/mapper/BlockMapper.java (1)
159-165: Critical: Confirm the OR-based filtering infindBlocksReturnis intentionalWe didn’t find any existing tests or documentation that validate the switch from AND to OR logic in the mapper:
- No unit or integration tests exercise
BlockMapper.findBlocksReturn(...)directly.- Service-layer tests mock the mapper and don’t assert on SQL behavior.
- Without coverage, the OR logic change may introduce unintended search results.
Please review and confirm that returning blocks matching any of the criteria (label, name, createdBy, or tags) is the desired behavior.
base/src/main/java/com/tinyengine/it/controller/BlockController.java (2)
260-266: LGTM! Addition of name parameter enhances filtering capabilities.The new
name_cn_containsparameter properly mapped tonameextends the filtering functionality and is correctly set on theNotGroupDtoobject.
269-271: LGTM! JSON encoding approach handles tags properly.The logic to JSON-encode the tags array when it's non-null and non-empty is appropriate and aligns with the DTO field type change from
String[]toString. TheJsonUtils.encode()method will handle the array-to-JSON conversion correctly.
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Bug Fixes
New Features
Tests