From 69be97e365bd0d4385a909b8077050772876e99e Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Fri, 26 Jun 2020 17:51:10 +0000 Subject: [PATCH] pointer: Remove pointer from API and page_params. There is still some miscellaneous cleanup that has to happen for things like analytics queries and dead code in node tests, but this should remove the main use of pointers in the backend. (We will also still need to drop the DB field.) --- templates/zerver/api/changelog.md | 6 ++ version.py | 2 +- zerver/lib/actions.py | 36 ------------ zerver/lib/events.py | 5 -- zerver/openapi/openapi.py | 2 +- zerver/tests/test_events.py | 14 +---- zerver/tests/test_home.py | 11 ---- zerver/tests/test_openapi.py | 2 - zerver/tests/test_unread.py | 93 ++++--------------------------- zerver/tests/test_urls.py | 6 +- zerver/tests/test_users.py | 46 --------------- zerver/tornado/event_queue.py | 7 +-- zerver/views/home.py | 10 ---- zerver/views/pointer.py | 27 --------- zproject/urls.py | 6 -- 15 files changed, 23 insertions(+), 250 deletions(-) delete mode 100644 zerver/views/pointer.py diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 8e61ff297b..41e865f444 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,12 @@ below features are supported. ## Changes in Zulip 3.0 +**Feature level 23** + +* `GET/PUT/POST /users/me/pointer`: Eliminated. We eliminated + the whole concept of the "global" user pointer leading up to + this commit. + **Feature level 22** * `GET /attachments`: The date when a message using the attachment was diff --git a/version.py b/version.py index a023dffc78..fbc8fad881 100644 --- a/version.py +++ b/version.py @@ -29,7 +29,7 @@ DESKTOP_WARNING_VERSION = "5.2.0" # # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md. -API_FEATURE_LEVEL = 22 +API_FEATURE_LEVEL = 23 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 5c6c0203a8..4044a61b31 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -4031,42 +4031,6 @@ def update_user_presence(user_profile: UserProfile, client: Client, log_time: da if new_user_input: update_user_activity_interval(user_profile, log_time) -def do_update_pointer(user_profile: UserProfile, client: Client, - pointer: int, update_flags: bool=False) -> None: - prev_pointer = user_profile.pointer - user_profile.pointer = pointer - user_profile.save(update_fields=["pointer"]) - - if update_flags: # nocoverage - # This block of code is compatibility code for the - # legacy/original Zulip Android app natively. It's a shim - # that will mark as read any messages up until the pointer - # move; we expect to remove this feature entirely before long, - # when we drop support for the old Android app entirely. - app_message_ids = UserMessage.objects.filter( - user_profile=user_profile, - message__id__gt=prev_pointer, - message__id__lte=pointer).extra(where=[ - UserMessage.where_unread(), - UserMessage.where_active_push_notification(), - ]).values_list("message_id", flat=True) - - UserMessage.objects.filter(user_profile=user_profile, - message__id__gt=prev_pointer, - message__id__lte=pointer).extra(where=[UserMessage.where_unread()]) \ - .update(flags=F('flags').bitor(UserMessage.flags.read)) - do_clear_mobile_push_notifications_for_ids([user_profile.id], app_message_ids) - event_time = timezone_now() - - count = len(app_message_ids) - do_increment_logging_stat(user_profile, COUNT_STATS['messages_read::hour'], - None, event_time, increment=count) - do_increment_logging_stat(user_profile, COUNT_STATS['messages_read_interactions::hour'], - None, event_time, increment=min(1, count)) - - event = dict(type='pointer', pointer=pointer) - send_event(user_profile.realm, event, [user_profile.id]) - def do_update_user_status(user_profile: UserProfile, away: Optional[bool], status_text: Optional[str], diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 6a1b22ac3b..ec3506959f 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -131,9 +131,6 @@ def fetch_initial_state_data(user_profile: UserProfile, if want('muted_topics'): state['muted_topics'] = get_topic_mutes(user_profile) - if want('pointer'): - state['pointer'] = user_profile.pointer - if want('presence'): state['presences'] = get_presences_for_realm(realm, slim_presence) @@ -398,8 +395,6 @@ def apply_event(state: Dict[str, Any], state['hotspots'] = event['hotspots'] elif event['type'] == "custom_profile_fields": state['custom_profile_fields'] = event['fields'] - elif event['type'] == "pointer": - state['pointer'] = max(state['pointer'], event['pointer']) elif event['type'] == "realm_user": person = event['person'] person_user_id = person['user_id'] diff --git a/zerver/openapi/openapi.py b/zerver/openapi/openapi.py index 5c5c3bff1c..0a54e2e1c1 100644 --- a/zerver/openapi/openapi.py +++ b/zerver/openapi/openapi.py @@ -20,7 +20,7 @@ EXCLUDE_PROPERTIES = { }, '/register': { 'post': { - '200': ['max_message_id', 'realm_emoji', 'pointer'], + '200': ['max_message_id', 'realm_emoji'], }, }, '/settings/notifications': { diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index a7c4405d96..2e9a11b21a 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -86,7 +86,6 @@ from zerver.lib.actions import ( do_update_message, do_update_message_flags, do_update_outgoing_webhook_service, - do_update_pointer, do_update_user_custom_profile_data_if_changed, do_update_user_group_description, do_update_user_group_name, @@ -293,9 +292,7 @@ class EventsEndpointTest(ZulipTestCase): # Check that we didn't fetch the messages data self.assertNotIn('max_message_id', result_dict) - # Check that the realm_emoji data is in there, and is correctly - # updated (presering our atomicity guaranteed), though of - # course any future pointer events won't be distributed + # Check that the realm_emoji data is in there. self.assertIn('realm_emoji', result_dict) self.assertEqual(result_dict['realm_emoji'], []) self.assertEqual(result_dict['queue_id'], '15:13') @@ -1252,14 +1249,6 @@ class EventsRegisterTest(ZulipTestCase): self.user_profile, get_client("ZulipAndroid/1.0"), timezone_now(), UserPresence.IDLE)) schema_checker_android('events[0]', events[0]) - def test_pointer_events(self) -> None: - schema_checker = self.check_events_dict([ - ('type', equals('pointer')), - ('pointer', check_int), - ]) - events = self.do_test(lambda: do_update_pointer(self.user_profile, get_client("website"), 1500)) - schema_checker('events[0]', events[0]) - def test_register_events(self) -> None: realm_user_add_checker = self.check_events_dict([ ('type', equals('realm_user')), @@ -3801,7 +3790,6 @@ class FetchQueriesTest(ZulipTestCase): hotspots=0, message=1, muted_topics=1, - pointer=0, presence=1, realm=0, realm_bot=1, diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index ad8b148a63..00ae60fee6 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -122,7 +122,6 @@ class HomeTest(ZulipTestCase): "password_min_guesses", "password_min_length", "pm_content_in_desktop_notifications", - "pointer", "poll_timeout", "presence_enabled", "presences", @@ -456,15 +455,6 @@ class HomeTest(ZulipTestCase): self.assertEqual(mock.call_args_list[0][0][0], "Invalid narrow requested, ignoring") self._sanity_check(result) - def test_bad_pointer(self) -> None: - user_profile = self.example_user('hamlet') - user_profile.pointer = 999999 - user_profile.save() - - self.login_user(user_profile) - result = self._get_home_page() - self._sanity_check(result) - def test_topic_narrow(self) -> None: self.login('hamlet') result = self._get_home_page(stream='Denmark', topic='lunch') @@ -658,7 +648,6 @@ class HomeTest(ZulipTestCase): page_params = self._get_page_params(result) self.assertEqual(page_params['narrow_stream'], stream_name) self.assertEqual(page_params['narrow'], [dict(operator='stream', operand=stream_name)]) - self.assertEqual(page_params['pointer'], -1) self.assertEqual(page_params['max_message_id'], -1) def test_invites_by_admins_only(self) -> None: diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index 6fd05f3ea9..0a0b403408 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -283,8 +283,6 @@ class OpenAPIArgumentsTest(ZulipTestCase): '/settings/display', # Much more valuable would be an org admin bulk-upload feature. '/users/me/profile_data', - # To be deprecated and deleted. - '/users/me/pointer', #### Should be documented as part of interactive bots documentation '/bot_storage', diff --git a/zerver/tests/test_unread.py b/zerver/tests/test_unread.py index c054d31c07..afb9d9f1fd 100644 --- a/zerver/tests/test_unread.py +++ b/zerver/tests/test_unread.py @@ -8,77 +8,19 @@ from zerver.lib.fix_unreads import fix, fix_unsubscribed from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import get_subscription, tornado_redirected_to_list from zerver.lib.topic_mutes import add_topic_mute -from zerver.models import Subscription, UserMessage, UserProfile, get_realm, get_stream, get_user +from zerver.models import Subscription, UserMessage, UserProfile, get_realm, get_stream -class PointerTest(ZulipTestCase): +class FirstUnreadAnchorTests(ZulipTestCase): + ''' + HISTORICAL NOTE: - def test_update_pointer(self) -> None: - """ - Posting a pointer to /update (in the form {"pointer": pointer}) changes - the pointer we store for your UserProfile. - """ + The two tests in this class were originally written when + we had the concept of a "pointer", and they may be a bit + redundant in what they now check. + ''' + def test_use_first_unread_anchor(self) -> None: self.login('hamlet') - self.assertEqual(self.example_user('hamlet').pointer, -1) - msg_id = self.send_stream_message(self.example_user("othello"), "Verona") - result = self.client_post("/json/users/me/pointer", {"pointer": msg_id}) - self.assert_json_success(result) - self.assertEqual(self.example_user('hamlet').pointer, msg_id) - - def test_api_update_pointer(self) -> None: - """ - Same as above, but for the API view - """ - user = self.example_user('hamlet') - email = user.email - self.assertEqual(user.pointer, -1) - msg_id = self.send_stream_message(self.example_user("othello"), "Verona") - result = self.api_post(user, "/api/v1/users/me/pointer", {"pointer": msg_id}) - self.assert_json_success(result) - self.assertEqual(get_user(email, user.realm).pointer, msg_id) - - def test_missing_pointer(self) -> None: - """ - Posting json to /json/users/me/pointer which does not contain a pointer key/value pair - returns a 400 and error message. - """ - self.login('hamlet') - self.assertEqual(self.example_user('hamlet').pointer, -1) - result = self.client_post("/json/users/me/pointer", {"foo": 1}) - self.assert_json_error(result, "Missing 'pointer' argument") - self.assertEqual(self.example_user('hamlet').pointer, -1) - - def test_invalid_pointer(self) -> None: - """ - Posting json to /json/users/me/pointer with an invalid pointer returns a 400 and error - message. - """ - self.login('hamlet') - self.assertEqual(self.example_user('hamlet').pointer, -1) - result = self.client_post("/json/users/me/pointer", {"pointer": "foo"}) - self.assert_json_error(result, "Bad value for 'pointer': foo") - self.assertEqual(self.example_user('hamlet').pointer, -1) - - def test_pointer_out_of_range(self) -> None: - """ - Posting json to /json/users/me/pointer with an out of range (< 0) pointer returns a 400 - and error message. - """ - self.login('hamlet') - self.assertEqual(self.example_user('hamlet').pointer, -1) - result = self.client_post("/json/users/me/pointer", {"pointer": -2}) - self.assert_json_error(result, "Bad value for 'pointer': -2") - self.assertEqual(self.example_user('hamlet').pointer, -1) - - def test_use_first_unread_anchor_interaction_with_pointer(self) -> None: - """ - Getting old messages (a get request to /json/messages) should never - return an unread message older than the current pointer, when there's - no narrow set. - """ - self.login('hamlet') - # Ensure the pointer is not set (-1) - self.assertEqual(self.example_user('hamlet').pointer, -1) # Mark all existing messages as read result = self.client_post("/json/mark_all_as_read") @@ -107,7 +49,6 @@ class PointerTest(ZulipTestCase): messages = self.get_messages( anchor=0, num_before=0, num_after=2, use_first_unread_anchor=False) old_message_id = messages[0]['id'] - next_old_message_id = messages[1]['id'] # Verify the message is marked as read user_message = UserMessage.objects.get( @@ -135,23 +76,8 @@ class PointerTest(ZulipTestCase): self.assertEqual(messages_response['messages'][0]['id'], old_message_id) self.assertEqual(messages_response['anchor'], old_message_id) - # Let's update the pointer to be *after* this old unread message (but - # still on or before the new unread message we just sent) - result = self.client_post("/json/users/me/pointer", - {"pointer": next_old_message_id}) - self.assert_json_success(result) - self.assertEqual(self.example_user('hamlet').pointer, - next_old_message_id) - - # Verify that moving the pointer didn't mark our message as read. - user_message = UserMessage.objects.get( - message_id=old_message_id, - user_profile=self.example_user('hamlet')) - self.assertFalse(user_message.flags.read) - def test_visible_messages_use_first_unread_anchor(self) -> None: self.login('hamlet') - self.assertEqual(self.example_user('hamlet').pointer, -1) result = self.client_post("/json/mark_all_as_read") self.assert_json_success(result) @@ -184,6 +110,7 @@ class PointerTest(ZulipTestCase): anchor="first_unread", num_before=0, num_after=1) self.assert_length(messages, 1) + class UnreadCountTests(ZulipTestCase): def setUp(self) -> None: super().setUp() diff --git a/zerver/tests/test_urls.py b/zerver/tests/test_urls.py index f30c6a5c41..9b4747a4dc 100644 --- a/zerver/tests/test_urls.py +++ b/zerver/tests/test_urls.py @@ -64,7 +64,6 @@ class PublicURLTest(ZulipTestCase): "/json/subscriptions/exists", "/api/v1/users/me/subscriptions/properties", "/json/fetch_api_key", - "/json/users/me/pointer", "/json/users/me/subscriptions", "/api/v1/users/me/subscriptions", "/json/export/realm", @@ -76,16 +75,13 @@ class PublicURLTest(ZulipTestCase): patch_urls = { 401: ["/json/settings"], } - put_urls = {401: ["/json/users/me/pointer"], - } + for status_code, url_set in get_urls.items(): self.fetch("client_get", url_set, status_code) for status_code, url_set in post_urls.items(): self.fetch("client_post", url_set, status_code) for status_code, url_set in patch_urls.items(): self.fetch("client_patch", url_set, status_code) - for status_code, url_set in put_urls.items(): - self.fetch("client_put", url_set, status_code) def test_get_gcid_when_not_configured(self) -> None: with self.settings(GOOGLE_CLIENT_ID=None): diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 423e073671..bc45dfa87d 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -1531,25 +1531,6 @@ class BulkUsersTest(ZulipTestCase): class GetProfileTest(ZulipTestCase): - def common_update_pointer(self, user: UserProfile, pointer: int) -> None: - self.login_user(user) - result = self.client_post("/json/users/me/pointer", {"pointer": pointer}) - self.assert_json_success(result) - - def common_get_pointer(self, user_id: str) -> Dict[str, Any]: - user_profile = self.example_user(user_id) - result = self.api_get(user_profile, "/json/users/me/pointer") - self.assert_json_success(result) - json = result.json() - return json - - def test_get_pointer(self) -> None: - user = self.example_user("hamlet") - self.login_user(user) - result = self.client_get("/json/users/me/pointer") - self.assert_json_success(result) - self.assertIn("pointer", result.json()) - def test_cache_behavior(self) -> None: """Tests whether fetching a user object the normal way, with `get_user`, makes 1 cache query and 1 database query. @@ -1617,33 +1598,6 @@ class GetProfileTest(ZulipTestCase): self.assertEqual(result['user']['email'], bot.email) self.assertTrue(result['user']['is_bot']) - def test_api_get_empty_profile(self) -> None: - """ - Ensure GET /users/me returns a max message id and returns successfully - """ - json = self.common_get_pointer("othello") - self.assertEqual(json['pointer'], -1) - - def test_profile_with_pointer(self) -> None: - """ - Ensure GET /users/me returns a proper pointer id after the pointer is updated - """ - - id1 = self.send_stream_message(self.example_user("othello"), "Verona") - id2 = self.send_stream_message(self.example_user("othello"), "Verona") - - hamlet = self.example_user('hamlet') - self.common_update_pointer(hamlet, id2) - json = self.common_get_pointer("hamlet") - self.assertEqual(json["pointer"], id2) - - self.common_update_pointer(hamlet, id1) - json = self.common_get_pointer("hamlet") - self.assertEqual(json["pointer"], id2) # pointer does not move backwards - - result = self.client_post("/json/users/me/pointer", {"pointer": 99999999}) - self.assert_json_error(result, "Invalid message ID") - def test_get_all_profiles_avatar_urls(self) -> None: hamlet = self.example_user('hamlet') result = self.api_get(hamlet, "/api/v1/users") diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index 1dcb4b42ad..67bd648dbc 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -303,7 +303,7 @@ class EventQueue: event['id'] = self.next_event_id self.next_event_id += 1 full_event_type = compute_full_event_type(event) - if (full_event_type in ["pointer", "restart"] or + if (full_event_type == "restart" or full_event_type.startswith("flags/")): if full_event_type not in self.virtual_events: self.virtual_events[full_event_type] = copy.deepcopy(event) @@ -313,9 +313,8 @@ class EventQueue: virtual_event["id"] = event["id"] if "timestamp" in event: virtual_event["timestamp"] = event["timestamp"] - if full_event_type == "pointer": - virtual_event["pointer"] = event["pointer"] - elif full_event_type == "restart": + + if full_event_type == "restart": virtual_event["server_generation"] = event["server_generation"] elif full_event_type.startswith("flags/"): virtual_event["messages"] += event["messages"] diff --git a/zerver/views/home.py b/zerver/views/home.py index 03774178da..8969358e17 100644 --- a/zerver/views/home.py +++ b/zerver/views/home.py @@ -212,15 +212,6 @@ def home_real(request: HttpRequest) -> HttpResponse: ) needs_tutorial = user_profile.tutorial_status == UserProfile.TUTORIAL_WAITING - if user_profile.pointer == -1: - # Put the new user's pointer at the bottom - # - # This improves performance, because we limit backfilling of messages - # before the pointer. It's also likely that someone joining an - # organization is interested in recent messages more than the very - # first messages on the system. - register_ret['pointer'] = register_ret['max_message_id'] - else: # nocoverage first_in_realm = False prompt_for_invites = False @@ -291,7 +282,6 @@ def home_real(request: HttpRequest) -> HttpResponse: page_params["narrow_topic"] = narrow_topic page_params["narrow"] = [dict(operator=term[0], operand=term[1]) for term in narrow] page_params["max_message_id"] = initial_pointer - page_params["pointer"] = initial_pointer page_params["enable_desktop_notifications"] = False statsd.incr('views.home') diff --git a/zerver/views/pointer.py b/zerver/views/pointer.py deleted file mode 100644 index 08e9a0756e..0000000000 --- a/zerver/views/pointer.py +++ /dev/null @@ -1,27 +0,0 @@ -from django.http import HttpRequest, HttpResponse -from django.utils.translation import ugettext as _ - -from zerver.lib.actions import do_update_pointer -from zerver.lib.request import REQ, JsonableError, has_request_variables -from zerver.lib.response import json_success -from zerver.lib.validator import to_non_negative_int -from zerver.models import UserProfile, get_usermessage_by_message_id - - -def get_pointer_backend(request: HttpRequest, user_profile: UserProfile) -> HttpResponse: - return json_success({'pointer': user_profile.pointer}) - -@has_request_variables -def update_pointer_backend(request: HttpRequest, user_profile: UserProfile, - pointer: int=REQ(converter=to_non_negative_int)) -> HttpResponse: - if pointer <= user_profile.pointer: - return json_success() - - if get_usermessage_by_message_id(user_profile, pointer) is None: - raise JsonableError(_("Invalid message ID")) - - request._log_data["extra"] = f"[{pointer}]" - update_flags = (request.client.name.lower() in ['android', "zulipandroid"]) - do_update_pointer(user_profile, request.client, pointer, update_flags=update_flags) - - return json_success() diff --git a/zproject/urls.py b/zproject/urls.py index c0ba941a2a..f3b0a06a3d 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -260,12 +260,6 @@ v1_api_and_json_patterns = [ path('users/me', rest_dispatch, {'GET': 'zerver.views.users.get_profile_backend', 'DELETE': 'zerver.views.users.deactivate_user_own_backend'}), - # PUT is currently used by mobile apps, we intend to remove the PUT version - # as soon as possible. POST exists to correct the erroneous use of PUT. - path('users/me/pointer', rest_dispatch, - {'GET': 'zerver.views.pointer.get_pointer_backend', - 'PUT': 'zerver.views.pointer.update_pointer_backend', - 'POST': 'zerver.views.pointer.update_pointer_backend'}), path('users/me/presence', rest_dispatch, {'POST': 'zerver.views.presence.update_active_status_backend'}), path('users/me/status', rest_dispatch,