Add group_names qs method for Acknowledgement list

[podliashanyk]

For Uninett/argus-htmx-frontend#25

NB!

The query is likely not as efficient as it could be. Help wanted.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.73%. Comparing base (cdc23e5) to head (6423868).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #838      +/-   ##
==========================================
- Coverage   84.81%   83.73%   -1.08%     
==========================================
  Files          76       72       -4     
  Lines        3846     3567     -279     
==========================================
- Hits         3262     2987     -275     
+ Misses        584      580       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Test results

    7 files    553 suites   21m 57s ⏱️
  449 tests   448 ✅ 1 💤 0 ❌
3 143 runs  3 136 ✅ 7 💤 0 ❌

Results for commit 6423868.

♻️ This comment has been updated with latest results.

[stveit]

Since everything else here seems to be a property, i guess this should be too?

[johannaengland]

It cannot be a property, when I try to add it I get the error "Remove 1 parameters; property getter methods receive only "self"."

[stveit]

ah yeah true

[podliashanyk]

group parameter is a must here, can't remove it. Without it I would just use an already existing acked property. But maybe acked_by should be moved to other model, say IncidentQuerySet?

[lunkwill42]

group parameter is a must here, can't remove it. Without it I would just use an already existing acked property. But maybe acked_by should be moved to other model, say IncidentQuerySet?

Since this is a boolean check for a single incident, that makes no sense. Keep it here, but rename to is_acked_by to signify that it's a boolean check (and yes, since it takes arguments, it cannot be a property).

If you want a query that filters a set of incidents based on group acknowledgement, that's when you want a method on the IncidentQuerySet

[hmpf]

Acknowledgment has the queryset method active() to find non-expired acks.

Incident.acks.active() gets non-expired acks on a specific incident. You could also have a queryset method on Acknowledgment that spits out a set of group names instead of actual acks, so, in code: "noc" in Incident.acks.active().group_names(). For templates you could have a filter acked_by_group which runs the previous code internally.

[hmpf]

In the actual incident list it will be necessary to always prefetch() events and acks and for geant also groups. Whether we should always prefetch grousp or toggle it somehow... yet another setting?

[elfjes]

Incident.acks.active() gets non-expired acks on a specific incident. You could also have a queryset method on Acknowledgment that spits out a set of group names instead of actual acks, so, in code: "noc" in Incident.acks.active().group_names(). For templates you could have a filter acked_by_group which runs the previous code internally.

That, joined with ack groups sounds like a simpler way than using an acked_by() method (be sure to prefetch acks and groups tho like you said)

[podliashanyk]

Thank you for suggestions @lunkwill42, @elfjes, @hmpf! 🙌

Landed on the following solution:
Since acks-by-group info is so far only relevant for https://github.com/Uninett/argus-htmx-frontend, the pre-fetches will be added in Uninett/argus-htmx-frontend#25. Ditto settings for toggling prefetch of group info.

Will reuse existing qs methods for acknowledgment list and add a new one for fetching user_groups.

Created #843 for further optimization if we see that pre-fetch and existing methods aren't sufficient.

[podliashanyk]

is_acked_by is no longer necessary, just the group_names one

[lunkwill42]

Good enough for me - but beware my comments about how this will actually be used. Calling it repeatedly for a list of incidents is not good (tm) :)

[hmpf]

You'll need a tiny test as well.

[hmpf]

You need to check if it has expired...

Do that and return False early if it has.

Also, you could probably reuse the property "acks", like so:

return self.acks.filter(event__actor__groups__name=group).exists()

(Both "acks" and "acked" need a refactor, frankly. Needs its own issue.)

[hmpf]

You could also just rename it to "has_been_acked_by" and ignore expiration.

[hmpf]

(The best solution is denormalizing, having whether this is a current ack on the incident itself, but that doesn't help in this specific case. we should not have a database field on incident for every not-expired ack per group...)

[podliashanyk]

I will stick to is_acked_by and check if ack has expired

[johannaengland]

You'll need a tiny test as well.

If you need help with that @podliashanyk, I can be of assistance :)

[hmpf]

def is_acked_by(self, groupname: str) -> bool:
    acks = self.acks.active()
    return acks.filter(event__actor__groups__name=groupname).exists()

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud