[ENG-5075] Add Admin Screen to Manage Duplicate Notifications #10701
Conversation
There was a problem hiding this comment.
I noticed that the acceptance criteria had "All unit tests passing" this didn't add unit tests so that's technically true, but new ones couldn't hurt. However this is a pretty ephemeral feature so I'm not sure if new unit tests are required.
Other then that just minor syntax changes, suggestions. good job, I approve just break up that long line and confirm @brianjgeiger wasn't requiring unit tests and it's g2g.
admin/notifications/views.py
Outdated
|
|
||
| @user_passes_test(osf_staff_check) | ||
| def handle_duplicate_notifications(request): | ||
| duplicates = NotificationSubscription.objects.values('user', 'node', 'event_name').annotate(count=Count('id')).filter(count__gt=1) |
There was a problem hiding this comment.
It's typically a linting error to have a line longer then 100 characters, but I don't think it's enforced in the admin app, try to keep that standard anyway and break up lines over 100 characters.
| @@ -0,0 +1,27 @@ | |||
| from django.core.management.base import BaseCommand | |||
There was a problem hiding this comment.
nit: I would change the change the name of the file to indicate it's for testing only. People might not read the help text fully.
uditijmehta
force-pushed
the
feature/duplicate-notifications-fix
branch
4 times, most recently
from
aa1d790
to
893e216
Compare
uditijmehta
force-pushed
the
feature/duplicate-notifications-fix
branch
from
893e216
to
8568ae6
Compare
Johnetordoff
merged commit b5d0d3c
into
CenterForOpenScience:feature/b-and-i-24-14
| NotificationSubscription.objects.values('user', 'node', 'event_name') | ||
| .annotate(count=Count('id')) |
There was a problem hiding this comment.
This doesn't necessarily find only legitimate dupes, I'd recommend using the following:
NotificationSubscription.objects.values('_id').annotate(count=Count('_id')).filter(count__gt=1)
Part of the reason behind that relates to NotificationSubscriptions attached to AbstractProvider-type objects. They have no user or node, and so when each provider notification has the same event_name, they show up as dupes with the existing query.
| ) | ||
|
|
||
| detailed_duplicates = [] | ||
| for dup in duplicates: |
There was a problem hiding this comment.
I ran a quick count on my suggested query above against prod data, and there were 764. This means that this function would run 765 individual queries (excluding any foreign relations) per page render, and generate a list at least 1528 rows long. There's a good chance it would time out and fail to render the page with prod data.
A couple possible recommendations:
- Pagination controls, with a default size of 10 dupes (i.e. 20 total)
- Include this information on the Node/Registration detail page (
<admin_base>/nodes/<guid>) instead, specific to each node, and allow deleting one there.
| 'id': notification.id, | ||
| 'user': notification.user, | ||
| 'node': notification.node, | ||
| 'event_name': notification.event_name, | ||
| 'created': notification.created, | ||
| 'count': dup['count'] |
There was a problem hiding this comment.
id and user are spurious for this purpose.
node, also, isn't serialized in a useful way -- it gets serialized as Node object (<primary_key[int]>), but that can be improved in the template
_id, for reference, is formatted as f'{node._id}_{event_name}, and could be swapped out for event_name.
A couple useful fields for determining which duplicate to remove are the M2M's none, email_transactional, and email_digest. These represent the individual users subscribed to each notification, and is often helpful to compare with the contributor list for a best-match. Optional, but if included, would recommend populating with guids, e.g. 'email_transactional': [u._id for u in notification.email_transactional.all()]
| @@ -311,6 +311,9 @@ | |||
| {% if perms.osf.change_maintenancestate %} | |||
| <li><a href="{% url 'maintenance:display' %}"><i class='fa fa-link'></i> <span>Maintenance Alerts</span></a></li> | |||
| {% endif %} | |||
| {% if perms.osf.view_notification %} | |||
There was a problem hiding this comment.
Minor: I think this perm doesn't actually exist, so this would only work for is_superuser=True users.
| <tr> | ||
| <td><input type="checkbox" name="selected_notifications" value="{{ notification.id }}"></td> | ||
| <td>{{ notification.user }}</td> | ||
| <td>{{ notification.node }}</td> |
There was a problem hiding this comment.
Could use notification.node._id here instead of changing serialization above.
Also, if keeping this approach (as opposed to putting this info in the node detail page), this would likely be much more helpful if it was a link, e.g. <a href="{{ notification.node | reverse_node }}">
| from django.utils.crypto import get_random_string | ||
| from django.utils import timezone | ||
|
|
||
| class Command(BaseCommand): |
There was a problem hiding this comment.
This command is unnecessary; it doesn't actually create duplicates that collide on _id
Purpose
Add Admin Screen to Manage Duplicate Notifications
Changes
Ticket
https://openscience.atlassian.net/browse/ENG-5075