search

UpstreamDataInc / goosebit

A simplistic, opinionated remote update server implementing hawkBit™'s DDI API.
https://goosebit.rtfd.io
Apache License 2.0
18 stars 5 forks source link

Improved selection handling #138

Closed b-rowan closed 1 month ago

b-rowan commented 2 months ago

Improves selection handling as well as some UI stuff. Main goal here is to standardize the forms a bit and add a search bar to software selection.

Modal for setting device firmware has changed pretty heavily, and is now using tabs based on update mode (rollout, latest, and manual).

Modal for rollout creation is mostly unchanged save for the addition of the search bar.

Note, there are some slightly hacky workarounds in here to allow validation to work properly and not look stupid, mostly CSS in nav.html.jinja. The CSS is targeting the search bar inside the select, which is an input element, meaning it is attempting to validate itself on form submit.

Fixes: #136

tsagadar commented 2 months ago

Really like the new "edit device" UI. In my case, I was able to assign a software which was not compatible with the device. After this, the UI always display an error while loading and the backend prints

  File "/Users/marcelmueller/development/gb3/goosebit/ui/bff/devices/responses.py", line 35, in convert
    data = [DeviceSchema.model_validate(d) for d in devices]

Likely two things missing:

  1. at a minimum the backend should prevent setting the assigned software to a device if it is not compatible. With the batch actions, I am not sure if it should simply fail if one of the devices is not compatible or only update the devices that are.
  2. If we could add a filtering that only displays compatible software in the UI, then it would be most convenient for the user and the backend could simply fail if one of the devices is not compatible.

Ideally, we could event add constraints to the database to disallow an invalid state. Not sure from the top of my head how to do this - and not sure if it then works for SqLite and Postgres.

b-rowan commented 2 months ago

In my case, I was able to assign a software which was not compatible with the device.

Hmm. I'll see what I can do. Maybe an API endpoint for POST ui/bff/devices/compatible/software or something? Then the UI just queries that, and it would filter based on software valid for all those devices?

Need something similar for rollouts I feel as well...

b-rowan commented 2 months ago

I am not sure if it should simply fail if one of the devices is not compatible or only update the devices that are.

My thought was if devices are compatible, use it. Keep track of incompatible devices, and return an error with those devices listed.

tsagadar commented 2 months ago

Tried out to add a safety guard as close to the database as possible. Looked too complex to implement with SQL triggers or check constraints. So opted for overwriting the save method: https://github.com/husqvarnagroup/goosebit/pull/new/mm/dev_selects

tsagadar commented 2 months ago

In my case, I was able to assign a software which was not compatible with the device.

Hmm. I'll see what I can do. Maybe an API endpoint for POST ui/bff/devices/compatible/software or something? Then the UI just queries that, and it would filter based on software valid for all those devices?

Need something similar for rollouts I feel as well...

Or rather extend the /bff/software to accept device ids as query params? The it could only return compatible softwares to be displayed.

b-rowan commented 2 months ago

Or rather extend the /bff/software to accept device ids as query params?

Yeah, I thought about this afterwards, I think this is the way to go.

b-rowan commented 1 month ago

Ok, I have done all that, but I'm doing it in a couple of PRs for now.

I also have the commit prepared for the frontend update to handle this, I'll merge it in here when #141 gets merged.

If you want a preview, it's on the dev_select_full_fix branch.

b-rowan commented 1 month ago

I think your fix is correct actually, the issue is that the related models aren't being loaded when we need them. Should be fixed now.

b-rowan commented 1 month ago

While testing software compatibility filters, I was changing the hw_revision value of a fake device to make sure the filters were working as expected. When I did this with a software assigned, the configData endpoint in DDI raises an error, as with the changes here, the Device cannot be updated in the DB while software and hardware are conflicting.

My question is, do we want to handle this? My gut feeling is we don't care, this should only every show up when tweaking things during testing, and never in production. Hardware revisions of a physical device should never change. That being said, it is an edge case that should maybe be discussed.

If we do want to fix this, I think the solution is some kind of ERROR status signalling, indicating that a device needs manual intervention of some kind, and setting assigned software to None in the DB. This leads to another discussion, unrelated, about having a page dedicated to devices in an error state (something I think we want in the future, especially once we add rollout abort support in v0.4.0).

tsagadar commented 1 month ago

My question is, do we want to handle this? My gut feeling is we don't care, this should only every show up when tweaking things during testing, and never in production. Hardware revisions of a physical device should never change. That being said, it is an edge case that should maybe be discussed.

If we detect a change in hw_model or hw_revision I think it is best to re-create the device and log a warning - i.e. falling back to the default update_mode "ROLLOUT". As you mention - should not really happen but maybe the simplest approach to avoid the device to become immutable.

Or we just leave it. Currently I'd not spend too much time on this edge case.

b-rowan commented 1 month ago

Also, I think we should sort the entries by version, in descending order. Currently, sorting seems to be alphabetic. Any chance we can get actual semantic version sorting?

I think we should wait (just a bit) on this. If we decide to transition to SQLModel rather than tortoise, then the mapping into SemVer will happen automatically, and we can sort in the backend much easier. If its a huge concern, I can add something to parse it now, but I think this is a small enough issue for now that we should leave it until we make a decision on the DB side.

b-rowan commented 1 month ago

Or we just leave it. Currently I'd not spend too much time on this edge case.

I think this is the best option for now.

If we detect a change in hw_model or hw_revision I think it is best to re-create the device and log a warning - i.e. falling back to the default update_mode "ROLLOUT".

I would rather not do it this way. I tend to think of logging things as failing silently to some extent. A user shouldn't have to look at the logs unless something is obviously wrong, and I dont think resetting to rollout is obvious enough. That was why I though we could have a failure state on the frontend, then it can be highlighted in red and bolded or something to make it obvious to the user that something is wrong that they have to fix.