Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion api/subscriptions/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def get_queryset(self):
id=Cast(OuterRef('object_id'), IntegerField()),
).values('guids___id')[:1]

return NotificationSubscription.objects.filter(
qs = NotificationSubscription.objects.filter(
notification_type__in=[
NotificationType.Type.USER_FILE_UPDATED.instance,
NotificationType.Type.NODE_FILE_UPDATED.instance,
Expand Down Expand Up @@ -91,6 +91,13 @@ def get_queryset(self):
),
)

# Apply manual filter for legacy_id if requested
filter_id = self.request.query_params.get('filter[id]')
if filter_id:
qs = qs.filter(legacy_id=filter_id)
# convert to list comprehension because legacy_id is an annotation, not in DB

return qs

class AbstractProviderSubscriptionList(SubscriptionList):
def get_queryset(self):
Expand Down
32 changes: 30 additions & 2 deletions api_tests/subscriptions/views/test_subscriptions_list.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ def test_list_complete(
user,
provider,
node,
file_updated_notification,
url
):
res = app.get(url, auth=user.auth)
Expand All @@ -91,8 +90,37 @@ def test_cannot_post_patch_put_or_delete(self, app, url, user):
assert put_res.status_code == 405
assert delete_res.status_code == 405

def test_multiple_values_filter(self, app, url, file_updated_notification, user):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we were adding the wrong type of notification before.

def test_multiple_values_filter(self, app, url, user):
res = app.get(url + '?filter[event_name]=comments,file_updated', auth=user.auth)
assert len(res.json['data']) == 2
for subscription in res.json['data']:
subscription['attributes']['event_name'] in ['global', 'comments']

def test_value_filter_id(
self,
app,
url,
user,
node,
):
# Request all subscriptions first, to confirm all are visible
res = app.get(url, auth=user.auth)
all_ids = [sub['id'] for sub in res.json['data']]
assert len(all_ids) == 2
assert f'{node._id}_file_updated' in all_ids
assert f'{user._id}_global_file_updated' in all_ids

# Now filter by a specific annotated legacy_id (the node file_updated one)
target_id = f'{node._id}_file_updated'
filtered_res = app.get(f'{url}?filter[id]={target_id}', auth=user.auth)

# Response should contain exactly one record matching that legacy_id
assert filtered_res.status_code == 200
data = filtered_res.json['data']
assert len(data) == 1
assert data[0]['id'] == target_id

# Confirm it’s the expected subscription object
attributes = data[0]['attributes']
assert attributes['event_name'] is None # event names are legacy
assert attributes['frequency'] in ['instantly', 'daily', 'none']
23 changes: 15 additions & 8 deletions notifications/listeners.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,24 @@ def subscribe_creator(resource):
NotificationSubscription.objects.get_or_create(
user=user,
notification_type=NotificationType.Type.USER_FILE_UPDATED.instance,
object_id=user.id,
content_type=ContentType.objects.get_for_model(user),
_is_digest=True,
message_frequency='instantly',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing message_frequency not strictly related to filtering issue, but necessary as NoneType values aren't recognized as default by FE.

)
except NotificationSubscription.MultipleObjectsReturned:
pass
try:
NotificationSubscription.objects.get_or_create(
user=user,
notification_type=NotificationType.Type.NODE_FILE_UPDATED.instance,
object_id=resource.id,
content_type=ContentType.objects.get_for_model(resource),
_is_digest=True,
message_frequency='instantly',
)
except NotificationSubscription.MultipleObjectsReturned:
pass
NotificationSubscription.objects.get_or_create(
user=user,
notification_type=NotificationType.Type.FILE_UPDATED.instance,
object_id=resource.id,
content_type=ContentType.objects.get_for_model(resource),
_is_digest=True,
)

@contributor_added.connect
def subscribe_contributor(resource, contributor, auth=None, *args, **kwargs):
Expand All @@ -54,7 +61,7 @@ def subscribe_contributor(resource, contributor, auth=None, *args, **kwargs):
),
NotificationSubscription(
user=contributor,
notification_type=NotificationType.Type.FILE_UPDATED.instance,
notification_type=NotificationType.Type.NODE_FILE_UPDATED.instance,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ultimately this was what was cause the issues, FILE_UPDATED is the item with the NODE_FILE_UPDATED digest.

object_id=resource.id,
content_type=ContentType.objects.get_for_model(resource),
_is_digest=True,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ def test_migrate_provider_subscription(self, users, provider, provider2):
assert subs.filter(object_id=obj.id, content_type=content_type).exists()

def test_migrate_node_subscription(self, users, user, node):
self.create_legacy_sub('file_updated', users, user=user, node=node)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is now not needed because subscription is now being correctly sent by the signal.

migrate_legacy_notification_subscriptions()
nt = NotificationType.objects.get(name=NotificationType.Type.NODE_FILE_UPDATED)
assert nt.object_content_type == ContentType.objects.get_for_model(Node)
Expand Down
Loading