paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-14 10:57:58 +00:00
Code Issues Packages Projects Releases Wiki Activity

event_schema: Extract check_realm_user_update.

Browse Source 
This a pretty big commit, but I really wanted it
to be atomic.

All realm_user/update events look the same from
the top:

    _check_realm_user_update = check_events_dict(
        required_keys=[
            ("type", equals("realm_user")),
            ("op", equals("update")),
            ("person", _check_realm_user_person),
        ]
    )

And then we have a bunch of fields for person that
are optional, and we usually only send user_id plus
one other field, with the exception of avatar-related
events:

    _check_realm_user_person = check_dict_only(
        required_keys=[
            # vertical formatting
            ("user_id", check_int),
        ],
        optional_keys=[
            ("avatar_source", check_string),
            ("avatar_url", check_none_or(check_string)),
            ("avatar_url_medium", check_none_or(check_string)),
            ("avatar_version", check_int),
            ("bot_owner_id", check_int),
            ("custom_profile_field", _check_custom_profile_field),
            ("delivery_email", check_string),
            ("full_name", check_string),
            ("role", check_int_in(UserProfile.ROLE_TYPES)),
            ("email", check_string),
            ("user_id", check_int),
            ("timezone", check_string),
        ],
    )

I would start the code review by just skimming the changes
to event_schema.py, to get the big picture of the complexity
here.  Basically the schema is just the combined superset of
all the individual schemas that we remove from test_events.

Then I would read test_events.py.

The simplest diffs are basically of this form:

    -  schema_checker = check_events_dict([
    -      ('type', equals('realm_user')),
    -      ('op', equals('update')),
    -      ('person', check_dict_only([
    -          ('role', check_int_in(UserProfile.ROLE_TYPES)),
    -          ('user_id', check_int),
    -      ])),
    -  ])

    # ...
    -  schema_checker('events[0]', events[0])
    +  check_realm_user_update('events[0]', events[0], {'role'})

Instead of a custom schema checker, we use the "superset"
schema checker, but then we pass in the set of fields that we
expect to be there.  Note that 'user_id' is always there.

So most of the heavy lifting happens in this new function
in event_schema.py:

    def check_realm_user_update(
        var_name: str, event: Dict[str, Any], optional_fields: Set[str],
    ) -> None:
        _check_realm_user_update(var_name, event)

        keys = set(event["person"].keys()) - {"user_id"}
        assert optional_fields == keys

But we still do some more custom checks in test_events.py.

custom profile fields: check keys of custom_profile_field

     def test_custom_profile_field_data_events(self) -> None:
+        self.assertEqual(
+            events[0]['person']['custom_profile_field'].keys(),
+            {"id", "value", "rendered_value"}
+        )

+        check_realm_user_update('events[0]', events[0], {"custom_profile_field"})
+        self.assertEqual(
+            events[0]['person']['custom_profile_field'].keys(),
+            {"id", "value"}
+        )

avatar fields: check more specific types, since the superset
    schema has check_none_or(check_string)

     def test_change_avatar_fields(self) -> None:
+        check_realm_user_update('events[0]', events[0], avatar_fields)
+        assert isinstance(events[0]['person']['avatar_url'], str)
+        assert isinstance(events[0]['person']['avatar_url_medium'], str)

+        check_realm_user_update('events[0]', events[0], avatar_fields)
+        self.assertEqual(events[0]['person']['avatar_url'], None)
+        self.assertEqual(events[0]['person']['avatar_url_medium'], None)

Also note that avatar_fields is a set of four fields that
are set in event_schema.

full name: no extra work!

     def test_change_full_name(self) -> None:
-        schema_checker('events[0]', events[0])
+        check_realm_user_update('events[0]', events[0], {'full_name'})

test_change_user_delivery_email_email_address_visibilty_admins:

    no extra work for delivery_email
    check avatar fields more directly

roles (several examples) -- actually check the specific role

     def test_change_realm_authentication_methods(self) -> None:
-            schema_checker('events[0]', events[0])
+            check_realm_user_update('events[0]', events[0], {'role'})
+            self.assertEqual(events[0]['person']['role'], role)

bot_owner_id: no extra work!

-        change_bot_owner_checker_user('events[1]', events[1])
+        check_realm_user_update('events[1]', events[1], {"bot_owner_id"})

-        change_bot_owner_checker_user('events[1]', events[1])
+        check_realm_user_update('events[1]', events[1], {"bot_owner_id"})

-        change_bot_owner_checker_user('events[1]', events[1])
+        check_realm_user_update('events[1]', events[1], {"bot_owner_id"})

timezone: no extra work!

-                timezone_schema_checker('events[1]', events[1])
+                check_realm_user_update('events[1]', events[1], {"email", "timezone"})
This commit is contained in:
Steve Howell
2020-07-23 14:04:06 +00:00
committed by Tim Abbott
parent 38bd66d8ae
commit 176ab66fc7
2 changed files with 94 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files

61
zerver/lib/event_schema.py
View File

@@ -5,7 +5,7 @@ It will contain schemas (aka validators) for Zulip events.
Right now it's only intended to be used by test code. Right now it's only intended to be used by test code.
""" """
from typing import Any, Dict, Sequence, Tuple, Union from typing import Any, Dict, Sequence, Set, Tuple, Union
from zerver.lib.topic import ORIG_TOPIC, TOPIC_LINKS, TOPIC_NAME from zerver.lib.topic import ORIG_TOPIC, TOPIC_LINKS, TOPIC_NAME
from zerver.lib.validator import ( from zerver.lib.validator import (
@@ -14,6 +14,7 @@ from zerver.lib.validator import (
check_dict, check_dict,
check_dict_only, check_dict_only,
check_int, check_int,
check_int_in,
check_list, check_list,
check_none_or, check_none_or,
check_string, check_string,
@@ -390,6 +391,64 @@ def check_realm_update(var_name: str, event: Dict[str, Any],) -> None:
raise AssertionError(f"Unexpected property type {property_type}") raise AssertionError(f"Unexpected property type {property_type}")
avatar_fields = {
"avatar_source",
"avatar_url",
"avatar_url_medium",
"avatar_version",
}
_check_custom_profile_field = check_dict_only(
required_keys=[
# vertical formatting
("id", check_int),
("value", check_string),
],
optional_keys=[
# vertical formatting
("rendered_value", check_string),
],
)
_check_realm_user_person = check_dict_only(
required_keys=[
# vertical formatting
("user_id", check_int),
],
optional_keys=[
("avatar_source", check_string),
("avatar_url", check_none_or(check_string)),
("avatar_url_medium", check_none_or(check_string)),
("avatar_version", check_int),
("bot_owner_id", check_int),
("custom_profile_field", _check_custom_profile_field),
("delivery_email", check_string),
("full_name", check_string),
("role", check_int_in(UserProfile.ROLE_TYPES)),
("email", check_string),
("user_id", check_int),
("timezone", check_string),
],
)
_check_realm_user_update = check_events_dict(
required_keys=[
("type", equals("realm_user")),
("op", equals("update")),
("person", _check_realm_user_person),
]
)
def check_realm_user_update(
var_name: str, event: Dict[str, Any], optional_fields: Set[str],
) -> None:
_check_realm_user_update(var_name, event)
keys = set(event["person"].keys()) - {"user_id"}
assert optional_fields == keys
check_stream_create = check_events_dict( check_stream_create = check_events_dict(
required_keys=[ required_keys=[
("type", equals("stream")), ("type", equals("stream")),

168
zerver/tests/test_events.py
View File

@@ -90,6 +90,7 @@ from zerver.lib.actions import (
try_update_realm_custom_profile_field, try_update_realm_custom_profile_field,
) )
from zerver.lib.event_schema import ( from zerver.lib.event_schema import (
avatar_fields,
basic_stream_fields, basic_stream_fields,
check_alert_words, check_alert_words,
check_custom_profile_fields, check_custom_profile_fields,
@@ -102,6 +103,7 @@ from zerver.lib.event_schema import (
check_realm_bot_remove, check_realm_bot_remove,
check_realm_bot_update, check_realm_bot_update,
check_realm_update, check_realm_update,
check_realm_user_update,
check_stream_create, check_stream_create,
check_stream_update, check_stream_update,
check_submessage, check_submessage,
@@ -135,7 +137,6 @@ from zerver.lib.validator import (
check_dict_only, check_dict_only,
check_float, check_float,
check_int, check_int,
check_int_in,
check_list, check_list,
check_none_or, check_none_or,
check_string, check_string,
@@ -643,31 +644,6 @@ class NormalActionsTest(BaseAction):
check_custom_profile_fields('events[0]', events[0]) check_custom_profile_fields('events[0]', events[0])
def test_custom_profile_field_data_events(self) -> None: def test_custom_profile_field_data_events(self) -> None:
schema_checker_basic = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('user_id', check_int),
('custom_profile_field', check_dict_only([
('id', check_int),
('value', check_none_or(check_string)),
])),
])),
])
schema_checker_with_rendered_value = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('user_id', check_int),
('custom_profile_field', check_dict_only([
('id', check_int),
('value', check_none_or(check_string)),
('rendered_value', check_none_or(check_string)),
])),
])),
])
field_id = self.user_profile.realm.customprofilefield_set.get( field_id = self.user_profile.realm.customprofilefield_set.get(
realm=self.user_profile.realm, name='Biography').id realm=self.user_profile.realm, name='Biography').id
field = { field = {
@@ -678,7 +654,11 @@ class NormalActionsTest(BaseAction):
lambda: do_update_user_custom_profile_data_if_changed( lambda: do_update_user_custom_profile_data_if_changed(
self.user_profile, self.user_profile,
[field])) [field]))
schema_checker_with_rendered_value('events[0]', events[0]) check_realm_user_update('events[0]', events[0], {"custom_profile_field"})
self.assertEqual(
events[0]['person']['custom_profile_field'].keys(),
{"id", "value", "rendered_value"}
)
# Test we pass correct stringify value in custom-user-field data event # Test we pass correct stringify value in custom-user-field data event
field_id = self.user_profile.realm.customprofilefield_set.get( field_id = self.user_profile.realm.customprofilefield_set.get(
@@ -691,7 +671,11 @@ class NormalActionsTest(BaseAction):
lambda: do_update_user_custom_profile_data_if_changed( lambda: do_update_user_custom_profile_data_if_changed(
self.user_profile, self.user_profile,
[field])) [field]))
schema_checker_basic('events[0]', events[0]) check_realm_user_update('events[0]', events[0], {"custom_profile_field"})
self.assertEqual(
events[0]['person']['custom_profile_field'].keys(),
{"id", "value"}
)
def test_presence_events(self) -> None: def test_presence_events(self) -> None:
fields = [ fields = [
@@ -1051,74 +1035,29 @@ class NormalActionsTest(BaseAction):
muted_topics_checker('events[0]', events[0]) muted_topics_checker('events[0]', events[0])
def test_change_avatar_fields(self) -> None: def test_change_avatar_fields(self) -> None:
schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('avatar_url', check_string),
('avatar_url_medium', check_string),
('avatar_version', check_int),
('avatar_source', check_string),
('user_id', check_int),
])),
])
events = self.verify_action( events = self.verify_action(
lambda: do_change_avatar_fields(self.user_profile, UserProfile.AVATAR_FROM_USER, acting_user=self.user_profile), lambda: do_change_avatar_fields(self.user_profile, UserProfile.AVATAR_FROM_USER, acting_user=self.user_profile),
) )
schema_checker('events[0]', events[0]) check_realm_user_update('events[0]', events[0], avatar_fields)
assert isinstance(events[0]['person']['avatar_url'], str)
assert isinstance(events[0]['person']['avatar_url_medium'], str)
schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('avatar_source', check_string),
('avatar_url', check_none_or(check_string)),
('avatar_url_medium', check_none_or(check_string)),
('avatar_version', check_int),
('user_id', check_int),
])),
])
events = self.verify_action( events = self.verify_action(
lambda: do_change_avatar_fields(self.user_profile, UserProfile.AVATAR_FROM_GRAVATAR, acting_user=self.user_profile), lambda: do_change_avatar_fields(self.user_profile, UserProfile.AVATAR_FROM_GRAVATAR, acting_user=self.user_profile),
) )
schema_checker('events[0]', events[0]) check_realm_user_update('events[0]', events[0], avatar_fields)
self.assertEqual(events[0]['person']['avatar_url'], None)
self.assertEqual(events[0]['person']['avatar_url_medium'], None)
def test_change_full_name(self) -> None: def test_change_full_name(self) -> None:
schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('full_name', check_string),
('user_id', check_int),
])),
])
events = self.verify_action( events = self.verify_action(
lambda: do_change_full_name( lambda: do_change_full_name(
self.user_profile, self.user_profile,
'Sir Hamlet', 'Sir Hamlet',
self.user_profile)) self.user_profile))
schema_checker('events[0]', events[0]) check_realm_user_update('events[0]', events[0], {'full_name'})
def test_change_user_delivery_email_email_address_visibilty_admins(self) -> None: def test_change_user_delivery_email_email_address_visibilty_admins(self) -> None:
schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('delivery_email', check_string),
('user_id', check_int),
])),
])
avatar_schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('avatar_source', check_string),
('avatar_url', check_string),
('avatar_url_medium', check_string),
('avatar_version', check_int),
('user_id', check_int),
])),
])
do_set_realm_property(self.user_profile.realm, "email_address_visibility", do_set_realm_property(self.user_profile.realm, "email_address_visibility",
Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS) Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS)
# Important: We need to refresh from the database here so that # Important: We need to refresh from the database here so that
@@ -1130,8 +1069,11 @@ class NormalActionsTest(BaseAction):
action, action,
num_events=2, num_events=2,
client_gravatar=False) client_gravatar=False)
schema_checker('events[0]', events[0])
avatar_schema_checker('events[1]', events[1]) check_realm_user_update('events[0]', events[0], {"delivery_email"})
check_realm_user_update('events[1]', events[1], avatar_fields)
assert isinstance(events[1]['person']['avatar_url'], str)
assert isinstance(events[1]['person']['avatar_url_medium'], str)
def test_change_realm_authentication_methods(self) -> None: def test_change_realm_authentication_methods(self) -> None:
schema_checker = check_events_dict([ schema_checker = check_events_dict([
@@ -1298,20 +1240,12 @@ class NormalActionsTest(BaseAction):
# for email being passed into this next function. # for email being passed into this next function.
self.user_profile.refresh_from_db() self.user_profile.refresh_from_db()
schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('role', check_int_in(UserProfile.ROLE_TYPES)),
('user_id', check_int),
])),
])
do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER) do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER)
for role in [UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_MEMBER]: for role in [UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_MEMBER]:
events = self.verify_action( events = self.verify_action(
lambda: do_change_user_role(self.user_profile, role)) lambda: do_change_user_role(self.user_profile, role))
schema_checker('events[0]', events[0]) check_realm_user_update('events[0]', events[0], {'role'})
self.assertEqual(events[0]['person']['role'], role)
def test_change_is_owner(self) -> None: def test_change_is_owner(self) -> None:
reset_emails_in_zulip_realm() reset_emails_in_zulip_realm()
@@ -1321,20 +1255,12 @@ class NormalActionsTest(BaseAction):
# for email being passed into this next function. # for email being passed into this next function.
self.user_profile.refresh_from_db() self.user_profile.refresh_from_db()
schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('role', check_int_in(UserProfile.ROLE_TYPES)),
('user_id', check_int),
])),
])
do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER) do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER)
for role in [UserProfile.ROLE_REALM_OWNER, UserProfile.ROLE_MEMBER]: for role in [UserProfile.ROLE_REALM_OWNER, UserProfile.ROLE_MEMBER]:
events = self.verify_action( events = self.verify_action(
lambda: do_change_user_role(self.user_profile, role)) lambda: do_change_user_role(self.user_profile, role))
schema_checker('events[0]', events[0]) check_realm_user_update('events[0]', events[0], {'role'})
self.assertEqual(events[0]['person']['role'], role)
def test_change_is_guest(self) -> None: def test_change_is_guest(self) -> None:
reset_emails_in_zulip_realm() reset_emails_in_zulip_realm()
@@ -1344,20 +1270,12 @@ class NormalActionsTest(BaseAction):
# for email being passed into this next function. # for email being passed into this next function.
self.user_profile.refresh_from_db() self.user_profile.refresh_from_db()
schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('role', check_int_in(UserProfile.ROLE_TYPES)),
('user_id', check_int),
])),
])
do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER) do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER)
for role in [UserProfile.ROLE_GUEST, UserProfile.ROLE_MEMBER]: for role in [UserProfile.ROLE_GUEST, UserProfile.ROLE_MEMBER]:
events = self.verify_action( events = self.verify_action(
lambda: do_change_user_role(self.user_profile, role)) lambda: do_change_user_role(self.user_profile, role))
schema_checker('events[0]', events[0]) check_realm_user_update('events[0]', events[0], {'role'})
self.assertEqual(events[0]['person']['role'], role)
def test_change_notification_settings(self) -> None: def test_change_notification_settings(self) -> None:
for notification_setting, v in self.user_profile.notification_setting_types.items(): for notification_setting, v in self.user_profile.notification_setting_types.items():
@@ -1652,22 +1570,13 @@ class NormalActionsTest(BaseAction):
check_realm_bot_update('events[0]', events[0], 'default_events_register_stream') check_realm_bot_update('events[0]', events[0], 'default_events_register_stream')
def test_change_bot_owner(self) -> None: def test_change_bot_owner(self) -> None:
change_bot_owner_checker_user = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('user_id', check_int),
('bot_owner_id', check_int),
])),
])
self.user_profile = self.example_user('iago') self.user_profile = self.example_user('iago')
owner = self.example_user('hamlet') owner = self.example_user('hamlet')
bot = self.create_bot('test') bot = self.create_bot('test')
action = lambda: do_change_bot_owner(bot, owner, self.user_profile) action = lambda: do_change_bot_owner(bot, owner, self.user_profile)
events = self.verify_action(action, num_events=2) events = self.verify_action(action, num_events=2)
check_realm_bot_update('events[0]', events[0], 'owner_id') check_realm_bot_update('events[0]', events[0], 'owner_id')
change_bot_owner_checker_user('events[1]', events[1]) check_realm_user_update('events[1]', events[1], {"bot_owner_id"})
self.user_profile = self.example_user('aaron') self.user_profile = self.example_user('aaron')
owner = self.example_user('hamlet') owner = self.example_user('hamlet')
@@ -1675,7 +1584,7 @@ class NormalActionsTest(BaseAction):
action = lambda: do_change_bot_owner(bot, owner, self.user_profile) action = lambda: do_change_bot_owner(bot, owner, self.user_profile)
events = self.verify_action(action, num_events=2) events = self.verify_action(action, num_events=2)
check_realm_bot_delete('events[0]', events[0]) check_realm_bot_delete('events[0]', events[0])
change_bot_owner_checker_user('events[1]', events[1]) check_realm_user_update('events[1]', events[1], {"bot_owner_id"})
previous_owner = self.example_user('aaron') previous_owner = self.example_user('aaron')
self.user_profile = self.example_user('hamlet') self.user_profile = self.example_user('hamlet')
@@ -1683,7 +1592,7 @@ class NormalActionsTest(BaseAction):
action = lambda: do_change_bot_owner(bot, self.user_profile, previous_owner) action = lambda: do_change_bot_owner(bot, self.user_profile, previous_owner)
events = self.verify_action(action, num_events=2) events = self.verify_action(action, num_events=2)
check_realm_bot_add('events[0]', events[0]) check_realm_bot_add('events[0]', events[0])
change_bot_owner_checker_user('events[1]', events[1]) check_realm_user_update('events[1]', events[1], {"bot_owner_id"})
def test_do_update_outgoing_webhook_service(self) -> None: def test_do_update_outgoing_webhook_service(self) -> None:
self.user_profile = self.example_user('iago') self.user_profile = self.example_user('iago')
@@ -2183,17 +2092,8 @@ class UserDisplayActionTest(BaseAction):
check_update_display_settings('events[0]', events[0]) check_update_display_settings('events[0]', events[0])
timezone_schema_checker = check_events_dict([
('type', equals('realm_user')),
('op', equals('update')),
('person', check_dict_only([
('email', check_string),
('user_id', check_int),
('timezone', check_string),
])),
])
if setting_name == "timezone": if setting_name == "timezone":
timezone_schema_checker('events[1]', events[1]) check_realm_user_update('events[1]', events[1], {"email", "timezone"})
def test_set_user_display_settings(self) -> None: def test_set_user_display_settings(self) -> None:
for prop in UserProfile.property_types: for prop in UserProfile.property_types: