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

Multiple links in message - Slack sanitization #4817

Merged
merged 10 commits into from
Feb 3, 2020
Merged

Conversation

vigneshp826
Copy link
Contributor

Proposed changes:

Status (please check what you already did):

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

Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

thanks a lot for the contribution 🚀 code itself looks good - can you please add some tests for the link/email replacement functionality?

@vigneshp826
Copy link
Contributor Author

@tmbo can you let me know where i need to do that?

@erohmensing
Copy link
Contributor

@vigneshp826 you could add examples here: https://github.com/RasaHQ/rasa/blob/master/tests/core/test_channels.py#L464

@erohmensing
Copy link
Contributor

@vigneshp826 are you still planning on finishing this PR?

@vigneshp826
Copy link
Contributor Author

@erohmensing little bit busy in my BAU activities. Will try t complete next week. Sorry for the delay

@stale stale bot removed the status:stale label Dec 18, 2019
@erohmensing
Copy link
Contributor

Okay, just wanted to check it wasn't going stale.

The mechanism for changelog has changed since, please remove your changelog file and follow these instructions

vigneshp826 and others added 2 commits January 24, 2020 15:43
test case addition for converting garbled url's into actuals
@vigneshp826
Copy link
Contributor Author

@tmbo @erohmensing added the test piece of code and changelog, let me know if any changes required

@vigneshp826 vigneshp826 requested a review from tmbo February 3, 2020 08:05
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

looks great - thanks a lot for the improvement 🚀

@tmbo tmbo merged commit f9d49a6 into RasaHQ:master Feb 3, 2020
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.

3 participants