Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(chat/messages): Implement retrieving thread messages #230

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

martinalbert
Copy link
Contributor

Description

Extends use-case GetMessages in profile chat/messages with new fields:

  • listThreadMessages: boolean that represents whether to list messages from channel or thread destination.
  • threadId: string that represents thread id

Motivation and Context

Implement getting thread messages for slack.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING document.
  • I haven't repeated the code. (DRY)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

- add `listThreadMessages` boolean to enable slack map list thread messages
- add `threadId` string to pass in thread destination
- implement retrieving thread messages for slack map
- make discord map compatible with same format of input parameters as slack map
- filter parent messages of threads when listing thread messages
@martinalbert martinalbert requested review from jnv and janhalama March 23, 2022 14:26
Copy link
Contributor

@jnv jnv left a comment

Choose a reason for hiding this comment

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

TBH I think this approach is flawed, but maybe I misunderstand the context

  • listThreadMessages seems redundant since it always needs to be set together with threadId
  • But maybe it is needed and I can set threadId with listThreadMessages = false?
  • Would it make sense to put this rather into a separate use case?

@@ -3,7 +3,7 @@ Chat Messages
Retrieve chat messages.
"""
name = "chat/messages"
version = "1.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Additional properties are minor version change (new features).

@martinalbert
Copy link
Contributor Author

Discord is able to get thread messages passing threadId as destination but slack has to specify thread id along with it which is not required in profile.

I can create new use-case for it which would remove listThreadMessages but discord use-case would be duplicate of its usecase GetMessages 🤷‍♂️

@janhalama janhalama removed their request for review January 26, 2023 08:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants