From 96726c00ce7afc2d5b7c6fb36c6b69962e0ffdc1 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 24 Sep 2019 13:46:53 -0700 Subject: [PATCH] export: Fix broken URLs in UI with S3 backend. Apparently, the Zulip notifications (and resulting emails) were correct, but the download links inside the Zulip UI were incorrectly not including S3 prefix on the URL, making them not work. While we're at this, we rewrite the somewhat convoluted previous system for formatting the data export output. --- static/js/settings_exports.js | 4 ++-- static/templates/admin_export_list.hbs | 4 ++-- zerver/lib/export.py | 6 +++++- zerver/lib/upload.py | 13 +++++++++++++ zerver/tests/test_events.py | 11 ++++------- zerver/tests/test_realm_export.py | 5 +++-- 6 files changed, 29 insertions(+), 14 deletions(-) diff --git a/static/js/settings_exports.js b/static/js/settings_exports.js index 5a778eb0b8..311728c79a 100644 --- a/static/js/settings_exports.js +++ b/static/js/settings_exports.js @@ -33,7 +33,7 @@ exports.populate_exports_table = function (exports) { var exports_list = list_render.create(exports_table, Object.values(exports), { name: "admin_exports_list", modifier: function (data) { - if (data.export_data.deleted_timestamp === undefined) { + if (data.deleted_timestamp === null) { return render_admin_export_list({ realm_export: { id: data.id, @@ -42,7 +42,7 @@ exports.populate_exports_table = function (exports) { event_time: timerender.last_seen_status_from_date( new XDate(data.export_time * 1000) ), - path: data.export_data.export_path, + url: data.export_url, }, }); } diff --git a/static/templates/admin_export_list.hbs b/static/templates/admin_export_list.hbs index 8b418e5083..4986589bd6 100644 --- a/static/templates/admin_export_list.hbs +++ b/static/templates/admin_export_list.hbs @@ -7,8 +7,8 @@ {{event_time}} - {{#if path}} - {{t 'Download' }} + {{#if url}} + {{t 'Download' }} {{else}} {{t 'The export URL is not yet available... Check back soon.' }} {{/if}} diff --git a/zerver/lib/export.py b/zerver/lib/export.py index 4ca9ab4d6a..414d0665fd 100644 --- a/zerver/lib/export.py +++ b/zerver/lib/export.py @@ -1707,10 +1707,14 @@ def get_realm_exports_serialized(user: UserProfile) -> List[Dict[str, Any]]: event_type=RealmAuditLog.REALM_EXPORTED) exports_dict = {} for export in all_exports: + export_data = ujson.loads(export.extra_data) + export_url = zerver.lib.upload.upload_backend.get_export_tarball_url( + user.realm, export_data['export_path']) exports_dict[export.id] = dict( id=export.id, export_time=export.event_time.timestamp(), acting_user_id=export.acting_user.id, - export_data=ujson.loads(export.extra_data) + export_url=export_url, + deleted_timestamp=export_data.get('deleted_timestamp'), ) return sorted(exports_dict.values(), key=lambda export_dict: export_dict['id']) diff --git a/zerver/lib/upload.py b/zerver/lib/upload.py index 3d2898b463..c6c47b0d2e 100644 --- a/zerver/lib/upload.py +++ b/zerver/lib/upload.py @@ -259,6 +259,10 @@ class ZulipUploadBackend: def delete_export_tarball(self, path_id: str) -> Optional[str]: raise NotImplementedError() + def get_export_tarball_url(self, realm: Realm, export_path: str) -> str: + raise NotImplementedError() + + ### S3 def get_bucket(conn: S3Connection, bucket_name: str) -> Bucket: @@ -453,6 +457,11 @@ class S3UploadBackend(ZulipUploadBackend): # ?x=x allows templates to append additional parameters with &s return "https://%s.s3.amazonaws.com/%s%s?x=x" % (bucket, hash_key, medium_suffix) + def get_export_tarball_url(self, realm: Realm, export_path: str) -> str: + bucket = settings.S3_AVATAR_BUCKET + # export_path has a leading / + return "https://%s.s3.amazonaws.com%s" % (bucket, export_path) + def upload_realm_icon_image(self, icon_file: File, user_profile: UserProfile) -> None: content_type = guess_type(icon_file.name)[0] bucket_name = settings.S3_AVATAR_BUCKET @@ -815,6 +824,10 @@ class LocalUploadBackend(ZulipUploadBackend): return path_id return None + def get_export_tarball_url(self, realm: Realm, export_path: str) -> str: + # export_path has a leading `/` + return realm.uri + export_path + # Common and wrappers if settings.LOCAL_UPLOADS_DIR is not None: upload_backend = LocalUploadBackend() # type: ZulipUploadBackend diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index ce3600b505..a9facdefdc 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -2773,9 +2773,8 @@ class EventsRegisterTest(ZulipTestCase): ('id', check_int), ('export_time', check_float), ('acting_user_id', check_int), - ('export_data', check_dict_only([ - ('export_path', check_string), - ])), + ('export_url', check_string), + ('deleted_timestamp', equals(None)), ]))), ]) @@ -2800,10 +2799,8 @@ class EventsRegisterTest(ZulipTestCase): ('id', check_int), ('export_time', check_float), ('acting_user_id', check_int), - ('export_data', check_dict_only([ - ('export_path', check_string), - ('deleted_timestamp', check_float), - ])), + ('export_url', check_string), + ('deleted_timestamp', check_float), ]))), ]) diff --git a/zerver/tests/test_realm_export.py b/zerver/tests/test_realm_export.py index 2ad9df5566..0337df761a 100644 --- a/zerver/tests/test_realm_export.py +++ b/zerver/tests/test_realm_export.py @@ -68,7 +68,8 @@ class RealmExportTest(ZulipTestCase): # Test that the export we have is the export we created. export_dict = result.json()['exports'] self.assertEqual(export_dict[0]['id'], audit_log_entry.id) - self.assertEqual(export_dict[0]['export_data'].get('export_path'), path_id) + self.assertEqual(export_dict[0]['export_url'], + 'https://test-avatar-bucket.s3.amazonaws.com' + path_id) self.assertEqual(export_dict[0]['acting_user_id'], admin.id) self.assert_length(export_dict, RealmAuditLog.objects.filter( @@ -126,7 +127,7 @@ class RealmExportTest(ZulipTestCase): # Test that the export we have is the export we created. export_dict = result.json()['exports'] self.assertEqual(export_dict[0]['id'], audit_log_entry.id) - self.assertEqual(export_dict[0]['export_data'].get('export_path'), path_id) + self.assertEqual(export_dict[0]['export_url'], admin.realm.uri + path_id) self.assertEqual(export_dict[0]['acting_user_id'], admin.id) self.assert_length(export_dict, RealmAuditLog.objects.filter(