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

[feature] Added support for device deactivation #560 #607

Merged
merged 14 commits into from
Nov 19, 2024

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Aug 2, 2024

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

What happens to the monitoring status of a device when it gets deactivated?
I was expecting to see some logic which would see some logic for the following:

  • Making sure monitoring checks are not run
  • Setting monitoring status to "UNKNOWN". Or, we can use a special status for this e,g, "DEACTIVATED" - I think "DEACTIVATED" is more explicit and preferable for UX
  • Replying with 404 for monitoring requests

I would also add that activating a device must reverse this: checks are run again, hence monitoring status should automatically become either OK or CRITICAL and device metrics shall be accepted.

We need tu update the docs to reflect these changes.

Please rebase on the latest master and reformat code if necessary.

@pandafy pandafy force-pushed the issues/560-deactivating-device branch from fd72d0e to 440d04c Compare August 8, 2024 18:37
@pandafy pandafy force-pushed the issues/560-deactivating-device branch from 8c668ae to 833656a Compare August 9, 2024 17:21
@pandafy pandafy force-pushed the issues/560-deactivating-device branch from 833656a to b584a40 Compare August 9, 2024 17:36
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Color

Let's make the color of deactivate status black (#000), both in the health status (which is now of the same color of unknown) and in the pie chart:

Screenshot from 2024-11-12 18-23-38

Device details: hide checks

Once the device is deactivated, I think we shouldn't show the checks in the device details anymore:

Screenshot from 2024-11-12 18-35-58

Cache invalidation

Let's ensure that once the device is deactivated, the monitoring API responds 404 correctly, during testing we noticed a cache invalidation issue.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Let's adjust the colors here as discussed:

Screenshot from 2024-11-13 12-14-40

@pandafy pandafy force-pushed the issues/560-deactivating-device branch from 311f645 to c821a2f Compare November 13, 2024 20:02
@pandafy pandafy force-pushed the issues/560-deactivating-device branch from 8dc6d57 to c6927eb Compare November 13, 2024 20:42
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

During testing, I noticed that reactivating a device leaves the health status as "DEACTIVATED", I think we should change this, the only option we have is "UNKNOWN" unless we add a new one, but I think "UNKNOWN" is good for now, what do you think?

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

We need to update this docs page:
https://openwisp.io/docs/dev/monitoring/user/device-health-status.html

To mention the new health status.

@pandafy pandafy force-pushed the issues/560-deactivating-device branch 2 times, most recently from 1c0a320 to 4a6bfbf Compare November 15, 2024 19:40
@pandafy pandafy force-pushed the issues/560-deactivating-device branch from 4a6bfbf to 3dca67f Compare November 19, 2024 13:23
@nemesifier nemesifier added the enhancement New feature or request label Nov 19, 2024
@nemesifier nemesifier merged commit 6a99044 into master Nov 19, 2024
24 checks passed
@nemesifier nemesifier deleted the issues/560-deactivating-device branch November 19, 2024 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Deactivating device should also disable monitoring
2 participants