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

PIG208 · 2024-07-18T06:00:28Z

No description provided.

gnprice

Cool! Looking forward to seeing the full version of this PR.

One high-level comment below, and one other:

Normally in our API code we just link to API docs and that covers the need for documentation. Here there aren't adequate API docs; so

Please link instead to whatever sources you've made use of to work out what the API is — e.g. the web implementation, the zulip-mobile implementation, the server implementation.
There are some doc comments on the API types in zulip-mobile. Those can I think be copied over mostly verbatim to describe the API semantics here — they're mostly playing the role that the docs at https://zulip.com/api/ normally play for us.

gnprice · 2024-07-18T16:40:45Z

lib/api/model/model.dart

@@ -570,6 +573,46 @@ enum MessageFlag {
  String toJson() => _$MessageFlagEnumMap[this]!;
 }

+/// https://zulip.com/api/get-messages#response
+@JsonSerializable(fieldRename: FieldRename.snake)
+class Submessage {


Let's put this in the same file as the details of polls, and call it submessage.dart.

That way it's a bit more general than just polls, and makes a natural home for todo lists when we add those in the future.

Once we have this feature all working, I'd like to avoid thinking about submessages as much as possible. 🙂 And one generally can do that — they don't come up except when specifically working on polls. So it's helpful to have this type not be in between these core message classes that come up all over the Zulip app model.

gnprice

Two other comments, from just taking a quick look today.

gnprice · 2024-07-20T03:52:32Z

lib/api/model/model.dart

+  // Only null when processed by store.
+  // See [Polls].
+  @JsonKey(disallowNullValue: true, required: true)
+  List<Submessage>? submessages;


Let's actually store null here in the case where the server gave an empty list, too. That's an optimization because it means less keeping around of a lot of empty lists (each of which is its own separate data structure allocated somewhere).

An existing place we use that optimization is reactions here on the same Message class. Not a coincidence that's also on Message, because that's a type we have tons of lying around and so where there's incentive to make this kind of optimization.

gnprice · 2024-07-20T03:57:31Z

lib/api/model/model.dart

+  @JsonKey(readValue: WidgetData.readFromMessage)
+  WidgetData? widgetData;


This needs a doc comment, because it doesn't correspond directly to anything in the Zulip API (and so the Zulip API docs linked from the class's doc don't fill that role).

PIG208 · 2024-07-23T00:08:45Z

I revamped the poll model and make it a part of the message object. Previously, the plan was to have a dedicated centralized store (like unreads) for polls, where we maintain a map keyed by the message ID. The old design has several downsides:

submessages do not get further deserialized until it is processed by the dedicated store, while the intermediate Submessage objects are not useful.
In contrast to unreads or typingStatus, polls have a one-to-one relationship to messages. Making them a property on Message simplifies the implementation.
It just seems more appropriate to handle poll events inside MessageStore, so that we don't maintain methods like handleMessageEvent and reconcileMessages that would be needed for a separate store.

PIG208 · 2024-07-23T00:10:38Z

The new approach will set submessages to null as soon as the constructor of Message is called. The constructor takes a look at the widget type and decides if the message contains a poll. If so, the converts the submessages into a singular Poll object and store that on the message.

PIG208 · 2024-07-25T01:20:26Z

UI Preview

PIG208 · 2024-08-10T00:51:53Z

This update should have addressed most comments except the ones I commented on. Will get back to finishing off those later.

PIG208 · 2024-08-10T06:47:46Z

Pushed again to remove preprocessing on Submessage.content and SubmessageEvent.content, and to add the workaround mentioned in #823 (comment).

gnprice

Thanks for the revision! Here's a complete review of the first few commits, describing the bulk of the API:
0d7a464 api [nfc]: Avoid the use of dynamic on Message.
47cc913 api: Add Submessage.
451dad9 submessage: Add SubmessageData classes for polls.

Those are close to merge now; the comments below are mostly small. I've inserted three additional commits for changes that were simpler to handle that way:
721e0d8 api [nfc]: Expand comments on Submessage; put fields in logical order
afd3c9b api: Give defaults to question/options on PollWidgetExtraData
9a975b4 api [nfc]: Expand comments on SubmessageData and subclasses

Then let's split up this PR, since the thread has gotten long. So after acting on the comments below, please drop the remaining commits from this PR and send them as a follow-up PR:
2043822 api: Construct polls data store from submessages.
de62dba event: Handle submessage event for polls.
acbd557 (optional) test: Add a test helper for inspecting logs.
6b94ed3 poll: Support read-only poll widget UI.

gnprice · 2024-08-11T23:12:09Z

test/api/model/submessage_test.dart

+void main() {
+  group('Message.submessages', () {
+    test('no crash on unrecognized submessage type', () {
+      final submessageJson = {


nit: this sounds like it should be valid JSON for a submessage, but it isn't quite; "base" is a good term for what it actually is, which is some JSON we'll use to construct JSON for submessages

Suggested change

final submessageJson = {

final baseJson = {

gnprice · 2024-08-11T23:14:32Z

test/example_data.dart

@@ -614,6 +631,29 @@ ReactionEvent reactionEvent(Reaction reaction, ReactionOp op, int messageId) {
  );
 }

+final pollWidgetDataFavoriteLetter = {


nit: should appear in the commit that starts using it

gnprice · 2024-08-11T23:15:31Z

test/example_data.dart

@@ -614,6 +631,29 @@ ReactionEvent reactionEvent(Reaction reaction, ReactionOp op, int messageId) {
  );
 }

+final pollWidgetDataFavoriteLetter = {


Suggested change

final pollWidgetDataFavoriteLetter = {

const pollWidgetDataFavoriteLetter = {

so this doesn't get mutated by one test, causing another to fail very confusingly

separate nit: this isn't about events but rather messages, so should go in the messages section of the file; right next to eg.submessage would be a good spot

gnprice · 2024-08-11T23:23:01Z

lib/api/model/submessage.dart

+  UnsupportedWidgetData({required this.json});
+
+  final Object? json;


nit: keep ordering consistent

Suggested change

UnsupportedWidgetData({required this.json});

final Object? json;

final Object? json;

UnsupportedWidgetData({required this.json});

gnprice · 2024-08-11T23:24:28Z

lib/api/model/submessage.dart

+  @JsonKey(includeToJson: true)
+  WidgetType get widgetType => WidgetType.unknown;
+
+  UnsupportedWidgetData({required this.json});


nit: can keep to the same pattern as other WidgetData subclasses:

Suggested change

UnsupportedWidgetData({required this.json});

UnsupportedWidgetData.fromJson(this.json);

(compare UnexpectedEvent as an example)

gnprice · 2024-08-11T23:51:45Z

lib/api/model/submessage.dart

+  /// The index of last [option] added by the sender.
+  @JsonKey(name: 'idx')
+  final int latestOptionIndex;


This doesn't really mean [option], as in the option field of this class, does it? (I'm not sure what that would mean if it did.) So it should just say "option" rather than "[option]".

Then what does it mean that it's the index of the last option (or "latest", in the field name) added by the sender? Does that mean it's not the index of this option itself — the one that this submessage is about adding — but some other option instead? (Like the last one this sender added before this one?)

After reading through some of the related code, I think what this really wants to say is it's an index, or more concretely a sequence number, for this option… but counting among only the options added by this same sender.

So let's stick with the API's name — "latest" seems misleading, and "option" is redundant, leaving "index" which would in itself be clearer than "idx" but not enough to justify the discrepancy from the server API. And then explain in a comment what sort of "index" is meant:

Suggested change

/// The index of last [option] added by the sender.

@JsonKey(name: 'idx')

final int latestOptionIndex;

/// A sequence number for this option, among options added to this poll

/// by this [Submessage.senderId].

///

/// See [PollEventSubmessage.optionKey].

final int idx;

Also expanded this to optionIndex -> idx of PollEventSubmessage.optionKey.

gnprice · 2024-08-12T00:04:30Z

lib/api/model/submessage.dart

+            option.voters.add(senderId);
+          case PollVoteOp.remove:
+            option.voters.remove(senderId);
+          case PollVoteOp.unknown:


Suggested change

case PollVoteOp.unknown:

case PollVoteOp.unknown: // TODO(log)

return;

gnprice · 2024-08-12T00:09:57Z

lib/api/model/submessage.dart

+
+/// As in [PollEventSubmessage.type].
+@JsonEnum(fieldRename: FieldRename.snake)
+enum PollEventSubmessageType {


nit: let's put this right after PollEventSubmessage — it's closely tied to that base class (for example the fromJson there has a switch that's on exactly these values), and it provides the outline of what the subclasses will be.

gnprice · 2024-08-12T00:33:19Z

lib/api/model/submessage.dart

+  final String question;
+  final List<String> options;


I think it's best to match the existing clients' behavior — that simplifies away any attempt to verify that unexpected shapes of data never happen. I've added a commit to do that:
afd3c9b api: Give defaults to question/options on PollWidgetExtraData

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

The PR has been updated. Split the second half of this PR to #885. Thanks for the review @gnprice!

gnprice

Thanks for the revision! One nit below, and then this will be ready to merge.

gnprice · 2024-08-13T04:57:17Z

lib/api/model/submessage.dart

+
+  final Map<String, Object?> json;
+
+  UnknownPollEventSubmessage({required this.json});


nit: same as #823 (comment)

PIG208 · 2024-08-13T18:49:42Z

Updated the PR.

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

We are not storing the list of `Submessage`s directly on the Message objects as they are pretty obscure until we further parse them into meaningful data structures for things like polls and todos. Signed-off-by: Zixuan James Li <[email protected]>

The sealed class `SubmessageData` is not actually in use, we could potentially implement a discriminator utilizing the sealed class to deserialize individual submessage content, but it is far easier to do so when we have access to the full list of submessages. `SubmessageData` is there for self-documentation. It is also worth noting that much of these class definitions are based on previous reverse engineering effort and the web implementation. See: - https://github.com/zulip/zulip/blob/304d948416465c1a085122af5d752f03d6797003/web/shared/src/poll_data.ts - https://github.com/zulip/zulip-mobile/blob/2217c858e207f9f092651dd853051843c3f04422/src/api/modelTypes.js#L800-L861 Due to the flexibility of the submessage API, these classes tend to be intentionally defensive against unknown or invalid values. Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-08-13T18:56:35Z

Thanks! Looks good — merging.

PIG208 force-pushed the poll-read-only branch 2 times, most recently from 4b914be to 8ce33d0 Compare July 18, 2024 06:51

gnprice reviewed Jul 18, 2024

View reviewed changes

PIG208 force-pushed the poll-read-only branch 4 times, most recently from 48f73c3 to ea4617c Compare July 19, 2024 22:40

gnprice reviewed Jul 20, 2024

View reviewed changes

PIG208 force-pushed the poll-read-only branch 6 times, most recently from c794815 to c5f3103 Compare July 22, 2024 23:57

PIG208 force-pushed the poll-read-only branch 9 times, most recently from 2d0ef6b to e6e333f Compare July 23, 2024 23:13

PIG208 mentioned this pull request Jul 25, 2024

content [nfc]: Use Divider instead of DecoratedBox. #836

Closed

PIG208 force-pushed the poll-read-only branch 2 times, most recently from a2bddaf to 1701b2c Compare July 25, 2024 01:17

PIG208 force-pushed the poll-read-only branch from 1701b2c to 7754309 Compare July 25, 2024 01:23

PIG208 force-pushed the poll-read-only branch 2 times, most recently from 18b1f75 to c6edba9 Compare August 10, 2024 06:47

PIG208 force-pushed the poll-read-only branch 5 times, most recently from 0a9cf7f to 6b94ed3 Compare August 10, 2024 07:19

gnprice force-pushed the poll-read-only branch from 6b94ed3 to 632fa30 Compare August 12, 2024 00:33

gnprice reviewed Aug 12, 2024

View reviewed changes

PIG208 force-pushed the poll-read-only branch 2 times, most recently from a1334af to 3ad8035 Compare August 12, 2024 19:35

PIG208 changed the title ~~Support Polls (read-only)~~ Prepare API models to support polls (read-only) Aug 12, 2024

PIG208 mentioned this pull request Aug 12, 2024

Support polls (read-only) #885

Merged

PIG208 force-pushed the poll-read-only branch from 3ad8035 to 3109e3e Compare August 12, 2024 20:03

gnprice reviewed Aug 13, 2024

View reviewed changes

PIG208 force-pushed the poll-read-only branch from 3109e3e to f9dca65 Compare August 13, 2024 18:49

PIG208 and others added 6 commits August 13, 2024 11:54

api [nfc]: Avoid the use of dynamic on Message.

1248c89

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

api: Add Submessage.

0b51072

We are not storing the list of `Submessage`s directly on the Message objects as they are pretty obscure until we further parse them into meaningful data structures for things like polls and todos. Signed-off-by: Zixuan James Li <[email protected]>

api [nfc]: Expand comments on Submessage; put fields in logical order

819a44a

api: Give defaults to question/options on PollWidgetExtraData

9938b27

api [nfc]: Expand comments on SubmessageData and subclasses

06ce603

gnprice force-pushed the poll-read-only branch from f9dca65 to 06ce603 Compare August 13, 2024 18:56

gnprice merged commit 06ce603 into zulip:main Aug 13, 2024

PIG208 deleted the poll-read-only branch August 13, 2024 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PIG208 commented Jul 18, 2024 •

edited

Loading

gnprice left a comment

gnprice Jul 18, 2024

gnprice left a comment

gnprice Jul 20, 2024

gnprice Jul 20, 2024

PIG208 commented Jul 23, 2024

PIG208 commented Jul 23, 2024

PIG208 commented Jul 25, 2024 •

edited

Loading

PIG208 commented Aug 10, 2024

PIG208 commented Aug 10, 2024

gnprice left a comment

gnprice Aug 11, 2024

gnprice Aug 11, 2024

gnprice Aug 11, 2024

gnprice Aug 11, 2024

gnprice Aug 11, 2024

gnprice Aug 11, 2024

gnprice Aug 11, 2024

gnprice Aug 11, 2024

PIG208 Aug 12, 2024

gnprice Aug 12, 2024

gnprice Aug 12, 2024

gnprice Aug 12, 2024

PIG208 commented Aug 12, 2024

gnprice left a comment

gnprice Aug 13, 2024

PIG208 commented Aug 13, 2024

gnprice commented Aug 13, 2024

		@JsonKey(readValue: WidgetData.readFromMessage)
		WidgetData? widgetData;

	final pollWidgetDataFavoriteLetter = {
	const pollWidgetDataFavoriteLetter = {

		UnsupportedWidgetData({required this.json});

		final Object? json;

	UnsupportedWidgetData({required this.json});
	UnsupportedWidgetData.fromJson(this.json);

-  /// The index of last [option] added by the sender.
-  @JsonKey(name: 'idx')
-  final int latestOptionIndex;
+  /// A sequence number for this option, among options added to this poll
+  /// by this [Submessage.senderId].
+  ///
+  /// See [PollEventSubmessage.optionKey].
+  final int idx;

	case PollVoteOp.unknown:
	case PollVoteOp.unknown: // TODO(log)
	return;


		final Map<String, Object?> json;

		UnknownPollEventSubmessage({required this.json});

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

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

Conversation

PIG208 commented Jul 18, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

PIG208 commented Jul 23, 2024

PIG208 commented Jul 23, 2024

PIG208 commented Jul 25, 2024 • edited Loading

PIG208 commented Aug 10, 2024

PIG208 commented Aug 10, 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

PIG208 commented Aug 12, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Aug 13, 2024

gnprice commented Aug 13, 2024

PIG208 commented Jul 18, 2024 •

edited

Loading

PIG208 commented Jul 25, 2024 •

edited

Loading