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

feat: make it work with OAuth 2 #4

Merged
merged 47 commits into from
May 31, 2024
Merged

Conversation

WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented May 16, 2024

Type of change

  • New sample
  • New feature
  • Bug fix
  • Documentation

Summary

This PR aims to implement a working version of the bolt python jira functions app

Testing

Follow the steps in the README.md

Requirements

  • I’ve checked my submission against the Samples Checklist to ensure it complies with all standards
  • I have ensured the changes I am contributing align with existing patterns and have tested and linted my code
  • I've read and agree to the Code of Conduct

@WilliamBergamin WilliamBergamin added the enhancement New feature or request label May 16, 2024
@WilliamBergamin WilliamBergamin requested a review from misscoded May 16, 2024 20:29
@WilliamBergamin WilliamBergamin self-assigned this May 16, 2024
@WilliamBergamin WilliamBergamin requested review from seratch and zimeg May 29, 2024 17:09
@WilliamBergamin WilliamBergamin marked this pull request as ready for review May 29, 2024 17:09
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Great work! Left a few comments

oauth/installation_store/installation_store.py Outdated Show resolved Hide resolved
oauth/installation_store/models.py Outdated Show resolved Hide resolved
oauth/state_store/memory.py Outdated Show resolved Hide resolved
oauth/state_store/models.py Outdated Show resolved Hide resolved
.sample.env Outdated Show resolved Hide resolved
internals.py Outdated Show resolved Hide resolved
internals.py Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
jira/client.py Outdated Show resolved Hide resolved
listeners/actions/disconnect_account.py Outdated Show resolved Hide resolved
@seratch
Copy link
Member

seratch commented May 30, 2024

Once my comments are all resolved, anyone else can approve this PR and you can go ahead with merging it!

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

This is structured super well! I imagine this'll be a very very helpful sample for showcasing a few very common functionalities. Really nice 👏

I agree with all of the previous comments and left a few of my own, but nothing new is blocking. Just a few nits and questions.

In the process of testing this but got caught at the localhost step 😵‍💫 But I did get as far as getting to the dev JIRA app and starting the localhost process. The app does run without error too! Hope to finish the review soon, but it's all looking good right now! 🚀

.github/dependabot.yml Show resolved Hide resolved
.github/workflows/lint.yml Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
manifest.json Show resolved Hide resolved
.sample.env Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.sample.env Outdated Show resolved Hide resolved
tests/functions/test_create_issue.py Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@WilliamBergamin WilliamBergamin requested review from zimeg and seratch May 30, 2024 21:14
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all of the feedback so quickly! 🚀 This is looking super solid to me so approving now ✅ but feel free to merge :octocat: whenever!

Comment on lines +23 to +26
mrkdwn: str = (
":no_entry: There is a problem with the `Create an issue` step, you did not connect a Jira Account, "
f"visit the <{CONTEXT.app_home_page_url}|App Home Page> to fix this!"
)
Copy link
Member

Choose a reason for hiding this comment

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

🙌 niceee!

@@ -97,7 +93,7 @@ def test_create_issue_fail(self):
mock_fail = MagicMock()
mock_complete = MagicMock()
mock_context = MagicMock(team_id=self.team_id, enterprise_id=self.enterprise_id)
mock_client = MagicMock(chat_postMessage=lambda channel, text: True)
mock_client = Mock(spec=WebClient)
Copy link
Member

Choose a reason for hiding this comment

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

This is super interesting! Nice update 👏

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thank you so much for quickly updating this PR! I left a comment on the global singleton access, but it's up to you this time!


def disconnect_account_callback(ack: Ack, context: BoltContext):
ack()
CONTEXT.jira_installation_store.delete_installation(
Copy link
Member

Choose a reason for hiding this comment

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

Accessing the contextual properties through BoltContext (rather than directly accessing a global singleton) would be even cleaner, but it's okay so far. If we come to think that we need more flexibility for it, we can do refactoring then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've thought about this but can't seem to figure out an elegant to implement it

I'll merge this PR as is since it may be needed very soon, but I would very much appreciate an example of this in order to open a follow up PR

@WilliamBergamin WilliamBergamin merged commit 76a1409 into main May 31, 2024
5 checks passed
@WilliamBergamin WilliamBergamin deleted the Initial-poc-with-workflow branch May 31, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants