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 #664 Django example installation store improvement #665

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

seratch
Copy link
Member

@seratch seratch commented Jun 2, 2022

This pull request resolves #664 by improving the installation store in Django example app.

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 bug Something isn't working area:examples issues related to example or sample code labels Jun 2, 2022
@seratch seratch added this to the 1.14.1 milestone Jun 2, 2022
@seratch seratch self-assigned this Jun 2, 2022
@seratch seratch force-pushed the issue-664-django branch from e4ac644 to d66619f Compare June 2, 2022 05:34
@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #665 (d66619f) into main (3457fb1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #665   +/-   ##
=======================================
  Coverage   92.05%   92.05%           
=======================================
  Files         172      172           
  Lines        5864     5864           
=======================================
  Hits         5398     5398           
  Misses        466      466           

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 3457fb1...d66619f. Read the comment docs.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM, I added one suggestion to a comment in the example to help potential future maintainers understand the workaround introduced here.

@@ -145,6 +139,24 @@ def find_installation(

if len(rows) > 0:
i = rows[0]
if user_id is not None:
# Fetch the latest bot token
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we add a bit more context to this comment to help remind future maintainers about this workaround.

Suggested change
# Fetch the latest bot token
# Above, when fetching applicable installations, we retrieved relevant installations but filtered on matching user ID
# In the case that this app uses both bot and user scopes, we also now fetch installations that match only on team/enterprise ID and contain a bot token
# If any such results are retrieved, we merged them into the user-id-filtered results.
# This is a fix for the situation described in https://github.com/slackapi/bolt-python/issues/664

Copy link

@DataGreed DataGreed left a comment

Choose a reason for hiding this comment

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

this PR fixes #664 for me

@srajiang srajiang merged commit 854208f into slackapi:main Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:examples issues related to example or sample code bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

One user gets "Although the app should be installed into this workspace", while other does not.
4 participants