Add support for (un)resolving topics in ZT. #1235

srdeotarse · 2022-06-20T12:18:30Z

What does this PR do?
This PR adds support for (un)resolving topics in ZT via TopicInfoView popup menu when topic is highlighted in left_stream_bar and i key is pressed to toggle topic settings.

[WIP] #1075

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

Commit flow

First commit adds TOPIC_DESC shortcut to keys.py
Second commit adds above shorcut to TopicButton in buttons.py
Third commit still figuring out a way to update topic name with RESOLVED_TOPIC_PREFIX in model.py
Fourth commit adds TopicInfoView to views.py
Last commit adds show_topic_info function to core.py

Notes & Questions

How to update the topic name by adding or removing RESOLVED_TOPIC_PREFIX when Topic Resolved checkbox is toggled?

Interactions

Visual changes

neiljp

@srdeotarse This is a good almost-functional start with good commit separation, but possibly missing some consideration of the commit ordering. Looking forward to some tests later too, at least initially for the model.

zulipterminal/config/keys.py

zulipterminal/ui_tools/buttons.py

zulipterminal/model.py

zulipterminal/ui_tools/views.py

zulipterminal/core.py

Rohitth007 · 2022-06-30T06:52:25Z

I think it's better to remove Topic settings from the popup because when we add things like Mark all as read, it's not exactly a setting. Also maybe lets use a button, that says either Resolve Topic or Unresolve Topic, instead of a checkbox because when we add things like Edit Topic, it won't exactly be a checkbox.

neiljp

@srdeotarse The commit ordering here is cleaner and I've tested manually and confirmed it works for me - at least on topics I've written to 👍

As per the note inline, if the server has editing disabled then I believe topics cannot currently be resolved (and this will lead to this PR erroring?), and at the other end if 'community editing' is enabled (or various other options) then anyone can resolve at any time (perhaps excluding guests?) (which will lead to this PR not working when it should?). That may motivate a refactor of the existing topic-editing code to extract this logic, and connect to adding the community-editing feature (ie. not needing to have posted to a topic to edit it). I've also possibly seen some discussion elsewhere on czo about having separate topic resolution settings, but I believe these are currently still based on topic editing, though this would be good to confirm - and of course for now this remains the case.

I'm unsure if the updated UI is improved, particularly since multiple ENTER/SPACE/CLICK doesn't toggle (or show it), and there's no way of either avoiding it toggling on exiting the popup, or knowing if there will be an action on exiting the popup.

While there may be updates to the model logic, now that we're past WIP in that it works, it would be useful to get some preliminary tests for that logic - and that may help think about the error cases we can get and what messages the user may see and when. You may find that extracting the logic for knowing if a user can edit the topic separately (and testing that) will make this function less tangled, if you address that first.

zulipterminal/model.py

zulipterminal/ui_tools/views.py

zulipterminal/config/keys.py

Rohitth007

Your code is looking better than before, especially after handling the API better.
I've mostly reviewed the API calls and not the Popup. I'll give that a look soon. Maybe
@neiljp should also comment on the new API changes introduced as he's more familiar with them.

zulipterminal/model.py

Rohitth007 · 2022-08-06T05:30:05Z

zulipterminal/model.py

+            if response["messages"] != []:
+                time_since_msg_sent = time.time() - response["messages"][0]["timestamp"]
+                edit_time_limit = self.initial_data[
+                    "realm_community_topic_editing_limit_seconds"


Didn't know topics had a time limit. Then why do we show "Only topic editing allowed. Time limit for editing the message body has been exceeded." when realm_message_content_edit_limit_seconds is crossed?

Who can resolve such a topic then, only admins? I assume it would be a future feature for ZT then? Probably comment it?

We haven't added support for 'community' topic editing in the regular edit feature, so we give a notice there which represents the feature we support.

zulipterminal/model.py

zulipterminal/ui_tools/buttons.py

neiljp

@srdeotarse I'm coming back to this after Rohitth did some reviews, but I think the basic UI will be fine for now, and I note you've updated the logic handling as per some of our discussion so I've mostly focused on reading the code 👍

I'm going to merge the first commit separately to shorten the PR and since this is independent and looks fine.

As some of the inline comments explain, I think the PR will benefit from introducing the can_user_edit_topic function at the start with all the error strings and checks you have in the toggle function, then simply going ahead in the toggle function otherwise.

I don't see a large reason to separate the first and second commits (currently 2nd and 3rd) changing the model, which would also make the can_user_edit_topic function more meaningful initially. The additional functionality for more featureful servers is perfectly fine to come later 👍
(if it modifies a mostly working can_user_edit_topic, then the upgraded functionality commits could potentially even come after the UI, and likely the commits will move fairly easily like that if those need further work, to enable merging of the basic+UI commits)

Note that the issue won't really be fixed until we have a UI element to access it, so that fixed line belongs in the last commit currently.

Along with my overall feedback here, note that we need tests, certainly initially for can_user_edit_topic and toggle_topic_resolved_status (which will then adjust in later commits with the extra server feature). I expect when you write the tests, you'll see how making the function split early simplifies them.

zulipterminal/model.py

neiljp

@srdeotarse This is looking cleaner 👍

I would have posted the review sooner, but I have concerns over how we should best handle the logic generally. That is not down to your implementation in particular, but rather how it is documented and wider issues observed elsewhere. That seems related to how topic editing is dependent upon the message id in particular and if it belongs to the active user or not, as well as user roles.
eg.

#issues > topic resolution order
#issues > if marked as resolved it duplicates the argument
#design > time restrictions on topic editing
#21837

For now I think we should proceed as we are, and with the logic isolated and with tests we can consider if it matches expectations and adapt later.

We may want to use the first message in the narrow, as one possible example change.

zulipterminal/model.py

tests/model/test_model.py

zulipterminal/model.py

neiljp · 2022-08-30T01:04:05Z

zulipterminal/model.py

+            if response["messages"] != []:
+                time_since_msg_sent = time.time() - response["messages"][0]["timestamp"]
+                edit_time_limit = self.initial_data[
+                    "realm_community_topic_editing_limit_seconds"


We haven't added support for 'community' topic editing in the regular edit feature, so we give a notice there which represents the feature we support.

zulipterminal/model.py

neiljp

@srdeotarse The reordering of commits is much easier to read now along with the tests. With extra tests I'd be comfortable starting to merge this, though as I've tried to clarify, we need to cover the different server versions properly, and the tests appear to describe strange editing behavior?

One other UI element to explore would be to lose the flashing cursor on the button. We do that elsewhere, so it's certainly achievable :)

I was testing this manually today and did trigger some thread tracebacks, but that's likely to be from the event handling instead.

zulipterminal/model.py

tests/model/test_model.py

zulipterminal/model.py

neiljp

@srdeotarse Just a partial review here, since I think you didn't update everything from before and we discussed a few other points, and I wanted to get some feedback to you now.

tests/model/test_model.py

zulipterminal/config/ui_mappings.py

tests/model/test_model.py

neiljp · 2022-09-10T07:04:18Z

@srdeotarse The last push was a good tidy following my review 👍 The first 3 commits were very close, so I just merged those with adjustments to commit messages, removed spaces at the start of some error strings, added a simple test case for no-edits in ZFL75 and the indexing point I mentioned at #1235 (comment)
(last commit of 3 was 3c75b96)

neiljp

@srdeotarse Here's some extra feedback after the partial merge. I followed up on some older comments too, but you may need to look further up the thread of comments for context.

Mostly it's regarding improved testing or fairly minor points.

In manual testing I did find one case where it claimed the topic/message was too old to be edited, but I could do so manually. I've not dug deeper to know if that's due to the logic in your remaining un-merged model function, since it's not as well-tested yet, or due to nuances in the server logic which we didn't incorporate in the code that's already merged. Once the function is better tested then that might be clearer, but we can treat fine-tuning to can_user_edit_topic after we've merged this PR.

zulipterminal/model.py

neiljp · 2022-09-14T16:44:50Z

zulipterminal/model.py

+                    edit_time_limit = self.initial_data[
+                        "realm_community_topic_editing_limit_seconds"
+                    ]


You've not addressed this point that the API can return null/None.

tests/model/test_model.py

neiljp · 2022-09-14T17:01:03Z

tests/model/test_model.py

+                " Time limit for editing topic has been exceeded.",
+                True,


These two lines are confusing, though not for the specific content in this test case.

Rather, since there is no return value from the function being tested. Instead you use the return_value string to refer to either the successful new topic name, or the error string, depending on the bool value. I understand it's the "result" of the function, so kind of a 'return value' used for different purposes, and while the test will work, the naming is misleading.

So here instead of these parameters, you could have eg. expected_new_topic_name and expected_footer_error, and have both be the string expected in each case.

From another test you may remember we sometimes use either None or an empty string to indicate a sort of 'not applicable' in a given test case.

neiljp · 2022-09-14T17:06:28Z

tests/model/test_model.py

+    ):
+        model.initial_data = initial_data
+        model.server_feature_level = server_feature_level
+        initial_data["realm_community_topic_editing_limit_seconds"] = 86400


This fixed value doesn't test when the server sets different values, if the value is missing (older servers), or if it's None (newer servers).

neiljp · 2022-09-14T17:23:58Z

zulipterminal/ui_tools/views.py

@@ -1480,6 +1481,59 @@ def keypress(self, size: urwid_Size, key: str) -> str:
        return super().keypress(size, key)


+class TopicInfoView(PopUpView):


We could add minimal tests for this, such as the expected title (since it varies) and the expected action of the button.

zulipterminal/config/themes.py

zulipterminal/ui_tools/views.py

neiljp · 2022-09-14T17:50:47Z

zulipterminal/ui_tools/views.py

+        )
+
+        if self.topic_name.startswith(RESOLVED_TOPIC_PREFIX):
+            self.resolve_topic_setting_btn_lbl = "Unresolve Topic"


Does this need to be an attribute? I suppose it could help with a test.

The function verifies whether editing a message or editing a community topic is disabled. Also if the current user is eligible to edit a topic according to organization permissions is checked. New test added.

Fixes zulip#1075 The function calls get_latest_message_in_topic to fetch recent message in topic to be (un)resolved. It verifies user and editing conditions using can_user_edit_topic function and finally add or remove RESOLVED_TOPIC_PREFIX from topic name.

Hotkeys updated and "i" shortcut key added to KEYS_TO_EXCLUDE in generate_hotkeys.py file.

neiljp

@srdeotarse Thanks for the update 👍

I went through some old comments, there are a few outstanding points to address from that and a quick read.

neiljp · 2023-04-15T00:13:39Z

tests/model/test_model.py

-            case(
-                {"allow_message_editing": False, "edit_topic_policy": 1},
-                {


Why is this test case being removed?

Generally I think the changes in this revised first commit are otherwise minor enough that we can drop them? The rest appear to be:

reformatting (strings)

errors prefixing with space

comment removed

In any case, if we do include parts in some way, since this would be a modification of the previous function, the commit title would need adjusting.

neiljp · 2023-04-18T16:02:04Z

tests/model/test_model.py

+                {
+                    "subject": "hi!",
+                    "timestamp": 11662271397,
+                    "id": 1,
+                },


This dict appears identical in each case, except for the time, so let's build it in the function to keep it simpler? You could test for the None return value case, but that may be simpler in a separate test.

neiljp · 2023-04-18T16:04:05Z

tests/model/test_model.py

+                    "id": 1,
+                },
+                12,
+                259200,


Is the timestamp within this time difference? The test case doesn't make it clear that's why we expect it to resolve.

neiljp · 2023-04-18T16:16:15Z

zulipterminal/model.py

+                time_since_msg_sent = time.time() - latest_msg["timestamp"]
+                # ZFL < 11, community_topic_editing_limit_seconds
+                # was hardcoded as int value in secs eg. 86400s (1 day) or None
+                if self.server_feature_level is None or self.server_feature_level >= 11:


See #1235 (comment)

For server 2.1, this will set edit_time_limit to None.
For server 3.0 (or ZFL 11 at least), this will set edit_time_limit to the value in the server (None or the value).
For servers between those (eg. ZFL 10), this will set edit_limit to the default, ie. 86400 - this is an edge case

My impression is that the expected server behavior was

old 86400 <-- 2.1

<11 86400 <-- edge case

>=11 86400 or None (per server value) <-- 3.0+

This appears to generate

old None

<11 86400

>=11 86400 or None (per server value)

Is this going to fail for 2.1? If so, could you update the test to fail first?

neiljp · 2023-04-18T16:33:58Z

tests/model/test_model.py

+        # default return_value=True
+        model.can_user_edit_topic = mocker.Mock(return_value=True)
+        model.get_latest_message_in_topic = mocker.Mock(return_value=msg_response)
+        model.update_stream_message = mocker.Mock(return_value={"result": "success"})


See #1235 (comment)

This does not need a return value.

neiljp · 2023-04-18T16:38:50Z

tests/model/test_model.py

+                10,
+                86400,
+                "hi!",
+                None,


This value is always None? Have we lost some test cases?

zulipbot · 2023-05-09T03:26:55Z

Heads up @srdeotarse, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: L [Automatic label added by zulipbot] label Jun 20, 2022

neiljp reviewed Jun 20, 2022

View reviewed changes

neiljp added area: UI General user interface update PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 20, 2022

srdeotarse force-pushed the un-resolve-topics-support branch from e7c85d8 to 767c94f Compare June 29, 2022 17:20

srdeotarse added the PR needs review PR requires feedback to proceed label Jun 29, 2022

neiljp added PR needs buddy review and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 2, 2022

srdeotarse force-pushed the un-resolve-topics-support branch 2 times, most recently from 497d317 to 0ecd465 Compare July 17, 2022 14:10

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jul 17, 2022

neiljp reviewed Jul 19, 2022

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 Jul 21, 2022

srdeotarse force-pushed the un-resolve-topics-support branch from 0ecd465 to 8184d3e Compare August 5, 2022 13:40

Rohitth007 reviewed Aug 6, 2022

View reviewed changes

srdeotarse force-pushed the un-resolve-topics-support branch from 8184d3e to f4d592e Compare August 22, 2022 13:29

srdeotarse 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 22, 2022

neiljp reviewed Aug 22, 2022

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

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 23, 2022

srdeotarse force-pushed the un-resolve-topics-support branch from f4d592e to 14459c7 Compare August 29, 2022 16:16

neiljp reviewed Aug 30, 2022

View reviewed changes

srdeotarse force-pushed the un-resolve-topics-support branch from 14459c7 to e8c42ec Compare September 2, 2022 06:22

srdeotarse 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 Sep 2, 2022

neiljp reviewed Sep 3, 2022

View reviewed changes

srdeotarse force-pushed the un-resolve-topics-support branch 2 times, most recently from 96c5af4 to 95590f7 Compare September 6, 2022 19:59

srdeotarse 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 Sep 6, 2022

neiljp reviewed Sep 7, 2022

View reviewed changes

srdeotarse force-pushed the un-resolve-topics-support branch from 95590f7 to de19497 Compare September 10, 2022 03:20

neiljp force-pushed the un-resolve-topics-support branch from de19497 to a10973d Compare September 10, 2022 06:58

srdeotarse changed the title ~~[WIP] Add support for (un)resolving topics in ZT.~~ Add support for (un)resolving topics in ZT. Sep 10, 2022

neiljp reviewed Sep 14, 2022

View reviewed changes

srdeotarse force-pushed the un-resolve-topics-support branch from a10973d to 7f11cba Compare October 13, 2022 11:50

neiljp removed the PR needs buddy review label Oct 20, 2022

srdeotarse force-pushed the un-resolve-topics-support branch 2 times, most recently from a89ad36 to d4ed95e Compare April 8, 2023 04:43

srdeotarse added 7 commits April 8, 2023 10:20

model: Add can_user_edit_topic function.

2b30a1d

The function verifies whether editing a message or editing a community topic is disabled. Also if the current user is eligible to edit a topic according to organization permissions is checked. New test added.

views: Add TopicInfoView to show pop up menu to change topic settings.

595260d

themes: Add 'area:topic' style for topic info popup menu.

d3ebc5f

core: Add show_topic_info function to show TopicInfoView pop up menu.

9f2c0da

keys: Add TOPIC_INFO shortcut using 'i' key to show/hide topic settings.

b8e2a84

Hotkeys updated and "i" shortcut key added to KEYS_TO_EXCLUDE in generate_hotkeys.py file.

buttons: Add TOPIC_INFO shortcut to TopicButton.

8504cfc

srdeotarse force-pushed the un-resolve-topics-support branch from d4ed95e to 8504cfc Compare April 8, 2023 05:09

neiljp mentioned this pull request Apr 17, 2023

Allow use of e from Message information popup #1383

Open

2 tasks

neiljp reviewed Apr 18, 2023

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 Apr 23, 2023

zulipbot added the has conflicts label May 9, 2023

neiljp added the PR completion candidate PR with reviews that may unblock merging label Apr 1, 2024

rsashank mentioned this pull request Sep 5, 2024

Support (un)resolving topics #1544

Open

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for (un)resolving topics in ZT. #1235

Add support for (un)resolving topics in ZT. #1235

srdeotarse commented Jun 20, 2022

neiljp left a comment

Rohitth007 commented Jun 30, 2022

neiljp left a comment

Rohitth007 left a comment •

edited

Loading

Rohitth007 Aug 6, 2022

Rohitth007 Aug 6, 2022

neiljp Aug 30, 2022

neiljp left a comment

neiljp left a comment

neiljp Aug 30, 2022

neiljp left a comment

neiljp left a comment

neiljp commented Sep 10, 2022 •

edited

Loading

neiljp left a comment

neiljp Sep 14, 2022

neiljp Sep 14, 2022

neiljp Sep 14, 2022

neiljp Sep 14, 2022

neiljp Sep 14, 2022

neiljp left a comment

neiljp Apr 15, 2023

neiljp Apr 18, 2023

neiljp Apr 18, 2023

neiljp Apr 18, 2023

neiljp Apr 18, 2023

neiljp Apr 18, 2023

zulipbot commented May 9, 2023

		@@ -1480,6 +1481,59 @@ def keypress(self, size: urwid_Size, key: str) -> str:
		return super().keypress(size, key)


		class TopicInfoView(PopUpView):

+,
+,
+                              "hi!",
+                              None,

Add support for (un)resolving topics in ZT. #1235

Are you sure you want to change the base?

Add support for (un)resolving topics in ZT. #1235

Conversation

srdeotarse commented Jun 20, 2022

neiljp left a comment

Choose a reason for hiding this comment

Rohitth007 commented Jun 30, 2022

neiljp left a comment

Choose a reason for hiding this comment

Rohitth007 left a comment • edited Loading

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

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

neiljp commented Sep 10, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented May 9, 2023

Rohitth007 left a comment •

edited

Loading

neiljp commented Sep 10, 2022 •

edited

Loading