Skip to content

Support polls (read-only) #885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import 'package:json_annotation/json_annotation.dart';
import '../../model/algorithms.dart';
import 'json.dart';
import 'model.dart';
import 'submessage.dart';

part 'events.g.dart';

Expand Down Expand Up @@ -63,6 +64,7 @@ sealed class Event {
case 'remove': return UpdateMessageFlagsRemoveEvent.fromJson(json);
default: return UnexpectedEvent.fromJson(json);
}
case 'submessage': return SubmessageEvent.fromJson(json);
case 'typing': return TypingEvent.fromJson(json);
case 'reaction': return ReactionEvent.fromJson(json);
case 'heartbeat': return HeartbeatEvent.fromJson(json);
Expand Down Expand Up @@ -942,6 +944,41 @@ class UpdateMessageFlagsMessageDetail {
Map<String, dynamic> toJson() => _$UpdateMessageFlagsMessageDetailToJson(this);
}

/// A Zulip event of type `submessage`: https://zulip.com/api/get-events#submessage
@JsonSerializable(fieldRename: FieldRename.snake)
class SubmessageEvent extends Event {
@override
@JsonKey(includeToJson: true)
String get type => 'submessage';

@JsonKey(unknownEnumValue: SubmessageType.unknown)
final SubmessageType msgType;
/// [SubmessageData] encoded in JSON.
// We cannot parse the String into one of the [SubmessageData] classes because
// information from other submessages are required. Specifically, we need
// the parsed [WidgetData] from the first [Message.submessages] of the
// corresponding message.
final String content;
final int messageId;
final int senderId;
final int submessageId;

SubmessageEvent({
required super.id,
required this.msgType,
required this.content,
required this.messageId,
required this.senderId,
required this.submessageId,
});

factory SubmessageEvent.fromJson(Map<String, dynamic> json) =>
_$SubmessageEventFromJson(json);

@override
Map<String, dynamic> toJson() => _$SubmessageEventToJson(this);
}

/// A Zulip event of type `typing`:
/// https://zulip.com/api/get-events#typing-start
/// https://zulip.com/api/get-events#typing-stop
Expand Down
27 changes: 27 additions & 0 deletions lib/api/model/events.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import 'package:json_annotation/json_annotation.dart';
import 'events.dart';
import 'initial_snapshot.dart';
import 'reaction.dart';
import 'submessage.dart';

export 'json.dart' show JsonNullable;
export 'reaction.dart';
Expand Down Expand Up @@ -533,6 +534,9 @@ sealed class Message {
final String senderRealmStr;
@JsonKey(name: 'subject')
String topic;
/// Poll data if "submessages" describe a poll, `null` otherwise.
@JsonKey(name: 'submessages', readValue: _readPoll, fromJson: Poll.fromJson, toJson: Poll.toJson)
Poll? poll;
final int timestamp;
String get type;

Expand Down Expand Up @@ -564,6 +568,13 @@ sealed class Message {
return list.map((raw) => MessageFlag.fromRawString(raw as String)).toList();
}

static Poll? _readPoll(Map<Object?, Object?> json, String key) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this and the related methods to live on Poll instead? (And/or Submessage?)

That'd be helpful for the sake of keeping the polls stuff out of the way when one is reading about messages, and not specifically thinking about the polls case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved them to [Submessage.parseSubmessagesJson], where we can also parse other types of Zulip widget in the future.

Copy link
Member Author

@PIG208 PIG208 Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved fromJson and toJson to Poll. toJson remains as a static method, and _readPoll is kept at where it was (now it just calls Submessage.parseSubmessagesJson).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return Submessage.parseSubmessagesJson(
json['submessages'] as List<Object?>? ?? [],
messageSenderId: (json['sender_id'] as num).toInt(),
);
}

Message({
required this.client,
required this.content,
Expand Down
6 changes: 4 additions & 2 deletions lib/api/model/model.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

177 changes: 173 additions & 4 deletions lib/api/model/submessage.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import 'dart:convert';

import 'package:json_annotation/json_annotation.dart';

import '../../log.dart';
import 'events.dart';

part 'submessage.g.dart';

/// Data used for Zulip "widgets" within messages, like polls and todo lists.
Expand Down Expand Up @@ -38,9 +43,33 @@ class Submessage {
// We cannot parse the String into one of the [SubmessageData] classes because
// information from other submessages are required. Specifically, we need:
// * the index of this submessage in [Message.submessages];
// * the [WidgetType] of the first [Message.submessages].
// * the parsed [WidgetType] from the first [Message.submessages].
final String content;

/// Parse a JSON list into a [Poll].
// TODO: Use a generalized return type when supporting other Zulip widgets.
static Poll? parseSubmessagesJson(List<Object?> json, {
required int messageSenderId,
}) {
final submessages = json.map((e) => Submessage.fromJson(e as Map<String, Object?>)).toList();
if (submessages.isEmpty) return null;

assert(submessages.first.senderId == messageSenderId);

final widgetData = WidgetData.fromJson(jsonDecode(submessages.first.content));
switch (widgetData) {
case PollWidgetData():
return Poll.fromSubmessages(
widgetData: widgetData,
pollEventSubmessages: submessages.skip(1),
messageSenderId: messageSenderId,
);
case UnsupportedWidgetData():
assert(debugLog('Unsupported widgetData: ${widgetData.json}'));
return null;
}
}

factory Submessage.fromJson(Map<String, Object?> json) =>
_$SubmessageFromJson(json);

Expand All @@ -65,7 +94,9 @@ enum SubmessageType {
/// * Zero or more submessages with content [PollEventSubmessage] if the
/// message is a poll (i.e. if the first submessage was a [PollWidgetData]),
/// and similarly for other types of widgets.
sealed class SubmessageData {}
sealed class SubmessageData {
Object? toJson();
}

/// The data encoded in a submessage to make the message a Zulip widget.
///
Expand All @@ -87,6 +118,7 @@ sealed class WidgetData extends SubmessageData {
};
}

@override
Object? toJson();
}

Expand Down Expand Up @@ -173,7 +205,7 @@ sealed class PollEventSubmessage extends SubmessageData {
///
/// For options that are a part of the initial [PollWidgetData], the
/// [senderId] should be `null`.
static String optionKey({required int? senderId, required int idx}) =>
static PollOptionKey optionKey({required int? senderId, required int idx}) =>
// "canned" is a canonical constant coined by the web client:
// https://github.com/zulip/zulip/blob/40f59a05c/web/shared/src/poll_data.ts#L238
'${senderId ?? 'canned'},$idx';
Expand All @@ -188,6 +220,7 @@ sealed class PollEventSubmessage extends SubmessageData {
}
}

@override
Map<String, Object?> toJson();
}

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

typedef PollOptionKey = String;

/// A poll event when an option is added.
///
/// See: https://github.com/zulip/zulip/blob/40f59a05c/web/shared/src/poll_data.ts#L112-L159
Expand Down Expand Up @@ -264,7 +299,7 @@ class PollVoteEventSubmessage extends PollEventSubmessage {
/// The key of the affected option.
///
/// See [PollEventSubmessage.optionKey].
final String key;
final PollOptionKey key;
@JsonKey(name: 'vote', unknownEnumValue: PollVoteOp.unknown)
final PollVoteOp op;

Expand Down Expand Up @@ -313,3 +348,137 @@ class UnknownPollEventSubmessage extends PollEventSubmessage {
@override
Map<String, Object?> toJson() => json;
}

/// States of a poll Zulip widget.
///
/// See also:
/// - https://zulip.com/help/create-a-poll
/// - https://github.com/zulip/zulip/blob/304d948416465c1a085122af5d752f03d6797003/web/shared/src/poll_data.ts
class Poll {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be tested in a later commit.

Why does the test wait for a later commit?

Copy link
Member Author

@PIG208 PIG208 Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been inclined to squash the handleSubmessageEvent commit with this commit since last update, because all the tests here would be moved to the later commit.

An alternative would be retrofitting the updated tests in the earlier commit with as little change as necessary (to make the diffs look better), then moving it. This way we have atomicity in both commits.

I think it makes sense to leave the isolated exercises of _applyEvent in the later commit that introduces submessage events, because that's where we have a public interface to access _applyEvent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@PIG208 PIG208 Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I overlooked the fact that those MessageEvent-based tests are not blocked by SubmessageEvent. My plan in an earlier version of this revision was to reintroduce all tests, including the applyEvent ones, in their rewritten form without event handling. I agree that leaving submessage_test.dart out and having applicable tests in message_test.dart from the very beginning is a better decision.

/// Construct a poll from submessages.
///
/// For a poll Zulip widget, the first submessage's content contains a
/// [PollWidgetData], and all the following submessages' content each contains
/// a [PollEventSubmessage].
factory Poll.fromSubmessages({
required PollWidgetData widgetData,
required Iterable<Submessage> pollEventSubmessages,
required int messageSenderId,
}) {
final poll = Poll._(
messageSenderId: messageSenderId,
question: widgetData.extraData.question,
options: widgetData.extraData.options,
);

for (final submessage in pollEventSubmessages) {
final event = PollEventSubmessage.fromJson(jsonDecode(submessage.content) as Map<String, Object?>);
poll._applyEvent(submessage.senderId, event);
}
return poll;
}

Poll._({
required this.messageSenderId,
required this.question,
required List<String> options,
}) {
for (int index = 0; index < options.length; index += 1) {
// Initial poll options use a placeholder senderId.
// See [PollEventSubmessage.optionKey] for details.
_addOption(senderId: null, idx: index, option: options[index]);
}
}

final int messageSenderId;
String question;

/// 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;

Iterable<PollOption> get options => _options.values;
/// Contains the text of all options from [_options].
final Set<String> _existingOptionTexts = {};
final Map<PollOptionKey, PollOption> _options = {};

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

void _applyEvent(int senderId, PollEventSubmessage event) {
switch (event) {
case PollNewOptionEventSubmessage():
_addOption(senderId: senderId, idx: event.idx, option: event.option);

case PollQuestionEventSubmessage():
if (senderId != messageSenderId) {
// Only the message owner can edit the question.
assert(debugLog('unexpected poll data: user $senderId is not allowed to edit the question')); // TODO(log)
return;
}

question = event.question;

case PollVoteEventSubmessage():
final option = _options[event.key];
if (option == null) {
assert(debugLog('vote for unknown key ${event.key}')); // TODO(log)
return;
}

switch (event.op) {
case PollVoteOp.add:
option.voters.add(senderId);
case PollVoteOp.remove:
option.voters.remove(senderId);
case PollVoteOp.unknown:
assert(debugLog('unknown vote op ${event.op}')); // TODO(log)
}

case UnknownPollEventSubmessage():
}
}

void _addOption({required int? senderId, required int idx, required String option}) {
if (idx > _maxIdx || idx < 0) return;

// The web client suppresses duplicate options, which can be created through
// the /poll command as there is no server-side validation.
if (_existingOptionTexts.contains(option)) return;

final key = PollEventSubmessage.optionKey(senderId: senderId, idx: idx);
assert(!_options.containsKey(key));
_options[key] = PollOption(text: option);
_existingOptionTexts.add(option);
}

static Poll? fromJson(Object? json) {
// [Submessage.parseSubmessagesJson] does all the heavy lifting for parsing.
return json as Poll?;
}

static List<Submessage> toJson(Poll? poll) {
// Rather than maintaining a up-to-date submessages list, return as if it is
// empty, because we are not sending the submessages to the server anyway.
return [];
}
}

class PollOption {
PollOption({required this.text});

final String text;
final Set<int> voters = {};

@override
String toString() => 'PollOption(text: $text, voters: {${voters.join(', ')}})';
}
Loading