Add basic support for polls (viewing and voting)

[WesleyAC]

This PR adds basic support for viewing and voting on polls.

Some notes:

• It's pretty ugly, largely because the underlying widget/submessage/polls code is ugly. I'm open to changing things where it's easy, but I don't want to go down the rabbit hole of cleaning up the fundamental way that polls work.
• I've implemented local echo, although I don't know how I feel about it — if the sendSubmessage fails, we wouldn't communicate that to the user. Happy to have thoughts on what the right approach is here. One thought is that maybe we want to change the background color, but not the number? That would be pretty ugly, but could communicate what's going on without building a bunch more UI. We could also add a loading spinner like we do on outbox messages.
• I have a $FlowFixMe for adding types to PollData. I'm would like to do this, although probably not complete types: there are some fundamental limits to how much we can get from typing here, since there's so much dynamism in how widgets work.
• I made the poll buttons 36x36px, since 48x48 seemed really large. I understand that we usually aim for 48x48px touch targets, but I think that for instance, reaction emoji are good precedent here for not doing that.

[chrisbobbe]

Thanks for working on this, @WesleyAC!

For the two polls on CZO here, here are some screenshots from iOS:

I have a few thoughts:

• In night mode, the vote count should be the same light color as the rest of the text, to contrast with the background.
• Should the vote counts be in a slightly bigger font size? They look pretty small, and it looks like they have plenty of space in the rounded rectangles to be bigger.
• The "New Choice" input isn't there yet. I agree that it's probably best to do after we've got a more basic version in.
• Even the un-selected choices give a splash of brand color that I think is rare in the message list, and (maybe?) so far reserved to mark something the user has done; emoji reactions come to mind. Would it make sense to reserve the brand color for just the choice(s) the user has chosen?
• I think the minimum touch target of 36 by 36 may end up being OK; as you've mentioned, reaction emoji aren't at least 48 by 48. One thought I had was to make the entire row touchable (so, extending the width by a lot, without extending the height). But then I thought that might conflict with a future interaction where we saw a truncated list of voters and wanted to expand to see more. If we do go for an interaction like that, and find that it doesn't conflict, perhaps something like that would be possible.
• I don't think the button layout does quite the right thing when the number of voters is very large. Here's 4 digits:
  
  I find that it does something nicer when I remove the display: flex; from the .poll-widget > ul > li, but that's just an observation and not necessarily the solution:
  

[chrisbobbe]

Is there a reason to use Sentry.captureMessage instead of logging.error?

[WesleyAC]

Nope, changed.

[chrisbobbe]

nit: this looks like a group of good, but independent formatting cleanups, that'd best go in its own commit

[chrisbobbe]

Looking at the note on the Message type (and ignoring Outbox, since message has been narrowed from Message | Outbox to just Message)—

  /** Older servers omit this; when omitted, equivalent to empty array. */
  submessages?: $ReadOnlyArray<Submessage>,

—it seems like the cases where submessages is missing will also be the cases where servers don't know how to handle "submessages" (at all?). So I think "To use, open on web or desktop" isn't quite the right message here.

I think the check for submessages just above widgetBody's callsite gives us the right user-facing behavior, which that comment on Message hints at: treat a message that doesn't know about submessages as though it doesn't have any.

I think it would be reasonable for widgetBody to declare in its jsdoc that it mustn't be called unless the message has submessages. Then, instead of returning incompatible_message, we can cleanly and confidently throw an error at the top if that contract isn't fulfilled, with something like

  invariant(
    message.submessages !== undefined && message.submessages.length > 0,
    'should have submessages',
  );

This will satisfy Flow a few lines downward that the .filter call on message.submessages is OK.

[WesleyAC]

Good catch, changed.

[chrisbobbe]

Huh, what's this line doing? Could you point me to a file or a doc that says what "events" means in this context, and how widget.content.type relates to that?

[WesleyAC]

The widget docs are the closes we have to docs for this, but it won't answer your question. PollData is where the code for this lives, which is the closest thing we have to documentation.

I'll go add some comments to that file about the format of what these messages look like. In order to develop this, I just looked at the messages from the API in order to develop this, since there weren't any docs.

Here's the message data for this message, in case that helps you review.

[WesleyAC]

"events" is I guess a term that I made up for the submessages that aren't the initial widget submessage (vote and new_option, and a few others), I'll add a comment explaining that.

[WesleyAC]

Added a comment about this term.

[chrisbobbe]

target.closest('.message')?

Is there a case where we expect target.closest('.message') to come up empty? If so, probably best to just not call sendMessage at all, and do some appropriate thing instead. If not, maybe cleanest to error, and we'll revise our assumptions or find bugs to fix if we see stuff in Sentry.

Otherwise, I think parseInt(undefined, 10) is NaN, and that comes out as null after JSON stringify/parse, and I think we'd send that to the server which I don't think it expects.

It'd be nice to get things working so we can use invariant in the WebView; not sure how much effort that'd take.

[WesleyAC]

target.closest('.message') should always be there, rewrote this to do that and also use some of the requireAttribute functions, which should make this more robust in general.

[WesleyAC]

@chrisbobbe should be ready for another look.

Fixed the CSS so that it doesn't wrap, the font is the right color in night mode, and the edges aren't BRAND_COLOR until voted on.

[chrisbobbe]

Cool, thanks for the revision! See one small comment below; also this comment is pretty small. Feel free to merge if you find a nice clear path to addressing them.

Here are some iOS screenshots:

Huh, it looks like the buttons are a bit wider on iOS? I've just tried it on Android and I'm seeing a width that looks the same as the Android screenshots you've posted. Hmm.

I think it's a left and right padding of 1em, coming from my user-agent stylesheet? See the screenshot below. If I give the element 1em left and right padding in an inline style with the dev tools, I see no change in its appearance. The base.css:130 reference is to the following, and if I change the padding there, I don't see the button's padding changing:

/* Messages! And "loading" pseudomessages. */
.message,
.loading {
  display: flex;
  word-wrap: break-word;
  /* This value is used for messageReadSlop in src/webview/js/js.js, if
   * updating one, please update the other */
  padding: 16px;
  -webkit-tap-highlight-color: transparent;
}

Possibly the fix is to give an explicit padding that's smaller than 1em (and doesn't use em); does that sound right? Or do you think the user-agent stylesheet is trying to tell us something important that we should pay attention to?

[chrisbobbe]

Why more left padding than right padding? Is it something like what Greg said at #4750 (comment), about one of Material Design's decisions:

Speculating, I think the idea in giving the text more left padding than right padding may be that because it's justified left, there's naturally some space between the actual text and the right edge of its box -- and so the text will look more centered if the box is actually somewhat right of center.

I notice that it makes the dividing line between the question and the options a bit off-center, horizontally:

This might be fine; is it deliberate?

[WesleyAC]

This was copied from the spoiler-block css, I'm not sure why it's like that.

[WesleyAC]

Updated the padding to be explicit :) Will merge EOD Friday if I don't hear otherwise by then.

[chrisbobbe]

Hmm, actually, could you try visiting https://chat.zulip.org/#narrow/stream/7-test-here/topic/poll.20test in the app and see if you get an error like this:

ExceptionsManager.js:179 TypeError: Cannot read property 'question' of null
    at widgetBody (messageAsHtml.js:121)
    at _default (messageAsHtml.js:190)
    at contentHtmlFromPieceDescriptors.js:29
    at Array.forEach (<anonymous>)
    at contentHtmlFromPieceDescriptors.js:25
    at Array.forEach (<anonymous>)
    at _default (contentHtmlFromPieceDescriptors.js:21)
    at updateContent (generateInboundEvents.js:50)
    at generateInboundEvents (generateInboundEvents.js:109)
    at MessageList._this.shouldComponentUpdate (MessageList.js:221)

It looks like the poll_widget it's having a problem with looks like this (JSON.stringified); extra_data is null:

{
    "id": 299,
    "message_id": 582872,
    "sender_id": 703,
    "msg_type": "widget",
    "content": {
        "widget_type": "poll",
        "extra_data": null
    }
}

[chrisbobbe]

Updated the padding to be explicit

Cool, looks good now.

[WesleyAC]

Hmmm, I'm curious how that was generated. I see a similar error in the console in the webapp:

in process_submessages: null has no properties

I'll make it so it doesn't crash, at least, but I think that message is generally malformed.

[chrisbobbe]

Interesting; @showell, do you know what might be going on? It's at least one message in https://chat.zulip.org/#narrow/stream/7-test-here/topic/poll.20test

[WesleyAC]

It's the first message that's the problem, specifically.

[WesleyAC]

Anyways, added a check for that :)

[chrisbobbe]

It might be good for the comment (or possibly the commit message) to point to the specific message(s) we know about? Without that, I could see the check lingering far after the problem gets fixed, because no one knows whether it's been fixed and there's no clear path to finding that out.

Or possibly the fact that messages are out there in the wild is enough to say that this logic has to remain permanently, since servers don't generally re-render old messages when the rendering logic changes (doc). Which would be a slightly unfortunate side effect on the codebase.

[WesleyAC]

IMO it should remain as long as there are messages like this out there. (Also, clients can send messages like this, and we don't want this as a DOS vector). Unfortunate, yeah, but there's not much we can do about it :/

[chrisbobbe]

Makes sense. Before merging, what do you think about seeing what Steve says (I @-mentioned him above), with a timeout on that of a few days?

[showell]

We should keep the defensive code here in the client.

[gnprice]

Thanks @WesleyAC, and thanks @chrisbobbe for the reviews! Will be good to have this implemented. Some comments below.

[gnprice]

Hmm, interesting.

So one consequence of this local-echo strategy is that if for whatever reason the message gets rerendered -- for example because another vote came in, or because some other message changed or a new message arrived in this narrow -- this change will get forgotten. That seems awkward.

What does the webapp do to handle the latency of voting?

[WesleyAC]

I'm not sure, @showell, any idea about that?

My inclination is that building a particularly fancy local-echo is not worth it, but I'd be open to suggestions about small tactical fixes.

[WesleyAC]

There is no local echo in the webapp. I don't think that's acceptable in the mobile app, where it's easy to think you mistapped if the feedback is delayed.

[gnprice]

Does this logic correspond to what the webapp does?

I think it'd be best to try to make this logic track what's in the webapp. Particularly in the absence of documentation.

It looks like the corresponding logic here is around do_process_submessages in static/js/submessage.js, and its callees.

[WesleyAC]

That code does this:

    // Right now, our only use of submessages is widgets.

    const data = events[0].data;

    if (data === undefined) {
        return;
    }

Which seems strictly worse, to me. (It assumes events[0] is the widget submessage, and it assumes that there's only one widget in the message).

I guess it'd probably be for the best to do if (poll_widgets.length == 0), so we're less likely to break in a different way from the webapp in the future. But I don't think we should replace the .filter() here with [0], for instance.

[gnprice]

There's some other things I see there:

    message.submessages.sort((m1, m2) => Number.parseInt(m1.id, 10) - Number.parseInt(m2.id, 10));

It seems like the submessages aren't guaranteed to be in order -- so we should do the same sort, unless we know a reason to say it's unnecessary.

(It assumes events[0] is the widget submessage, and it assumes that there's only one widget in the message).

The code in the PR already assumes there's only one poll widget. It kind of has to -- the model doesn't have a way for votes to identify a particular poll within the message that they refer to. The system isn't really designed for having several different widgets in one message.

It also wouldn't make any sense for votes to come after the creation of the poll itself.

Basically I would much rather be encoding the assumptions that are in the web implementation -- which was written by Steve, who also specced out this subsystem in the first place -- than encoding a different set of implicit assumptions from our own differently-shaped implementation.

Having a different set of assumptions also makes it harder to directly compare our implementation with the webapp's, to spot other differences of behavior. The sorting is one; I'll make a separate inline comment with another I just spotted.

[WesleyAC]

Added the sorting code, thanks for catching that.

It also wouldn't make any sense for votes to come after the creation of the poll itself.

Presumably you mean before, not after?

Basically I would much rather be encoding the assumptions that are in the web implementation -- which was written by Steve, who also specced out this subsystem in the first place -- than encoding a different set of implicit assumptions from our own differently-shaped implementation.

I think that this is reasonable in general, be we do actually have a slightly different set of constraints than the webapp — the mobile app needs to be more robust to future server versions, whereas the webapp can safely assume that (for instance), all submessages will be widgets, since the clientside code will change at the same time as the server side code.

I've rewritten the logic to be much more similar to the webapp, but there are still a couple checks that I've added that I think make sense to be there.

[gnprice]

Presumably you mean before, not after?

Ah, yep.

the mobile app needs to be more robust to future server versions, whereas the webapp can safely assume that (for instance), all submessages will be widgets, since the clientside code will change at the same time as the server side code.

True! I'm happy to have checks for things like that, to handle forward-compatibility that the webapp can handle by just changing the client code when the time comes.

The really essential thing I want to avoid here is the opposite direction, where we encode assumptions that aren't in the web implementation nor documented for the API. That's how we wind up with bugs like the one where we assumed a new message sent by self was never unread.

And then the practical way to accomplish that -- at least for the situation this subsystem is in, where it's impossible to use only assumptions that are documented in the API, because there is no API documentation -- is to keep our implementation's logic generally shaped in a similar way to the logic in the web implementation, so that it's possible to compare without a lot of work.

Another aspect of forward compatibility is that if we encounter something from the server that's outside the current known patterns, it's not enough to just do something; we have to do the right thing, or do an appropriate kind of nothing. If we go and give meanings to a wide swath of currently-unused protocol space, that's just as capable of conflicting with future API extensions as any bad assumption.

Then in order to effectively avoid doing that, what's needed is basically to have some kind of coherent, and more or less explicit, theory of how the API is meant to be extended in the future. That's a key part of what our API types are for. For example the new WidgetData type discusses this pretty directly:

/**
 * The data encoded in a submessage to make the message a widget.
 *
 * Note that future server versions might introduce new types of widgets, so
 * `widget_type` could be a value not included here.  But when it is one of
 * these values, the rest of the object will follow this type.
 */
// Ideally we'd be able to express both the known and the unknown widget
// types: we'd have another branch of this union which looked like
//   | {| +widget_type: (string *other than* those above), +extra_data?: { ... } |}
// But there doesn't seem to be a way to express that in Flow.
export type WidgetData =
  | {|
      +widget_type: 'poll',
      +extra_data?: {| +question?: string, +options?: $ReadOnlyArray<string> |},
    |}
  // We can write these down more specifically when we implement these widgets.
  | {| +widget_type: 'todo', +extra_data?: { ... } |}
  | {| +widget_type: 'zform', +extra_data?: { ... } |};

Then it's possible to

• look at the code, and compare it to the explicit expectations, and check they're aligned;
• and look at the explicit expectations, and discuss them or think about them from an API perspective and evaluate whether they seem right, without having to reverse-engineer the implementation to extract its assumptions as part of that discussion.

[WesleyAC]

Yeah, that all makes sense 👍

[gnprice]

• I have a $FlowFixMe for adding types to PollData. I'm would like to do this, although probably not complete types: there are some fundamental limits to how much we can get from typing here, since there's so much dynamism in how widgets work.

Yeah, we should definitely do at least something minimal here.

I think there actually isn't even all that much dynamism -- a novel unknown type of widget can have arbitrary shapes of data, but for a specific widget type (like polls) it's pretty well specified, and PollData is about a specific widget type.

But in any case we should do at least something minimal, so that if there's anything difficult we can see concretely what it is. See typing_status.js.flow for an example of how to add types in the shared tree.

[WesleyAC]

Should be ready for another review! I'll open a PR for adding basic types in the shared repo. I'd rather not block this PR on that, though.

[gnprice]

Tracing through to the callees of the webapp's do_process_submessages as mentioned at #4704 (comment) , it looks like the webapp has a different behavior here:

    extra_data: {question = "", options = []} = {},

So in particular if there's a poll that starts with no options, the webapp will go ahead and present it normally -- with new_option submessages, it can end up as a perfectly functional poll. We should do the same thing, unless we know of a reason to behave differently.

[WesleyAC]

I don't believe that's what this is checking for — if you do /poll in the webapp, the mobile app handles that fine. This is for a particular weird message that as far as I can tell, no official client will send anymore. From what I saw, the webapp threw an error in the console for the only message that I found that triggered this condition.

Do let me know which specific part of the webapp codebase you're looking at that you think handles this if that's not the case, though.

[gnprice]

Comment below, and others just above and on threads in my original review #4704 (review) .

One somewhat more high-level thing that I'd like to see before merging this is to have effective types on this code. It'd be good to have them on the API boundary with the shared PollData, but the priority is that we should have them on the data we're getting from the server -- in particular, the result of that JSON.parse(submessage.content). As is, that ends up just being any.

The fundamental reason I want types on that data is that it's part of making explicit what our assumptions are about the shape of the data. That's important here because those assumptions come basically from some reverse-engineering work -- and it's a big pain to repeat that work just in order to read this code. Effectively that means that when revisiting this code we just won't realistically know exactly what assumptions it's relying on, which makes it fragile.

Much better to have the results written down; that lets us read the code and straightforwardly check it against the assumptions, and at the same time it makes it much easier to check the assumptions themselves against a new server/webapp version, or against API docs if and when this subsystem gets some of those.

I'll push to the PR branch a commit that adds relevant types to src/api/modelTypes.js, based on what I've looked at while reviewing this PR. Then you can use a type annotation to say that the result of that JSON.parse is a SubmessageData. From there it should be possible to write the code so that Flow accepts it with no fixmes, though you might need an assertion or two for assumptions that aren't encoded in the type itself.

[gnprice]

I don't like this filtering for type being present -- it doesn't reflect anything in the web implementation, and it feels basically accidental that it happens to work right now.

I'd rather handle this with the same logic as in the webapp: the first submessage (after sorting by id) introduces the widget, and the remaining submessages are the events that act on it.

That would also make this logic easier to think about. As is, we take one subset of the submessages (by widget_type === 'poll'), and take another subset (by type being present), and do two different things with them (including that we ignore any but the first of the first subset); but it's not clear if we expect them to overlap, or to exhaust the list of submessages, or maybe one is a subset of the other.

In fact, we expect a pretty specific structure: first one submessage with widget_type, then the rest of the submessages are events on it. And once the reader knows that expectation, it becomes a lot easier to make sense of what we're doing with the data, in particular with the [0] and the handle_event loop.

[WesleyAC]

Yeah, that makes sense. I hadn't realized that PollData was robust to receiving the initial widget submessage in addition to the "event" submessages, but it seems that it is. Done.

[WesleyAC]

Pushed some changes that should address most of this :)

I wasn't able to get flow to figure out that checking that pollWidget.content.widget_type is poll means that extra_data will always have question and options, I'd be happy for you to take a look at writing that in a way that flow understands.

[showell]

We should block this until zulip/zulip#18805 is merged on the server side. (It has a one-line change to how PollData is constructed.)

[WesleyAC]

Updated to use the new version of the shared PollData code from zulip/zulip#18805.

@gnprice, can you take a look at the flow errors here?

[WesleyAC]

Ah, I figured out the flow issue on my own, should be good for another review now :)

[gnprice]

Thanks for the revision! Here's a couple of comments. More to come; but GitHub won't seem to let me reply to threads above with this review pending, so here it is so far.

[gnprice]

nit: Can equivalently say pollWidget.content.extra_data.question ?? '', and that makes it a bit more obvious what's going on.

[WesleyAC]

That sounds lovely, but when I try it, flow reports:

Cannot get pollWidget.content.extra_data.question because property question is missing in property
extra_data of unknown type [1]. [incompatible-use]

 [1] 116│   if (pollWidget.content.extra_data == null) {
        :
     123│     message_sender_id: message.sender_id,
     124│     current_user_id: ownUserId,
     125│     is_my_poll: message.sender_id === ownUserId,
     126│     question: pollWidget.content.extra_data.question ?? '',

[WesleyAC]

(I actually had the conditional backwards in the code I pushed, but even with the conditional correct, it passes flow, but doesn't with the ??)

[gnprice]

FTR this is now fixed by the cast, with a fixme, to WidgetData. The error is correct -- this logic is relying on assumptions that aren't in the types, because the structure of these submessages arrays is kind of quirky and can't be expressed in the type system.

(We never did figure out why there wasn't an error in the ? : case.)

[gnprice]

I hadn't realized that PollData was robust to receiving the initial widget submessage in addition to the "event" submessages, but it seems that it is.

Hmm yeah, it looks like it is. It looks for type and drops anything that doesn't have that, and the initial submessage doesn't have it.

I'm not sure that's really part of its interface, though -- it seems like a bit of an odd thing to do. The web caller doesn't re-pass it that first submessage; in activate in widgetize.js the flow looks like this:

    events.shift();

    // …

    // Replay any events that already happened.  (This is common
    // when you narrow to a message after other users have already
    // interacted with it.)
    if (events.length > 0) {
        widget_elem.handle_events(events);
    }

So that shift drops the first element so that it doesn't go to the handle_events call. (Which in turn is basically a loop with handle_event.)

It's pretty easy to similarly skip the first element from what we use for handle_event, and that makes it a bit clearer to understand what's going on, so let's do that.

[WesleyAC]

Ok, I pushed a revision that addresses everything except the ??, which flow seems to treat differently than the ternary operator.

Should be ready for another look :)

[WesleyAC]

@gnprice Pushed a revision based on what we talked about on VC + in person.

[gnprice]

Thanks @WesleyAC for the revisions! A few comments below, to complete the threads we were talking about the other day; then I think this will be ready to merge.

(If not for the extra_data bit, I'd probably add the types comment as an extra commit, merge, and send a quick followup PR with the other, non-comment tweaks. But the extra_data item is more complex because it involves some data you've looked at that I don't have a link to.

And since we are doing a quick last round here, please take care of those other items too -- they'll be just about as quick for you as they would for me 🙂 )

[gnprice]

So from the webapp logic I quoted at #4704 (comment) , it looks like the webapp's behavior in this case should be more like:

  const { extra_data = {} } = pollWidgetContent;

We can vary from that, but I'd like to be explicit about doing so, because that makes it possible to compare the two implementations and spot discrepancies.

I think you showed me on your screen the other day the old message you had in mind, in the webapp, and it showed up with a behavior more like this, with just some text like "/poll". (There was an error in the JS console, suggesting the code may have errored out before it reached the code I quoted.) I think you may have suggested some reasoning for why showing the raw content like this is preferable to giving extra_data a default, like: at least then it'll say "/poll" instead of just looking empty.

Would you put that reasoning, and a link to that message, into the commit? And in the code, a comment saying this is different from web's behavior in this error case but we think this is better.

[WesleyAC]

I commented this case, filed #19145, and added a link to the message. I didn't modify the commit message, since it seems redundant to do that given the comment.

I tried to word it carefully, since the behaviour of this code is identical to the behaviour of the webapp as a whole, except for the error message printed to the console. The local behaviour is different, so it seems worth commenting still.

[WesleyAC]

@gnprice revision pushed :)

[gnprice]

Thanks! Looks good -- merging 🎉