Add support for Polls Widget on ZT. #1551

rsashank · 2024-10-09T00:39:32Z

What does this PR do, and why?

Add support to view polls

External discussion & connections

Discussed in #zulip-terminal in Polls - Widget Support #T986 #T1477
Fully fixes #
Partially fixes issue TRACKING: Increase support for widgets (**poll**, todo, trivia, ...) #986
Builds upon previous unmerged work in PR Add support for TODO Widget on ZT #1549 (first 2 commits)
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

Before	After	Webapp

neiljp

@rsashank This update looks good, and being able to see the poll votes is useful. This looks fine in a separate popup for now 👍

Having polls update via events would be the obvious next step here, though we may want to focus on merging the todo PR first to make integrating this easier?

zulipterminal/widget.py

zulipterminal/ui_tools/messages.py

zulipterminal/ui_tools/views.py

neiljp · 2024-10-21T18:15:16Z

zulipterminal/core.py

+            voter_names = []
+            for voter_id in voter_ids:
+                user_info = self.model.get_user_info(voter_id)
+                if user_info:
+                    voter_names.append(user_info["full_name"])


Is this new data structure code here, instead of the new view class, since it accesses the model, or for some other reason?

In any case, user_name_from_id would be a lighter-weight function to use, if only accessing the name.

Yes, I included it here since I'm accessing the model.

Got it, updated with user_name_from_id.

You addressed the latter point, but I'm not sure why the conversion into the format required for the popup occurs here, rather than in the popup code itself?
(this was my first point above)

Oops my bad, I had a response written that I forgot to submit as a review.

zulipterminal/core.py

Added process_poll_widget function to process submessages containing a poll widget. Returns the question of the poll along with options and their voters in a dict.

SHOW_POLL_VOTES : 'v' to check votes for a given poll.

Takes poll_question and options as an input and displays voters for each option.

Creates an instance of PollResultsView class and shows the popup.

This prevents an empty-title popup in the Poll Results view.

neiljp

@rsashank Thanks for the update - this looks rather close to being ready 🎉 I tested this manually and had to remind myself why the poll updates were working even though you didn't add them in this PR! 😮

Generally, there are a few nits I noticed similar to the other PR and to avoid duplication, a few improvements to the popup, and some small possible restructuring.

The main point left to discuss is probably the key to use - or where to connect the popup to. That's not critical before merging, though I'm often wary of accepting a new hotkey unless we know it's "safe" and won't impact things later. It's easy enough to change in a later commit, but since we're not currently releasing regularly then in principle anyone may use this and start using the hotkey!

If the latter becomes a sticking point, we can certainly merge the main part first, and leave the popup part to another PR (or similar).

neiljp · 2024-11-04T22:16:19Z

zulipterminal/ui_tools/messages.py

+                poll_widget = (
+                    f"<strong>Poll\n{poll_question}</strong>"
+                    if poll_question
+                    else "No poll question provided. Please add one via the web app."


Nit: For this line and one further below, we might consider using a fixed string common to both cases, and adding a TODO comment which relates to the lack of this feature so far (in ZT).

neiljp · 2024-11-04T22:24:53Z

zulipterminal/ui_tools/messages.py

+                if not self.poll_question:
+                    # If no poll question is provided, set a message to display
+                    # in Poll Results popup.
+                    self.poll_question = "No poll question provided."


We can do this last commit as part of an earlier commit when you first write this code, ie. setting a variable. This same text is used directly above, so that will avoid duplication. When you do the adding of the self. prefix, the variable will already be available.

neiljp · 2024-11-04T23:05:21Z

zulipterminal/ui_tools/messages.py

@@ -1185,4 +1186,6 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
            self.model.controller.show_emoji_picker(self.message)
        elif is_command_key("MSG_SENDER_INFO", key):
            self.model.controller.show_msg_sender_info(self.message["sender_id"])
+        elif is_command_key("SHOW_POLL_VOTES", key) and self.widget_type == "poll":


For no options, the popup operates but shows no data - maybe we should just skip the popup if there is no extra data to show?

neiljp · 2024-11-04T23:48:34Z

tests/widget/test_widget.py

+
+
+@pytest.mark.parametrize(
+    "submessage, expected_poll_question, expected_options",


Nit: plural, as in other PR.

neiljp · 2024-11-04T23:49:59Z

tests/widget/test_widget.py

+                "canned,1": {"option": "No", "votes": []},
+                "32159,1": {"option": "Maybe", "votes": [32159]},
+            },
+            id="poll_widget_with_multiple_voters",


Nit: This test is known to be a poll_widget already, as with other PR.

neiljp · 2024-11-05T02:52:04Z

zulipterminal/ui_tools/views.py

+        self,
+        controller: Any,
+        poll_question: str,
+        poll_options: Dict[str, Dict[str, Any]],


This type seems inaccurate? This is one benefit of defining more complex data types that you're passing around, even if via typeddicts, since you can refer to them as a name and update them in one place.

neiljp · 2024-11-05T02:52:21Z

zulipterminal/ui_tools/views.py

+            voters_display = (
+                "\n".join(map(str, voter_names))
+                if voter_names
+                else f"{INVALID_MARKER} No votes yet"


This does make it stand out better 👍

neiljp · 2024-11-05T02:53:33Z

zulipterminal/core.py

+
+            options_with_names[option_key] = {
+                "option": option_text,
+                "votes": voter_names if voter_names else [],


Nit: Is this conditional necessary?

neiljp · 2024-11-05T02:53:57Z

zulipterminal/core.py

+    def show_poll_vote(
+        self,
+        poll_question: str,
+        options: Dict[str, Dict[str, Any]],


Assuming this is poll_options, this type is different again?

neiljp · 2024-11-05T02:56:50Z

zulipterminal/ui_tools/messages.py

+            elif self.widget_type == "poll":
+                self.poll_question, self.poll_options = process_poll_widget(


Nit: You've added self.widget_type, but not initialized the others earlier in this commit.

zulipbot added the size: XL [Automatic label added by zulipbot] label Oct 9, 2024

rsashank added PR needs review PR requires feedback to proceed area: widgets labels Oct 10, 2024

rsashank force-pushed the polls branch from 7c22ad2 to ec42a03 Compare October 12, 2024 06:12

neiljp reviewed Oct 23, 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 23, 2024

rsashank added 6 commits November 4, 2024 14:40

widget: Add process_poll_widget function.

827ef2c

Added process_poll_widget function to process submessages containing a poll widget. Returns the question of the poll along with options and their voters in a dict.

messages: Implement logic to display polls on ZT.

af39cee

keys: Add SHOW_POLL_VOTES hotkey.

17d3ade

SHOW_POLL_VOTES : 'v' to check votes for a given poll.

views: Add PollResultsView class.

3b29642

Takes poll_question and options as an input and displays voters for each option.

core: Add show_poll_vote method to Controller.

990118a

Creates an instance of PollResultsView class and shows the popup.

messages: Add logic to handle the SHOW_POLL_VOTES hotkey.

78fd5df

rsashank force-pushed the polls branch from ec42a03 to 78b3eaf Compare November 4, 2024 09:51

messages: Update poll_question with a default text if empty.

9d75f50

This prevents an empty-title popup in the Poll Results view.

rsashank force-pushed the polls branch from 78b3eaf to 9d75f50 Compare November 4, 2024 09:53

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 Nov 4, 2024

neiljp reviewed Nov 5, 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 Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Polls Widget on ZT. #1551

Add support for Polls Widget on ZT. #1551

rsashank commented Oct 9, 2024

neiljp left a comment

neiljp Oct 21, 2024

rsashank Nov 4, 2024

neiljp Nov 5, 2024

rsashank Nov 5, 2024

neiljp left a comment

neiljp Nov 4, 2024

neiljp Nov 4, 2024

neiljp Nov 4, 2024

neiljp Nov 4, 2024

neiljp Nov 4, 2024

neiljp Nov 5, 2024

neiljp Nov 5, 2024

neiljp Nov 5, 2024

neiljp Nov 5, 2024

neiljp Nov 5, 2024



		@pytest.mark.parametrize(
		"submessage, expected_poll_question, expected_options",

		elif self.widget_type == "poll":
		self.poll_question, self.poll_options = process_poll_widget(

Add support for Polls Widget on ZT. #1551

Are you sure you want to change the base?

Add support for Polls Widget on ZT. #1551

Conversation

rsashank commented Oct 9, 2024

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

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