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.
This commit is contained in:
Riken Shah 2021-05-08 09:00:12 +00:00 committed by Tim Abbott
parent 0c159c5f47
commit 8d633cc368
10 changed files with 105 additions and 30 deletions

View File

@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with.
## Changes in Zulip 8.0 ## 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** **Feature level 229**
* [`PATCH /messages/{message_id}`](/api/update-message), [`POST * [`PATCH /messages/{message_id}`](/api/update-message), [`POST

View File

@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3"
# Changes should be accompanied by documentation explaining what the # Changes should be accompanied by documentation explaining what the
# new level means in api_docs/changelog.md, as well as "**Changes**" # new level means in api_docs/changelog.md, as well as "**Changes**"
# entries in the endpoint's documentation in `zulip.yaml`. # 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 # 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 # only when going from an old version of the code to a newer version. Bump

View File

@ -177,12 +177,14 @@ exports.fixtures = {
title: "About topics", title: "About topics",
description: "Topics are good.", description: "Topics are good.",
delay: 1.5, delay: 1.5,
has_trigger: false,
}, },
{ {
name: "compose", name: "compose",
title: "Compose box", title: "Compose box",
description: "This is where you compose messages.", description: "This is where you compose messages.",
delay: 3.14159, delay: 3.14159,
has_trigger: false,
}, },
], ],
}, },

View File

@ -338,6 +338,7 @@ _hotspot = DictType(
("title", str), ("title", str),
("description", str), ("description", str),
("delay", NumberType()), ("delay", NumberType()),
("has_trigger", bool),
] ]
) )

View File

@ -1,6 +1,6 @@
# See https://zulip.readthedocs.io/en/latest/subsystems/hotspots.html # See https://zulip.readthedocs.io/en/latest/subsystems/hotspots.html
# for documentation on this subsystem. # for documentation on this subsystem.
from typing import Dict, List from typing import Dict, List, Union
from django.conf import settings from django.conf import settings
from django.utils.translation import gettext_lazy from django.utils.translation import gettext_lazy
@ -8,7 +8,7 @@ from django_stubs_ext import StrPromise
from zerver.models import UserHotspot, UserProfile from zerver.models import UserHotspot, UserProfile
INTRO_HOTSPOTS: Dict[str, Dict[str, StrPromise]] = { INTRO_HOTSPOTS: Dict[str, Dict[str, Union[StrPromise, str]]] = {
"intro_streams": { "intro_streams": {
"title": gettext_lazy("Catch up on a stream"), "title": gettext_lazy("Catch up on a stream"),
"description": gettext_lazy( "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 # 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 # a part of the initial tutorial. To that end, classifying them into
# categories which are aggregated in ALL_HOTSPOTS, seems like a good start. # categories which are aggregated in ALL_HOTSPOTS, seems like a good start.
ALL_HOTSPOTS: Dict[str, Dict[str, StrPromise]] = { ALL_HOTSPOTS: Dict[str, Dict[str, Union[StrPromise, str, bool]]] = {
**INTRO_HOTSPOTS, **{
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: if settings.ALWAYS_SEND_ALL_HOTSPOTS:
return [ return [
{ {
"name": hotspot, **base_hotspot,
"title": str(ALL_HOTSPOTS[hotspot]["title"]), "name": name,
"description": str(ALL_HOTSPOTS[hotspot]["description"]), "title": str(base_hotspot["title"]),
"description": str(base_hotspot["description"]),
"delay": 0, "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 a Zulip server has disabled the tutorial, never send hotspots.
if not settings.TUTORIAL_ENABLED: if not settings.TUTORIAL_ENABLED:
return [] return []
if user.tutorial_status == UserProfile.TUTORIAL_FINISHED:
return []
seen_hotspots = frozenset( seen_hotspots = frozenset(
UserHotspot.objects.filter(user=user).values_list("hotspot", flat=True) UserHotspot.objects.filter(user=user).values_list("hotspot", flat=True)
) )
for hotspot in INTRO_HOTSPOTS:
if hotspot not in seen_hotspots: hotspots = []
return [
{ for name, base_hotspot in NON_INTRO_HOTSPOTS.items():
"name": hotspot, if name in seen_hotspots:
"title": str(INTRO_HOTSPOTS[hotspot]["title"]), continue
"description": str(INTRO_HOTSPOTS[hotspot]["description"]),
"delay": 0.5, 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.tutorial_status = UserProfile.TUTORIAL_FINISHED
user.save(update_fields=["tutorial_status"]) user.save(update_fields=["tutorial_status"])
return [] return hotspots
def copy_hotspots(source_profile: UserProfile, target_profile: UserProfile) -> None: def copy_hotspots(source_profile: UserProfile, target_profile: UserProfile) -> None:

View File

@ -2050,6 +2050,10 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings): # type
avatar_version = models.PositiveSmallIntegerField(default=1) avatar_version = models.PositiveSmallIntegerField(default=1)
avatar_hash = models.CharField(null=True, max_length=64) 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_WAITING = "W"
TUTORIAL_STARTED = "S" TUTORIAL_STARTED = "S"
TUTORIAL_FINISHED = "F" TUTORIAL_FINISHED = "F"

View File

@ -2286,6 +2286,7 @@ paths:
"title": "Catch up on a stream", "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.", "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, "delay": 0.5,
"has_trigger": false,
}, },
], ],
"id": 0, "id": 0,
@ -18496,6 +18497,13 @@ components:
description: | description: |
The description of the hotspot, as will be displayed to the The description of the hotspot, as will be displayed to the
user. 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: RealmEmoji:
type: object type: object
additionalProperties: false additionalProperties: false

View File

@ -1249,7 +1249,7 @@ class FetchQueriesTest(ZulipTestCase):
self.login_user(user) 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: with mock.patch("zerver.lib.events.always_want") as want_mock:
fetch_initial_state_data(user) fetch_initial_state_data(user)
@ -1259,7 +1259,7 @@ class FetchQueriesTest(ZulipTestCase):
default_streams=1, default_streams=1,
default_stream_groups=1, default_stream_groups=1,
drafts=1, drafts=1,
hotspots=0, hotspots=1,
message=1, message=1,
muted_topics=1, muted_topics=1,
muted_users=1, muted_users=1,

View File

@ -256,7 +256,7 @@ class HomeTest(ZulipTestCase):
self.client_post("/json/bots", bot_info) self.client_post("/json/bots", bot_info)
# Verify succeeds once logged-in # 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: with patch("zerver.lib.cache.cache_set") as cache_mock:
result = self._get_home_page(stream="Denmark") result = self._get_home_page(stream="Denmark")
self.check_rendered_logged_in_app(result) self.check_rendered_logged_in_app(result)
@ -445,7 +445,7 @@ class HomeTest(ZulipTestCase):
def test_num_queries_for_realm_admin(self) -> None: def test_num_queries_for_realm_admin(self) -> None:
# Verify number of queries for Realm admin isn't much higher than for normal users. # Verify number of queries for Realm admin isn't much higher than for normal users.
self.login("iago") 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: with patch("zerver.lib.cache.cache_set") as cache_mock:
result = self._get_home_page() result = self._get_home_page()
self.check_rendered_logged_in_app(result) self.check_rendered_logged_in_app(result)
@ -476,7 +476,7 @@ class HomeTest(ZulipTestCase):
self._get_home_page() self._get_home_page()
# Then for the second page load, measure the number of queries. # 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() result = self._get_home_page()
# Do a sanity check that our new streams were in the payload. # Do a sanity check that our new streams were in the payload.

View File

@ -2,7 +2,7 @@ from typing_extensions import override
from zerver.actions.create_user import do_create_user from zerver.actions.create_user import do_create_user
from zerver.actions.hotspots import do_mark_hotspot_as_read 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.lib.test_classes import ZulipTestCase
from zerver.models import UserHotspot, UserProfile, get_realm from zerver.models import UserHotspot, UserProfile, get_realm
@ -22,6 +22,22 @@ class TestGetNextHotspots(ZulipTestCase):
self.assert_length(hotspots, 1) self.assert_length(hotspots, 1)
self.assertEqual(hotspots[0]["name"], "intro_streams") 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: 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_streams")
do_mark_hotspot_as_read(self.user, "intro_compose") do_mark_hotspot_as_read(self.user, "intro_compose")
@ -29,11 +45,16 @@ class TestGetNextHotspots(ZulipTestCase):
self.assert_length(hotspots, 1) self.assert_length(hotspots, 1)
self.assertEqual(hotspots[0]["name"], "intro_topics") 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): 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) self.assertNotEqual(self.user.tutorial_status, UserProfile.TUTORIAL_FINISHED)
for hotspot in INTRO_HOTSPOTS: for hotspot in INTRO_HOTSPOTS:
do_mark_hotspot_as_read(self.user, hotspot) do_mark_hotspot_as_read(self.user, hotspot)
self.assertEqual(self.user.tutorial_status, UserProfile.TUTORIAL_FINISHED) self.assertEqual(self.user.tutorial_status, UserProfile.TUTORIAL_FINISHED)
self.assertEqual(get_next_hotspots(self.user), []) self.assertEqual(get_next_hotspots(self.user), [])