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

[FEATURE] modify conversation id of server to accomodate viber id #6055

Merged
merged 81 commits into from
Aug 25, 2020

Conversation

IzidoroBaltazar
Copy link
Contributor

@IzidoroBaltazar IzidoroBaltazar commented Jun 23, 2020

Proposed changes:

  • Change conversation id format of server api to accommodate Viber channel

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)

@sara-tagger sara-tagger requested a review from degiz June 23, 2020 12:00
@sara-tagger
Copy link
Collaborator

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

@degiz
Copy link
Contributor

degiz commented Jul 9, 2020

Hey @IzidoroBaltazar

Could you please give example/point me to the viber doc with their conversation id formats?

I'm asking because I can't find examples with \ or / in them, and the +-_ are covered by the current string type.

@IzidoroBaltazar
Copy link
Contributor Author

Here is general documentation.
https://developers.viber.com/docs/api/rest-bot-api/#get-user-details

From documentation you can see that = is used in user_id. We also know that user_id contains /, but documentation does not mention that. However we found multiple user_ids that contain / which caused problems with current server setup.

I have aggregated few problematic user_ids I see these characters:

+/1032547698=ACBEDGFIHKJMLONQPSRUTWVYXZacbedgfihkjmlonqpsrutwvyxz

Slash is usually somewhere in the string.

@degiz
Copy link
Contributor

degiz commented Jul 21, 2020

@IzidoroBaltazar

I see, thanks.
The case with +/1032547698=ACBEDGFIHKJMLONQPSRUTWVYXZacbedgfihkjmlonqpsrutwvyxz is covered if you use path type for the parameter, check here

Would it work for you? Seems easier to maintain compared to the regex.

Could you please also write an explicit test for this and call it something like test_post_conversation_id_with_slash?

@IzidoroBaltazar
Copy link
Contributor Author

I have replaced regex with Sanic path it's working fine. I have modified the test test_push_multiple_events. This test is using problematic customer_id characters.

@degiz
Copy link
Contributor

degiz commented Jul 21, 2020

I have modified the test test_push_multiple_events

Exactly, we're trying to have one test per problem. So if test_push_multiple_events fails in the future bc of special symbols, it will be confusing. That's the reason I'm asking to add an explicit test_post_conversation_id_with_special_symbols.

Copy link
Contributor

@degiz degiz 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 wrapping that up! 🚀

@rasabot
Copy link
Collaborator

rasabot commented Aug 6, 2020

Could not update branch. Most likely this is due to a merge conflict. Please update the branch manually and fix any issues.

@rasabot
Copy link
Collaborator

rasabot commented Aug 24, 2020

Could not update branch. Most likely this is due to a merge conflict. Please update the branch manually and fix any issues.

@degiz degiz merged commit 0c34ff7 into RasaHQ:master Aug 25, 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.

4 participants