From 155f6da8ba7af44e002f4f967b4015245c0dad1b Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Sun, 10 May 2020 17:21:08 +0000 Subject: [PATCH] bots: Add owner_id to bot-related payloads. For the below payloads we want `owner_id` instead of `owner`, which we should deprecate. (The `owner` field is actually an email, which is not a stable key.) page_params.realm_bots realm_bot/add realm_bot/update IMPORTANT NOTE: Some of the data served in these payloads is cached with the key `bot_dicts_in_realm_cache_key`. For page_params, we get the new field via `get_owned_bot_dicts`. For realm_bot/add, we modified `created_bot_event`. For realm_bot/update, we modified `do_change_bot_owner`. On the JS side, we no longer look up the bot's owner directly in `server_events_dispatch` when we get a realm_bot/update event. Instead, we delegate that job to `bot_data.js`. I modified the tests accordingly. --- frontend_tests/node_tests/bot_data.js | 9 +++++++++ frontend_tests/node_tests/dispatch.js | 17 ----------------- static/js/bot_data.js | 25 ++++++++++++++++++++++--- static/js/server_events_dispatch.js | 3 --- templates/zerver/api/changelog.md | 6 ++++++ zerver/lib/actions.py | 4 +++- zerver/lib/cache.py | 20 ++++++++++++++------ zerver/lib/events.py | 7 ++++++- zerver/tests/test_bots.py | 4 ++++ zerver/tests/test_events.py | 3 +++ zerver/tests/test_home.py | 1 + 11 files changed, 68 insertions(+), 31 deletions(-) diff --git a/frontend_tests/node_tests/bot_data.js b/frontend_tests/node_tests/bot_data.js index 496026ada4..e7195deaa4 100644 --- a/frontend_tests/node_tests/bot_data.js +++ b/frontend_tests/node_tests/bot_data.js @@ -80,6 +80,15 @@ run_test('test_basics', () => { assert.equal('New Bot 1', bot.full_name); assert.equal(2, services[0].interface); assert.equal('http://baz.com', services[0].base_url); + + const change_owner_event = { + owner_id: fred.user_id, + }; + bot_data.update(43, change_owner_event); + + bot = bot_data.get(43); + assert.equal(bot.owner_id, fred.user_id); + assert.equal(bot.owner, fred.email); }()); (function test_embedded_bot_update() { diff --git a/frontend_tests/node_tests/dispatch.js b/frontend_tests/node_tests/dispatch.js index 6bc150479c..3c3f59b1ea 100644 --- a/frontend_tests/node_tests/dispatch.js +++ b/frontend_tests/node_tests/dispatch.js @@ -380,17 +380,6 @@ const event_fixtures = { }, }, - realm_bot__update_owner: { - type: 'realm_bot', - op: 'update', - bot: { - email: 'the-bot@example.com', - user_id: 4321, - full_name: 'The Bot Has A New Name', - owner_id: test_user.user_id, - }, - }, - realm_emoji: { type: 'realm_emoji', realm_emoji: { @@ -1110,12 +1099,6 @@ with_overrides(function (override) { assert_same(args.update_bot_data, event.bot); }); }); - - event = event_fixtures.realm_bot__update_owner; - override('bot_data.update', noop); - override('settings_users.update_user_data', noop); - dispatch(event); - assert_same(event.bot.owner, 'test@example.com'); }); with_overrides(function (override) { diff --git a/static/js/bot_data.js b/static/js/bot_data.js index 03a360bf6b..ea19d30263 100644 --- a/static/js/bot_data.js +++ b/static/js/bot_data.js @@ -1,7 +1,20 @@ const bots = new Map(); -const bot_fields = ['api_key', 'avatar_url', 'default_all_public_streams', - 'default_events_register_stream', 'default_sending_stream', - 'email', 'full_name', 'is_active', 'owner', 'bot_type', 'user_id']; + +const bot_fields = [ + 'api_key', + 'avatar_url', + 'bot_type', + 'default_all_public_streams', + 'default_events_register_stream', + 'default_sending_stream', + 'email', + 'full_name', + 'is_active', + 'owner', // TODO: eliminate + 'owner_id', + 'user_id', +]; + const services = new Map(); const services_fields = ['base_url', 'interface', 'config_data', 'service_name', 'token']; @@ -39,6 +52,12 @@ exports.update = function (bot_id, bot_update) { if (typeof bot_update.services !== 'undefined' && bot_update.services.length > 0) { Object.assign(service, _.pick(bot_update.services[0], services_fields)); } + + // TODO: eliminate `owner` (which is an email) + if (bot.owner_id) { + const bot_owner = people.get_by_user_id(bot.owner_id); + bot.owner = bot_owner.email; + } send_change_event(); }; diff --git a/static/js/server_events_dispatch.js b/static/js/server_events_dispatch.js index cc3295218f..a85d26c287 100644 --- a/static/js/server_events_dispatch.js +++ b/static/js/server_events_dispatch.js @@ -205,9 +205,6 @@ exports.dispatch_normal_event = function dispatch_normal_event(event) { bot_data.del(event.bot.user_id); settings_users.update_user_data(event.bot.user_id, event.bot); } else if (event.op === 'update') { - if (Object.prototype.hasOwnProperty.call(event.bot, 'owner_id')) { - event.bot.owner = people.get_by_user_id(event.bot.owner_id).email; - } bot_data.update(event.bot.user_id, event.bot); settings_users.update_user_data(event.bot.user_id, event.bot); } diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 6012310000..36e2d85a06 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,12 @@ below features are supported. ## Changes in Zulip 2.2 +**Feature level 5** +* [`GET /events`](/api/get-events-from-queue): `realm_bot` events, + sent when changes are made to bot users, now contain an + integer-format `owner_id` field, replacing the `owner` field (which + was an email address). + **Feature level 4** * `jitsi_server_url`, `development_environment`, `server_generation`, diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 764049a53d..9b07d5e0b6 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -477,6 +477,7 @@ def created_bot_event(user_profile: UserProfile) -> Dict[str, Any]: # set the owner key while reactivating them. if user_profile.bot_owner is not None: bot['owner'] = user_profile.bot_owner.email + bot['owner_id'] = user_profile.bot_owner.id return dict(type="realm_bot", op="add", bot=bot) @@ -5732,7 +5733,8 @@ def get_owned_bot_dicts(user_profile: UserProfile, 'default_sending_stream': botdict['default_sending_stream__name'], 'default_events_register_stream': botdict['default_events_register_stream__name'], 'default_all_public_streams': botdict['default_all_public_streams'], - 'owner': botdict['bot_owner__email'], + 'owner': botdict['bot_owner__email'], # TODO: eliminate + 'owner_id': botdict['bot_owner__id'], 'avatar_url': avatar_url_from_dict(botdict), 'services': services_by_ids[botdict['id']], } diff --git a/zerver/lib/cache.py b/zerver/lib/cache.py index 9bbb3c6386..aa9f81c64c 100644 --- a/zerver/lib/cache.py +++ b/zerver/lib/cache.py @@ -461,13 +461,21 @@ def active_non_guest_user_ids_cache_key(realm_id: int) -> str: return "active_non_guest_user_ids:%s" % (realm_id,) bot_dict_fields: List[str] = [ - 'id', 'full_name', 'short_name', 'bot_type', 'email', - 'is_active', 'default_sending_stream__name', - 'realm_id', - 'default_events_register_stream__name', - 'default_all_public_streams', 'api_key', - 'bot_owner__email', 'avatar_source', + 'api_key', + 'avatar_source', 'avatar_version', + 'bot_owner__id', + 'bot_owner__email', # TODO: eliminate + 'bot_type', + 'default_all_public_streams', + 'default_events_register_stream__name', + 'default_sending_stream__name', + 'email', + 'full_name', + 'id', + 'is_active', + 'realm_id', + 'short_name', ] def bot_dicts_in_realm_cache_key(realm: 'Realm') -> str: diff --git a/zerver/lib/events.py b/zerver/lib/events.py index beeea1ab18..fa865e8568 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -480,7 +480,12 @@ def apply_event(state: Dict[str, Any], for bot in state['realm_bots']: if bot['email'] == event['bot']['email']: if 'owner_id' in event['bot']: - bot['owner'] = get_user_profile_by_id(event['bot']['owner_id']).email + bot_owner_id = event['bot']['owner_id'] + bot['owner_id'] = bot_owner_id + + # TODO: eliminate `owner` field. + bot_owner = get_user_profile_by_id(bot_owner_id) + bot['owner'] = bot_owner.email else: bot.update(event['bot']) diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py index 2c43d993cb..d507cc2e47 100644 --- a/zerver/tests/test_bots.py +++ b/zerver/tests/test_bots.py @@ -181,6 +181,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): default_events_register_stream=None, default_all_public_streams=False, services=[], + owner_id=hamlet.id, owner=hamlet.email, ), ), @@ -314,6 +315,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): default_events_register_stream=None, default_all_public_streams=False, services=[], + owner_id=user.id, # Important: This is the product-facing # email, not the delivery email, for this # user. TODO: Migrate this to an integer ID. @@ -417,6 +419,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): default_events_register_stream=None, default_all_public_streams=False, services=[], + owner_id=user_profile.id, owner=user_profile.email, ), ), @@ -492,6 +495,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): default_events_register_stream='Denmark', default_all_public_streams=False, services=[], + owner_id=user_profile.id, owner=user_profile.email, ), ), diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 889bed3bd1..90ac48e75a 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -2110,6 +2110,7 @@ class EventsRegisterTest(ZulipTestCase): ('default_all_public_streams', check_bool), ('avatar_url', check_string), ('owner', check_string), + ('owner_id', check_int), ('services', check_services), ])), ]) @@ -2303,6 +2304,7 @@ class EventsRegisterTest(ZulipTestCase): ('default_all_public_streams', check_bool), ('avatar_url', check_string), ('owner', check_string), + ('owner_id', check_int), ('services', check_services), ])), ]) @@ -2374,6 +2376,7 @@ class EventsRegisterTest(ZulipTestCase): ('default_all_public_streams', check_bool), ('avatar_url', check_string), ('owner', check_none_or(check_string)), + ('owner_id', check_none_or(check_int)), ('services', check_list(check_dict_only([ ('base_url', check_url), ('interface', check_int), diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index bdd6627694..f418812c43 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -279,6 +279,7 @@ class HomeTest(ZulipTestCase): 'full_name', 'is_active', 'owner', + 'owner_id', 'services', 'user_id', ]