api: Add add-reaction and remove-reaction routes #272
Conversation
lib/api/route/messages.dart
Outdated
| Future<AddReactionResult> addReaction(ApiConnection connection, { | ||
| required int messageId, | ||
| required ReactionType reactionType, | ||
| required String emojiCode, | ||
| required String emojiName, | ||
| }) { |
There was a problem hiding this comment.
Maybe addReaction and removeReaction could take a Reaction object instead of reactionType, emojiCode, and emojiName individually. Would that be better, I wonder?
lib/api/model/model.dart
/// As in [Message.reactions].
@JsonSerializable(fieldRename: FieldRename.snake)
class Reaction {
final String emojiName;
final String emojiCode;
final ReactionType reactionType;
final int userId;
// final Map<String, dynamic> user; // deprecated; ignore
Reaction({
required this.emojiName,
required this.emojiCode,
required this.reactionType,
required this.userId,
});
factory Reaction.fromJson(Map<String, dynamic> json) =>
_$ReactionFromJson(json);
Map<String, dynamic> toJson() => _$ReactionToJson(this);
@override
String toString() => 'Reaction(emojiName: $emojiName, emojiCode: $emojiCode, reactionType: $reactionType, userId: $userId)';
}There was a problem hiding this comment.
Maybe not better: the Reaction instance would be just a one-off wrapper for those three fields, and the new bindings don't need Reaction's userId field.
There was a problem hiding this comment.
Yeah, I agree that wouldn't be better, because the userId wouldn't belong.
There was a problem hiding this comment.
Thanks! Generally looks good; comments below.
Also a commit-message nit:
ApiConnection: Implement `delete` method
I'd prefer to keep this under the "api" prefix (like previous changes found in git log lib/api/core.dart). So e.g. "api: Add ApiConnection.delete".
lib/api/route/messages.dart
Outdated
| 'reaction_type': RawParameter(switch (reactionType) { | ||
| ReactionType.unicodeEmoji => 'unicode_emoji', | ||
| ReactionType.realmEmoji => 'realm_emoji', | ||
| ReactionType.zulipExtraEmoji => 'zulip_extra_emoji', |
There was a problem hiding this comment.
Hmm, awkward to have to repeat this switch. Can we get json_serializable to do this for us?
Yeah, looks like we can say this:
zulipExtraEmoji;
String toJson() => _$ReactionTypeEnumMap[this]!;
}and then I think this can become RawParameter(reactionType.toJson()).
lib/api/route/messages.dart
Outdated
| } | ||
|
|
||
|
|
||
| /// https://zulip.com/api/remove-reaction |
There was a problem hiding this comment.
nit:
| } | |
| /// https://zulip.com/api/remove-reaction | |
| } | |
| /// https://zulip.com/api/remove-reaction |
lib/api/route/messages.dart
Outdated
| class AddReactionResult { | ||
| AddReactionResult(); |
There was a problem hiding this comment.
Hmm, this class doesn't really do anything, does it :-)
I think when there's no content in the response, it's probably cleanest to have the route binding just return Future<void>. The fromJson callback can look like (_) {}.
lib/api/route/messages.dart
Outdated
| Future<AddReactionResult> addReaction(ApiConnection connection, { | ||
| required int messageId, | ||
| required ReactionType reactionType, | ||
| required String emojiCode, | ||
| required String emojiName, | ||
| }) { |
There was a problem hiding this comment.
Yeah, I agree that wouldn't be better, because the userId wouldn't belong.
test/example_data.dart
Outdated
| @@ -195,6 +195,20 @@ Reaction unicodeEmojiReaction = Reaction( | |||
| userId: selfUser.userId, | |||
| ); | |||
|
|
|||
| Reaction realmEmoji = Reaction( | |||
There was a problem hiding this comment.
| Reaction realmEmoji = Reaction( | |
| Reaction realmEmojiReaction = Reaction( |
This object isn't just an emoji, but an emoji reaction. In particular it specifies a user ID, which isn't part of specifying an emoji.
chrisbobbe
force-pushed
the
pr-api-add-remove-reaction
branch
from
ed3268c to
c66b920
Compare
|
Thanks for the review! Revision pushed. |
| 'reaction_type': _$ReactionTypeEnumMap[instance.reactionType]!, | ||
| 'reaction_type': instance.reactionType, |
There was a problem hiding this comment.
Hmm interesting. I guess that makes sense.
We'll call this directly soon, when we'll want reusable code to translate an enum value into a 'snake_case' string, for API bindings for the add- and remove-reaction endpoints. With a `toJson` method, instances can now be handled by `encodeJson`. That's nice, but not really a goal here; we don't have an imminent need to pass an instance through `encodeJson`. Anyway, it means that json_serializable's generated _$ReactionToJson and _$ReactionEventToJson functions don't need the enum-to-string conversion on their reactionType fields in order for jsonEncode to accept those functions' output. And it looks like json_serializable has removed that conversion in both functions. So, adjust our code to not depend on it.
gnprice
force-pushed
the
pr-api-add-remove-reaction
branch
from
c66b920 to
7e798bb
Compare
Related: #125