Skip to content

Fix #260 Enable to use respond utility in app.view listeners (only when response_urls exists) - #288

Merged
seratch merged 2 commits into
slackapi:mainfrom
seratch:issue-260-response-urls
Apr 16, 2021
Merged

seratch merged 2 commits into
slackapi:mainfrom
seratch:issue-260-response-urls

Conversation

@seratch

@seratch seratch commented Apr 10, 2021

Copy link
Copy Markdown
Contributor

This pull request resolves #260 by enabling respond utility in view_submission listeners. Check the issue for learning the goal.

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

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements (place an x in each [ ])

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

  • 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 ./scripts/install_all_and_run_tests.sh after making the changes.

@seratch seratch added this to the 1.5.0 milestone Apr 10, 2021
@seratch seratch self-assigned this Apr 10, 2021

@seratch seratch left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

comments for reviewers

def build_async_context(
context: AsyncBoltContext,
payload: Dict[str, Any],
body: Dict[str, Any],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just for consistency with internals.py. This method name does not start with _ but this is part of internals source file. I think it's safe to change the name in the next minor release as no one relies on the argument name.

"app_installed_team_id": "T111",
"bot_id": "B111",
},
"response_urls": [

Copy link
Copy Markdown
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 field this pull request takes care of. This array can have an element only when a modal has a conversations_select block element with response_url_enabled: true option.

@codecov

codecov Bot commented Apr 10, 2021 •

Copy link
Copy Markdown

Codecov Report

Merging #288 (54dfe7f) into main (8babac6) will decrease coverage by 0.03%.
The diff coverage is 86.36%.

❗ Current head 54dfe7f differs from pull request most recent head 4480f96. Consider uploading reports for the commit 4480f96 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
- Coverage   91.35%   91.32%   -0.04%     
==========================================
  Files         164      164              
  Lines        5135     5151      +16     
==========================================
+ Hits         4691     4704      +13     
- Misses        444      447       +3     
Impacted Files Coverage Δ
slack_bolt/request/internals.py 92.80% <77.77%> (-1.05%) ⬇️
slack_bolt/request/async_internals.py 96.15% <92.30%> (-3.85%) ⬇️

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 8babac6...4480f96. Read the comment docs.

@mwbrooks mwbrooks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very straight forward addition and clean implementation! 👌🏻

@seratch

seratch commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

@naruhodou

naruhodou commented Sep 16, 2021 •

Copy link
Copy Markdown

Can you share any example on how to implement this in python, even one response url will work @seratch ?

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable to use respond utility in app.view listeners (only when response_urls exists)

3 participants