Delete guest user when SetYearDaySchedule is failed#2876
Delete guest user when SetYearDaySchedule is failed#2876HunsupJung wants to merge 2 commits intomainfrom
Conversation
|
Invitation URL: |
Test Results 72 files 502 suites 0s ⏱️ Results for commit 2c02717. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 2c02717 |
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
c3e1b38 to
9b916c7
Compare
| else | ||
| device.log.warn(string.format("Failed to clear user: %s", status)) | ||
| end |
There was a problem hiding this comment.
In the case this fails, what is the expected result?
Given the current logic, a failure would mean that
- lockCredentials commandResult is never emitted (HIGH importance)
- lockUsers commandResult for defaultSchedules will be emitted below, which will fail (MEDIUM importance)
Should we retry the ClearUser command again? If so, how many times?
Also, if it fails after some number of retries, I think we should emit a success result for lockCredentials. What do you think?
There was a problem hiding this comment.
lockCredentials commandResult is never emitted (HIGH importance)
lockUsers commandResult for defaultSchedules will be emitted below, which will fail (MEDIUM importance)
I don't understand what you said. The logic up to here is as follows.
- Plugin sends
setCredentialfor guest - Hub sends
SetCredentialto Door Lock - Door Lock creates a user and set the credential
- Door Lock returns a success result
- Hub sends
SetYearDaySchedulefor guest - Door Lock returns a failure result
- Hub sends
ClearUserto Door Lock - Door Lock clear the user and return a success result
- Hub set the
commandResultoflockCredentialsto failure
There is no additional action. I don't think it's the right way to continue follow-up on failures. And now we are putting the default schedule directly from the driver for guest, but I will discuss with Plugin and ask Plugin to put the default schedule on its own. (It is dangerous for the driver to put the schedule on device because the schedule operation may vary from device to device.)
ctowns
left a comment
There was a problem hiding this comment.
@HunsupJung could you also fill out the Summary of Completed Tests section on the top PR comment?
| delete_aliro_from_table_as_user(device, userIdx) | ||
| delete_week_schedule_from_table_as_user(device, userIdx) | ||
| delete_year_schedule_from_table_as_user(device, userIdx) | ||
| if cmdName == "defaultSchedule" then |
There was a problem hiding this comment.
I think this handling that is specific to the defaultSchedule command could benefit from a short comment that explains why this is needed to improve readability.
There was a problem hiding this comment.
Okay, I will add the comment.
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
Type of Change
Checklist
Description of Change
Summary of Completed Tests