-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: master
Are you sure you want to change the base?
Conversation
8e47262
to
e2e8641
Compare
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
Likely two things missing:
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. |
Hmm. I'll see what I can do. Maybe an API endpoint for Need something similar for rollouts I feel as well... |
My thought was if devices are compatible, use it. Keep track of incompatible devices, and return an error with those devices listed. |
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 |
Or rather extend the /bff/software to accept device ids as query params? The it could only return compatible softwares to be displayed. |
Yeah, I thought about this afterwards, I think this is the way to go. |
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.
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.
2c0d796
to
d9fba15
Compare
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 |
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.
d9fba15
to
1b8a10e
Compare
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.
There was a problem hiding this 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")) |
There was a problem hiding this comment.
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))))
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct.
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