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

The implementation of #1208 is different from the node-slack-sdk version #1302

Closed
mar3mar3 opened this issue Nov 17, 2022 · 5 comments · Fixed by #1303
Closed

The implementation of #1208 is different from the node-slack-sdk version #1302

mar3mar3 opened this issue Nov 17, 2022 · 5 comments · Fixed by #1303
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 3x web-client
Milestone

Comments

@mar3mar3
Copy link
Contributor

mar3mar3 commented Nov 17, 2022

Original specification in node-slack-sdk
slackapi/node-slack-sdk#1476 (comment)

The implementation of the pull request created with reference to the above was different.
#1208

The following condition in node-slack-sdk is different.
Even if there is no top-level text, the warning is not displayed if there is a fallback in all attachments.
https://github.com/slackapi/node-slack-sdk/blob/bcbe7cc8753a134d73daf88ac66eec0f359c2df7/packages/web-api/src/WebClient.ts#L868

Reproducible in:

The Slack SDK version

slack-sdk==3.19.2

Python runtime version

Python 3.11.0

OS info

ProductName: macOS
ProductVersion: 12.6
BuildVersion: 21G115
Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10 PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64

Steps to reproduce:

For example...

from slack_sdk import WebClient
import os

slack_token = os.environ["SLACK_BOT_TOKEN"]
client = WebClient(token=slack_token)

attachments = [
    {
        'fallback': 'title-message-fallback',
        'color': 'red',
        'fields': [
            {
                'title': 'title',
                'value': 'message',
            }]
    }
]

result = client.chat_postMessage(
    channel="C0XXXXXX",
    attachments=attachments,
    as_user=True)

Expected result:

No warnings are displayed.

Actual result:

The following warning is displayed.

UserWarning: The top-level `text` argument is missing in the request payload for a chat.postMessage call - It’s a best practice to always provide a `text` argument when posting a message. The `text` argument is used in places where content cannot be rendered such as: system push notifications, assistive technology such as screen readers, etc.
  warnings.warn(message, UserWarning)

maybe...

the following implementation?

    # if text argument is missing, Warn the user about this.
    # However, do not warn if a fallback field exists for all attachments, since this can be substituted.
    missing_text_message  = (
        f"The top-level `text` argument is missing in the request payload for a {endpoint} call - "
        f"It's a best practice to always provide a `text` argument when posting a message. "
        f"The `text` argument is used in places where content cannot be rendered such as: "
        "system push notifications, assistive technology such as screen readers, etc."
    )

    # https://api.slack.com/reference/messaging/attachments
    # Check if the fallback field exists for all the attachments
    # Not all attachments have a fallback property; warn about this too!
    missing_fallback_message = (
        f"Additionally, the attachment-level `fallback` argument is missing in the request payload for a {endpoint} call"
        f" - To avoid this warning, it is recommended to always provide a top-level `text` argument when posting a"
        f" message. Alternatively you can provide an attachment-level `fallback` argument, though this is now considered"
        f" a legacy field (see https://api.slack.com/reference/messaging/attachments#legacy_fields for more details)."
    )

    # Additionally, specifically for attachments, there is a legacy field available at the attachment level called `fallback`
    # Even with a missing text, one can provide a `fallback` per attachment.
    # More details here: https://api.slack.com/reference/messaging/attachments#legacy_fields
    attachments = kwargs.get("attachments")
    # Note that this method does not verify attachments
    # if the value is already serialized as a single str value.
    if (
        attachments is not None
        and isinstance(attachments, list)
    ):
        if (not all(
                [isinstance(attachment, dict)
                and len(attachment.get("fallback", "").strip()) > 0 for attachment in attachments]
            )
        ):
            warnings.warn(missing_text_message, UserWarning)
            warnings.warn(missing_fallback_message, UserWarning)
    else:
        warnings.warn(missing_text_message, UserWarning)
@eddyg
Copy link
Contributor

eddyg commented Nov 17, 2022

I was just looking at this as well! This warning used to be suppressed properly; it would be nice to get this fixed.

FWIW, I tested your proposed code change and the warning went away. 👍

@zimeg zimeg added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented web-client Version: 3x and removed untriaged labels Nov 17, 2022
@zimeg zimeg added this to the 3.19.5 milestone Nov 17, 2022
@zimeg
Copy link
Member

zimeg commented Nov 17, 2022

Hi @mar3mar3! Thank you for writing this in and sharing your implementation for a fix! This is definitely something we'll want to patch in a future release. If you'd want to make a PR with your changes, that'd be welcomed! Otherwise, we'll work on fixing this soon!

@seratch
Copy link
Member

seratch commented Nov 18, 2022

@mar3mar3 Thanks for flagging this! The initial implementation across three bolt frameworks were consistent but recently Node SDK code has been improved by slackapi/node-slack-sdk#1529. However, we haven't updated Python and Java SDKs. If you are fine to use your time more on this, your pull request would be greatly appreciated. Otherwise, I will resolve it quickly later on.

@mar3mar3
Copy link
Contributor Author

@eddyg
Thanks, the code I fixed seems to be working fine there.

@E-Zim @seratch
Thanks, I guess I was right.
Now I'll get ready to make my pull request.

@mar3mar3
Copy link
Contributor Author

I made #1303.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 3x web-client
Projects
None yet
4 participants