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

Introduce reactions #428

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Introduce reactions #428

merged 3 commits into from
Aug 15, 2024

Conversation

rzadp
Copy link
Contributor

@rzadp rzadp commented Aug 14, 2024

Analogous to how the tip bot works, introduced reactions.

The user will get immediate feedback that the command has been received.

@rzadp rzadp requested a review from a team as a code owner August 14, 2024 10:41
Copy link
Contributor

@mordamax mordamax left a comment

Choose a reason for hiding this comment

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

👀👍

Comment on lines +124 to 128
sendReaction(roomId, eventId, "👀");
if (!arg0) {
logger.warn("Address not provided, skipping");
sendReaction(roomId, eventId, "😕");
sendMessage(roomId, `${sender} Please provide an address to be funded.`);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like there's no need to send 👀 reaction, if we're sending a 😕 right after.
Why not move sendReaction(roomId, eventId, "👀"); after synchronous checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, also thought about it.

In my pedantic mind it's more consistent to always have the same pattern:

  • seen 👀 -> 👍 OK
  • seen 👀 -> 😕 FAIL

Changing this would introduce a 😕 FAIL for a message that "has not been seen" in contrast with others.

Those were just my thoughts, don't know if it makes sense :D

@rzadp
Copy link
Contributor Author

rzadp commented Aug 14, 2024

Note: investigating the failing E2E tests. Not on issue with the PR, they fail on clean master as well.

Edit: Master fixed, now we have some actual failure on the PR.

@rzadp rzadp requested a review from mutantcornholio August 15, 2024 12:59
@rzadp rzadp merged commit 06040bf into main Aug 15, 2024
9 checks passed
@rzadp rzadp deleted the rzadp/reactions branch August 15, 2024 16:00
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.

3 participants