[change] Added checks & alertsettings permission for device tab #424

[Aryamanz29]

Closes #424

Closes #426

Checks:

• [x] I have manually tested the proposed changes
• [x] I have written new test cases to avoid regressions (if necessary)
• [x] I have updated the documentation (e.g. README.rst)

[coveralls]

Coverage increased (+0.01%) to 98.726% when pulling 7fe54a9339b181e1ffff4d0dabc2960a6ab11969 on issue-424/add-permission-checks-alerts into 073e1ba1c143a46a00c9abcaae8054a13847b3c7 on gsoc22-iperf.

[Aryamanz29]

@Aryamanz29 I found this weird behaviour with Alert Settings inline. It not occurring with Checks inline.

@pandafy https://github.com/openwisp/openwisp-monitoring/pull/431#discussion_r950689689 We can customize this form behaviour with get_extra hook as follows:

     def get_extra(self, request, obj=None, **kwargs):
        # Get full alertsettings form
        # when user has add permission
        if self.has_add_permission(request, obj):
            return 1
        return super().get_extra(request, obj, **kwargs)

[Aryamanz29]

https://user-images.githubusercontent.com/56113566/186871310-78f5cf55-2406-45f2-ac8b-eaee69881036.mp4

[pandafy]

While testing locally, I see a lot more empty alert settings, is this happening to you too? If yes, why?

I can confirm this behaviour on my local environment. With both superuser and non-superuser accounts.

[Aryamanz29]

@nemesisdesign @pandafy, I think it is due to extra=1 in AlertSettingsInline when user has_add_permission

So currently we have following options for AlertsettingsInline:

• Let fix extra=0 to avoid those empty alertsettings, then it will show Add another Alert settings button only for those metrics that doesn't have any alertsettings.

• But in that case, @pandafy reported that the form doesn't look good. See below :

• We could change MetricInline verbose_name to verbose_name = _('Metrics') for that. 

Now question is why don't we just filter out metric queryset [ie. super().get_queryset(request).filter(alertsettings__isnull=False)] ❔ We can't do this because if user try to create a metric from the 'AlertSettings' tab without having alertsettings then that metric can't be visible due to alertsettings__isnull=False filter.

If there is something we can try other than what is mentioned above, please let me know 😄

[Aryamanz29]

@nemesisdesign @pandafy

• [x] Can we show the metrics which have the alert settings first?

• Yes, it is possible. We can change MetricInline  ordering = [F('alertsettings').desc(nulls_last=True)]

• 

• [x] check name: Can we add validation logic that copies the check config name in the name field (if the name is empty) for consistency?

• Yes, as shown below, overriding the CheckInline formset save_new() method will work.

• 

• [x] metric name: Can we add validation logic that copies the metric config name in the name field (if the name is empty) for consistency?

• Yes, overriding MetricInline form save() method will work, see below:

-

(Note : I have removed the check name and metric name fields from the form because it will be the same as configuration chosen by the user) 

[Aryamanz29]

I have manually tested the changes and it works as expected.

The ordering of the AlertSettings objects in the inline appears to be random (apart from showing the Metrics without AlertSettings at the last). Can we add another ordering field name?

I have suggested some improvements to the test to make it more readable.

@pandafy I tried adding both orders, but only one works, which I believe is due to one ordering with desc(nulls_last=True) and then ordering with name, which simply overrides the previous one. 

[Aryamanz29]

In my recent testing, I found out that it is not possible to remove an AlertSetting form the the inline without deleting the metric.

Steps to replicate the issue:

1. Edit a traffic metric and add update Operator, Tolerance and Threshold to some value
2. Save the device
3. I tried to remove the AlertSetting by clearing out the field, but that didn't work and it used the default values as fallback.

I think it will be better to address this problem in a separate issue.

@pandafy I opened this here : https://github.com/openwisp/openwisp-monitoring/issues/435