Support polls (read-only) #885

PIG208 · 2024-08-12T19:35:45Z

Stacked on top of #823.

Fixes: #165

gnprice · 2024-08-13T05:03:15Z

For ease of cross-reference: the previous PR #823 originally contained these changes too, and had a round of UX review starting with these screenshots:
#823 (comment)

PIG208 · 2024-08-13T20:44:21Z

Moving this to integration review because these changes went through maintainer review in the original PR.

gnprice

Thanks! Here's a review focused on the first two commits, which set up the model:
144d5af api: Construct polls data store from submessages.
8388936 event: Handle submessage event for polls.

I didn't read through all of the tests, because I left some high-level comments there that I think will make some of them easier to read.

Left for a future round are the remaining commits:
6cc7d0d (optional) test: Add a test helper for inspecting logs.
3a0f853 poll: Support read-only poll widget UI.

gnprice · 2024-08-14T02:39:12Z

lib/api/model/submessage.dart

+  /// The limit of options any single user can add to a poll.
+  ///
+  /// See https://github.com/zulip/zulip/blob/304d948416465c1a085122af5d752f03d6797003/web/shared/src/poll_data.ts#L69-L71
+  static const maxIdx = 1000; // TODO validate


What's involved in this TODO?

gnprice · 2024-08-14T02:43:34Z

lib/api/model/submessage.dart

+  final Set<String> _optionNames = {};
+  final Map<String, PollOption> _options = {};


What's the relationship between these two data structures?

Relatedly, what are the keys in the _options map?

Those would both be good questions to answer in dartdoc on these fields.

Extracted a OptionKey type alias to make it clearer that _options's keys are keys.
Also renamed _optionNames to _existingOptionTexts with a new comment.

gnprice · 2024-08-14T02:47:23Z

lib/api/model/submessage.dart

+  Poll({
+    required this.pollSenderId,
+    required this.question,
+    required final List<String> options,


Suggested change

required final List<String> options,

required List<String> options,

The final just stops us from mutating this parameter as a local variable, right? So that it doesn't have any effect on the interface for callers.

We don't routinely use that feature, and I don't think it's unusually helpful on this constructor. So let's leave it out, just to not add another thing for the reader looking at the interface to have to process.

gnprice · 2024-08-14T02:48:29Z

lib/api/model/submessage.dart

+    for (int index = 0; index < options.length; index += 1) {
+      // Initial poll options use a placeholder senderId.
+      // See [PollEventSubmessage.optionKey] for details.
+      _addOption(null, options[index], idx: index);


This call site is probably clearer if the parameters are made named. (Or the senderId, anyway, but then they might as well all three be named.)

Also probably clearer if senderId comes first, then idx, then option. The first two are basically specifying a key at which the latter will be set as a value.

gnprice · 2024-08-14T02:50:03Z

lib/api/model/submessage.dart

+/// - https://zulip.com/help/create-a-poll
+/// - https://github.com/zulip/zulip/blob/304d948416465c1a085122af5d752f03d6797003/web/shared/src/poll_data.ts
+class Poll {
+  Poll({


This can be made private — its only caller is the other constructor, and that seems like how it should stay.

Then put the public constructor first.

gnprice · 2024-08-14T04:22:14Z

test/model/message_test.dart

+    });
+
+    test('message has no submessages', () async {
+      final streamMessage = eg.streamMessage(id: 123);


nit:

Suggested change

final streamMessage = eg.streamMessage(id: 123);

final message = eg.streamMessage(id: 123);

The fact it's a stream message isn't important, right? So can leave that out of the story this tells to the reader. (It's still there in the eg.streamMessage call, but gets less emphasis.)

gnprice · 2024-08-14T04:24:47Z

test/model/message_test.dart

+      final messageId = await preparePollMessage();
+      await store.handleEvent(
+        eg.submessageEvent(
+          streamMessage.id,
+          eg.selfUser.userId,
+          content: jsonEncode(PollQuestionEventSubmessage(question: 'New question'))));


Suggested change

final messageId = await preparePollMessage();

await store.handleEvent(

eg.submessageEvent(

streamMessage.id,

eg.selfUser.userId,

content: jsonEncode(PollQuestionEventSubmessage(question: 'New question'))));

final messageId = await preparePollMessage(question: 'Old question');

await store.handleEvent(

eg.submessageEvent(

streamMessage.id,

eg.selfUser.userId,

content: jsonEncode(PollQuestionEventSubmessage(question: 'New question'))));

This way the test case is self-contained for the reader — the contrast between the original question value and the new question value is explicit within the test case's own source code.

(Similar to my comment above about eg.pollWidgetDataFavoriteLetter.)

That's a great catch. I find it to be quite a good demonstration.

gnprice · 2024-08-14T04:27:39Z

test/model/message_test.dart

@@ -576,4 +603,197 @@ void main() {
        .reactions.isNotNull().jsonEquals([eg.unicodeEmojiReaction]);
    });
  });
+
+  group('handleSubmessageEvent', () {
+    late StreamMessage streamMessage;


Suggested change

late StreamMessage streamMessage;

late Message message;

Similar to my preceding comment (on code a bit below this) — the detail that it's a stream message doesn't matter to the narrative, so can be left out.

gnprice · 2024-08-14T04:36:40Z

test/test_log.dart

+  logHistory = [];
+  try {
+    await callback();
+    return check(logHistory).isA<List<String>>();


Suggested change

return check(logHistory).isA<List<String>>();

return check(logHistory!);

If logHistory is back to null at this point, that seems like not just a normal failure of the calling test case — it means something that shouldn't be messing with that variable has a surprising bug where it's doing so anyway. So we can avoid cluttering test failure messages with a line about how the log history was expected to be (and is) a List<String>.

gnprice · 2024-08-14T04:37:47Z

lib/log.dart


 /// Whether [debugLog] should do anything.
 ///
 /// This has an effect only in a debug build.
 bool debugLogEnabled = false;

+/// Used for log inspection in tests.
+@visibleForTesting
+List<String>? logHistory;


Suggested change

List<String>? logHistory;

List<String>? debugLogHistory;

This will live in our global namespace (e.g. it'll appear in autocomplete from all over the app) so the "debug" prefix is important context.

PIG208 · 2024-08-14T08:08:51Z

Thanks for the review @gnprice! I have addressed them and updated the PR.

gnprice

Thanks for the revision! Comments below.

I didn't get to a careful read-through of this revision today, but wanted to get a round of feedback to you since it's been a couple of days.

gnprice · 2024-08-16T07:47:27Z

test/example_data.dart

-    'options': ['A', 'B', 'C'],
-  }
-};
+Map<String, Object> pollWidgetDataJson({


Can this return a PollWidgetData instead?

That's what we generally do in other example-data functions — it's good for keeping things well-typed. The caller can always say .toJson() when that's what it needs. (And for a lot of purposes it won't even need to: e.g. passing to eg.submessage, where the jsonEncode should take care of that.)

gnprice · 2024-08-16T07:48:17Z

test/example_data.dart

+
+Submessage submessage({
+  SubmessageType? msgType,
+  required Object? content,


Then this can be typed SubmessageData, which I think would help make the interface clear.

gnprice · 2024-08-16T08:04:03Z

lib/api/model/submessage.dart

+  @override
+  bool operator ==(Object other) {
+    if (other is! PollOption) return false;
+
+    return other.hashCode == hashCode;
+  }


I see. This override is still incorrect, though, for the reason I gave in the first comment (even in this revision which has fixed the secondary issue in the third comment). See the doc on Object.==.

It may work in tests because the right-hand side of the comparison is a throwaway that won't be mutated later… but an override here isn't restricted to use in tests, so having an incorrect == implementation is dangerous as a source of future bugs.

Looking at those tests, you can express those checks without a == override by using .deepEquals(<Condition<Object?>>[ … ]). See test/notifications/display_test.dart for an example, in the checkNotification helper. See also #878 (not yet merged) for further examples in a slightly different style, with the conditionActiveNotif and conditionSummaryActiveNotif helpers.

gnprice · 2024-08-16T08:06:01Z

lib/api/model/model.dart

+      : null;
+  }
+
+  static Poll? _readPoll(Map<Object?, Object?> json, String key) {


Cool. Yeah, the new nicely-short _readPoll makes sense to keep on Message, as it's expressing the details that are about property names on message objects.

gnprice · 2024-08-16T08:09:39Z

lib/api/model/model.g.dart

+  )..poll = Message._readPoll(json, 'submessages') == null
+      ? null
+      : Poll.fromJson(Message._readPoll(json, 'submessages'));


Ow, this generated code is inefficient. (Since _readPoll includes all the work of actually parsing the JSON.)

If there's not a quick and clean way to rearrange things to avoid this double parsing, that's fine and it shouldn't block merging this — polls aren't that common so it's not a major performance issue. But in that case let's leave a TODO comment about it at Message.poll.

gnprice · 2024-08-16T08:11:40Z

test/api/model/submessage_test.dart

+            eg.submessage(
+              content: event,
+              senderId: senderId,
+            ),


nit: fold onto one line

gnprice · 2024-08-16T08:15:24Z

test/api/model/submessage_test.dart

+        events: [
+          (eg.otherUser.userId, PollNewOptionEventSubmessage(idx: 0, option: 'D')),
+          (eg.otherUser.userId, PollVoteEventSubmessage(key: '${eg.otherUser.userId},0', op: PollVoteOp.add)),
+          (eg.otherUser.userId, PollVoteEventSubmessage(key: '${eg.selfUser.userId},0', op: PollVoteOp.add)),
+          (eg.otherUser.userId, PollVoteEventSubmessage(key: '${eg.selfUser.userId},0', op: PollVoteOp.remove)),


Does this latter key correspond to an option? It looks like there's the initial options, then one created by eg.otherUser, and none by eg.selfUser.

gnprice · 2024-08-16T08:17:15Z

test/api/model/submessage_test.dart

+    test('applyEvent: adding repeated option', () {
+      final poll = messageWithSubmessageContents(pollWidgetData).poll!;
+      check(poll.options).deepEquals(defaultOptions);
+      poll.applyEvent(eg.otherUser.userId, PollNewOptionEventSubmessage(
+        option: defaultOptions[0].text, idx: 0));
+      check(poll.options).deepEquals(defaultOptions);


In either the name or a one-line comment within the test, say explicitly how the story ends: IIUC, the answer is that the event gets ignored.

This would do it:

Suggested change

test('applyEvent: adding repeated option', () {

final poll = messageWithSubmessageContents(pollWidgetData).poll!;

check(poll.options).deepEquals(defaultOptions);

poll.applyEvent(eg.otherUser.userId, PollNewOptionEventSubmessage(

option: defaultOptions[0].text, idx: 0));

check(poll.options).deepEquals(defaultOptions);

test('applyEvent: option with duplicate text ignored', () {

final poll = messageWithSubmessageContents(pollWidgetData).poll!;

check(poll.options).deepEquals(defaultOptions);

poll.applyEvent(eg.otherUser.userId, PollNewOptionEventSubmessage(

option: defaultOptions[0].text, idx: 0));

check(poll.options).deepEquals(defaultOptions);

gnprice · 2024-08-16T08:19:52Z

test/api/model/submessage_test.dart

+          ]);
+    });
+
+    test('applyEvent: adding repeated option', () {


Having these test cases for error paths is great. Let's also have test cases for the normal happy paths, though. As a bonus, those can make the error tests clearer because they serve as a baseline for the reader to compare to.

As is, if I'm reading right it looks like these tests would all pass if applyEvent just always did nothing 🙂

Hmm I guess the tests in lib/model/message_test.dart end up covering those happy paths.

I think it's clearest if the success cases and error cases have their tests next to each other — as I said above, the success tests make a helpful baseline for reading the error tests, and also the reader of the success tests may get nervous if it looks like there are no tests for the error cases.

I like the layer that the lib/model/message_test.dart tests are expressed at: calling store.handleEvent with a Zulip server event, just as if the event had come in from the server in the polling loop. That's how we try to write most of our model tests, following this principle:
https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing

So let's try to move these error tests to that file and convert them to that file's style.

In fact let's go a step farther: let's move even the "parse poll …" test cases the same way. Then this whole "Message.poll" test group gets replaced by a comment saying the tests for deserializing [Message.poll] can be found over there.

Really what's going on here is that for this data structure, we've fused the job of deserializing it with the job of setting up a nontrivial data structure, including applying some significant model logic to apply all those poll-event submessages. So the deserialization code has a lot of the nature of model code, and it's fitting if its tests therefore wind up in the model code's test files. And then the tests for later events coming in definitely belong on the model side (as discussed above), and they're exercising most of the same code as the deserialization… so the deserialization tests belong next to the later-events tests, in test/model/.

gnprice · 2024-08-16T09:01:38Z

lib/api/model/submessage.dart

+    final Object? decodedContent;
+    try {
+      decodedContent = jsonDecode(event.content);
+    } catch (e) {
+      assert(debugLog('Invalid JSON from submessage event data for poll: $e\n${jsonEncode(event)}')); // TODO(log)
+      return;
+    }
+
+    final PollEventSubmessage? pollEventSubmessage;
+    try {
+      pollEventSubmessage = PollEventSubmessage.fromJson(decodedContent as Map<String, Object?>);
+    } catch (e) {
+      assert(debugLog('Malformed submessage event data for poll: $e\n${jsonEncode(event)}')); // TODO(log)
+      return;
+    }


Oh one more comment: we can simplify this a bit by making this just one try/catch that doesn't distinguish between whether it was the jsonDecode or the fromJson call that threw. Conceptually those are two phases of a single operation anyway: deserialize a string into a PollEventSubmessage.

Similarly if you walk through the logic in ApiConnection.send, I'm pretty sure the behavior there is that both kinds of failures throw very similar exceptions (instances of MalformedServerResponseException). So it's not worth getting more fine-grained here than we are there, the main place we deserialize JSON from the server.

(following up on #885 (comment) )

PIG208 · 2024-08-16T23:06:08Z

Partially updated. The tests reorganization is WIP.

PIG208 · 2024-08-17T06:02:38Z

The tests from "Message.poll" are rewritten in two destinations. Here's a broad overview:

"new message with initial submessages" under "handleMessageEvent":

Contains all the "parse poll" tests. Rewritten so that we handle new MessageEvents for polls. Because these tests are about processing the initial state of the message, there is no SubmessageEvents to handle.
"handleSubmessageEvent":

Contains all the "applyEvent" tests. Each of them got moved to the corresponding PollEventSubmessageType group.

Extra note: dropped the "no poll if submessages is empty" test because it is a duplicate of the existing "message has no submessages" test.

gnprice

Thanks for the revision! Here's a next round, covering all the first N-1 commits:

2bfbff0 message test [nfc]: Move handleDeleteMessageEvent
ed5f68e submessage: Extract OptionKey.
2d8f11e submessage [nfc]: Minor wording change to a comment.
ae9f87c submessage test [nfc]: Extract pollWidgetDataJson.
a85dbe4 api: Construct polls data store from submessages.
727b9d8 event: Handle submessage event for polls.
b00a64f (optional) test: Add a test helper for inspecting logs.

and leaving only the last commit for a future round:
bf599f2 poll: Support read-only poll widget UI.

gnprice · 2024-08-20T18:23:47Z

lib/api/model/model.dart

+  static Map<String, Object?> _readPoll(Map<Object?, Object?> json, String key) {
+    return {'data': json[key], 'sender_id': json['sender_id']};


This allocates a new Map, for every message we read. How about just returning json?

Or having this call Submessage.parseSubmessagesJson, like in the revision as of my previous review.

gnprice · 2024-08-20T18:41:10Z

test/example_data.dart

+
+Submessage submessage({
+  SubmessageType? msgType,
+  required Object? content,


gnprice · 2024-08-20T18:48:35Z

lib/api/model/submessage.dart

+  // It will start returning `Poll` as we support more Zulip widgets.
+  static Poll? parseSubmessagesJson(List<Object?> json, {


This comment doesn't seem right — this is on Submessage so won't remain specific to Poll in that future.

Perhaps this was intended for Poll.fromJson?

The Poll in the comment was unintended. Made it a TODO instead:

// TODO: Use a generalized return type when supporting other Zulip widgets.

gnprice · 2024-08-20T23:07:00Z

lib/api/model/submessage.dart

@@ -205,6 +239,8 @@ enum PollEventSubmessageType {
    .map((key, value) => MapEntry(value, key));
 }

+typedef OptionKey = String;


Similar to #823 (comment) :

Suggested change

typedef OptionKey = String;

typedef PollOptionKey = String;

gnprice · 2024-08-20T23:09:56Z

test/example_data.dart

-    'options': ['A', 'B', 'C'],
-  }
-};
+PollWidgetData pollWidgetData({


nit: commit message doesn't match:

submessage test [nfc]: Extract pollWidgetDataJson.

gnprice · 2024-08-20T23:38:56Z

test/model/message_test.dart

+
+        await prepare();
+        await store.handleEvent(MessageEvent(id: 0, message: message));
+        return check(store.messages[message.id]).isNotNull();


Suggested change

return check(store.messages[message.id]).isNotNull();

The callers don't use this; and it makes sense to leave that to the checkPoll helper.

gnprice · 2024-08-20T23:40:20Z

test/model/message_test.dart

+      final defaultOptionConditions = [
+        conditionPollOption('foo'),
+        conditionPollOption('bar'),
+      ];
+
+      final defaultPollWidgetData = eg.pollWidgetData(
+        question: 'example question',


nit: swap order, for a more logical order

gnprice · 2024-08-20T23:44:01Z

test/model/message_test.dart

+  /// Perform a single-message initial message fetch for [messageList] with
+  /// submessages.
+  ///
+  /// The test case must have already called [prepare] to initialize the state.
+  Future<void> prepareMessageWithSubmessages(
+    Message message, List<Submessage> submessages, {
+    bool foundOldest = false,
+  }) async {


Just like we did in the lib/api/ code, I'd like to avoid having poll-related or submessage-related code scattered through this test file. That will get in the way when trying to read and maintain this code for everything not related to submessages (because one usually doesn't want or need to think about them); and it also makes the poll/submessages test code itself harder to read, because one has to skip back and forth between definitions far apart in the file.

Instead, they can all go in a group call at the end. That won't strictly follow the ordering of the code under test, but I think the better grouping will be worth it.

gnprice · 2024-08-20T23:49:20Z

test/model/message_test.dart

+        checkPoll(message).question.equals('Old question');
+      });
+
+      test('ignore submessage event with malformed content', () async {


nit: move out of "question event" group, to go after "message has no submessages" above — this happens to say 'type': 'question' but isn't really about a particular type of poll event

gnprice · 2024-08-21T00:02:27Z

test/model/message_test.dart

+          }))))..length.equals(2)
+               ..last.contains('Malformed submessage event data for poll');


So, thinking about this checkLogs sort of test check now that I've reached this commit:

This feels to me like the sort of thing that's going to be quite brittle. (E.g. why length 2 — I guess the first is the one in PerAccountStore.handleEvent?) If we start widely adopting this pattern, then any time we add or remove a debugLog call there's going to be a bunch of random tests all over that need to be updated for it.

Also when we change the content of a debugLog call, we'll have to update the corresponding test. That's OK for a lot of things (if it's really the test aimed at the thing one is changing), but it makes changes more heavyweight. And these log messages are internal details, in the sense that they're aimed only at us the developers — they're not something our users, or any external system, ever see.

If we find a place where there's a debugLog call that we feel is really critical to ensure doesn't accidentally get broken, then that could be a good use case for something like this despite those costs. But I think this debugLog call is typical in that it isn't critical to that degree. Let's therefore leave this check out.

(As for the server tests:

This is inspired by `TestCase.assertLogs`, which is widely adapted in the Zulip server tests.

I have the sense that a lot of those uses are motivated mostly by the need to suppress the logs so they don't spew into the output of running the tests. (That's especially clear when they don't end up asserting anything about the logs emitted; and I suspect it's the only important motivation in many tests that do assert something about them too.) In this codebase we just handle that systematically using debugLogEnabled.

The other motivation for assertLogs checks in the server is that in the server, those log messages go into the production server log, which is an important operational tool for Zulip server operators including ourselves. That has somewhat the same shape as our debugLog calls for debugging… with the crucial difference that when we see debugLog output, the app is a debug build, and so we're presumably sitting there debugging it and can easily add more log lines as we wish and then hot-reload and repeat whatever we did. That's very different from the scenario where something went wrong an hour ago on one's live production server and you're frantically trying to pin down what happened.)

Thanks for the explanation! Dropping the commit.

Another motivation for adding checkLogs was to have a convenient way of verifying we expect to happen in our code -- too convenient that it relies heavily on implementation details. I agree that testing the externally visible interface is preferred.

PIG208 · 2024-08-21T07:05:27Z

I have updated the PR. In this update:

added some tests back to the commit that introduces Poll, before moving them later;
rearrange test grouping.

gnprice

Thanks for the revision! I haven't yet read the final commit closely:
42c9c10 poll: Support read-only poll widget UI.

but the rest all generally look good. A few comments below.

I'll also push a revision that I think demonstrates the main comment below, about the tests — please take a look and see if I'm missing something.

gnprice · 2024-08-21T22:14:29Z

lib/widgets/poll.dart

+    String getNameByUserId(int userId) =>
+        store.users[userId]?.fullName ?? zulipLocalizations.unknownUserName;


Ah, when at #885 (comment) I mentioned a local variable instead of a method:

It can probably just be a local variable in that build method, not a method or anything.

I was thinking of the result of map and join as a local variable.

I think that'd be a good further tweak — the widget expression below has a lot of details of layout, so it'd be good to move the more interesting computations out of it to happen above.

gnprice · 2024-08-21T22:29:30Z

lib/api/model/submessage.dart

+/// See also:
+/// - https://zulip.com/help/create-a-poll
+/// - https://github.com/zulip/zulip/blob/304d948416465c1a085122af5d752f03d6797003/web/shared/src/poll_data.ts
+class Poll {


I see, in the new revision this earlier commit adds tests in test/api/model/submessage_test.dart, and then the later commit moves them and adapts them to test/model/message_test.dart.

I don't really understand why the tests can't be in the latter form already in the earlier commit. The polls are there on the messages in the message store, right? So the tests that find them in the message store should work fine.

gnprice · 2024-08-21T22:32:19Z

lib/api/model/submessage.dart

+/// See also:
+/// - https://zulip.com/help/create-a-poll
+/// - https://github.com/zulip/zulip/blob/304d948416465c1a085122af5d752f03d6797003/web/shared/src/poll_data.ts
+class Poll {


If we really would have to move around and rewrite the tests like that, then it'd be better to squash the commits.

gnprice · 2024-08-21T22:35:37Z

test/model/message_test.dart

@@ -576,4 +584,346 @@ void main() {
        .reactions.isNotNull().jsonEquals([eg.unicodeEmojiReaction]);
    });
  });
+
+  group('handleSubmessageEvent', () {


Oh, one other nit:

This group isn't specific to handleSubmessageEvent — it's about interpreting Message objects that come in the first place with submessages, too, where SubmessageEvent may never be involved.

Gave the outer group a different name: "handle Poll related events". The name "handleSubmessageEvent" should be applicable to the subgroup of tests that do take care of SubmessageEvent. To parallel that, I renamed the other subgroup to "handleMessageEvent with initial submessages"

PIG208 · 2024-08-22T23:27:37Z

Pushed a minor formatting update.

gnprice · 2024-08-22T23:51:53Z

Thanks for the revision! Those changes all look good. All the commits before the last one are now ready, so I'm going to go ahead and merge them.

I won't make it to a review of the last commit today, though, before I go on vacation:
798f1b3 poll: Support read-only poll widget UI.

So please send that as a fresh PR. Let's also have @chrisbobbe do the first review on that — I know a similar commit was in #823 which he reviewed before, but it's been a bit of a marathon series of changes 🙂 (which is natural because this is a complex feature), so he may have new comments to make when looking at it with fresh eyes.

to match the order the events are defined. Signed-off-by: Zixuan James Li <[email protected]>

Semantically, we expect strings of a specific shape to be the key for options. This make it clearer to the reader without complicating things with extra validation. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

The goal is to make the relevant context of the test data local to the tests, and keep the example data as boring as possible. We also combine the smoke test into the test for invalid widget types, contrasting it with the failing cases. Signed-off-by: Zixuan James Li <[email protected]>

For now, we only consider the case when the submessages describe a poll, and disgard them otherwise. It will be a simple refactor later to support other Zulip widgets like todo lists, by extracting a common ancestors for such widget data structures. The `Poll` data structure will become useful when we support submessage events, where updates to a poll can happen. Some more comments are added here instead of earlier because of their references to `SubmessageData`. Signed-off-by: Zixuan James Li <[email protected]>

We could potentially avoid notifying listeners in some cases when handling submessage event, but it wouldn't be critically necessary. Signed-off-by: Zixuan James Li <[email protected]>

PIG208 changed the title ~~Support poll (read-only)~~ Support polls (read-only) Aug 12, 2024

PIG208 mentioned this pull request Aug 12, 2024

Prepare API models to support polls (read-only) #823

Merged

PIG208 force-pushed the poll-2 branch 2 times, most recently from be7ead6 to 3a0f853 Compare August 13, 2024 20:41

PIG208 added the integration review Added by maintainers when PR may be ready for integration label Aug 13, 2024

PIG208 requested a review from gnprice August 13, 2024 20:43

gnprice reviewed Aug 14, 2024

View reviewed changes

PIG208 force-pushed the poll-2 branch 2 times, most recently from 284f08b to 5b82669 Compare August 14, 2024 08:04

gnprice reviewed Aug 16, 2024

View reviewed changes

PIG208 force-pushed the poll-2 branch from 5b82669 to 0675ffd Compare August 16, 2024 23:05

PIG208 force-pushed the poll-2 branch from 0675ffd to 2aa7ec5 Compare August 17, 2024 06:02

PIG208 force-pushed the poll-2 branch 2 times, most recently from 9b4b999 to bf599f2 Compare August 17, 2024 06:22

gnprice reviewed Aug 21, 2024

View reviewed changes

PIG208 force-pushed the poll-2 branch from bf599f2 to ac199c5 Compare August 21, 2024 06:51

PIG208 requested a review from gnprice August 21, 2024 07:06

gnprice reviewed Aug 21, 2024

View reviewed changes

gnprice force-pushed the poll-2 branch from ac199c5 to 42c9c10 Compare August 21, 2024 22:33

gnprice reviewed Aug 21, 2024

View reviewed changes

PIG208 force-pushed the poll-2 branch 3 times, most recently from 213b802 to 3a216b2 Compare August 21, 2024 23:42

PIG208 force-pushed the poll-2 branch from 3a216b2 to 798f1b3 Compare August 22, 2024 23:27

PIG208 requested a review from gnprice August 22, 2024 23:27

PIG208 added 6 commits August 22, 2024 16:52

message test [nfc]: Move handleDeleteMessageEvent

6322484

to match the order the events are defined. Signed-off-by: Zixuan James Li <[email protected]>

submessage: Extract PollOptionKey.

1530978

Semantically, we expect strings of a specific shape to be the key for options. This make it clearer to the reader without complicating things with extra validation. Signed-off-by: Zixuan James Li <[email protected]>

submessage [nfc]: Minor wording change to a comment.

2d41e66

Signed-off-by: Zixuan James Li <[email protected]>

event: Handle submessage event for polls.

fd10d4f

We could potentially avoid notifying listeners in some cases when handling submessage event, but it wouldn't be critically necessary. Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the poll-2 branch from 798f1b3 to fd10d4f Compare August 22, 2024 23:52

gnprice merged commit fd10d4f into zulip:main Aug 22, 2024

		final Set<String> _optionNames = {};
		final Map<String, PollOption> _options = {};

	required final List<String> options,
	required List<String> options,

	final streamMessage = eg.streamMessage(id: 123);
	final message = eg.streamMessage(id: 123);

	return check(logHistory).isA<List<String>>();
	return check(logHistory!);

		static Map<String, Object?> _readPoll(Map<Object?, Object?> json, String key) {
		return {'data': json[key], 'sender_id': json['sender_id']};

		// It will start returning `Poll` as we support more Zulip widgets.
		static Poll? parseSubmessagesJson(List<Object?> json, {

		}))))..length.equals(2)
		..last.contains('Malformed submessage event data for poll');

		String getNameByUserId(int userId) =>
		store.users[userId]?.fullName ?? zulipLocalizations.unknownUserName;

Support polls (read-only) #885

Support polls (read-only) #885

Conversation

PIG208 commented Aug 12, 2024 • edited Loading

gnprice commented Aug 13, 2024

PIG208 commented Aug 13, 2024

gnprice 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Aug 14, 2024

gnprice 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

gnprice Aug 16, 2024 • 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

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

PIG208 commented Aug 16, 2024

PIG208 commented Aug 17, 2024 • edited Loading

gnprice 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

PIG208 commented Aug 21, 2024

gnprice 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

PIG208 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

PIG208 commented Aug 22, 2024

gnprice commented Aug 22, 2024

PIG208 commented Aug 12, 2024 •

edited

Loading

gnprice Aug 16, 2024 •

edited

Loading

PIG208 commented Aug 17, 2024 •

edited

Loading

PIG208 Aug 21, 2024 •

edited

Loading

PIG208 Aug 21, 2024 •

edited

Loading