1026494 - Loop server - Add URL info to call data

Status: VERIFIED FIXED

(Hello (Loop) :: Server, defect, P1)

Description

[Fernando Jiménez Moreno [:ferjm]]

In order to differentiate between an incoming call originated from a link clicker or from a registered user and to allow the client to associate incoming calls with urls and to provide more context for the URL revocation, we need to store and expose the URL info associated with the call data. I believe storing the URL token in the call DB record should be enough.

Comment 1

[Adam Roach [:abr]]

Thanks; I've added this to https://wiki.mozilla.org/Loop/Architecture/MVP#GET_.2Fcalls.3Fversion.3D.3Cversion.3E

Comment 2

[Tarek Ziadé (:tarek)]

Adam, any reason why it's 'call_url' instead of 'callUrl' like every other field ?

Comment 3

[Alexis Metaireau (:alexis)]

let's keep consistent ;)

Comment 4

[Adam Roach [:abr]]

(In reply to Tarek Ziadé (:tarek) from comment #2)
> Adam, any reason why it's 'call_url' instead of 'callUrl' like every other
> field ?

This was done for consistency with |POST /call-url|, which used underscores rather than camel case for call_url. Unfortuantely, the initial API was somewhat split on which convention to use, and rationalization at this point would be difficult.

That said, if you want call_url for |POST /call-url| and callUrl for |GET /calls?version=<version>|, I'm happy with that also. Let me know which direction you're going, and I'll update the documentation.

Comment 5

[Adam Roach [:abr]]

P.S. There are a lot of bugs on the project; if you need action from me, please make sure you tag me in a needinfo.

Comment 6

[Tarek Ziadé (:tarek)]

I don't have a strong opinion on this. I guess would prefer staying consistent for the same variable name across all APIs, so I guess 'call_url' everywhere, unless Alexis feels differently 

> P.S. There are a lot of bugs on the project; if you need action from me, please make sure you tag me in a needinfo.

sure thanks!

Comment 7

[Alexis Metaireau (:alexis)]

We should stick with camelCase whenever possible. The only exception currently is for the simplepush url, and we should provide a way to support the camelCase version and deprecate the old format.

Since we don't already have legacy clients, I believe we should remove the use of underscores.

Adam, I don't see why it would be hard to rationalize?

Comment 8

[Alexis Metaireau (:alexis)]

Ah, I think I know what you mean, Adam; Rationale is:

- In the urls, we use dashes.
- In the parameters, we should stick with camelCase (I don't have strong opinion on which one to use, we should just stick to the same one everywhere, which is camelCase for now).

Currently, that's not the case on the server implementation and we should aim to fix that.

Comment 9

[Tarek Ziadé (:tarek)]

Pull request at https://github.com/mozilla-services/loop-server/pull/121

Comment 10

[Alexis Metaireau (:alexis)]

https://github.com/mozilla-services/loop-server/commit/0b1d8e4b7e6df8fc9c5a4a7accde079f9f2dab78

Comment 11

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #7)
> We should stick with camelCase whenever possible. The only exception
> currently is for the simplepush url, and we should provide a way to support
> the camelCase version and deprecate the old format.

Sounds good -- do you want to open a bug to accept either format (simple_push_url or simplePushUrl) for the time being? Basically the same thing as I've proposed for the client change to callUrl:

 req.simplePushURL = req.body.simple_push_url || req.body.simplePushUrl;

(along with checking for the presence of one or the other rather than just the underscore version)

Comment 12

[Alexis Metaireau (:alexis)]

Since we don't have any legacy client, do we really want to support that, or should we just update client and server code for the time being?

Comment 13

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #12)
> Since we don't have any legacy client, do we really want to support that, or
> should we just update client and server code for the time being?

I know that people will use a Nightly for a few days at a time, and I'd like to avoid a bunch of questions about why things suddenly stopped working. Let's go ahead and put transitional code in there, at least for a week or two.

Comment 14

[Alexis Metaireau (:alexis)]

Created https://bugzilla.mozilla.org/show_bug.cgi?id=1032966 about that.

Comment 15

[James Bonacci [:jbonacci]]

Verified that the code has changed since this commit, but the core additions are still in code and unit tests.