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

Slack channel fixes #11305

Merged
merged 6 commits into from
Jul 8, 2022
Merged

Slack channel fixes #11305

merged 6 commits into from
Jul 8, 2022

Conversation

Maxinho96
Copy link
Contributor

@Maxinho96 Maxinho96 commented Jul 6, 2022

Proposed changes:

  • Fixed response when slack sends retries (None was giving error)
  • Running on_new_message as an asyncio background task instead of a blocking await (so that we immediately return 200 to Slack and we don't reach the 3 seconds timeout if the action execution is slow)

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@Maxinho96 Maxinho96 requested a review from a team as a code owner July 6, 2022 13:48
@Maxinho96 Maxinho96 requested review from sanchariGr and removed request for a team July 6, 2022 13:48
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

@Maxinho96 Thank you for your contribution 🙌🏻
Looks great 💯 Before I approve this PR, I would like you to:

  • add a unit test that fails when you revert your changes and passes with your fixes (the unit test should be placed under tests/core/channels/test_slack.py)
  • add a changelog entry in the changelog directory, you can use the PR id as changelog entry ID, please use type bugfix.

Also please note that since your PR is targeting main, this change won't be released until the next minor and that is tentatively scheduled towards the end of Q3. Alternatively, you can recreate your PR to target 3.2.x and we can release a micro much earlier.

@Maxinho96 Maxinho96 changed the base branch from main to 3.2.x July 6, 2022 18:25
@Maxinho96 Maxinho96 requested a review from a team as a code owner July 6, 2022 18:25
@Maxinho96 Maxinho96 requested review from aerowithanl and ancalita and removed request for a team July 6, 2022 18:25
@Maxinho96
Copy link
Contributor Author

@ancalita done :)

@ancalita
Copy link
Member

ancalita commented Jul 7, 2022

@Maxinho96 To double-check did you also rebase your branch locally and then forced push? It seems the PR now contains a lot of extra commits 😬 This will have to be fixed before I can review again.

@Maxinho96
Copy link
Contributor Author

@ancalita Sorry, I have fixed with a rebase :)

@ancalita ancalita removed the request for review from aerowithanl July 7, 2022 08:11
Copy link
Member

@ancalita ancalita 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 the speedy implementations, looks great 💯
I just have two more suggestions for expanding a bit on the issue and solution in the changelog entry & for refactoring a bit the tests. Let me know how you get on.

changelog/11305.bugfix.md Outdated Show resolved Hide resolved
tests/core/channels/test_slack.py Show resolved Hide resolved
@Maxinho96 Maxinho96 requested a review from ancalita July 8, 2022 06:45
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

🎉

@ancalita ancalita enabled auto-merge July 8, 2022 13:07
@ancalita ancalita merged commit 3f69f12 into RasaHQ:3.2.x Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants