From 8d633cc368520856bd134d4128e73bde297308ee Mon Sep 17 00:00:00 2001 From: Riken Shah Date: Sat, 8 May 2021 09:00:12 +0000 Subject: [PATCH] hotspot: Add backend changes for non-intro hotspots. This commit introduces non-intro hotspots. They are a bit different than intro hotspots in the following ways: * All the non-intro hotspots are sent at once instead of sending them one by one like intro hotspots. * They only activate when a specific event occurs, unlike intro hotspot where they activate after the previous hotspot is read. --- api_docs/changelog.md | 6 +++ version.py | 2 +- web/tests/lib/events.js | 2 + zerver/lib/event_schema.py | 1 + zerver/lib/hotspots.py | 77 ++++++++++++++++++++++--------- zerver/models.py | 4 ++ zerver/openapi/zulip.yaml | 8 ++++ zerver/tests/test_event_system.py | 4 +- zerver/tests/test_home.py | 6 +-- zerver/tests/test_hotspots.py | 25 +++++++++- 10 files changed, 105 insertions(+), 30 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index c2eb725d76..da8e04e688 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 230** + +* [`GET /events`](/api/get-events): Added `has_trigger` field in + hotspots events to identify if a hotspot will activate only when + some specific event occurs. + **Feature level 229** * [`PATCH /messages/{message_id}`](/api/update-message), [`POST diff --git a/version.py b/version.py index 7f77715976..3148f33c42 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 229 +API_FEATURE_LEVEL = 230 # 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/web/tests/lib/events.js b/web/tests/lib/events.js index 86d39968e5..ff20d6bb72 100644 --- a/web/tests/lib/events.js +++ b/web/tests/lib/events.js @@ -177,12 +177,14 @@ exports.fixtures = { title: "About topics", description: "Topics are good.", delay: 1.5, + has_trigger: false, }, { name: "compose", title: "Compose box", description: "This is where you compose messages.", delay: 3.14159, + has_trigger: false, }, ], }, diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 1fe7f3b172..a8e9fa8069 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -338,6 +338,7 @@ _hotspot = DictType( ("title", str), ("description", str), ("delay", NumberType()), + ("has_trigger", bool), ] ) diff --git a/zerver/lib/hotspots.py b/zerver/lib/hotspots.py index 114f6148bb..86e0bbb14c 100644 --- a/zerver/lib/hotspots.py +++ b/zerver/lib/hotspots.py @@ -1,6 +1,6 @@ # See https://zulip.readthedocs.io/en/latest/subsystems/hotspots.html # for documentation on this subsystem. -from typing import Dict, List +from typing import Dict, List, Union from django.conf import settings from django.utils.translation import gettext_lazy @@ -8,7 +8,7 @@ from django_stubs_ext import StrPromise from zerver.models import UserHotspot, UserProfile -INTRO_HOTSPOTS: Dict[str, Dict[str, StrPromise]] = { +INTRO_HOTSPOTS: Dict[str, Dict[str, Union[StrPromise, str]]] = { "intro_streams": { "title": gettext_lazy("Catch up on a stream"), "description": gettext_lazy( @@ -39,11 +39,21 @@ INTRO_HOTSPOTS: Dict[str, Dict[str, StrPromise]] = { }, } + +NON_INTRO_HOTSPOTS: Dict[str, Dict[str, Union[StrPromise, str]]] = {} + # We would most likely implement new hotspots in the future that aren't # a part of the initial tutorial. To that end, classifying them into # categories which are aggregated in ALL_HOTSPOTS, seems like a good start. -ALL_HOTSPOTS: Dict[str, Dict[str, StrPromise]] = { - **INTRO_HOTSPOTS, +ALL_HOTSPOTS: Dict[str, Dict[str, Union[StrPromise, str, bool]]] = { + **{ + hotspot_name: {**INTRO_HOTSPOTS[hotspot_name], "has_trigger": False} + for hotspot_name in INTRO_HOTSPOTS + }, + **{ + hotspot_name: {**NON_INTRO_HOTSPOTS[hotspot_name], "has_trigger": True} # type: ignore[arg-type] # reason: Its a temporary hack + for hotspot_name in NON_INTRO_HOTSPOTS + }, } @@ -59,38 +69,61 @@ def get_next_hotspots(user: UserProfile) -> List[Dict[str, object]]: if settings.ALWAYS_SEND_ALL_HOTSPOTS: return [ { - "name": hotspot, - "title": str(ALL_HOTSPOTS[hotspot]["title"]), - "description": str(ALL_HOTSPOTS[hotspot]["description"]), + **base_hotspot, + "name": name, + "title": str(base_hotspot["title"]), + "description": str(base_hotspot["description"]), "delay": 0, } - for hotspot in ALL_HOTSPOTS + for name, base_hotspot in ALL_HOTSPOTS.items() ] # If a Zulip server has disabled the tutorial, never send hotspots. if not settings.TUTORIAL_ENABLED: return [] - if user.tutorial_status == UserProfile.TUTORIAL_FINISHED: - return [] - seen_hotspots = frozenset( UserHotspot.objects.filter(user=user).values_list("hotspot", flat=True) ) - for hotspot in INTRO_HOTSPOTS: - if hotspot not in seen_hotspots: - return [ - { - "name": hotspot, - "title": str(INTRO_HOTSPOTS[hotspot]["title"]), - "description": str(INTRO_HOTSPOTS[hotspot]["description"]), - "delay": 0.5, - } - ] + + hotspots = [] + + for name, base_hotspot in NON_INTRO_HOTSPOTS.items(): + if name in seen_hotspots: + continue + + hotspot = { + **base_hotspot, + "name": name, + "title": str(base_hotspot["title"]), + "description": str(base_hotspot["description"]), + "delay": 0, + "has_trigger": True, + } + hotspots.append(hotspot) + + if user.tutorial_status == UserProfile.TUTORIAL_FINISHED: + return hotspots + + for name, base_hotspot in INTRO_HOTSPOTS.items(): + if name in seen_hotspots: + continue + + # Make a copy to set delay and finalize i18n strings. + hotspot = { + **base_hotspot, + "name": name, + "title": str(base_hotspot["title"]), + "description": str(base_hotspot["description"]), + "delay": 0.5, + "has_trigger": False, + } + hotspots.append(hotspot) + return hotspots user.tutorial_status = UserProfile.TUTORIAL_FINISHED user.save(update_fields=["tutorial_status"]) - return [] + return hotspots def copy_hotspots(source_profile: UserProfile, target_profile: UserProfile) -> None: diff --git a/zerver/models.py b/zerver/models.py index d269de31d7..6b92d4b69e 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -2050,6 +2050,10 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings): # type avatar_version = models.PositiveSmallIntegerField(default=1) avatar_hash = models.CharField(null=True, max_length=64) + # TODO: TUTORIAL_STATUS was originally an optimization designed to + # allow us to skip querying the UserHotspot table when loading + # /. This optimization is no longer effective, so it's possible we + # should delete it. TUTORIAL_WAITING = "W" TUTORIAL_STARTED = "S" TUTORIAL_FINISHED = "F" diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 82430b4413..676f5eef95 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -2286,6 +2286,7 @@ paths: "title": "Catch up on a stream", "description": "Messages sent to a stream are seen by everyone subscribed to that stream. Try clicking on one of the stream links below.", "delay": 0.5, + "has_trigger": false, }, ], "id": 0, @@ -18496,6 +18497,13 @@ components: description: | The description of the hotspot, as will be displayed to the user. + has_trigger: + type: boolean + description: | + The has_trigger is used to identify if the hotspot will activate + only when some specific event occurs. + + **Changes**: New in Zulip 8.0 (feature level 230). RealmEmoji: type: object additionalProperties: false diff --git a/zerver/tests/test_event_system.py b/zerver/tests/test_event_system.py index b25845f813..4cf089f4f6 100644 --- a/zerver/tests/test_event_system.py +++ b/zerver/tests/test_event_system.py @@ -1249,7 +1249,7 @@ class FetchQueriesTest(ZulipTestCase): self.login_user(user) - with self.assert_database_query_count(39): + with self.assert_database_query_count(40): with mock.patch("zerver.lib.events.always_want") as want_mock: fetch_initial_state_data(user) @@ -1259,7 +1259,7 @@ class FetchQueriesTest(ZulipTestCase): default_streams=1, default_stream_groups=1, drafts=1, - hotspots=0, + hotspots=1, message=1, muted_topics=1, muted_users=1, diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 76a43465f3..a73905a20b 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -256,7 +256,7 @@ class HomeTest(ZulipTestCase): self.client_post("/json/bots", bot_info) # Verify succeeds once logged-in - with self.assert_database_query_count(49): + with self.assert_database_query_count(50): with patch("zerver.lib.cache.cache_set") as cache_mock: result = self._get_home_page(stream="Denmark") self.check_rendered_logged_in_app(result) @@ -445,7 +445,7 @@ class HomeTest(ZulipTestCase): def test_num_queries_for_realm_admin(self) -> None: # Verify number of queries for Realm admin isn't much higher than for normal users. self.login("iago") - with self.assert_database_query_count(50): + with self.assert_database_query_count(51): with patch("zerver.lib.cache.cache_set") as cache_mock: result = self._get_home_page() self.check_rendered_logged_in_app(result) @@ -476,7 +476,7 @@ class HomeTest(ZulipTestCase): self._get_home_page() # Then for the second page load, measure the number of queries. - with self.assert_database_query_count(44): + with self.assert_database_query_count(45): result = self._get_home_page() # Do a sanity check that our new streams were in the payload. diff --git a/zerver/tests/test_hotspots.py b/zerver/tests/test_hotspots.py index bdc9effe45..ee39341430 100644 --- a/zerver/tests/test_hotspots.py +++ b/zerver/tests/test_hotspots.py @@ -2,7 +2,7 @@ from typing_extensions import override from zerver.actions.create_user import do_create_user from zerver.actions.hotspots import do_mark_hotspot_as_read -from zerver.lib.hotspots import ALL_HOTSPOTS, INTRO_HOTSPOTS, get_next_hotspots +from zerver.lib.hotspots import ALL_HOTSPOTS, INTRO_HOTSPOTS, NON_INTRO_HOTSPOTS, get_next_hotspots from zerver.lib.test_classes import ZulipTestCase from zerver.models import UserHotspot, UserProfile, get_realm @@ -22,6 +22,22 @@ class TestGetNextHotspots(ZulipTestCase): self.assert_length(hotspots, 1) self.assertEqual(hotspots[0]["name"], "intro_streams") + # Remove this test once we have a non-intro hotspot. + def test_non_intro_hotspots(self) -> None: + NON_INTRO_HOTSPOTS["dummy_hotspot"] = { + "title": "Dummy Hotspot", + "description": "This is a dummy hotspot", + } + hotspots = get_next_hotspots(self.user) + self.assert_length(hotspots, 2) + self.assertEqual(hotspots[0]["name"], "dummy_hotspot") + self.assertEqual(hotspots[1]["name"], "intro_streams") + do_mark_hotspot_as_read(self.user, "dummy_hotspot") + hotspots = get_next_hotspots(self.user) + self.assert_length(hotspots, 1) + self.assertEqual(hotspots[0]["name"], "intro_streams") + NON_INTRO_HOTSPOTS.pop("dummy_hotspot") + def test_some_done_some_not(self) -> None: do_mark_hotspot_as_read(self.user, "intro_streams") do_mark_hotspot_as_read(self.user, "intro_compose") @@ -29,11 +45,16 @@ class TestGetNextHotspots(ZulipTestCase): self.assert_length(hotspots, 1) self.assertEqual(hotspots[0]["name"], "intro_topics") - def test_all_intro_hotspots_done(self) -> None: + def test_all_hotspots_done(self) -> None: with self.settings(TUTORIAL_ENABLED=True): + self.assertNotEqual(self.user.tutorial_status, UserProfile.TUTORIAL_FINISHED) + for hotspot in NON_INTRO_HOTSPOTS: + do_mark_hotspot_as_read(self.user, hotspot) + self.assertNotEqual(self.user.tutorial_status, UserProfile.TUTORIAL_FINISHED) for hotspot in INTRO_HOTSPOTS: do_mark_hotspot_as_read(self.user, hotspot) + self.assertEqual(self.user.tutorial_status, UserProfile.TUTORIAL_FINISHED) self.assertEqual(get_next_hotspots(self.user), [])