Improve notifications when somebody left the group (#1619)

[mvellasco]

Issue

https://github.com/yunity/karrot-frontend/issues/1619

Description

• Added new user queryset filter condition to match users that have GROUP_LEAVE histories in one of current user's groups

[codecov[bot]]

Codecov Report

Merging #1104 (3d79cf7) into master (1ea996f) will increase coverage by 0.58%. The diff coverage is 100.00%.

:exclamation: Current head 3d79cf7 differs from pull request most recent head f1cb23e. Consider uploading reports for the commit f1cb23e to get more accurate results

@@            Coverage Diff             @@
##           master    #1104      +/-   ##
==========================================
+ Coverage   93.07%   93.66%   +0.58%     
==========================================
  Files         533      480      -53     
  Lines       20677    18135    -2542     
  Branches     2438     1228    -1210     
==========================================
- Hits        19245    16986    -2259     
+ Misses       1133      928     -205     
+ Partials      299      221      -78     

Impacted Files	Coverage Δ
karrot/notifications/models.py	98.33% <100.00%> (-0.08%)	:arrow_down:
karrot/notifications/receivers.py	96.37% <100.00%> (-0.16%)	:arrow_down:
karrot/notifications/tests/test_receivers.py	100.00% <100.00%> (ø)
karrot/users/api.py	100.00% <100.00%> (ø)
karrot/users/tests/test_api.py	98.87% <100.00%> (+0.11%)	:arrow_up:
karrot/status/helpers.py	65.15% <0.00%> (-18.40%)	:arrow_down:
karrot/history/serializers.py	70.73% <0.00%> (-4.74%)	:arrow_down:
karrot/activities/tasks.py	84.61% <0.00%> (-2.59%)	:arrow_down:
karrot/issues/receivers.py	92.30% <0.00%> (-1.81%)	:arrow_down:
karrot/webhooks/receivers.py	81.25% <0.00%> (-1.77%)	:arrow_down:
... and 162 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[nicksellen]

(I copied in the comment from https://github.com/yunity/karrot-frontend/issues/1619#issuecomment-754326542 to keep the discussion to the PR, seems best!)

@nicksellen Right now I'm not in any groups, I'm trying to set up one though in my local town(Campos dos Goytacazes - Rio de Janeiro - Brazil). About the solution, you guys had in mind to copy the user data into the history model, it might be even simpler if I didn't overlook anything. How about instead of adding a new DB field for that we just add another filter to the query for users matching if they have a GROUP_LEFT type history in the current user's group? I took the liberty to make a draft implementation and no tests broke so it should be fairly safe. In any case, let me know so I can submit the PR alongside the bell notification as well. Thanks!

Thanks! I think it's a good idea. I think it might be worth adding a grace period though, maybe only history entries within 3 or 6 months (@dwaxweiler mentioned 2 weeks but seems a bit short to me)? If you leave a group I think it's reasonable that your information is no longer visible at some point.

One downside of doing it this way is that if we wanted to just restrict it to their name (but hide the email address) we don't have a way of limiting visibility for the left users.

But this PR improves the current behaviour so seems good to me.

Could you implement a test?

[mvellasco]

(I copied in the comment from yunity/karrot-frontend#1619 (comment) to keep the discussion to the PR, seems best!)

@nicksellen Right now I'm not in any groups, I'm trying to set up one though in my local town(Campos dos Goytacazes - Rio de Janeiro - Brazil). About the solution, you guys had in mind to copy the user data into the history model, it might be even simpler if I didn't overlook anything. How about instead of adding a new DB field for that we just add another filter to the query for users matching if they have a GROUP_LEFT type history in the current user's group? I took the liberty to make a draft implementation and no tests broke so it should be fairly safe. In any case, let me know so I can submit the PR alongside the bell notification as well. Thanks!

Thanks! I think it's a good idea. I think it might be worth adding a grace period though, maybe only history entries within 3 or 6 months (@dwaxweiler mentioned 2 weeks but seems a bit short to me)? If you leave a group I think it's reasonable that your information is no longer visible at some point.

One downside of doing it this way is that if we wanted to just restrict it to their name (but hide the email address) we don't have a way of limiting visibility for the left users.

But this PR improves the current behaviour so seems good to me.

Could you implement a test?

I'm glad it'll help!

I'll implement the tests and add the grace period logic with the bell ring notification code as well.

About that Q object issue, you're correct, it shouldn't have reference to more than one field per Q object, so I'll change that to :

has_left_group = (
    Q(history__group__in=groups) & Q(history__typus=HistoryTypus.GROUP_LEAVE)
)

Regarding the multiple deeper lookup, in this case, it won't be an issue since it will be translated into a simple history.group IN something on the generated SQL after the Q object is processed.

[mvellasco]

@nicksellen About that caveat of showing the email, I can add a conditional on the user viewset like that to mitigate the problem. Please let me know if you think that's a good idea. Thanks!

def profile(self, request, pk=None):
    user = self.get_object()
    if not any(group in user.groups.all() for group in request.user.groups.all()):
        serializer = UserInfoSerializer(user)
    else:
        serializer = self.get_serializer(user)
    return Response(serializer.data)

[nicksellen]

Great!

I like the idea of using the serializer to show different info as you did, a complexity is that because we keep the frontend up to date (via signals and websockets) we have to send the updated information when memberships change. it might JustWork :tm: but also might not... there are some issues there already.

I think it also might cause a lot of queries, as it's per user, although ensuring that the groups are prefetched in the original query should mitigate that.

On balance I think maybe leaving that out of this PR is fine, the need for it might just be in my head after all!

I'll await the other changes you mentioned, and can do a more in depth code review :)

... oh, and regarding the bell notification for user leaving, does one already exist? (I can't remember, and too lazy to check right now...)

[mvellasco]

@nicksellen I've addressed the new bell notification creating when the user has left and the changes on the Q objects query. Also I've created a PR on the frontend as well to properly display the notification info, I'll ping you for a initial review there. Thanks for the patience and sorry it took so long to come back to this, it has been quite a crazy year so far :slightly_smiling_face: