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

Improved selection handling #138

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Improved selection handling #138

wants to merge 7 commits into from

Conversation

b-rowan
Copy link
Contributor

@b-rowan b-rowan commented Sep 20, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

b-rowan commented Sep 23, 2024

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
Copy link
Contributor Author

b-rowan commented Sep 23, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

b-rowan commented Sep 23, 2024

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.

UpstreamData added a commit that referenced this pull request Sep 23, 2024
Use `Pydantic` and `Depends` to automatically parse the DataTables AJAX request to a typed format.

Precursor to a fix required in #138, which should use query parameters to validate software compatibility.
b-rowan pushed a commit that referenced this pull request Sep 24, 2024
Use `Pydantic` and `Depends` to automatically parse the DataTables AJAX request to a typed format.

Precursor to a fix required in #138, which should use query parameters to validate software compatibility.
@b-rowan
Copy link
Contributor Author

b-rowan commented Sep 24, 2024

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.

UpstreamData and others added 6 commits September 23, 2024 22:46
Improve modal presentation of software selection on device page.
This should make the devices page a bit more user-friendly and intuitive to use.
Make rollouts add modal and software selection consistent.
Added a search bar into the select elements for submitting software.
This also adds a scrollbar when there are more than 5 elements in a searchable select.
If saving the device fails, we should keep the old state in the cache and
not the new one.
Validate software compatibility in UI when manually assigning to a device.
Software must be compatible with all selected devices to be shown in the UI.
Copy link
Collaborator

@tsagadar tsagadar left a comment

Choose a reason for hiding this comment

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

After manual software assignment the device no longer could be loaded in my tests: BFFDeviceResponse.convert failed when calling data = [DeviceSchema.model_validate(d) for d in devices]. This I could fix by preloading compatibility in GET /bff/devices endpoint like this:

query = Device.all().prefetch_related("assigned_software", "assigned_software__compatibility", "hardware")

Don't think this is the proper way though. And also not sure why I see the issue now and not before. Won't be able to dig deeper in the next 8 hours - so just leaving this comment.

uuids = request.query_params.get("uuids")
if uuids:
hardware = await Hardware.filter(devices__uuid__in=uuids.split(",")).distinct()
filters.append(Q(*[Q(compatibility__id=c.id) for c in hardware], join_type="AND"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit more optimized for the DB

        filters.append(Q(compatibility__id__in=list(map(lambda hw:hw.id, hardware_ids))))

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 think this is an OR join, not an AND join. My goal here was to ensure the compatibility was valid for all selected devices, I think this change would show all software compatible with any selected device?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not all software entries are listed in "Update Mode" dropdown
3 participants