Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Z-Wave controller hard reset device action (certification req) #18216

Merged
merged 14 commits into from
Oct 24, 2023

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Oct 14, 2023

Proposed change

This PR adds a new device action for the controller to hard reset it which is a Z-Wave certification requirement. We want it to be hard to do to avoid accidental resets which is why there are multiple confirmations. There is an issue that we may need to resolve beforehand where the config entry drops and reloads but need opinions on that. Also is it weird that the device info underneath the dialog goes to Device / service not found? CC @MartinHjelmare @marcelveldt @AlCalzone - please review the copy and provide feedback.

Upstream core changes are already merged.

Device action:
action

First warning:
first warning

Second warning:
second warning

In progress:
In Progress

Complete:
Complete

Final device info page when you close the dialog:
not found

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@AlCalzone
Copy link
Contributor

Looks good to me. As for the last error, the factory reset changes the home ID of the controller. I think this is why the device isn't being found anymore.

@raman325
Copy link
Contributor Author

Yeah that's a good point. I don't think we want the device to be the same so I was more just wondering about the experience. There may be a way to back out of that when you close the dialog, I'm just not sure how at the moment. That does give me an idea though.... 🤔

That may be related to the experience with the config entry where it needs to disconnect and reconnect, but really I just need to enable debug logs so I can see what's going on

@raman325
Copy link
Contributor Author

I added logic which works when it's not wrapped in the conditional but doesn't work within the conditional (how the code looks now). I feel like there is something obvious I must be missing...

@raman325 raman325 changed the title Add Z-Wave controller hard reset device action Add Z-Wave controller hard reset device action (certification req) Oct 15, 2023
})
) {
await hardResetController(this.hass, this.entry_id!);
this._done = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Navigate to the devices page of the config entry here:

navigate(`/config/devices/dashboard?config_entry=${this.entry_id}`)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works, but the downside is that it takes the user to potentially a new location instead of going back to where they were, which is, in my opinion better. But I am realizing part of the problem is that the history state will include the pop ups. So if I want to do this, I think I need to store the previous page from the history and then navigate to that when done. Does that sound right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I guess you aren't allowed to do that... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented it with your suggestion for now, but if there's another way that we can truly go back to the previous page, I think that would be the better option

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best user experience would be to navigate to the device page of the newly created device, but I'm not sure how the timing is, is the new device available immediately? Maybe hardResetController could return the new device id or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so I got it working but:

  1. If the user closes the dialog, the redirect doesn't happen. I think this could be solved by changing hardResetController to only return when the new device is available but I will have to do some testing.
  2. I'm guessing the method I am using to track the device is inefficient. Might be solved by the solution for 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so made the updates above. Now the only problem is that when I navigate to the new device page, it says the device doesn't exist until I refresh the page. Not sure if there's something else I can wait for to guarantee the device is available before redirecting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code the page should update when new devices are added... And a refresh should not be needed...

I also see that happening in my tests...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so we can remove the refresh and log a bug for this maybe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a quick repo for me for this?

@codyc1515
Copy link
Contributor

Is it worth using the word Warning somewhere in a header / title for those fast-clickers among us?

bramkragten
bramkragten previously approved these changes Oct 23, 2023
bramkragten
bramkragten previously approved these changes Oct 23, 2023
@bramkragten bramkragten merged commit d8d16c4 into home-assistant:dev Oct 24, 2023
12 checks passed
@raman325 raman325 deleted the hard_reset branch October 24, 2023 23:00
@AlCalzone
Copy link
Contributor

It's a bit awkward that the "back" button goes back to the hard reset dialog atm.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Z-Wave JS: Controller hard reset needs to be exposed
4 participants