nyaruka / gocommon

Common utility library for the TextIt platform.
Other
7 stars 11 forks source link

Add FreshChat URN #22

Closed tybritten closed 5 years ago

tybritten commented 5 years ago

First PR for adding FreshChat support. the URN for freshchat will be freshchat:user_uuid?channel_uuid

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.007%) to 99.638% when pulling 295257e498c486de2e57ecc282a88146ed794e75 on tybritten:master into 1d7c5b9991ea0e63c7499a7d5ee708a48d39166e on nyaruka:master.

nicpottier commented 5 years ago

Note that we won't merge this until July 1st as we are in a freeze for new things. This probably needs some validation functions as well. (what is a reasonable id)

I'm guessing channel_uuid does not belong in the URN, that's usually not a URN thing but rather something RP keeps track of. If it is part of the URN, it should be represented in some other way, the ? is going to get encoded in the path and its all going to get really confusing.

tybritten commented 5 years ago

So the reason channel uuid is in the URN is Freshchat only supports a single outbound webhook. so if I have multiple channels setup (say an apple biz chat one and another one) I can't configure a RP channel for each of them because I can't point the webhook to two courier paths. So by making the channel id part of the URN- "user A on channel X" and "user A on channel Y" are two different RP users because freshchat requires a channel- there's no concept of 'direct messaging' a user.

nicpottier commented 5 years ago

Ah.. ok, well, probably best to come up with a new name rather than channel then, since that's going to be really confusing for everybody if those are fresh chat channels and not ours. Is that the name they use for those as well?

Point stands about ? as well, can we use a separate delimiter? And does it need to be generic to the point of having key/values vs just being user_id:channel_id?

tybritten commented 5 years ago

sure, they call it channel_id but i can absolutely call it something else. I went with the ? seperator because that's what was in the URN splitter code- i do need to split it out when sending to populate the API call- they're two sep fields. right now in courier i'm doing

user := fmt.Sprintf("%v", msg.URN().Path())
channelid := fmt.Sprintf("%v", msg.URN().RawQuery())
nicpottier commented 5 years ago

Some of these are decisions you need to make on the integration. For example, if a user connects through two different freshbook channels, is that the same URN or two different URNs?

The path portion of the URN needs to be uniquely identifiable, so if your answer to the above is two URNs, then the freshbook channel id needs to be part of the path, not in the query. If you think those are the SAME URN, then this all gets a lot more complicated and you'll need to be setting / updating that query whenever you receive a message, basically associating which is the "preferred" channel for that contact.

tybritten commented 5 years ago

Ah ok that makes a lot of sense. we're going to chat about it and see which approach makes the most sense.

tybritten commented 5 years ago

Ok, decided to go with the userid:channelid as the full URN and treat them as unique especially since the main use case is apple business chat.

nicpottier commented 5 years ago

Nice. Conceptually I wonder if you are going this route then a more clear parent/child format like channel_uuid/user_uuid would be clearer. (realize I suggested the : approach just occurred to me that the : is slightly overloaded and / represents hierarchy more clearly) That said, I'm fine as is if you want to go this way.

tybritten commented 5 years ago

Makes sense, made that change.

nicpottier commented 5 years ago

Cool, thanks! Looks like your tests need a tweak but after that will merge since this doesn't directly affect anything until we update them.

tybritten commented 5 years ago

I was thinking about the position swap and forgot about the : with the tests. long day haha