Show helpful message when uploading duplicate firmware #1627
Conversation
lib/nerves_hub_web/live/firmware.ex
Outdated
| [constraint: :unique, constraint_name: "firmwares_product_id_uuid_index"]} | ||
| ] | ||
| }} -> | ||
| error_feedback(socket, "Duplicate firmware uploads are not allowed") |
There was a problem hiding this comment.
Do we get the uuid back in that changeset somewhere? It would be really cool to say something like Duplicate firmware - <link-to-firmware>
There was a problem hiding this comment.
Oh, good idea! I'll try.
There was a problem hiding this comment.
Done! Updated the gif in the PR description.
nshoes
force-pushed
the
show-helpful-error-duplicate-firmware-upload
branch
from
b0c00ca
to
9d68cfd
Compare
lib/nerves_hub_web/live/firmware.ex
Outdated
|
|
||
| error_feedback( | ||
| socket, | ||
| "Duplicate firmware - <a href='#{route}' target='blank'>View Firmware</a>" |
There was a problem hiding this comment.
I'd prefer to find a way to not include an <a href> in a string error message, and then also use raw for displaying the message.
It might require a little refactoring, eg. instead of line 164 adding an error message to the assigns, maybe we add an error_code, like :duplicate_firmware and then use a helper function in the template which uses a case statement to define the long form message.
There was a problem hiding this comment.
I like that a lot more, I'll implement!
There was a problem hiding this comment.
Changes pushed. I think instead of handling this scenario in a pattern-match with error_feedback, it could just be handled inline since we have to pattern-match on the UUID error and then pattern-match it again which feels weird. Lmk what you think!
nshoes
force-pushed
the
show-helpful-error-duplicate-firmware-upload
branch
2 times, most recently
from
02de318
to
3936a85
Compare
lib/nerves_hub_web/live/firmware.ex
Outdated
| |> assign(:error_code, :duplicate_firmware) | ||
| |> assign( | ||
| :firmware_path, | ||
| "/org/#{socket.assigns.org.name}/#{socket.assigns.product.name}/firmware/#{changeset.changes.uuid}" | ||
| ) |
There was a problem hiding this comment.
| |> assign(:error_code, :duplicate_firmware) | |
| |> assign( | |
| :firmware_path, | |
| "/org/#{socket.assigns.org.name}/#{socket.assigns.product.name}/firmware/#{changeset.changes.uuid}" | |
| ) | |
| |> assign(:error_code, {:duplicate_firmware, uuid}) |
Since we have control of this structure, we could just include the duplicate UUID and let the case handling of that view build the URL for it (or whatever else). That way we're not so coupled to 2 independent pieces of data having to exist in the assigns (@error_code and @firmware_path)
There was a problem hiding this comment.
Ah, good point. Fixed. We still need to keep track of the uuid to create the path, however. But I like constructing the path in the view more.
nshoes
force-pushed
the
show-helpful-error-duplicate-firmware-upload
branch
from
3936a85
to
6cce83d
Compare
Fixes #1473.
This adds an error clause to provide helpful feedback when uploading firmware that has already been uploaded. There's already a test for this constraint, but let me know if a LiveView test is wanted/needed.