Added Feature to handle reaction add & get message text of a reaction #28
Added Feature to handle reaction add & get message text of a reaction #28songupta7 wants to merge 5 commits intoExpediaGroup:masterfrom
Conversation
…o command - added failed go unit tests
…o command - added reaction files
| ``` | ||
| { "count": 50 , //default value is 100 mandatory | ||
| "message": "" , | ||
| "threadTimestamp":"...", | ||
| "reactionUser":"...", // mandatory list of reactions for a user | ||
| "channelId": "...", | ||
| "threadTimestamp":"..." | ||
| } | ||
| ``` |
There was a problem hiding this comment.
tabs/spaces seem to be off on this one. can you align to left with just one tab pelase?
| "threadTimestamp":"...", | ||
| "reactionUser":"...", // mandatory list of reactions for a user | ||
| "channelId": "...", | ||
| "threadTimestamp":"..." |
There was a problem hiding this comment.
threadTimestamp is mentioned twice 🤔
There was a problem hiding this comment.
Yes, this is typo I would rectify this.
| This command would retrieve the message text of reaction sent by a user. | ||
|
|
||
| ``` | ||
| { "count": 50 , //default value is 100 mandatory |
There was a problem hiding this comment.
Not sure yet what this is doing. What count means?
There was a problem hiding this comment.
The count of reactions in list sorted via timestamp. Hope this clarifies.
| ``` | ||
|
|
||
|
|
||
| ### GetReactionMessageInfo |
There was a problem hiding this comment.
Trying to understand use case for this one and what it actually does 🤔
There was a problem hiding this comment.
problem with the previous implementation: JSON standard does not allow such tokens.
To fix this, I have removed the ... token.
Trying to understand use case for this one and what it actually does 🤔
| } | ||
| func (m *MockClient) ListReactions(params slack.ListReactionsParameters) ([]slack.ReactedItem, *slack.Paging, error) { | ||
| params = params | ||
| //TODO mock the list reactions |
There was a problem hiding this comment.
No TODOs please. Please add tests for new functionality.
There was a problem hiding this comment.
I believe this is still hasn't been dealt with
pamelin
left a comment
pamelin
left a comment
There was a problem hiding this comment.
Please add relevant tests in the slack_test.go
| } | ||
|
|
||
| ### ReactionAdded | ||
| { |
There was a problem hiding this comment.
This seems to be wrong according to
type reactionAddedEvent struct {
ReactionUser user `json:"user"`
ReactionName string `json:"reaction"`
EventTimestamp string `json:"eventTimestamp"`
ItemType string `json:"type"`
ItemTimestamp string `json:"itemTimestamp"`
ItemUser user `json:"itemUser"`
ChannelId string `json:"channelId"`
}
There is no such thing as item and users are not simple string but objects/structs in itself
There was a problem hiding this comment.
There is some reason, which go slack module version are you referring to? I am referring to slack@v0.9.1>websocket_reactions.go contains below definition. Can you please cross check again?
// reactionItem is a lighter-weight item than is returned by the reactions list. type reactionItem struct { Type stringjson:"type"Channel stringjson:"channel,omitempty"File stringjson:"file,omitempty"FileComment stringjson:"file_comment,omitempty"Timestamp stringjson:"ts,omitempty"`
}
type reactionEvent struct {
Type string json:"type"
User string json:"user"
ItemUser string json:"item_user"
Item reactionItem json:"item"
Reaction string json:"reaction"
EventTimestamp string json:"event_ts"
}
// ReactionAddedEvent represents the Reaction added event
type ReactionAddedEvent reactionEvent
// ReactionRemovedEvent represents the Reaction removed event
type ReactionRemovedEvent reactionEvent
`
Co-authored-by: pamelin <pamelin@hotels.com>
Co-authored-by: pamelin <pamelin@hotels.com>
|
@songupta7 this has conflicts with master. Can you please pull the latest and update accordingly? One of the changes to pay attention to is removal of |
|
This should be 2 separate PRs:
There is no shared functionality between these 2. It is harder to review due to the need to switch context. Can they be split in 2 please? This way we can release quicker since both of them might have its own quirks which needs to be resolved. It will be quicker to handle one at the time. And when we relase these if anything wrong we know which part to blame for. |
|
ok @pamelin, I will split the PR in two parts and close this PR. I will also pull-in the changes recently done for zerologger. |
|
cool 👍 |
|
@songupta7 can this PR be closed now? |
|
Hi @pamelin I can close this after I get the review inputs on PR-33 which is created after breaking this PR into two parts. |
2 participants
What has been changed?
Please see the documentation for understanding the usage of these features.