Skip to content

Conversation

@seratch
Copy link
Contributor

@seratch seratch commented Feb 24, 2021

Summary

This pull request improves the default HTML page for successful Slack app installation. I got a feedback from a customer, saying the person uses only the browser version of Slack and the slack:// deep link navigation does not work for me at all. As this is a common use case, I see rooms for improvement on this feature.

We may want to apply the same (or similar) change to @slack/oauth npm package and the Java SDK's default behavior.

Category (place an x in each of the [ ])

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.models (UI component builders)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.rtm (RTM client)
  • slack_sdk.signature (Request Signature Verifier)
  • /docs-src (Documents, have you run ./docs.sh?)
  • /docs-src-v2 (Documents, have you run ./docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python setup.py validate after making the changes.

@seratch seratch added enhancement M-T: A feature request for new functionality Version: 3x oauth labels Feb 24, 2021
@seratch seratch added this to the 3.4.1 milestone Feb 24, 2021
@seratch seratch self-assigned this Feb 24, 2021
<body>
<h2>Thank you!</h2>
<p>Redirecting to the Slack App... click <a href="{url}">here</a></p>
<p>Redirecting to the Slack App... click <a href="{url}">here</a>. If you use the browser version of Slack, click <a href="{browser_url}" target="_blank">this link</a> instead.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change affecting users. I would like to know better sentences for this if you have suggestions.

app = Flask(__name__)
app.debug = True

import logging
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was just broken when I updated a few days ago 🤦

scopes=scopes,
user_scopes=user_scopes,
)
redirect_page_renderer = RedirectUriPageRenderer(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the samples to use the renderer and I've verified the behavior with these two example apps.

@seratch seratch force-pushed the add-browser-version-link branch from 9478b17 to 9b44e8d Compare February 24, 2021 06:14
@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #963 (9b44e8d) into main (633fb45) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #963      +/-   ##
==========================================
- Coverage   87.34%   87.33%   -0.02%     
==========================================
  Files          94       94              
  Lines        8521     8522       +1     
==========================================
  Hits         7443     7443              
- Misses       1078     1079       +1     
Impacted Files Coverage Δ
...k_sdk/oauth/redirect_uri_page_renderer/__init__.py 94.73% <100.00%> (+0.29%) ⬆️
slack_sdk/socket_mode/builtin/internals.py 72.72% <0.00%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 633fb45...9b44e8d. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement M-T: A feature request for new functionality oauth Version: 3x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants