Feature: Improve Stream Info Popup #880

zee-bit · 2021-01-19T16:08:13Z

Add Stream Members hotkey in README. [merged commit 8ce2edb]
Add Stream creation date in Stream Info popup [New in Server v4.0, ZFL 30]
Add Stream email
bugfix: Fix case when Weekly message count is None
Add Message retention days [New in server v3.0, ZFL 17]
Notification settings [issue Add notification checkboxes in Stream Info view. #887]

neiljp · 2021-01-19T20:15:06Z

@zee-bit We should have included the README in the previous iteration, so it's good to see it here 👍

For the stream creation date, it's good to see the check vs server feature level here, but note the type of this property, and we'll want to test against a range of values.

Also note that we may want to adjust the model data of the subscriptions to be uniform across different levels (abbreviated to ZFL in places), and then have the UI just use the data that's guaranteed to be present - ie. place the conditional(s) in the model. Ultimately this could be part of a move to store/expose only StreamData rather than the underlying data from the server.

zee-bit · 2021-01-23T09:22:55Z

@zulipbot add "PR needs review"

neiljp

@zee-bit This generally looks good 👍 Comments inline.

neiljp · 2021-01-26T21:26:11Z

tests/ui_tools/test_popups.py

@@ -550,24 +550,62 @@ def test_keypress_navigation(self, mocker, widget_size,
        super_keypress.assert_called_once_with(size, expected_key)


+@pytest.mark.parametrize('stream, stream_id, server_feature_level', [


This parametrizes all tests in this class, right? Is that the intent?

Yes. I thought it will increase our scope of testing, and potentially detect/eliminate any bug that may get unnoticed, if we tried to localize the parameters inside the class?

I have removed the class parameterization earlier and moved the fixtures to another test function(test_popup_height), since the popup height will change depending on the server version running - which corresponds to the numbers of fields supported in that version. :)

README.md

zulipterminal/ui_tools/views.py

zulipterminal/model.py

tests/ui_tools/test_popups.py

neiljp · 2021-01-26T22:10:10Z

zulipterminal/ui_tools/views.py

+        weekly_msg_count = ('N/A' if stream['stream_weekly_traffic'] is None
+                            else str(stream['stream_weekly_traffic']))


You're referring to a commit, not a PR, in this commit text.

If None is always due to it being just-created, it would be useful to indicate that, and expand N/A to something clearer in any case.

I'd be happy merging this commit sooner, but the tests in the first commit are dependencies of all the others.

I'll edit the commit title accordingly. Thanks for pointing it out. :)

I can add a comment explaining this behavior, and replace 'N/A' with something like, New stream. Data unavailable? for reference, webapp displays Stream created recently.

Adding this commit in the same PR has led to this. I asked in #zulip-terminal about adding this fix in this PR or another PR, and we decided to go with the former. If you want, I can add this to the PR I mentioned few comments above. This should segregate this fix from the class parameters I declared in the prior commit?

zulipterminal/model.py

zee-bit · 2021-02-21T19:54:17Z

@neiljp PR has been updated.
Using Overlay for pyperclip error was looking ugly(and, not in place with our current UI), so I have decided to use the already implemented NoticeView, prompting those specific users to install the required utility package for pyperclip. Other previous reviews have been addressed as well(except maybe class parameterization, since I wasn't sure if you wanted me to remove those?).

zee-bit · 2021-02-21T19:55:24Z

@zulipbot add "PR needs review"
@zulipbot remove "PR needs update"

zee-bit · 2021-02-26T21:35:13Z

On second look, I dont think we use Class parameterization anywhere else in our codebase. So, I have removed it, and moved the parameters to another function that tests stream_info's popup height.

neiljp

@zee-bit This addresses a lot of my structural points, probably most left are the details 👍

The use of the fixture looks good to me, certainly for now. We may want to follow-up with private variations and similar later, but we can build on this.

tests/core/test_core.py

neiljp · 2021-08-03T22:23:12Z

zulipterminal/core.py

+            clipboard_text = pyperclip.paste()
+            assert clipboard_text == stream_email
+            self.view.set_footer_text("Stream E-mail copied successfully!", 3)
+        except Exception:


Right; should this only be a pyperclip-specific exception, that we can use? The (online) docs for it don't include that, but the source possibly does - and you have that in the test? The package doesn't seem particularly active, otherwise I'd suggest proposing updating the documentation or opening an issue.

zulipterminal/core.py

zulipterminal/model.py

neiljp · 2021-08-03T22:57:21Z

tests/ui_tools/test_popups.py

-                {"date_created": 1472047124},
+                {
+                    "date_created": 1472047124,
+                    "message_retention_days": "72 [Organization default]",


This would be clearer if this text wasn't overwritten, as per my other comment.

neiljp · 2021-08-03T22:59:04Z

tests/ui_tools/test_popups.py

+                id="ZFL=None__no_date_created_no_retention_days",
            ),
            case(
-                {"date_created": None},
+                {
+                    "date_created": None,
+                    "message_retention_days": "Indefinite [Organization default]",
+                },
                29,
-                11,
-                id="ZFL<30__no_date_created",
+                12,
+                id="ZFL<30__no_date_created_default_indefinite_retention_days",


Is it meaningful here to include a 17-30 case, given that one feature is introduced at 30 and another by 17?

preetmishra

@zee-bit Thanks for the updates! 👍 This continues to work well. I've left some in-line comments.

tests/conftest.py

zulipterminal/core.py

neiljp · 2021-08-12T22:39:27Z

@zee-bit For the record, I just merged the first 7 commits as 0e3d91e and those prior 🎉

This should allow focus on the last commit by itself 👍

neiljp

@zee-bit This last commit is mainly minor points since we've discussed a lot, but the early Zulip version behavior still seems unclear. Other than the existing commit being quite large, having the text extraction in a separate commit will help with focusing anyone else on how we're handling it, without having display code in the way.

zulipterminal/model.py

zulipterminal/ui_tools/views.py

zulipterminal/model.py

zulipterminal/ui_tools/views.py

tests/model/test_model.py

neiljp · 2021-08-12T23:24:41Z

tests/model/test_model.py

+            case(
+                {1: {}},
+                None,
+                10,
+                {1: "Indefinite [Organization default]"},
+                id="ZFL=None_no_stream_retention_realm_retention=None",


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?

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)

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

This commit normalizes the stream message_retention_days field and caches a message retention text that can be later used to update the UI. This response was added in Server v3, feature level 17. For older versions, the "realm_message_retention_days" field from /register is stored. The current logic dictates: * If this field is absent in subscription (i.e. ZFL<17) then add this field explicitly with value None, and then cache a retention text with the organization default i.e. realm_message_retention_days response. * If the field is absent and realm_message_retention_days is None (for server v<3.0) or -1 (server v3+), then the messages would be stored indefinitely. * If the field is already present with value None, then the default organization-level setting for this field is inherited. * If the field is already present with value -1, then the messages in this stream is stored indefinitely. Reference: https://zulip.com/api/get-subscriptions#response Tests amended.

This commit adds Message retention day field in StreamInfo popup. It uses the model.cached_retention_text to retrieve and display the message retention response in a user-friendly format. Tests amended.

neiljp · 2021-08-14T05:36:46Z

@zee-bit Thanks for getting this final commit work done 👍 This now passes CI after the pytz fixup. I'm going to merge this now 🎉 (we can follow-up with other features in separate PRs - could you file issues for what we discussed?)

@mateuszmandera Thanks for confirmation of the findings - do we want to improve the API docs for this?

zulipbot added the size: S [Automatic label added by zulipbot] label Jan 19, 2021

zee-bit force-pushed the StreamInfo branch from 9fd6e10 to facd867 Compare January 20, 2021 19:40

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Jan 20, 2021

zee-bit force-pushed the StreamInfo branch 5 times, most recently from 53ba780 to 0331f3d Compare January 22, 2021 20:37

zee-bit changed the title ~~[WIP] Feature: Add extra fields in Stream Info popup~~ Feature: Improve Stream Info Popup Jan 23, 2021

zulipbot added the PR needs review PR requires feedback to proceed label Jan 23, 2021

zee-bit mentioned this pull request Jan 23, 2021

Add notification checkboxes in Stream Info view. #887

Open

4 tasks

neiljp reviewed Jan 26, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jan 26, 2021

neiljp added this to the Release after next milestone Jan 28, 2021

Base automatically changed from master to main January 30, 2021 20:31

zulipbot added the has conflicts label Jan 30, 2021

zee-bit force-pushed the StreamInfo branch from 0331f3d to 7c784cd Compare February 21, 2021 19:39

zulipbot removed the has conflicts label Feb 21, 2021

zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Feb 21, 2021

zee-bit force-pushed the StreamInfo branch 2 times, most recently from 560bf38 to 08c60a8 Compare February 26, 2021 21:27

zee-bit force-pushed the StreamInfo branch from 08c60a8 to c15b52b Compare March 1, 2021 06:53

neiljp reviewed Aug 3, 2021

View reviewed changes

zee-bit force-pushed the StreamInfo branch from 1227417 to c61872d Compare August 7, 2021 13:57

zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Aug 7, 2021

preetmishra reviewed Aug 8, 2021

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

zulipterminal/core.py Outdated Show resolved Hide resolved

zulipterminal/core.py Outdated Show resolved Hide resolved

zee-bit force-pushed the StreamInfo branch from c61872d to 8170a48 Compare August 8, 2021 17:15

zee-bit mentioned this pull request Aug 8, 2021

Define and use Stream data fixture from conftest #1115

Merged

4 tasks

zulipbot added the has conflicts label Aug 8, 2021

zee-bit force-pushed the StreamInfo branch from 8170a48 to 07e3e46 Compare August 9, 2021 07:08

zulipbot removed the has conflicts label Aug 9, 2021

zee-bit force-pushed the StreamInfo branch from 07e3e46 to 34c9127 Compare August 9, 2021 07:19

neiljp force-pushed the StreamInfo branch from 34c9127 to 607dceb Compare August 12, 2021 22:36

neiljp reviewed Aug 12, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Aug 12, 2021

zee-bit force-pushed the StreamInfo branch from 607dceb to 45cd8a1 Compare August 13, 2021 20:18

zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Aug 13, 2021

zee-bit added 2 commits August 13, 2021 22:25

views: Add a Message retention days field in Stream info view.

b933405

This commit adds Message retention day field in StreamInfo popup. It uses the model.cached_retention_text to retrieve and display the message retention response in a user-friendly format. Tests amended.

neiljp force-pushed the StreamInfo branch from 45cd8a1 to b933405 Compare August 14, 2021 05:27

neiljp merged commit 17d32a1 into zulip:main Aug 14, 2021

neiljp removed the PR needs review PR requires feedback to proceed label Aug 14, 2021

zee-bit mentioned this pull request Aug 14, 2021

Include more elements in StreamInfo view. #1119

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Improve Stream Info Popup #880

Feature: Improve Stream Info Popup #880

zee-bit commented Jan 19, 2021 •

edited

Loading

neiljp commented Jan 19, 2021

zee-bit commented Jan 23, 2021

neiljp left a comment

neiljp Jan 26, 2021

zee-bit Jan 27, 2021

zee-bit Mar 27, 2021 •

edited

Loading

neiljp Jan 26, 2021

zee-bit Jan 27, 2021 •

edited

Loading

zee-bit commented Feb 21, 2021

zee-bit commented Feb 21, 2021

zee-bit commented Feb 26, 2021

neiljp left a comment

neiljp Aug 3, 2021

neiljp Aug 3, 2021

neiljp Aug 3, 2021

preetmishra left a comment

neiljp commented Aug 12, 2021

neiljp left a comment

neiljp Aug 12, 2021

zee-bit Aug 13, 2021 •

edited

Loading

mateuszmandera Aug 13, 2021

neiljp commented Aug 14, 2021

		@@ -550,24 +550,62 @@ def test_keypress_navigation(self, mocker, widget_size,
		super_keypress.assert_called_once_with(size, expected_key)


		@pytest.mark.parametrize('stream, stream_id, server_feature_level', [

		weekly_msg_count = ('N/A' if stream['stream_weekly_traffic'] is None
		else str(stream['stream_weekly_traffic']))

Feature: Improve Stream Info Popup #880

Feature: Improve Stream Info Popup #880

Conversation

zee-bit commented Jan 19, 2021 • edited Loading

neiljp commented Jan 19, 2021

zee-bit commented Jan 23, 2021

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Mar 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

zee-bit commented Feb 21, 2021

zee-bit commented Feb 21, 2021

zee-bit commented Feb 26, 2021

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetmishra left a comment

Choose a reason for hiding this comment

neiljp commented Aug 12, 2021

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Aug 14, 2021

zee-bit commented Jan 19, 2021 •

edited

Loading

zee-bit Mar 27, 2021 •

edited

Loading

zee-bit Jan 27, 2021 •

edited

Loading

zee-bit Aug 13, 2021 •

edited

Loading