Setup improvements #24

qedi-r · 2025-02-11T12:35:41Z

This automatically adds new MAC address as a list on the index page for adoption or ignoring permanently.

bkuhlmann · 2025-02-17T18:53:30Z

Hey Ryan. 👋 Sorry about the recent flurry of work going on this repository. I'm trying to get this project setup so all specs pass (including the CI builds) and making Docker easier to use too. Due to this, your branch is out of date. Would you mind rebasing to pick up the latest changes?

I think we should try to get this folded in but I'll need to take a closer look after your rebase.

qedi-r · 2025-02-17T19:36:57Z

Updated - I xit'ed the two failing tests that require an image to display. I assume we don't want to also support returning the setup image as a base64 encoded file.

bkuhlmann

Thanks for this, I think there are some good ideas here but this needs some refinement first before this can be folded in.

Also, it'd be really nice to rebase your feature branch off of master because too many commits are being merged which makes this code harder to review.

Also, there are a bunch of ideas at play in this work so would be good to extract this out into smaller reviews if that make sense.

bkuhlmann · 2025-02-17T20:16:55Z

app.rb

@@ -3,6 +3,7 @@
 require 'sinatra'
 require 'sinatra/activerecord'
 require 'debug'
+require 'uri'


💭 This is being required but not used. Am I missing something?

Hm. I thought it was used in the URI parser on line 17, but I guess if that went in without it, it's included by default. I'll remove.

bkuhlmann · 2025-02-17T20:17:46Z

db/migrate/20250209163618_unregistered_devices.rb

+  def change
+    add_column :devices, :adopted, :boolean, default: false, null: false
+
+    binding.break


🪓 Looks like this debug statement was accidentally left in and is safe to remove.

bkuhlmann · 2025-02-17T20:26:05Z

app.rb

@@ -123,7 +165,7 @@ def devices_form(device)
      special_function: 'sleep'
    }.to_json
  else
-    { status: 404 }.to_json
+    return 404, { status: 404 }.to_json


🎨 Can this be used instead since it's more idiomatic Sinatra?

status 404 { status: 404 }.to_json

ℹ️ We should also start adhering to RFC 9457 in order to render consistent errors. Can't tackle this now but planting the seed.

Yeah, having a goal like that makes sense. I'm not experienced in Sinatra, and don't know the idioms (ruby generally seems to have it's fair amount). I just thought we should at least stop returning 200s for errors.

Yeah, no worries. My goal is to keep the code super consistent so it's easier for others to contribute. If patterns start to diverge then it's easy for folks to get confused and even do the wrong thing.

bkuhlmann · 2025-02-17T20:37:27Z

app.rb

 # FIRMWARE SETUP
 get '/api/setup/' do
  content_type :json
  @device = Device.find_by_mac_address(env['HTTP_ID']) # => ie "41:B4:10:39:A1:24"

-  if @device
+  status = 404


🎨 Could we encapsulate this functionality into a specialized object instead? Ideally, we should have something like this (which leverages the Command Pattern):

get "/api/setup/" do code, body = Register.new.call(**atrributes) status code body end

This would help keep the routes concise and clean while allowing registration functionality to be more easily tested without brining in the entire Rack stack.

bkuhlmann · 2025-02-17T20:45:12Z

db/schema.rb

-ActiveRecord::Schema[8.0].define(version: 2024_12_12_172428) do
-  # These are extensions that must be enabled in order to support this database
-  enable_extension "pg_catalog.plpgsql"
+ActiveRecord::Schema[8.0].define(version: 2025_02_09_163618) do


💭 Is there a design, specification, or issue related to how these tables are suppose to be used. I think this might need to be extracted to a different code review because this seems like separate work to support?

This should only be in the schedules branch - it was added here unintentionally - squashing the branch will fix it and remove it from the review.

bkuhlmann · 2025-02-17T20:51:45Z

app.rb

  erb :"index"
 end

+def is_valid_mac(mac_address)
+  return mac_address && mac_address.match(/^[A-Fa-f0-9][048Cc26AaEe][-:]([A-Fa-f0-9]{2}[-:]){4}[A-Fa-f0-9]{2}$/)


🎨 Could we encapsulate this functionality into a specialized object instead? This would also improve testing because you could test in isolation without having to incorporate the entire Rack stack.

hm. good point. I should have noticed the testing implications here. 👍

Yeah, there are several other routes that need this too. I plan to fix that up soon so people have better examples on how to unit test and then fold in more routes like this for easier testing.

bkuhlmann · 2025-02-17T20:52:38Z

app.rb

@@ -107,7 +107,7 @@ def is_valid_mac(mac_address)
 end

 get '/setup/adopt/:id' do
-  unadopted_device = UnadoptedDevice.find(id)
+  unadopted_device = UnadoptedDevice.find(params[:id])


🎨 Can this be rebased into the previous commit? This looks like a fixup commit. Not need to have it exist on it's own.

bkuhlmann · 2025-02-17T20:53:32Z

Gemfile.lock

@@ -170,6 +170,7 @@ DEPENDENCIES
  rspec
  sinatra-activerecord
  sqlite3
+  timecop


🪓 Can this be removed since I don't see this gem used anywhere?

bkuhlmann · 2025-02-17T20:56:24Z

Gemfile

@@ -29,8 +29,10 @@ group :development, :test do
  gem 'nokogiri'
  gem 'rack-test'
  gem 'rspec'
+  gem 'rubocop'


🪓 I agree we need to bring in RuboCop so this is a good catch...but I'd rather bring in the full tooling as managed by Caliber because there are a bunch of other corrections we need to make to this repository first. Let's delete this commit for now.

bkuhlmann · 2025-02-17T20:58:21Z

spec/display_spec.rb

                         })
    header('ACCESS_TOKEN', dev.api_key)
    _, body = get_json '/api/display/'
    expect(body['reset_firmware']).to eq(false)
  end

-  it 'test_it_has_a_base64_display_path_with_a_device' do
+  # requries having a last image to display
+  xit 'test_it_has_a_base64_display_path_with_a_device_in_the_query_params' do


🛑 Can we fix these instead of disabling them? It's important to keep all specs passing.

Sure. I just kind of assumed these were the ones you referred to that we expect these to be broken. Easy to fix though.

Yeah, thanks for fixing up. I need to work on improving the test suite more but this is a good start.

bkuhlmann reviewed Feb 17, 2025

View reviewed changes

bkuhlmann mentioned this pull request Feb 17, 2025

initial schedules work #32

Closed

qedi-r force-pushed the setup-improvements branch 2 times, most recently from 29fc8e9 to 9f6f098 Compare February 18, 2025 03:50

automatically add newly seen mac addresses to devices as unadopted

7514e67

qedi-r force-pushed the setup-improvements branch from 9f6f098 to 7514e67 Compare February 20, 2025 12:41

Merge branch 'master' into setup-improvements

8a16647

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup improvements #24

Setup improvements #24

qedi-r commented Feb 11, 2025

bkuhlmann commented Feb 17, 2025 •

edited

Loading

qedi-r commented Feb 17, 2025

bkuhlmann left a comment

bkuhlmann Feb 17, 2025

qedi-r Feb 18, 2025

bkuhlmann Feb 18, 2025

bkuhlmann Feb 17, 2025

bkuhlmann Feb 17, 2025

qedi-r Feb 18, 2025

bkuhlmann Feb 18, 2025

bkuhlmann Feb 17, 2025

bkuhlmann Feb 17, 2025

qedi-r Feb 18, 2025

bkuhlmann Feb 17, 2025

qedi-r Feb 18, 2025

bkuhlmann Feb 18, 2025 •

edited

Loading

bkuhlmann Feb 17, 2025

bkuhlmann Feb 17, 2025

bkuhlmann Feb 17, 2025

bkuhlmann Feb 17, 2025

qedi-r Feb 18, 2025

bkuhlmann Feb 18, 2025

Setup improvements #24

Are you sure you want to change the base?

Setup improvements #24

Conversation

qedi-r commented Feb 11, 2025

bkuhlmann commented Feb 17, 2025 • edited Loading

qedi-r commented Feb 17, 2025

bkuhlmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkuhlmann Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkuhlmann commented Feb 17, 2025 •

edited

Loading

bkuhlmann Feb 18, 2025 •

edited

Loading