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

Refresh device-related assigns when receiving connection online message, replace mox with mimic #1738

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

nshoes
Copy link
Contributor

@nshoes nshoes commented Jan 7, 2025

No description provided.

Copy link
Collaborator

@joshk joshk left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this.

Do you think we should add a test to confirm data is updated on the page when a device connects?

@@ -43,15 +43,11 @@ defmodule NervesHubWeb.Live.Devices.Show do
|> page_title("Device #{device.identifier} - #{product.name}")
|> assign(:tab_hint, :devices)
|> assign(:device, device)
|> assign(:device_connection, device_connection(device))
|> assign(:console_active?, Tracker.console_active?(device))
|> assign(:deployment, device.deployment)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to move into general_assigns as well

Comment on lines 79 to 82
socket =
socket
|> general_assigns(device)
|> assign(:device_connection, Connections.get_latest_for_device(device.id))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should reload the device, then do the general_assigns, and since we will be reloading the device we won't need to do line 82, instead we can move |> assign(:device_connection, device_connection(device)) from mount into general_assigns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When setting assigns.device_connection, which should we prefer? Connections.get_latest_for_device(device.id) or device_connection(device)? I think that in the lifecycle events that we're talking about (mount, connection status inline, connection status offline) we should always fetch the latest device connection record. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are fetching the Device, the latest device connection is included, but if we aren't fetching the Device (like when the status changes to "offline") we don't need to fetch the Device, we can just use Connections.get_latest_for_device(device.id), does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Thank you 🙇

@nshoes
Copy link
Contributor Author

nshoes commented Jan 7, 2025

Do you think we should add a test to confirm data is updated on the page when a device connects?

Yes! On it.

@nshoes nshoes force-pushed the refresh-device-assigns-when-coming-online branch 2 times, most recently from ba27bb2 to 0501845 Compare January 14, 2025 02:01
@nshoes nshoes force-pushed the refresh-device-assigns-when-coming-online branch from 0501845 to 7ecd5ab Compare January 14, 2025 02:08
@nshoes nshoes force-pushed the refresh-device-assigns-when-coming-online branch from 7ecd5ab to df70ecb Compare January 14, 2025 02:13
@nshoes
Copy link
Contributor Author

nshoes commented Jan 14, 2025

@joshk test added!

Do you have any opinion on mox vs mimic for stubbing in tests? We've used mimic at SR and really enjoy it. My main issue with mox is the ceremony of setting up a behaviour when I just want to mock quickly and move on. I added mimic and used it in my test as an example. I checked the usage of mox already implemented and it would be very easy to convert to mimic, their APIs are very similar.

@nshoes nshoes requested a review from joshk January 14, 2025 02:25
@joshk
Copy link
Collaborator

joshk commented Jan 14, 2025

@joshk test added!

Do you have any opinion on mox vs mimic for stubbing in tests? We've used mimic at SR and really enjoy it. My main issue with mox is the ceremony of setting up a behaviour when I just want to mock quickly and move on. I added mimic and used it in my test as an example. I checked the usage of mox already implemented and it would be very easy to convert to mimic, their APIs are very similar.

Go for gold!

Copy link
Collaborator

@joshk joshk left a comment

Choose a reason for hiding this comment

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

Awesome work. One small comment, then I can merge it in!

lib/nerves_hub_web/live/devices/show.ex Outdated Show resolved Hide resolved
@nshoes nshoes changed the title Refresh device-related assigns when receiving connection online message Refresh device-related assigns when receiving connection online message, replace mox with mimic Jan 14, 2025
@nshoes nshoes force-pushed the refresh-device-assigns-when-coming-online branch from 5817b49 to 640f1cb Compare January 14, 2025 04:01
@@ -104,7 +102,6 @@
"telemetry_metrics": {:hex, :telemetry_metrics, "1.0.0", "29f5f84991ca98b8eb02fc208b2e6de7c95f8bb2294ef244a176675adc7775df", [:mix], [{:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "f23713b3847286a534e005126d4c959ebcca68ae9582118ce436b521d1d47d5d"},
"telemetry_metrics_statsd": {:hex, :telemetry_metrics_statsd, "0.7.1", "3502235bb5b35ce50d608bf0f34369ef76eb92a4dbc8708c7e8780ca0da2d53e", [:mix], [{:nimble_options, "~> 0.4 or ~> 1.0", [hex: :nimble_options, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}, {:telemetry_metrics, "~> 0.6 or ~> 1.0", [hex: :telemetry_metrics, repo: "hexpm", optional: false]}], "hexpm", "06338d9dc3b4a202f11a6e706fd3feba4c46100d0aca23688dea0b8f801c361f"},
"telemetry_poller": {:hex, :telemetry_poller, "1.1.0", "58fa7c216257291caaf8d05678c8d01bd45f4bdbc1286838a28c4bb62ef32999", [:rebar3], [{:telemetry, "~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "9eb9d9cbfd81cbd7cdd24682f8711b6e2b691289a0de6826e58452f28c103c8f"},
"telemetry_registry": {:hex, :telemetry_registry, "0.3.2", "701576890320be6428189bff963e865e8f23e0ff3615eade8f78662be0fc003c", [:mix, :rebar3], [{:telemetry, "~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "e7ed191eb1d115a3034af8e1e35e4e63d5348851d556646d46ca3d1b4e16bab9"},
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 and opentelemetry_liveview above are unused deps that weren't unlocked and cleaned, so I removed them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great find!

@joshk joshk merged commit a467823 into main Jan 14, 2025
2 checks passed
@joshk joshk deleted the refresh-device-assigns-when-coming-online branch January 14, 2025 06:04
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.

2 participants