From 956bd749056b062369a028cc769ccd5ea46475d1 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Fri, 4 May 2018 13:57:36 -0700 Subject: [PATCH] attachments: Send events for attachment updates. We send add events on upload, update events when sending a message referencing it, and delete updates on removal. This should make it possible to do real-time sync for the attachments UI. Based in part on work by Aastha Gupta. --- static/js/server_events_dispatch.js | 4 ++ zerver/lib/actions.py | 12 +++- zerver/lib/events.py | 4 ++ zerver/lib/upload.py | 9 ++- zerver/tests/test_events.py | 86 +++++++++++++++++++++++++++++ zerver/views/attachments.py | 2 + 6 files changed, 113 insertions(+), 4 deletions(-) diff --git a/static/js/server_events_dispatch.js b/static/js/server_events_dispatch.js index 767f9d6919..66297c0e2c 100644 --- a/static/js/server_events_dispatch.js +++ b/static/js/server_events_dispatch.js @@ -10,6 +10,10 @@ exports.dispatch_normal_event = function dispatch_normal_event(event) { alert_words_ui.render_alert_words_ui(); break; + case 'attachment': + // Do nothing for now. + break; + case 'default_streams': stream_data.set_realm_default_streams(event.default_streams); settings_streams.update_default_streams_table(); diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 7d6a4e1120..cd1c9613cd 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -4516,6 +4516,15 @@ def do_get_streams(user_profile: UserProfile, include_public: bool=True, return streams +def notify_attachment_update(user_profile: UserProfile, op: str, + attachment_dict: Dict[str, Any]) -> None: + event = { + 'type': 'attachment', + 'op': op, + 'attachment': attachment_dict, + } + send_event(event, [user_profile.id]) + def do_claim_attachments(message: Message) -> None: attachment_url_list = attachment_url_re.findall(message.content) @@ -4540,7 +4549,8 @@ def do_claim_attachments(message: Message) -> None: user_profile.id, path_id, message.id)) continue - claim_attachment(user_profile, path_id, message, is_message_realm_public) + attachment = claim_attachment(user_profile, path_id, message, is_message_realm_public) + notify_attachment_update(user_profile, "update", attachment.to_dict()) def do_delete_old_unclaimed_attachments(weeks_ago: int) -> None: old_unclaimed_attachments = get_old_unclaimed_attachments(weeks_ago) diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 1c409f4d65..530cb6e9ab 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -560,6 +560,10 @@ def apply_event(state: Dict[str, Any], elif event['type'] == 'typing': # Typing notification events are transient and thus ignored pass + elif event['type'] == "attachment": + # Attachment events are just for updating the "uploads" UI; + # they are not sent directly. + pass elif event['type'] == "update_message_flags": # We don't return messages in `/register`, so most flags we # can ignore, but we do need to update the unread_msgs data if diff --git a/zerver/lib/upload.py b/zerver/lib/upload.py index 182f62efd3..dfe36544d4 100644 --- a/zerver/lib/upload.py +++ b/zerver/lib/upload.py @@ -540,16 +540,19 @@ def upload_message_file(uploaded_file_name: Text, uploaded_file_size: int, def claim_attachment(user_profile: UserProfile, path_id: Text, message: Message, - is_message_realm_public: bool) -> None: + is_message_realm_public: bool) -> Attachment: attachment = Attachment.objects.get(path_id=path_id) attachment.messages.add(message) attachment.is_realm_public = attachment.is_realm_public or is_message_realm_public attachment.save() + return attachment def create_attachment(file_name: Text, path_id: Text, user_profile: UserProfile, file_size: int) -> bool: - Attachment.objects.create(file_name=file_name, path_id=path_id, owner=user_profile, - realm=user_profile.realm, size=file_size) + attachment = Attachment.objects.create(file_name=file_name, path_id=path_id, owner=user_profile, + realm=user_profile.realm, size=file_size) + from zerver.lib.actions import notify_attachment_update + notify_attachment_update(user_profile, 'add', attachment.to_dict()) return True def upload_message_image_from_request(request: HttpRequest, user_file: File, diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index d2022fb078..75c29c4b2c 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -10,11 +10,13 @@ from django.conf import settings from django.http import HttpRequest, HttpResponse from django.test import TestCase from django.utils.timezone import now as timezone_now +from io import StringIO from zerver.models import ( get_client, get_realm, get_stream_recipient, get_stream, get_user, Message, RealmDomain, Recipient, UserMessage, UserPresence, UserProfile, Realm, Subscription, Stream, flush_per_request_caches, UserGroup, Service, + Attachment, ) from zerver.lib.actions import ( @@ -79,6 +81,7 @@ from zerver.lib.actions import ( do_update_user_presence, log_event, lookup_default_stream_groups, + notify_attachment_update, notify_realm_custom_profile_fields, check_add_user_group, do_update_user_group_name, @@ -110,6 +113,7 @@ from zerver.lib.validator import ( check_bool, check_dict, check_dict_only, check_float, check_int, check_list, check_string, equals, check_none_or, Validator, check_url ) +from zerver.lib.upload import upload_backend, attachment_url_to_path_id from zerver.views.events_register import _default_all_public_streams, _default_narrow from zerver.views.users import add_service @@ -2096,6 +2100,88 @@ class EventsRegisterTest(ZulipTestCase): result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) self.assertEqual(result['max_message_id'], -1) + def test_add_attachment(self) -> None: + schema_checker = self.check_events_dict([ + ('type', equals('attachment')), + ('op', equals('add')), + ('attachment', check_dict_only([ + ('id', check_int), + ('name', check_string), + ('size', check_int), + ('path_id', check_string), + ('create_time', check_float), + ('messages', check_list(check_dict_only([ + ('id', check_int), + ('name', check_float), + ]))), + ])), + ]) + + self.login(self.example_email("hamlet")) + fp = StringIO("zulip!") + fp.name = "zulip.txt" + data = {'uri': None} + + def do_upload() -> None: + result = self.client_post("/json/user_uploads", {'file': fp}) + + self.assert_json_success(result) + self.assertIn("uri", result.json()) + uri = result.json()["uri"] + base = '/user_uploads/' + self.assertEqual(base, uri[:len(base)]) + data['uri'] = uri + + events = self.do_test( + lambda: do_upload(), + num_events=1, state_change_expected=False) + error = schema_checker('events[0]', events[0]) + self.assert_on_error(error) + + # Verify that the DB has the attachment marked as unclaimed + entry = Attachment.objects.get(file_name='zulip.txt') + self.assertEqual(entry.is_claimed(), False) + + # Now we send an actual message using this attachment. + schema_checker = self.check_events_dict([ + ('type', equals('attachment')), + ('op', equals('update')), + ('attachment', check_dict_only([ + ('id', check_int), + ('name', check_string), + ('size', check_int), + ('path_id', check_string), + ('create_time', check_float), + ('messages', check_list(check_dict_only([ + ('id', check_int), + ('name', check_float), + ]))), + ])), + ]) + + self.subscribe(self.example_user("hamlet"), "Denmark") + body = "First message ...[zulip.txt](http://localhost:9991" + data['uri'] + ")" + events = self.do_test( + lambda: self.send_stream_message(self.example_email("hamlet"), "Denmark", body, "test"), + num_events=2) + error = schema_checker('events[0]', events[0]) + self.assert_on_error(error) + + # Now remove the attachment + schema_checker = self.check_events_dict([ + ('type', equals('attachment')), + ('op', equals('remove')), + ('attachment', check_dict_only([ + ('id', check_int), + ])), + ]) + + events = self.do_test( + lambda: self.client_delete("/json/attachments/%s" % (entry.id,)), + num_events=1, state_change_expected=False) + error = schema_checker('events[0]', events[0]) + self.assert_on_error(error) + class FetchInitialStateDataTest(ZulipTestCase): # Non-admin users don't have access to all bots def test_realm_bots_non_admin(self) -> None: diff --git a/zerver/views/attachments.py b/zerver/views/attachments.py index a39a805255..07d1a77517 100644 --- a/zerver/views/attachments.py +++ b/zerver/views/attachments.py @@ -1,6 +1,7 @@ from django.http import HttpRequest, HttpResponse from zerver.models import UserProfile +from zerver.lib.actions import notify_attachment_update from zerver.lib.validator import check_int from zerver.lib.response import json_success from zerver.lib.attachments import user_attachments, remove_attachment, \ @@ -14,5 +15,6 @@ def list_by_user(request: HttpRequest, user_profile: UserProfile) -> HttpRespons def remove(request: HttpRequest, user_profile: UserProfile, attachment_id: int) -> HttpResponse: attachment = access_attachment_by_id(user_profile, attachment_id, needs_owner=True) + notify_attachment_update(user_profile, "remove", {"id": attachment.id}) remove_attachment(user_profile, attachment) return json_success()