Skip to content
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

Add reactions to conversation history #239

Merged
merged 5 commits into from
Aug 10, 2021

Conversation

aybbr
Copy link
Member

@aybbr aybbr commented Aug 9, 2021

Added reactions to the LiteMessage model to be able to retrieve the list or reactions attached to a slack message.

@aybbr aybbr requested a review from omotnyk August 9, 2021 11:46
Copy link
Contributor

@omotnyk omotnyk left a comment

Choose a reason for hiding this comment

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

Good job👍
I left a few small comments.

@JsonProperty("count")
Integer getCount();
@JsonProperty("users")
List<String> getUser();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this to getUsers? I imagine the list is returned when multiple users reacted with the same emoji.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we won't need JsonProperty annotation in this case.

Comment on lines 16 to 20
@JsonProperty("name")
String getName();
@JsonProperty("count")
Integer getCount();
@JsonProperty("users")
Copy link
Contributor

Choose a reason for hiding this comment

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

We tend to use @JsonProperty only if the name of the method is different from JSON key.
In this case, we don't need to name each property explicitly since for example count is automatically deserialized and stored in getCount. That's the magic of the immutables.

@JsonProperty("name")
String getName();
@JsonProperty("count")
Integer getCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

Integer -> int

Comment on lines +39 to +50
"reactions": [
{
"name": "astonished",
"count": 3,
"users": [ "U061F7AUR", "U062F7AUR", "U063F7AUR"]
},
{
"name": "facepalm",
"count": 34,
"users": [ "U061F7AUR", "U062F7AUR", "U063F7AUR", "U064F7AUR", "U065F7AUR" ]
}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these.
Can we also please make sure that these reactions are parsed in the corresponding integration test?

@aybbr
Copy link
Member Author

aybbr commented Aug 9, 2021

Thanks for the quick feedback @omotnyk. I addressed the previous comments so feel free to take a second look.

@aybbr aybbr requested a review from omotnyk August 9, 2021 13:45
Copy link
Contributor

@omotnyk omotnyk left a comment

Choose a reason for hiding this comment

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

Thanks for all your work. I have one small comment left but everything LGTM 🚢

Comment on lines 36 to 39
assertThat(response.getMessages().get(1).getReactions().size()).isEqualTo(2);
assertThat(response.getMessages().get(1).getReactions().get(0).getName()).isEqualTo("astonished");
assertThat(response.getMessages().get(1).getReactions().get(0).getCount()).isEqualTo(3);
assertThat(response.getMessages().get(3).getReactions().get(1).getUsers()).isEqualTo(Arrays.asList("U061F7AUR", "U062F7AUR", "U063F7AUR"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's either assert that facepalm reaction is deserialized correctly here as well or remove it from the JSON fixture.

@aybbr aybbr merged commit 4a75cc2 into master Aug 10, 2021
@aybbr aybbr deleted the add-reactions-to-conversation-history branch August 10, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants