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.

Sign up for GitHub

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

Jump to bottom

Make text optional in chat.postMessage arguments and add warn logging if it's missing #1173

Merged
merged 4 commits into from
Mar 18, 2021
Merged

Make text optional in chat.postMessage arguments and add warn logging if it's missing #1173

merged 4 commits into from
Mar 18, 2021

Conversation

kentac55
Copy link
Contributor

Summary

Made text property of ChatPostMessageArguments optional.

close: #1172

Requirements (place an x in each [ ])

@gitwave gitwave bot added the untriaged label Feb 10, 2021
@kentac55 kentac55 marked this pull request as draft February 11, 2021 03:44
@kentac55 kentac55 marked this pull request as ready for review February 11, 2021 04:38
@kentac55
Copy link
Contributor Author

Note:

If we do postMessage without specifying either text or blocks attribute, there will be no problem with the type check, but the API will return no_text error.

To prevent this, we should use generic type like that I think.

@stevengill stevengill added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented and removed untriaged labels Feb 11, 2021
@seratch seratch added the pkg:web-api applies to `@slack/web-api` label Feb 16, 2021
@seratch
Copy link
Member

seratch commented Feb 16, 2021

Thanks a lot for taking the time to make this pull request 🙇 Can you check my comment in the issue? #1172 (comment)

@kentac55 kentac55 marked this pull request as draft February 16, 2021 14:02
@kentac55 kentac55 marked this pull request as ready for review February 20, 2021 06:12
@kentac55
Copy link
Contributor Author

@seratch added warning logic 🤗

Can you check this?

seratch
seratch requested changes Feb 20, 2021
View reviewed changes
Copy link
Member

@seratch seratch 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 updating! Please check my comment.

packages/web-api/src/WebClient.ts Outdated
* @param logger instance of we clients logger
*/
function warnMissingTextArgument(method: string, logger: Logger, options?: WebAPICallOptions): void {
const methodsWithOptionalText = ['chat.postMessage'];
Copy link
Member

Choose a reason for hiding this comment

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

Can you add other chat.* methods here like I implemented in Python SDK? https://github.com/slackapi/python-slack-sdk/pull/910/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for revieing!

added warning logic for other methods and updated test 😄

@kentac55 kentac55 force-pushed the fix/1172 branch 3 times, most recently from 9c4fef4 to 9ae0812 Compare February 20, 2021 13:01
@kentac55
Copy link
Contributor Author

@seratch Sorry for late response. I forgot to mention you 🙇

Can you check the new changes?

seratch
seratch requested changes Mar 3, 2021
View reviewed changes
packages/web-api/src/WebClient.ts
const methodsWithOptionalText = ['chat.postEphemeral', 'chat.postMessage', 'chat.scheduleMessage', 'chat.update'];
const isNeededToCheckMethod = methodsWithOptionalText.includes(method);

const isEmptyText = (args: WebAPICallOptions) =>
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, the implementation I mentioned had lack of consideration for attachments. Can you update this logic to check fallback in the case of attachments? Refer to slackapi/python-slack-sdk#966 for the logic.

@seratch
Copy link
Member

seratch commented Mar 15, 2021

@kentac55 Thanks for being patient here and updating the code for the attachments pattern! If you don't mind, could you rename the local variables like this? seratch@b963985

@kentac55
Copy link
Contributor Author

@seratch Sorry for late response 🙇

cherry-picked your commit 😃

seratch
seratch approved these changes Mar 15, 2021
View reviewed changes
Copy link
Member

@seratch seratch 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 working on this! The changes look great to me but I would like to have other maintainers' eyes before merging it.

@seratch
Copy link
Member

seratch commented Mar 18, 2021

OK, there is no concerns about this. Let me merge this PR. Thanks for your contribution! @kentac55

@seratch seratch merged commit 6003f63 into slackapi:main Mar 18, 2021
@seratch seratch changed the title fix type definition of ChatPostMessageArguments Make text optional in chat.postMessage arguments and add warn logging if it's missing Mar 24, 2021
This was referenced Mar 24, 2021
Merged
Closed
@seratch seratch added this to the web-api@6.2 milestone Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch approved these changes

Assignees
No one assigned
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:web-api applies to `@slack/web-api`
Projects
None yet
Milestone
web-api@6.2
Development

Successfully merging this pull request may close these issues.

type definition of ChatPostMessageArguments has incorrect property
3 participants
@kentac55 @seratch @stevengill