Skip to content

Feature: Improve Stream Info Popup #880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 14, 2021
Merged

Feature: Improve Stream Info Popup #880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22934ab
model: Normalize stream message_retention_days and cache text response.
zee-bit Jan 22, 2021
b933405
views: Add a Message retention days field in Stream info view.
zee-bit Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ def general_stream() -> Dict[str, Any]:
"stream_weekly_traffic": 0,
"push_notifications": False,
"email_address": "[email protected]",
"message_retention_days": 10,
"subscribers": [1001, 11, 12],
}

Expand All @@ -240,6 +241,7 @@ def secret_stream() -> Dict[str, Any]:
"is_old_stream": True,
"desktop_notifications": False,
"stream_weekly_traffic": 0,
"message_retention_days": -1,
"push_notifications": False,
"subscribers": [1001, 11],
}
Expand Down Expand Up @@ -267,6 +269,7 @@ def streams_fixture(
"desktop_notifications": False,
"stream_weekly_traffic": 0,
"push_notifications": False,
"message_retention_days": i + 30,
"email_address": f"stream{i}@example.com",
"subscribers": [1001, 11, 12],
}
Expand Down Expand Up @@ -742,6 +745,7 @@ def initial_data(
},
"twenty_four_hour_time": True,
"realm_emoji": realm_emojis,
"realm_message_retention_days": 74,
"last_event_id": -1,
"muted_topics": [],
"realm_user_groups": [],
Expand Down
67 changes: 67 additions & 0 deletions tests/model/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,73 @@ def test_register_initial_desired_events(self, mocker, initial_data):
include_subscribers=True,
)

@pytest.mark.parametrize(
[
"to_vary_in_stream_dict",
"realm_msg_retention_days",
"feature_level",
"expect_msg_retention_text",
],
[
case(
{1: {}},
None,
10,
{1: "Indefinite [Organization default]"},
id="ZFL=None_no_stream_retention_realm_retention=None",
Comment on lines +231 to +236
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is the None case that I queried elsewhere, where the organization value slips through to the stream level.

Was None a realm value, though? I assume it followed the original rule, ie. -1 for indefinite and +n for days, so I'm not sure about None?

The #api-design topic doesn't seem to have clarified this exactly, so perhaps Mateusz could answer in a follow-up or you could refer him here?

Copy link
Member Author

@zee-bit zee-bit Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is why we were handling the days == None case earlier.
To clarify, None was a realm value earlier (between server v2-3, as per my testing on the dev server) and it had the same meaning as the current -1 for indefinite which was changed by @mateuszmandera's commit afterwards. So to summarize, I believe this is how the server's representation for message retention changed over time:

  • Server version 2-3: realm value could have None for indefinite and +n for days. No stream value
  • Server version 3.0: added stream message retention days, with None for org default, -1 for indefinite and +n for days.
  • Server version 3+: realm's None value was changed to -1 by Mateusz's above commit for consistency with the stream value

(I hope Mateusz should get notification from the above mention, still, I'll refer him here from the stream to confirm my hypothesis)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that was an interesting dive through old commits 😀 This overview sounds correct to me based on that. To be a bit more precise, message_retention_days was a thing for streams before 3.0 but only in the sense of the backend logic - but it indeed wasn't exposed via the API in any way (the value had to be set manually via manage.py shell or db query). I presume that zulip-terminal only cares about the API history here though, where indeed this is only a thing since 3.0 - zulip/zulip@c488a35

),
case(
{1: {}, 2: {}},
-1,
16,
{
1: "Indefinite [Organization default]",
2: "Indefinite [Organization default]",
},
id="ZFL=16_no_stream_retention_realm_retention=-1",
),
case(
{2: {"message_retention_days": 30}},
60,
17,
{2: "30"},
id="ZFL=17_stream_retention_days=30",
),
case(
{
1: {"message_retention_days": None},
2: {"message_retention_days": -1},
},
72,
18,
{1: "72 [Organization default]", 2: "Indefinite"},
id="ZFL=18_stream_retention_days=[None, -1]",
),
],
)
def test_normalize_and_cache_message_retention_text(
self,
model,
stream_dict,
to_vary_in_stream_dict,
realm_msg_retention_days,
feature_level,
expect_msg_retention_text,
):
model.stream_dict = stream_dict
model.server_feature_level = feature_level
model.initial_data["realm_message_retention_days"] = realm_msg_retention_days
for stream_id in to_vary_in_stream_dict:
model.stream_dict[stream_id].update(to_vary_in_stream_dict[stream_id])

model.normalize_and_cache_message_retention_text()

for stream_id in to_vary_in_stream_dict:
assert (
model.cached_retention_text[stream_id]
== expect_msg_retention_text[stream_id]
)

@pytest.mark.parametrize("msg_id", [1, 5, set()])
@pytest.mark.parametrize(
"narrow",
Expand Down
68 changes: 50 additions & 18 deletions tests/ui_tools/test_popups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,6 +1127,7 @@ def mock_external_classes(self, mocker, general_stream):
mocker.patch(LISTWALKER, return_value=[])
self.stream_id = general_stream["stream_id"]
self.controller.model.server_feature_level = 40
self.controller.model.cached_retention_text = {self.stream_id: "10"}
self.controller.model.stream_dict = {self.stream_id: general_stream}
self.stream_info_view = StreamInfoView(self.controller, self.stream_id)

Expand All @@ -1145,56 +1146,87 @@ def test_keypress_stream_members(self, key, widget_size):
)

@pytest.mark.parametrize(
["to_vary_in_stream_data", "server_feature_level", "expected_height"],
[
"to_vary_in_stream_data",
"cached_message_retention_text",
"server_feature_level",
"expected_height",
],
[
case(
{"date_created": None},
"74 [Organization default]",
None,
11,
id="ZFL=None__no_date_created",
12,
id="ZFL=None_no_date_created__no_retention_days",
),
case(
{"date_created": None},
"74 [Organization default]",
16,
12,
id="ZFL<30_no_date_created__ZFL<17_no_retention_days",
),
case(
{"date_created": None, "message_retention_days": 200},
"200",
17,
12,
id="ZFL<30_no_date_created__ZFL=17_custom_finite_retention_days",
),
case(
{"date_created": None, "message_retention_days": None},
"Indefinite [Organization default]",
29,
11,
id="ZFL<30__no_date_created",
12,
id="ZFL<30_no_date_created__ZFL>17_default_indefinite_retention_days",
),
case(
{"date_created": 1472091253},
{"date_created": 1472091253, "message_retention_days": 31},
"31",
30,
12,
id="ZFL=30__with_date_created",
13,
id="ZFL=30_with_date_created__ZFL>17_custom_finite_retention_days",
),
case(
{"date_created": 1472047124},
{"date_created": 1472047124, "message_retention_days": None},
"72 [Organization default]",
40,
12,
id="ZFL>30__with_date_created",
13,
id="ZFL>30_with_date_created__ZFL>17_default_finite_retention_days",
),
case(
{"date_created": 1472046489, "stream_weekly_traffic": None},
{
"date_created": 1472046489,
"stream_weekly_traffic": None,
"message_retention_days": 60,
},
"60",
50,
12,
id="ZFL>30__new_stream_with_date_created",
13,
id="ZFL>30_new_stream_with_date_created__ZFL>17_finite_retention_days",
),
],
)
def test_popup_height(
self,
general_stream,
to_vary_in_stream_data,
cached_message_retention_text,
server_feature_level,
expected_height,
):
model = self.controller.model
stream_id = general_stream["stream_id"]
self.controller.model.stream_dict = {stream_id: general_stream}
self.controller.model.stream_dict[stream_id].update(to_vary_in_stream_data)
self.controller.model.server_feature_level = server_feature_level
model.stream_dict = {stream_id: general_stream}
model.stream_dict[stream_id].update(to_vary_in_stream_data)
model.cached_retention_text = {stream_id: cached_message_retention_text}
model.server_feature_level = server_feature_level

stream_info_view = StreamInfoView(self.controller, stream_id)

# height = 1(description) + 2(blank lines) + 2(category)
# + 3(checkboxes) + [2-4](fields, depending upon server_feature_level)
# + 3(checkboxes) + [2-5](fields, depending upon server_feature_level)
assert stream_info_view.height == expected_height

@pytest.mark.parametrize("key", keys_for_command("COPY_STREAM_EMAIL"))
Expand Down
34 changes: 34 additions & 0 deletions zulipterminal/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ def __init__(self, controller: Any) -> None:
for stream in self.stream_dict.values():
stream["date_created"] = None

self.normalize_and_cache_message_retention_text()

# NOTE: The expected response has been upgraded from
# [stream_name, topic] to [stream_name, topic, date_muted] in
# feature level 1, server version 3.0.
Expand Down Expand Up @@ -192,6 +194,38 @@ def __init__(self, controller: Any) -> None:
self.new_user_input = True
self._start_presence_updates()

def message_retention_days_response(self, days: int, org_default: bool) -> str:
suffix = " [Organization default]" if org_default else ""
return ("Indefinite" if (days == -1 or days is None) else str(days)) + suffix

def normalize_and_cache_message_retention_text(self) -> None:
# NOTE: The "message_retention_days" field was added in server v3.0, ZFL 17.
# For consistency, we add this field on server iterations even before this
# assigning it the value of "realm_message_retention_days" from /register.
# The server defines two special values for this field:
# • None: Inherits organization-level setting i.e. realm_message_retention_days
# • -1: Messages in this stream are stored indefinitely
# We store the abstracted message retention text for each stream mapped to its
# sream_id in model.cached_retention_text. This will be displayed in the UI.
self.cached_retention_text: Dict[int, str] = {}
realm_message_retention_days = self.initial_data["realm_message_retention_days"]
if self.server_feature_level is None or self.server_feature_level < 17:
for stream in self.stream_dict.values():
stream["message_retention_days"] = None

for stream in self.stream_dict.values():
message_retention_days = stream["message_retention_days"]
is_organization_default = message_retention_days is None
final_msg_retention_days = (
realm_message_retention_days
if is_organization_default
else message_retention_days
)
message_retention_response = self.message_retention_days_response(
final_msg_retention_days, is_organization_default
)
self.cached_retention_text[stream["stream_id"]] = message_retention_response

def get_focus_in_current_narrow(self) -> Union[int, Set[None]]:
"""
Returns the focus in the current narrow.
Expand Down
8 changes: 8 additions & 0 deletions zulipterminal/ui_tools/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1329,6 +1329,13 @@ def __init__(self, controller: Any, stream_id: int) -> None:
else []
)

message_retention_days = [
(
"Message retention days",
self.controller.model.cached_retention_text[self.stream_id],
)
]

total_members = len(stream["subscribers"])
member_keys = ", ".join(map(repr, keys_for_command("STREAM_MEMBERS")))
self.stream_email = stream["email_address"]
Expand All @@ -1354,6 +1361,7 @@ def __init__(self, controller: Any, stream_id: int) -> None:
(
"Stream Details",
date_created
+ message_retention_days
+ [
("Weekly Message Count", str(weekly_msg_count)),
(
Expand Down