From 697b00dd6e44ebfe4709e9c4384e23428ba5622d Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Sun, 22 Mar 2020 19:29:49 +0000 Subject: [PATCH] default streams: Change remove api to use stream_id. This is a full-stack change: - server - JS code - templates It's all pretty simple--just use stream_id instead of stream_name. I am 99% sure we don't document this API nor use it in mobile, so it should be a safe change. --- frontend_tests/casper_tests/10-admin.js | 24 +++++++------------ static/js/settings_streams.js | 8 +++---- .../templates/admin_default_streams_list.hbs | 2 +- zerver/tests/test_subs.py | 8 +++---- zerver/views/streams.py | 9 ++++--- 5 files changed, 23 insertions(+), 28 deletions(-) diff --git a/frontend_tests/casper_tests/10-admin.js b/frontend_tests/casper_tests/10-admin.js index 11b884bdee..b00c000ba0 100644 --- a/frontend_tests/casper_tests/10-admin.js +++ b/frontend_tests/casper_tests/10-admin.js @@ -437,25 +437,17 @@ casper.then(function () { }); casper.then(function () { - casper.waitUntilVisible('.default_stream_row[id=' + stream_name + ']', function () { - casper.test.assertSelectorHasText('.default_stream_row[id=' + stream_name + '] .default_stream_name', stream_name); + var stream_id = common.get_stream_id(stream_name); + var row = ".default_stream_row[data-stream-id='" + stream_id + "']"; + casper.waitUntilVisible(row, function () { + casper.test.assertSelectorHasText(row + ' .default_stream_name', stream_name); + casper.click(row + ' button.remove-default-stream'); + casper.waitWhileVisible(row, function () { + casper.test.assertDoesntExist(row); + }); }); }); -casper.then(function () { - casper.waitUntilVisible('.default_stream_row[id=' + stream_name + ']', function () { - casper.test.assertSelectorHasText('.default_stream_row[id=' + stream_name + '] .default_stream_name', stream_name); - casper.click('.default_stream_row[id=' + stream_name + '] button.remove-default-stream'); - }); -}); - -casper.then(function () { - casper.waitWhileVisible('.default_stream_row[id=' + stream_name + ']', function () { - casper.test.assertDoesntExist('.default_stream_row[id=' + stream_name + ']'); - }); -}); - - // TODO: Test stream deletion // Test uploading realm icon image diff --git a/static/js/settings_streams.js b/static/js/settings_streams.js index d713cf6ff8..c8bedd9aaa 100644 --- a/static/js/settings_streams.js +++ b/static/js/settings_streams.js @@ -78,9 +78,9 @@ function make_stream_default(stream_name) { }); } -exports.delete_default_stream = function (stream_name, default_stream_row, alert_element) { +exports.delete_default_stream = function (stream_id, default_stream_row, alert_element) { channel.del({ - url: "/json/default_streams" + "?" + $.param({ stream_name: stream_name }), + url: "/json/default_streams" + "?" + $.param({ stream_id: stream_id }), error: function (xhr) { ui_report.generic_row_button_error(xhr, alert_element); }, @@ -135,8 +135,8 @@ exports.build_page = function () { $("body").on("click", ".default_stream_row .remove-default-stream", function (e) { const row = $(this).closest(".default_stream_row"); - const stream_name = row.attr("id"); - exports.delete_default_stream(stream_name, row, $(e.target)); + const stream_id = parseInt(row.attr("data-stream-id"), 10); + exports.delete_default_stream(stream_id, row, $(e.target)); }); }; diff --git a/static/templates/admin_default_streams_list.hbs b/static/templates/admin_default_streams_list.hbs index d36ab41dd6..1771297fc4 100644 --- a/static/templates/admin_default_streams_list.hbs +++ b/static/templates/admin_default_streams_list.hbs @@ -1,5 +1,5 @@ {{#with stream}} - + {{#if invite_only}}{{/if}} {{name}} diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index b5ef34bac3..db84b33642 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -1208,7 +1208,7 @@ class DefaultStreamTest(ZulipTestCase): self.login_user(user_profile) stream_name = 'stream ADDED via api' - ensure_stream(user_profile.realm, stream_name) + stream = ensure_stream(user_profile.realm, stream_name) result = self.client_post('/json/default_streams', dict(stream_name=stream_name)) self.assert_json_success(result) self.assertTrue(stream_name in self.get_default_stream_names(user_profile.realm)) @@ -1237,13 +1237,13 @@ class DefaultStreamTest(ZulipTestCase): self.assertTrue(len(other_streams) > 0) # and remove it - result = self.client_delete('/json/default_streams', dict(stream_name=stream_name)) + result = self.client_delete('/json/default_streams', dict(stream_id=stream.id)) self.assert_json_success(result) self.assertFalse(stream_name in self.get_default_stream_names(user_profile.realm)) # Test admin can't add unsubscribed private stream stream_name = "private_stream" - self.make_stream(stream_name, invite_only=True) + stream = self.make_stream(stream_name, invite_only=True) self.subscribe(self.example_user('iago'), stream_name) result = self.client_post('/json/default_streams', dict(stream_name=stream_name)) self.assert_json_error(result, "Invalid stream name '%s'" % (stream_name,)) @@ -1255,7 +1255,7 @@ class DefaultStreamTest(ZulipTestCase): # Test admin can remove unsubscribed private stream self.unsubscribe(user_profile, stream_name) - result = self.client_delete('/json/default_streams', dict(stream_name=stream_name)) + result = self.client_delete('/json/default_streams', dict(stream_id=stream.id)) self.assert_json_success(result) self.assertFalse(stream_name in self.get_default_stream_names(user_profile.realm)) diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 7bdf7838a5..0d3b00e547 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -134,9 +134,12 @@ def remove_default_stream_group(request: HttpRequest, user_profile: UserProfile, @has_request_variables def remove_default_stream(request: HttpRequest, user_profile: UserProfile, - stream_name: str=REQ()) -> HttpResponse: - (stream, recipient, sub) = access_stream_by_name(user_profile, stream_name, - allow_realm_admin=True) + stream_id: int=REQ(validator=check_int)) -> HttpResponse: + (stream, recipient, sub) = access_stream_by_id( + user_profile, + stream_id, + allow_realm_admin=True + ) do_remove_default_stream(stream) return json_success()