openwisp / openwisp-firmware-upgrader

Firmware upgrade solution for OpenWRT with possibility to add support for other embedded OSes. Provides features like automatic retry for network failures, mass upgrades, REST API and more.
https://openwisp.io/docs/dev/firmware-upgrader/
Other
53 stars 60 forks source link

[bug] Opening change page for device without DeviceConnection object logs error #249

Closed pandafy closed 5 months ago

pandafy commented 7 months ago

get_upgrader_class_from_device_connection logs an exception when the conn is None

'NoneType' object has no attribute 'update_strategy'
Traceback (most recent call last):
  File "/opt/openwisp2/env/lib/python3.10/site-packages/openwisp_firmware_upgrader/utils.py", line 36, in get_upgrader_class_from_device_connection
    upgrader_class = app_settings.UPGRADERS_MAP[device_conn.update_strategy]
AttributeError: 'NoneType' object has no attribute 'update_strategy'

We need to investigate what code is calling this function with None argument.

https://github.com/openwisp/openwisp-firmware-upgrader/blob/f1b0397944489db857cde986c2933675b3758001/openwisp_firmware_upgrader/utils.py#L34C5-L41

pandafy commented 7 months ago

Steps to replicate

  1. Create a device
  2. Ensure that this device does not have a credentials object. If the device has one (due to auto_add), remove the credential and reload the page.

Actual Outcome

An AttributeError is logged because the form generation requires schema of the upgrader class:

https://github.com/openwisp/openwisp-firmware-upgrader/blob/f1b0397944489db857cde986c2933675b3758001/openwisp_firmware_upgrader/admin.py#L422-L427

Since this device does not have a DeviceConnection object, the get_upgrader_schema_for_device calls the get_upgrader_class_from_device_connection function with None argument through get_upgrader_class_for_device.

'NoneType' object has no attribute 'update_strategy'
Traceback (most recent call last):
  File "/home/pandafy/openwisp/openwisp-firmware-upgrader/openwisp_firmware_upgrader/utils.py", line 36, in get_upgrader_class_from_device_connection
    upgrader_class = app_settings.UPGRADERS_MAP[device_conn.update_strategy]
AttributeError: 'NoneType' object has no attribute 'update_strategy'
pandafy commented 7 months ago

I see an opportunity for improving UX here. This is how the form looks when the upgrader class schema is not available Screenshot from 2024-02-16 18-42-54

There are no options for the upgrade because the DeviceConnection.update_strategy is used for getting the correct upgrader class which in-turns provide the schema.

In this case, if the user choose a FirmwareImage and go ahead anyway, they will see an error on submitting the form

Screenshot from 2024-02-16 18-52-30

I think, we can use this exception to pro-actively tell users to add a credential first.

nemesifier commented 7 months ago

Seems a good plan. I think for "DEVICE CONNECTIONS" you meant credentials?

pandafy commented 7 months ago

Yes, we should also update that error message.