messages: WIP poll widget

[rsashank]

What does this PR do, and why?

Implementation for the polls widget WIP

Previous:

Current PR:

Outstanding aspect(s)

• [ ]

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 #
• 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 This looks to work well as a first version in practice 👍

I left some detailed feedback, but essentially separating the model and messages code will keep things more organized and easier to test, as well as make it easier to move forward from this point. The sooner we can handle submessage events, the more dynamic and useful polls will be.

[neiljp]

None of this code (733-749) is related to rendering/UI, which is primarily what the message-box code is concerned with.

Specifically, this code is only concerned with handling the (sub)messages, including in .process_poll_data, so it could easily belong somewhere else - it doesn't touch this object itself, and the new method is a class method (and doesn't use cls).

As I noted at the end of March in the channel, it'd be cleaner to separate submessage processing into the model, and have that return the equivalent kind of data as from process_poll_data, or possibly in the form of a new object. Did you understand my comment there?

The lines here could basically become something like

submessage_content = model.processed_submessages_for_message(message_id)
if submessage_content is not None:
    if submessage_content.type == "poll":
        ...

We also may want the processing in the model since:

• the model will also need to handle updating of submessages (eg. new votes coming in, questions)
• the model is where we have historically wrapped sending requests to the server (eg. active user voting in other polls)
• the processed data could be cached in the model, if necessary (only updated on events, not recalculated)

It may be reasonable to place some of the processing code into another file instead of the model, like we do with rendering tables, but let's see what we end up with first.

[mounilKshah]

Extending Neil's comment, since this function is to be added in the model file, the code patch for process_poll_data and its test can be shifted to a separate commit which comes before this commit.

[neiljp]

This formatting looks fine for now, just for testing 👍

For various future iterations, possibly in order of importance:

• Each poll appears to have a blank line trailing, which is likely since you're adding a "...\n" each time, rather than "\n".join(list_of_strings); check the difference if you're not familiar with that :)
• It may be enough to prefix the title/question with only Poll: ?
• Keeping the Poll: styling strong and distinct from a not-strong title/question might make it clearer that it's a poll? (and confirm the original text)
• Until we support updates, having 'Add Poll Question' looks somewhat odd when it's not possible, so maybe have (no title) or skip the line?
• Similarly it may be worth showing some indicator that there are no options if that's the case, eg. (no options available)
• Counts of eg. 0 and 11 don't line up correctly, or look oddly spaced; if we want a space either side of all numbers, we maybe want the width of 3 to be dynamic instead
• We could treat voter names, etc. like we do with limiting reacting user names beyond a certain number, at least in the message itself; that said, I suspect a poll or widget popup will be likely, so that information can be shown there, as you said.

As per my note in March, while we overwrite the content for /me, I'd prefer we avoid that pattern, since that modifies the original message - if only slightly in that case. It would be reasonable to refactor before this change to pass eg. a new content_to_render to transform_content, which might initially be the direct message content or the f-string from /me. Then you can add a further alternative in this work, which would set that string to the one you build above.

[neiljp]

Please have ids inline - there are only 3 cases, but they're each quite long.

[neiljp]

I was going to ask why you didn't just let black handle this, rather than use noqa, but this is related to the content being json. I agree that noqa could be easier for pasting test cases in reliably - pasting the line, just adding noqa - but it's still kinda annoying to read.

I'd suggest we could split the string and rely on automatic string concatenation, which could be easier to read; for example, that allows this:

assert (
   "string1 "
   "string2"
) == "string1 string2"

Alternatively, particularly as your cases become more complex, you may wish to use a factory fixture to generate the submessages (or even a full message with submessages). One reason for this is that the message_id should be identical for submessages, I'd expect, and msg_type is likely widget for all polls - and widget_type is poll, etc. That all needs confirming, but could allow you to have something in a test like the following which would generate data from certain templates:

submessages = poll_factory(
    {"question": "what?", "options": ["cheese"], "sender_id": 5},
    {"vote", ..., "sender_id": 6},
    message_id=1813721  # or just rely on a default
)

[mounilKshah]

Thanks @rsashank for taking this up! It will be great to have this feature on ZT
I tested this on my system and it worked as per expectations. I didn't face any glitches.
Apart from the test function that you have added for process_poll_data, I think it will be a good idea from integration test point of view to add a test case for transform_content which tests for poll data. I'll take this up in the stream.

[mounilKshah]

Extending Neil's comment, since this function is to be added in the model file, the code patch for process_poll_data and its test can be shifted to a separate commit which comes before this commit.

[mounilKshah]

I missed this point.
A lot of test cases failed due to KeyError at this line. You can try using get() instead of using key to fetch the data from the dictionary

[neiljp]

@rsashank To ensure my feedback is visible, following up from some points elsewhere, my suggestions at this point would be to:

• treat the poll data processing separately, even in a new file; this will make it easier to read and think about separately, and think of tests to cover it well
• render polls very simply for now - polls don't support markup (Support some Markdown in polls zulip#21917); this means we can go direct to urwid markup for them, use a conditional vs the normal processing of Zulip HTML into urwid (this also supports not overwriting message data, as per my comments above)
• separate at least the code for the 'is a poll'/'get the poll data' and 'render the poll'; the rendering part only cares about taking the processed poll data and rendering it (not the raw data), the model knows best about the other parts
• the parts for 'is a poll' and 'get the poll data' (processed) could be separate or a single model method; the different approaches just affect how you call them and whether data is implicitly processed.