Add `Reaction` class; uncomment `Message.reactions` and update on events #256

chrisbobbe · 2023-08-03T18:32:41Z

Related: #121

chrisbobbe · 2023-08-03T18:34:18Z

lib/api/model/events.dart

+/// A Zulip event of type `reaction`.
+abstract class ReactionEvent extends Event {
+  @override
+  @JsonKey(includeToJson: true)
+  String get type => 'reaction';
+
+  String get op;
+
+  ReactionEvent({required super.id});
+}


All of ReactionAddEvent's and ReactionRemoveEvent's fields are the same except op… do you think it would be better to deduplicate by putting the fields on this ReactionEvent base class?

Yeah, I think so. I think the agreement in list of fields is not really a coincidence: this reactions data model is designed so that all these fields give the coordinates of one single hypothetical reaction, and then the only things that can happen are that that hypothetical reaction is created or deleted.

Potentially even go further: just have ReactionEvent, make it concrete, and give it final ReactionOp op; where that's an enum with values add/remove.

Relatedly, in the consuming code, I think we don't care whether it's an add or a remove until relatively late in handling it. For example MessageListView can sensibly have just one method that takes a ReactionEvent and does the appropriate thing (which means PerAccountStore.handleEvent doesn't care about the details), and then that method can go use messageId to find the relevant message (and potentially determine it doesn't have it and there's nothing to do) and only after finding the message does it consult op.

chrisbobbe · 2023-08-03T20:50:29Z

Updated, resolving small conflicts with #238.

gnprice

Thanks! Comments below.

I think it'll likely make most sense to merge this as part of a PR that's also handling the events. That way we see the whole chain of how they're used, which is helpful for that question about how far to unify the two variants of the event type.

gnprice · 2023-08-03T22:55:02Z

lib/api/model/model.dart

+  final String emojiCode;
+  final ReactionType reactionType;
+  final int userId;
+  // final Map<String, dynamic> user // deprecated; ignore


nit:

Suggested change

// final Map<String, dynamic> user // deprecated; ignore

// final Map<String, dynamic> user; // deprecated; ignore

gnprice · 2023-08-03T22:55:19Z

lib/api/model/model.dart

+@JsonSerializable(fieldRename: FieldRename.snake)
+class Reaction {


Suggested change

@JsonSerializable(fieldRename: FieldRename.snake)

class Reaction {

/// As in [Message.reactions].

@JsonSerializable(fieldRename: FieldRename.snake)

class Reaction {

This way there's a breadcrumb trail to find the relevant API docs.

gnprice · 2023-08-03T22:58:15Z

lib/api/model/events.dart

+/// A Zulip event of type `reaction`.
+abstract class ReactionEvent extends Event {


Breadcrumbs to API docs:

Suggested change

/// A Zulip event of type `reaction`.

abstract class ReactionEvent extends Event {

/// A Zulip event of type `reaction`.

///

/// The corresponding API docs are in several places for

/// different values of `op`; see subclasses.

abstract class ReactionEvent extends Event {

(Copying language I added just today in StreamEvent and RealmUserEvent.)

gnprice · 2023-08-03T23:10:47Z

lib/api/model/events.dart

+/// A Zulip event of type `reaction`.
+abstract class ReactionEvent extends Event {
+  @override
+  @JsonKey(includeToJson: true)
+  String get type => 'reaction';
+
+  String get op;
+
+  ReactionEvent({required super.id});
+}


Yeah, I think so. I think the agreement in list of fields is not really a coincidence: this reactions data model is designed so that all these fields give the coordinates of one single hypothetical reaction, and then the only things that can happen are that that hypothetical reaction is created or deleted.

Potentially even go further: just have ReactionEvent, make it concrete, and give it final ReactionOp op; where that's an enum with values add/remove.

Relatedly, in the consuming code, I think we don't care whether it's an add or a remove until relatively late in handling it. For example MessageListView can sensibly have just one method that takes a ReactionEvent and does the appropriate thing (which means PerAccountStore.handleEvent doesn't care about the details), and then that method can go use messageId to find the relevant message (and potentially determine it doesn't have it and there's nothing to do) and only after finding the message does it consult op.

chrisbobbe · 2023-08-04T21:16:35Z

Thanks for the review! Revision pushed. This one updates the reactions field on Message events tracked in MessageListViews, in response to events.

If you spot ways to make the tests more concise, please let me know. 🙂

gnprice

Thanks! A couple of comments below.

On how to make the tests more concise, one thought is below. Then there's a variety of small things most of which apply to the existing tests in that file. For those, I think I'll just go try to work up now a small PR, which may be the most efficient way to describe them.

gnprice · 2023-08-04T21:37:16Z

lib/model/message_list.dart

+      case ReactionOp.add:
+        message.reactions.add(reaction);
+      case ReactionOp.remove:
+        message.reactions.removeWhere((r) => r == reaction);


Hmm interesting. This differs from what zulip-mobile does (in messagesReducer.js):

reactions: oldMessage.reactions.filter( x => !(x.emoji_name === action.emoji_name && x.user_id === action.user_id), ),

which is to match only on (message and) user ID and emoji name.

The API docs at https://zulip.com/api/get-events#reaction-remove aren't real clear on how to interpret this aspect.

In the server database, the unique key is user, message, reaction type, and emoji code:

class AbstractReaction(AbstractEmoji): class Meta: abstract = True unique_together = ("user_profile", "message", "reaction_type", "emoji_code")

(Seems like that should really be message first, then user — without an index that begins with the message, a query that asks for all the reactions on a given message or set of messages is going to be slow. But the order of columns in the key only affects performance, not semantics.)

That suggests that that's what we should key on as well, in order to correctly simulate what we'd expect to see if we fetched the same message from scratch.

And indeed that looks to be what web does too.

So yeah, let's match on user ID, reaction type, and emoji code here. If there's a match on those but the name was different, we should remove it.

gnprice · 2023-08-04T21:41:29Z

test/model/message_list_test.dart

+        final event = ReactionEvent(
+          id: 1,
+          op: ReactionOp.add,
+          emojiName: eg.unicodeEmojiReaction.emojiName,
+          emojiCode: eg.unicodeEmojiReaction.emojiCode,


One way to make the tests more concise would be to make a local helper function to create these ReactionEvent objects, which could take a Reaction as a single argument. (Plus can skip the id parameter.)

gnprice · 2023-08-04T23:20:12Z

Then there's a variety of small things most of which apply to the existing tests in that file. For those, I think I'll just go try to work up now a small PR, which may be the most efficient way to describe them.

Sent #258.

Related: zulip#121

chrisbobbe · 2023-08-07T22:27:00Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Just small comments remaining, all on the tests.

gnprice · 2023-08-07T23:32:45Z

test/api/model/model_checks.dart

@@ -5,15 +5,43 @@ extension MessageChecks on Subject<Message> {
  Subject<Map<String, dynamic>> get toJson => has((e) => e.toJson(), 'toJson');

  void jsonEquals(Message expected) {
-    toJson.deepEquals(expected.toJson());
+    final expected_ = expected.toJson();


How about expectedJson — that way the name expresses how it differs from the other "expected".

gnprice · 2023-08-07T23:39:01Z

test/api/model/model_checks.dart

@@ -5,15 +5,43 @@ extension MessageChecks on Subject<Message> {
  Subject<Map<String, dynamic>> get toJson => has((e) => e.toJson(), 'toJson');

  void jsonEquals(Message expected) {
-    toJson.deepEquals(expected.toJson());
+    final expected_ = expected.toJson();
+    expected_['reactions'] = it()..isA<List<Reaction>>().jsonEquals(expected.reactions);


Hmm, interesting.

This feels a bit more cumbersome to me than the toDeepJson (deepToJson?) approach we discussed in the office on Friday, where in test code we'd have an extension like Message.deepToJson that had the effect of a recursive toJson. But this approach seems fine.

(I might experiment afterward with the deepToJson approach, either ad hoc for Message or the generic version that we sketched.)

Sure, that approach makes sense; please do go ahead with those experiments if you'd like. 🙂 I may have forgotten some points we discussed on Friday, but I got stuck thinking about how to make it nice to compare two List<Reaction>s in a test, when you don't necessarily want to compare two whole Message objects. And the new ReactionsChecks on Subject<List<Reaction>> in this revision seemed convenient for that, and also as a helper for the Message.jsonEquals function you've highlighted here.

gnprice · 2023-08-08T00:06:44Z

test/model/message_list_test.dart

+        final messageList = await messageListViewWithMessages([originalMessage], stream, narrow);
+
+        final message = messageList.messages.single;
+        check(message).reactions.not(it()..jsonEquals([eg.unicodeEmojiReaction]));


I think this more direct version would be fine here:

Suggested change

check(message).reactions.not(it()..jsonEquals([eg.unicodeEmojiReaction]));

check(message).reactions.jsonEquals([]);

The reader can compare that with the jsonEquals below and see that the expected values are clearly different — one's empty and one's a singleton.

gnprice · 2023-08-08T00:09:15Z

test/model/message_list_test.dart

+      }
+
+      test('add reaction', () async {
+        final originalMessage = eg.streamMessage(id: 243, stream: stream, reactions: []);


Can we drop id here? I think we can.

(Same is probably true of some of the test cases above; I guess that's a cleanup I missed in #258.)

gnprice · 2023-08-08T00:10:23Z

test/model/message_list_test.dart

+          emojiName: 'wave',                  emojiCode: '1f44b', userId: 1);
+
+        // Same emoji, different user. Not to be removed.
+        final reaction2 = Reaction.fromJson(eventReaction.toJson()..update('user_id', (_) => 2));


Suggested change

final reaction2 = Reaction.fromJson(eventReaction.toJson()..update('user_id', (_) => 2));

final reaction2 = Reaction.fromJson(eventReaction.toJson()

..update('user_id', (_) => 2));

(to make the parallelism clear with the others below)

gnprice · 2023-08-08T00:12:38Z

test/model/message_list_test.dart

+        final originalMessage = eg.streamMessage(id: 243, stream: stream,
+          reactions: [eventReaction, reaction2, reaction3, reaction4]);


To make this consistent with non-buggy server behavior, this should leave out eventReaction — that can't coexist with reaction4. (Because they have the same message, user, reaction type, and emoji code, and in the database those form a unique key.)

gnprice · 2023-08-08T00:14:40Z

test/model/message_list_test.dart

+        final messageList = await messageListViewWithMessages([originalMessage], stream, narrow);
+
+        final message = messageList.messages.single;
+        check(message).reactions.not(it()..jsonEquals([reaction2, reaction3]));


Yeah, this test is definitely clearer by stating what it expects the starting point to be, rather than that it's different from the expected ending point. :-)

I think in fact this is clear enough if you just leave this check out — the definition of originalMessage is a few lines earlier and seems equally explicit.

I think in fact this is clear enough if you just leave this check out

Could the same be said for the other added tests, I wonder:

test('add reaction', () async { final originalMessage = eg.streamMessage(stream: stream, reactions: []); final messageList = await messageListViewWithMessages([originalMessage], stream, narrow); final message = messageList.messages.single; check(message).reactions.jsonEquals([]);

test('add reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: []); final messageList = await messageListViewWithMessages([someMessage], stream, narrow); check(messageList.messages.single).reactions.jsonEquals([]);

test('remove reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: [eg.unicodeEmojiReaction]); final messageList = await messageListViewWithMessages([someMessage], stream, narrow); check(messageList.messages.single).reactions.jsonEquals([eg.unicodeEmojiReaction]);

Yeah, I think so.

gnprice · 2023-08-08T00:22:07Z

test/model/message_list_test.dart

+        final reaction4 = Reaction.fromJson(eventReaction.toJson()
+          ..update('emoji_name', (_) => 'tools'));


I think this aspect doesn't affect the substance of the test, but for realism (which can be helpful for a reader): tools should correspond to the same emoji code as working_on_it, rather than the same as wave.

We want the emojiCode to match the one in the event, but we want emojiName not to match, right. Since this is the reaction in the event—

final eventReaction = Reaction(reactionType: ReactionType.unicodeEmoji, emojiName: 'wave', emojiCode: '1f44b', userId: 1);

—how about this for reaction4:

final reaction4 = Reaction.fromJson(eventReaction.toJson() ..update('emoji_name', (_) => 'hello'));

? 🙂

chrisbobbe · 2023-08-08T03:47:13Z

Thanks for the review! Revision pushed, and I made some replies inline above.

gnprice · 2023-08-08T23:14:23Z

test/model/message_list_test.dart

+        // So we mimic that behavior; see discussion:
+        //   https://github.com/zulip/zulip-flutter/pull/256#discussion_r1284865099
+        final reaction4 = Reaction.fromJson(eventReaction.toJson()
+          ..update('emoji_name', (_) => 'hello'));


This line (and similar ones above) can be simplified with a convenient variant of the .. cascade syntax:

Suggested change

..update('emoji_name', (_) => 'hello'));

..['emoji_name'] = 'hello');

That is, the []= operator can be used in cascade form too, much like a setter can be.

Interesting, thanks! I thought I tried that, but I guess I didn't try it properly. 🤷‍♂️

gnprice · 2023-08-08T23:15:17Z

test/model/message_list_test.dart

+        final messageList = await messageListViewWithMessages([originalMessage], stream, narrow);
+
+        final message = messageList.messages.single;
+        check(message).reactions.not(it()..jsonEquals([reaction2, reaction3]));


Yeah, I think so.

gnprice · 2023-08-08T23:16:14Z

Thanks!

All looks good; small nits above. Please go ahead and merge whenever ready.

We don't yet have UI to show the events (zulip#121), but now at least we're keeping our Message objects up-to-date with reactions. Related: zulip#121

chrisbobbe · 2023-08-08T23:26:02Z

Thanks for the review!

Please go ahead and merge whenever ready.

Merging, with those changes.

chrisbobbe added a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) labels Aug 3, 2023

chrisbobbe requested a review from gnprice August 3, 2023 18:32

chrisbobbe commented Aug 3, 2023

View reviewed changes

chrisbobbe force-pushed the wip-reaction-events branch from 828324b to 80192ae Compare August 3, 2023 20:50

gnprice reviewed Aug 3, 2023

View reviewed changes

chrisbobbe changed the title ~~Add Reaction class; uncomment Message.reactions; represent reaction events~~ Add Reaction class; uncomment Message.reactions and update on events Aug 4, 2023

chrisbobbe force-pushed the wip-reaction-events branch from 80192ae to 272c366 Compare August 4, 2023 21:15

gnprice reviewed Aug 4, 2023

View reviewed changes

gnprice mentioned this pull request Aug 4, 2023

Clean up and tighten UpdateMessageEvent tests #258

Merged

model: Add [Reaction] class; uncomment [Message.reactions]

2fd0c4c

Related: zulip#121

chrisbobbe force-pushed the wip-reaction-events branch from 272c366 to 1da3c2f Compare August 7, 2023 22:25

gnprice reviewed Aug 8, 2023

View reviewed changes

chrisbobbe force-pushed the wip-reaction-events branch from 1da3c2f to e119b93 Compare August 8, 2023 03:46

gnprice reviewed Aug 8, 2023

View reviewed changes

api: Add reaction events

15099f4

We don't yet have UI to show the events (zulip#121), but now at least we're keeping our Message objects up-to-date with reactions. Related: zulip#121

chrisbobbe force-pushed the wip-reaction-events branch from e119b93 to 15099f4 Compare August 8, 2023 23:25

chrisbobbe merged commit 15099f4 into zulip:main Aug 8, 2023

chrisbobbe deleted the wip-reaction-events branch August 8, 2023 23:26

gnprice mentioned this pull request Aug 9, 2023

test: Write a generic jsonEquals check and deepToJson function #259

Merged

gnprice mentioned this pull request Nov 29, 2023

☂️ Handle all Zulip events to keep our data up to date #135

Open

72 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Reaction` class; uncomment `Message.reactions` and update on events #256

Add `Reaction` class; uncomment `Message.reactions` and update on events #256

chrisbobbe commented Aug 3, 2023

chrisbobbe Aug 3, 2023

gnprice Aug 3, 2023

chrisbobbe commented Aug 3, 2023

gnprice left a comment

gnprice Aug 3, 2023

gnprice Aug 3, 2023

gnprice Aug 3, 2023

gnprice Aug 3, 2023

chrisbobbe commented Aug 4, 2023 •

edited

Loading

gnprice left a comment

gnprice Aug 4, 2023

gnprice Aug 4, 2023

gnprice commented Aug 4, 2023

chrisbobbe commented Aug 7, 2023

gnprice left a comment

gnprice Aug 7, 2023

gnprice Aug 7, 2023

chrisbobbe Aug 8, 2023

gnprice Aug 9, 2023

gnprice Aug 8, 2023

gnprice Aug 8, 2023

gnprice Aug 8, 2023

gnprice Aug 8, 2023

gnprice Aug 8, 2023

chrisbobbe Aug 8, 2023

gnprice Aug 8, 2023

gnprice Aug 8, 2023

chrisbobbe Aug 8, 2023

chrisbobbe commented Aug 8, 2023

gnprice Aug 8, 2023

chrisbobbe Aug 8, 2023

gnprice Aug 8, 2023

gnprice commented Aug 8, 2023

chrisbobbe commented Aug 8, 2023

	// final Map<String, dynamic> user // deprecated; ignore
	// final Map<String, dynamic> user; // deprecated; ignore

		@JsonSerializable(fieldRename: FieldRename.snake)
		class Reaction {

		/// A Zulip event of type `reaction`.
		abstract class ReactionEvent extends Event {

	check(message).reactions.not(it()..jsonEquals([eg.unicodeEmojiReaction]));
	check(message).reactions.jsonEquals([]);

	final reaction2 = Reaction.fromJson(eventReaction.toJson()..update('user_id', (_) => 2));
	final reaction2 = Reaction.fromJson(eventReaction.toJson()
	..update('user_id', (_) => 2));

		final originalMessage = eg.streamMessage(id: 243, stream: stream,
		reactions: [eventReaction, reaction2, reaction3, reaction4]);

		final reaction4 = Reaction.fromJson(eventReaction.toJson()
		..update('emoji_name', (_) => 'tools'));

	..update('emoji_name', (_) => 'hello'));
	..['emoji_name'] = 'hello');

Add Reaction class; uncomment Message.reactions and update on events #256

Add Reaction class; uncomment Message.reactions and update on events #256

Conversation

chrisbobbe commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 3, 2023

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

chrisbobbe commented Aug 4, 2023 • 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

gnprice commented Aug 4, 2023

chrisbobbe commented Aug 7, 2023

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

chrisbobbe commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Aug 8, 2023

chrisbobbe commented Aug 8, 2023

Add `Reaction` class; uncomment `Message.reactions` and update on events #256

Add `Reaction` class; uncomment `Message.reactions` and update on events #256

chrisbobbe commented Aug 4, 2023 •

edited

Loading