Support (un)resolving topics #1544

rsashank · 2024-09-05T23:11:04Z

What does this PR do, and why?

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.

Changes with respect to completion candidate PR:

Added support beyond ZFL 183:
Before Zulip 7.0 (feature level 183), the realm_community_topic_editing_limit_seconds property was returned by the response. It was removed because it had not been in use since the realm setting move_messages_within_stream_limit_seconds was introduced in feature level 162.
Changes to tests
Dropped the first commit

External discussion & connections

Discussed in #zulip-terminal in Support (un)resolving topics #T1235 (fix #T1075)
Fully fixes Support (un)resolving topics #1075
Partially fixes issue #
Builds upon previous unmerged work in PR Add support for (un)resolving topics in ZT. #1235
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

neiljp

@rsashank Thanks for the followup 👍

I may have had a few more thoughts, but wanted to get these comments to you before I go to bed :)

Re co-authoring, please note that

Shivam's email doesn't appear to match his original commits
some commits look entirely to be his with no additions; can you cherry-pick those from the original PR instead to indicate that?

zulipterminal/model.py

neiljp · 2024-09-07T22:35:58Z

tests/model/test_model.py

+        initial_data[
+            "realm_community_topic_editing_limit_seconds"
+        ] = topic_editing_limit_seconds
+        initial_data[
+            "realm_move_messages_within_stream_limit_seconds"
+        ] = move_messages_within_stream_limit_seconds


Strictly this doesn't cover the ZFL 183+ (Zulip 7) situation, where the first of these parameters was removed?

We could treat the two parameters as a patch to initial data via a dict, but unfortunately that then leaves assumptions as to whether the two values are in initial_data, ie. for later versions we strictly want to remove the field.

I've added cases for ZFL184, where the value of realm_community_topic_editing_limit_seconds is None.

My point was that some fields are actually missing at some feature levels, not that they are None, or I'd at least expect that at early server versions - and if an option is actually removed at a later version?

zulipterminal/model.py

tests/model/test_model.py

zulipterminal/ui_tools/views.py

neiljp · 2024-09-08T23:29:00Z

tests/ui_tools/test_popups.py

+    def test_toggle_resolve_status(self) -> None:
+        resolve_button = self.topic_info_view.widgets[-1]
+        resolve_button._emit("click")
+
+        self.controller.model.toggle_topic_resolve_status.assert_called_once_with(
+            stream_id=self.stream_id, topic_name=self.topic
+        )
+
+        self.controller.exit_popup.assert_called_once()


It looks like you added this extra test, and the one above this?

I'm not sure about using _emit here, at least compared to our normal use of keypress, but that should be fine - what inspired _emit?

I initially considered using keypress, but assumed the Enter key essentially uses ACTIVATE which triggers a click event in the context of an Urwid Button, just a NIT but thought it would be straightforward to use _emit.

I can change it back to use keypress with ACTIVATE_BUTTON

Urwid Docs

Send ‘click’ signal on ‘activate’ command.

This is fine, though I'll leave this comment unresolved since it may be useful to take this to a discussion of how best to integrate urwid and our own key handling further in this way.

neiljp · 2024-09-09T05:18:40Z

zulipterminal/config/keys.py

@@ -312,6 +312,11 @@ class KeyBinding(TypedDict):
        'help_text': 'Show/hide stream information & modify settings',
        'key_category': 'stream_list',
    },
+    'TOPIC_INFO': {
+        'keys': ['i'],
+        'help_text': 'Show/hide topic information & modify settings',


This is only useful for this feature right now, so perhaps we should update this slightly to mention updating the resolve status.

Is it alright now?

Yes, that seems fine for now 👍

zulipterminal/config/keys.py

neiljp · 2024-09-09T05:22:39Z

zulipterminal/ui_tools/views.py

+        )
+
+        self.widgets.append(resolve_topic_setting)
+        super().__init__(controller, self.widgets, "TOPIC_INFO", popup_width, title)


This TOPIC_INFO has no meaning here yet. Not sure of the ordering; let's leave it for now.

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. Co-authored-by: Shivam Deotarse <[email protected]>

Tests added. Co-authored-by: Shivam Deotarse <[email protected]>

Co-authored-by: Shivam Deotarse <[email protected]>

Fixes zulip#1075.

rsashank · 2024-09-30T13:40:24Z

@neiljp Thanks for the feedback!

Updated this PR :)

neiljp · 2024-10-03T23:03:03Z

tests/model/test_model.py

+
+        model.toggle_topic_resolve_status(stream_id, topic_name)
+
+        report_error.assert_called_once_with(expected_footer_error)


For comparison with the previous test, we can also assert that the update method is not called, and vice versa in the other test.

I mentioned this point indirectly when the tests were combined - each branch can assert on each element.

neiljp · 2024-10-03T23:07:12Z

tests/model/test_model.py

+        model.get_latest_message_in_topic = mocker.Mock(
+            return_value={
+                "subject": topic_name,
+                "timestamp": timestamp,
+                "id": 1,
+            }
+        )


Minor nits: if you have arbitrary constants, it's often useful to have them different, in case they 'work' when they are accidentally used in place of one another.

It's debatable whether a message id of 1 makes sense here, but it's a parameter so easily changed or parametrized later.

neiljp

@rsashank Thanks for the update 👍 My comments are mostly re tests and error handling, and many are hopefully small.

Please let me know if you have questions!

I did notice that, while I mentioned that Shivam's email for this was different, his merged commits look like another email address. This should be fine either way, as we can update the .mailmap for this if necessary.

neiljp · 2024-10-03T23:11:05Z

tests/model/test_model.py

+            ),
+        ],
+    )
+    def test_toggle_topic_resolve_status_no_footer_error(


Nit: We've been switching towards a double underscore between the function name and the test meaning/motivation - it makes it clearer which part is which. (same elsewhere)

neiljp · 2024-10-03T23:21:10Z

tests/model/test_model.py

+            case(
+                "hi!",
+                0,
+                12,
+                86400,
+                None,
+                " Time limit for editing topic has been exceeded.",
+                id="time_limit_exceeded:Zulip2.1+:ZFL12",
+            ),
+            case(
+                "hi!",
+                0,
+                162,
+                259200,
+                259200,
+                " Time limit for editing topic has been exceeded.",
+                id="time_limit_exceeded:Zulip7.0+:ZFL162",
+            ),
+            case(
+                "hi!",
+                0,
+                184,
+                None,
+                86400,
+                " Time limit for editing topic has been exceeded.",
+                id="topic_resolved:Zulip2.1+:ZFL184",


Looking at these cases alone, it's not clear to me what's different in each one?

Except for the last case (with incorrect id?) the id starts with time_limit_exceeded; that's currently implied by the test, though might not be, if you extend the code to handle other errors.

Does the topic name need to be parametrized? We're not testing unresolving as it stands, but could parametrize over that separately instead (using a nested decorator).

Does the latest_message_timestamp matter? It appears to be zero in every case, which is constant among the cases (can it be a constant like message_id above?), but it's unclear what that value is trying to achieve?

Is the error string a constant between each case? (again, it may not be if the error handling is extended)

neiljp · 2024-10-03T23:35:29Z

zulipterminal/model.py

+        latest_msg = self.get_latest_message_in_topic(stream_id, topic_name)
+        if not self.can_user_edit_topic() or not latest_msg:
+            return


These should report errors, based on how the function currently works - this was the error handling I mentioned was absent, and would be more obvious if restructured to be like this.

neiljp · 2024-10-03T23:40:05Z

zulipterminal/model.py

+        # ZFL < 11, community_topic_editing_limit_seconds
+        # was hardcoded as int value in secs eg. 86400s (1 day) or None
+        else:
+            edit_time_limit = 86400


I know this comment was moved, but it doesn't map to the value being None?

My related question is whether edit_time_limit can end up being None for a well-specified server? It's checked against None below, which might just be related to the .get calls above, or if the 'seconds' values can actually have a None/null value that has a distinct meaning.

neiljp · 2024-10-23T23:34:16Z

zulipterminal/config/keys.py

@@ -312,6 +312,11 @@ class KeyBinding(TypedDict):
        'help_text': 'Show/hide stream information & modify settings',
        'key_category': 'stream_list',
    },
+    'TOPIC_INFO': {
+        'keys': ['i'],
+        'help_text': 'Show/hide topic information & modify settings',


Yes, that seems fine for now 👍

neiljp · 2024-10-24T00:28:17Z

tests/ui_tools/test_popups.py

+        "topic_name, expected_topic_button_label",
+        [
+            ("hi!", "Resolve Topic"),
+            (f"{RESOLVED_TOPIC_PREFIX}" + "hi!", "Unresolve Topic"),


Nit: Unless you plan to extract the 'hi!', this can be a combined f-string without the addition.

neiljp · 2024-10-24T00:29:43Z

tests/ui_tools/test_popups.py

+    def test_toggle_resolve_status(self) -> None:
+        resolve_button = self.topic_info_view.widgets[-1]
+        resolve_button._emit("click")
+
+        self.controller.model.toggle_topic_resolve_status.assert_called_once_with(
+            stream_id=self.stream_id, topic_name=self.topic
+        )
+
+        self.controller.exit_popup.assert_called_once()


This is fine, though I'll leave this comment unresolved since it may be useful to take this to a discussion of how best to integrate urwid and our own key handling further in this way.

neiljp · 2024-10-24T00:32:11Z

zulipterminal/themes/gruvbox_dark.py

    'area:stream'      : (Color.DARK0_HARD,            Color.BRIGHT_BLUE),
+    'area:topic'       : (Color.DARK0_HARD,            Color.BRIGHT_BLUE),


It is useful to have distinct styles to enable changing them later, but I'm wondering if we should use different colors, rather than simply having duplicates of area:stream.

neiljp · 2024-10-24T00:34:08Z

zulipterminal/core.py

        self.show_pop_up(show_stream_view, "area:stream")

+    def show_topic_info(self, stream_id: int, topic_name: str) -> None:
+        show_topic_view = TopicInfoView(self, stream_id, topic_name)
+        self.show_pop_up(show_topic_view, "area:topic")
+
    def show_stream_members(self, stream_id: int) -> None:
        stream_members_view = StreamMembersView(self, stream_id)
        self.show_pop_up(stream_members_view, "area:stream")


Nit: This would be better not between two stream-related functions.

neiljp · 2024-10-24T00:40:36Z

zulipterminal/model.py

+                # ZFL < 11, community_topic_editing_limit_seconds
+                # was hardcoded as int value in secs eg. 86400s (1 day) or None
+                if 11 <= self.server_feature_level < 162:
+                    edit_time_limit = self.initial_data.get(
+                        "realm_community_topic_editing_limit_seconds", None
+                    )
+                # ZFL >= 162, realm_move_messages_within_stream_limit_seconds was
+                # introduced in place of realm_community_topic_editing_limit_seconds
+                elif self.server_feature_level >= 162:
+                    edit_time_limit = self.initial_data.get(
+                        "realm_move_messages_within_stream_limit_seconds", None
+                    )
+                else:
+                    edit_time_limit = 86400


This is OK for now, though a TODO comment above the block of code would be useful.

Note that my comments re the tests could end up splitting the resolve/unresolve and version-dependent parts, at least in terms of parametrize, which is potentially part way towards the simplification I mentioned would be possible in my previous comment.

zulipbot added the size: XL [Automatic label added by zulipbot] label Sep 5, 2024

rsashank force-pushed the un-resolve-topics branch 3 times, most recently from fb82ade to a570d28 Compare September 5, 2024 23:31

rsashank mentioned this pull request Sep 6, 2024

Add linting for HELP_CATEGORIES #1545

Closed

18 tasks

neiljp reviewed Sep 9, 2024

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback version parity: 5 labels Sep 9, 2024

rsashank force-pushed the un-resolve-topics branch 2 times, most recently from 7bea76f to 02963cd Compare September 30, 2024 13:14

rsashank and others added 5 commits September 30, 2024 18:46

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

59b9ab9

Tests added. Co-authored-by: Shivam Deotarse <[email protected]>

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

16cff3b

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

7fdf686

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

1c85d9d

Co-authored-by: Shivam Deotarse <[email protected]>

rsashank force-pushed the un-resolve-topics branch from 02963cd to 955fb11 Compare September 30, 2024 13:19

buttons: Add TOPIC_INFO shortcut to TopicButton.

76fafdf

Fixes zulip#1075.

rsashank force-pushed the un-resolve-topics branch from 955fb11 to 76fafdf Compare September 30, 2024 13:27

rsashank 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 30, 2024

neiljp reviewed Oct 3, 2024

View reviewed changes

neiljp reviewed Oct 24, 2024

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 Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support (un)resolving topics #1544

Support (un)resolving topics #1544

rsashank commented Sep 5, 2024 •

edited

Loading

neiljp left a comment

neiljp Sep 7, 2024

rsashank Sep 30, 2024

neiljp Oct 24, 2024

neiljp Sep 8, 2024

rsashank Sep 30, 2024

neiljp Oct 24, 2024

neiljp Sep 9, 2024

rsashank Sep 30, 2024

neiljp Oct 23, 2024

neiljp Sep 9, 2024

rsashank commented Sep 30, 2024

neiljp Oct 3, 2024 •

edited

Loading

neiljp Oct 3, 2024

neiljp left a comment

neiljp Oct 3, 2024

neiljp Oct 3, 2024

neiljp Oct 3, 2024

neiljp Oct 3, 2024

neiljp Oct 23, 2024

neiljp Oct 24, 2024

neiljp Oct 24, 2024

neiljp Oct 24, 2024

neiljp Oct 24, 2024

neiljp Oct 24, 2024


		model.toggle_topic_resolve_status(stream_id, topic_name)

		report_error.assert_called_once_with(expected_footer_error)

		'area:stream' : (Color.DARK0_HARD, Color.BRIGHT_BLUE),
		'area:topic' : (Color.DARK0_HARD, Color.BRIGHT_BLUE),

Support (un)resolving topics #1544

Are you sure you want to change the base?

Support (un)resolving topics #1544

Conversation

rsashank commented Sep 5, 2024 • edited Loading

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

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

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

rsashank commented Sep 30, 2024

neiljp Oct 3, 2024 • edited Loading

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

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

rsashank commented Sep 5, 2024 •

edited

Loading

neiljp Oct 3, 2024 •

edited

Loading