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

Fix #534 respond support in view_submission listeners (it works only when response_url_enabled is true) #889

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Apr 19, 2021

Summary

This pull request resolves #534 by adding respond support in app.view listeners.

see also:

Requirements (place an x in each [ ])

…ks only when response_url_enabled is true)
@seratch seratch added enhancement M-T: A feature request for new functionality semver:minor labels Apr 19, 2021
@seratch seratch added this to the 3.4.0 milestone Apr 19, 2021
@seratch seratch self-assigned this Apr 19, 2021
@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #889 (4754b2a) into main (22f938e) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 4754b2a differs from pull request most recent head 88c9109. Consider uploading reports for the commit 88c9109 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #889      +/-   ##
==========================================
+ Coverage   66.11%   66.19%   +0.08%     
==========================================
  Files          13       13              
  Lines        1201     1204       +3     
  Branches      354      355       +1     
==========================================
+ Hits          794      797       +3     
  Misses        338      338              
  Partials       69       69              
Impacted Files Coverage Δ
src/App.ts 82.91% <100.00%> (+0.14%) ⬆️

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 22f938e...88c9109. Read the comment docs.

Comment on lines +770 to +771
// This can exist only when view_submission payloads - response_url_enabled: true
listenerArgs.respond = buildRespondFn(this.axios, body.response_urls[0].response_url);
Copy link
Member

Choose a reason for hiding this comment

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

Just for my own knowledge, are we only handling the first response_urls[0] because that keeps the functionality consistent with other areas of Bolt-JS that only handle the first response url?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mwbrooks Good point. As of today, the response_urls can have only one element and we don't have any plans to have more elements in the short term. That's the reason why we can safely pick up the first element here, and thus far we can provide respond utility, which is consistent with other areas.

If the array start having multiple options in the future, we still can provide respond with the first element. When a developer would like to use a non-primary element, the right way to do so is to select the one from the array and then use axios directly with the URL. We may want to provide an easy-to-use utility for it.

@seratch seratch merged commit 3db8da7 into slackapi:main Apr 21, 2021
@seratch seratch deleted the issue-534-respond-in-modals branch April 21, 2021 22:05
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 semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

respond support in view_submission listeners (it works only when response_url_enabled is true)
2 participants