Closed Bug 1026494 Opened 8 years ago Closed 8 years ago

Loop server - Add URL info to call data

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ferjm, Assigned: tarek)

References

Details

(Whiteboard: [qa?] ft:Loop)

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.
Blocks: 1000134
Whiteboard: [qa?]
Whiteboard: [qa?] → [qa?] ft:Loop
Priority: -- → P1
Assignee: nobody → tarek
Adam, any reason why it's 'call_url' instead of 'callUrl' like every other field ?
let's keep consistent ;)
(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.
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.
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!
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?
Flags: needinfo?(adam)
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.
Flags: needinfo?(adam)
(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)
Flags: needinfo?(alexis+bugs)
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?
Flags: needinfo?(alexis+bugs) → needinfo?(adam)
(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.
Flags: needinfo?(adam) → needinfo?(alexis+bugs)
Created https://bugzilla.mozilla.org/show_bug.cgi?id=1032966 about that.
Flags: needinfo?(alexis+bugs)
Verified that the code has changed since this commit, but the core additions are still in code and unit tests.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.