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

Add better data input management for MS botframwork #8190

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thaume
Copy link
Contributor

@thaume thaume commented Mar 12, 2021

Proposed changes:
As discussed in #8164

We have been using this "feature" in production for 2 years now, I felt it was time to share it with everyone else using buttons in Teams' adaptive cards.

It's not a groundbreaking feature but it is very important.

I'll add the tests once we validate that we need this feature here :)

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)

@m-vdb m-vdb linked an issue Mar 12, 2021 that may be closed by this pull request
@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @b-quachtran will take a look at it as soon as possible ✨

Copy link
Collaborator

@m-vdb m-vdb left a comment

Choose a reason for hiding this comment

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

looks good @thaume 💯 sorry for the late review

rasa/core/channels/botframework.py Outdated Show resolved Hide resolved
rasa/core/channels/botframework.py Show resolved Hide resolved
@m-vdb m-vdb removed the request for review from b-quachtran April 21, 2021 16:08
@thaume
Copy link
Contributor Author

thaume commented Apr 22, 2021

@m-vdb PR fixed following your comments

Copy link
Collaborator

@m-vdb m-vdb 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 updating the docstring. I've added a suggesting to fix the failing lint. Could you also take care of the remaining steps from the PR description?

  • 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)

rasa/core/channels/botframework.py Outdated Show resolved Hide resolved
@thaume
Copy link
Contributor Author

thaume commented Apr 22, 2021

Awesome I've updated the docstring, taking care of the other steps now

@thaume
Copy link
Contributor Author

thaume commented Apr 22, 2021

@m-vdb I updated everything, I'm just wondering about the tests : since there are none it seems a bit overkill (and I won't have time right now) to write all the tests for this channel, what do you think ?

In terms of documentation, there is no impact, it will just return a correct value everytime.

Let me know if you need me to change anything else

Copy link
Collaborator

@m-vdb m-vdb 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 adding the changelog item. I agree with you that the documentation part can be skipped ✅

Regarding the test, asking you to write all the tests is a bit too much for sure 😅 but could you write 3 unit tests that check for the base use case and the ones you introduced?

  1. postdata contains text
  2. postdata doesn't contain text but contains value, which in turns contains value
  3. postdata doesn't contain text but contains value, which in turns doesn't contain value

You could take some inspiration from the tests written for the Slack channel.

Comment on lines +217 to +218
""" Webhook receiving requests from MS Botframework.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oups, still some whitespaces left here:

Suggested change
""" Webhook receiving requests from MS Botframework.
"""Webhook receiving requests from MS Botframework.

@m-vdb
Copy link
Collaborator

m-vdb commented Jul 21, 2021

@thaume following up on this one, do you need help to finalise the PR?

@thaume
Copy link
Contributor Author

thaume commented Jul 21, 2021

@m-vdb hey ! I'll never find the time to wrap my head around the test strategy and write them myself. That's the main concern here 🙂

@stale
Copy link

stale bot commented Apr 16, 2022

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@tmbo tmbo requested a review from a team as a code owner September 12, 2022 19:48
@tmbo tmbo requested review from sanchariGr and removed request for a team September 12, 2022 19:48
@stale stale bot removed stale labels Sep 12, 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.

Buttons click handling MS Botframework
3 participants