[1.3] Fix Door Lock cluster to not repond DUPLICATE when there is no duplication of credentials.#39279
Conversation
…ation of credentials. Spec issue tracking the spec not being clear: CHIP-Specifications/connectedhomeip-spec#11707 Cherry-pick of project-chip#39254
There was a problem hiding this comment.
Code Review
This pull request addresses an issue in the Door Lock cluster where setting a credential to its existing value would incorrectly result in a DUPLICATE response. The fix in door-lock-server.cpp correctly modifies the duplicate check logic to ignore the credential slot being operated on, which resolves the issue. The added YAML test case in DL_UsersAndCredentials.yaml specifically verifies this scenario, ensuring that modifying a credential to its current value is now allowed and returns a success status.
The changes are well-targeted, and the code is clear. The accompanying comments and test case enhance understanding and maintainability. Overall, this is a good fix that improves the correctness of the credential management logic.
Summary of Findings
- Code Correctness: The modification in
door-lock-server.cppcorrectly adjusts the duplicate credential check to ensure that setting a credential to its existing value does not trigger aDUPLICATEerror. This aligns with the intended behavior described in the associated spec issue. - Testing: The new YAML test case in
DL_UsersAndCredentials.yamleffectively validates the fix by attempting to modify a credential to its current value and expecting a successful outcome. This is a valuable addition for regression prevention. - Clarity and Maintainability: The code changes are minimal and easy to understand. The comment added in the C++ code provides excellent context regarding the spec ambiguity and the rationale for the change.
Merge Readiness
The changes in this pull request appear to be correct and well-tested for the specific issue they address. The code is clear and follows existing patterns. While I cannot approve the pull request myself, based on this review, the changes seem ready for merging pending successful CI checks and any other necessary reviews. No critical or high-severity issues were found in the changed code.
Spec issue tracking the spec not being clear:
https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/11707
Cherry-pick of #39254
Testing
YAML test added.