1095358 - Add subject to direct calls (aka as conversations)

Status: RESOLVED FIXED

(Hello (Loop) :: Server, defect)

Description

[Maria Angeles Oteo (:oteo)]

Product team wants users making direct calls to have the option to include a subject for the call so that they can tell the other party what the communication is going to be about.

At a glance, it seems this requires changes in:

- POST /calls (to indicate the subject when making the call)
- GET /calls?version=<version> (to retrieve the subject when receiving the call)
- POST /call-url (not sure if this is going to be really used with the new room model but maybe we should add it for the sake of consistency)

Comment 1

[Alexis Metaireau (:alexis)]

In addition, PUT /call-url/{token} should also be able to get this new "subject" option.

I also need some more information in order to cap the value to a maximum. Would 124 chars be enough for a subject?

I'm okay with this change, but would need a seal of approval by Adam ;)

Comment 2

[Maria Angeles Oteo (:oteo)]

(In reply to Alexis Metaireau (:alexis) from comment #1)
> In addition, PUT /call-url/{token} should also be able to get this new
> "subject" option.
Thanks for taking care of it!!
> 
> I also need some more information in order to cap the value to a maximum.
> Would 124 chars be enough for a subject?
I've checked with Product ream and it's more than enough :)

> I'm okay with this change, but would need a seal of approval by Adam ;)
of course!!

Comment 3

[Maire Reavy [:mreavy]]

Is this a change for mobile only or would desktop need to make the same change?

Our (desktop's) development schedule for the next few weeks is very tight, and I'd need to discuss with Darrin what the UX implications are for desktop if we make this change. Plus, I'd have to get a new UX design from Darrin for direct calls. 

If this can be a mobile-only change (at least for the short term), then many of my concerns and objections go away.

Comment 4

[Rémy Hubscher (:natim)]

AFAIK, I think it can be an optional thing so if desktop do implements it they can still continue to use loop-server as they are doing today.

Comment 5

[Alexis Metaireau (:alexis)]

Yes, this is an optional parameter. In case it's not present, it won't be used.

Comment 6

[:shell escalante]

Hi Maria,

Can you catch me up on which product team discussion/decision this bug is around?  This has impact on desktop implementation and UX as well.  Can we catch up before we make server changes, to make sure all the parts work together?  :)

Comment 7

[Maria Angeles Oteo (:oteo)]

Hi Shell, this is one of the User Stories for 1.1.1 Loop Mobile version, so clearing my ni and setting it to Jorge as this is a Product call.

Anyway, as far as I know, this subject field is an optional parameter so there should not be any problem if Desktop does not use it.

Comment 8

[jmunuera]

Hi Shell, this field is optional and we would like to use it in the mobile app. As Alexis said, is it not present, it wont be used.

Comment 9

[Adam Roach [:abr]]

Sorry for the delay here. This makes perfect sense from a protocol perspective, and I think we should plan to support it on the Desktop as time allows (i.e., we should open and schedule a bug to handle this on the desktop client -- one UX, one implementation). No rush, of course -- but it would be good to make sure they don't get dropped on the floor.

Maire: can I have you open these desktop bugs and make sure they get tracked?

Alexis: can I have you update the docs with a new parameter?

Comment 10

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Proposed patch (WIP)

This is a WIP patch.
Pending Unit-Tests

Comment 11

[Alexis Metaireau (:alexis)]

Comment on attachment 8522866 [details] [review]
Proposed patch (WIP)

I did a first round of feedback on the pull request, thanks!

Comment 12

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8522866 [details] [review]
Proposed patch (WIP)

Tests updated.

Comment 13

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Documentation update

Comment 14

[Alexis Metaireau (:alexis)]

Comment on attachment 8522866 [details] [review]
Proposed patch (WIP)

The way tests are done is not correct. Please see the pull request comments for reference.

Comment 15

[Alexis Metaireau (:alexis)]

Comment on attachment 8525177 [details] [review]
Documentation update

Needs to be revised, thanks!

Comment 16

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8525177 [details] [review]
Documentation update

Changes done.

Comment 17

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8522866 [details] [review]
Proposed patch (WIP)

Changes done.

Comment 18

[Alexis Metaireau (:alexis)]

Comment on attachment 8525177 [details] [review]
Documentation update

thanks, doc is correct!

Comment 19

[Alexis Metaireau (:alexis)]

Comment on attachment 8522866 [details] [review]
Proposed patch (WIP)

We're almost there. I've made other comments there. You need to update the tests to not create new blocks when they're not needed and also update the documentation of the function which takes new parameters.

Thanks!

Comment 20

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Thank you alexis.
Tests fixed and function documentation too.

I cann't land the doc since I haven't write access.

Comment 21

[Alexis Metaireau (:alexis)]

Comment on attachment 8522866 [details] [review]
Proposed patch (WIP)

https://github.com/mozilla-services/loop-server/commit/6a27bad10eba44e38d5bf6e043a6f8822707457c
https://github.com/mozilla-services/docs/commit/0e5c784dc98509c346d1966842d08ae06c1019b0

Comment 22

[Maria Angeles Oteo (:oteo)]

Alexis, this bug has the [loop-server 0.14] whiteboard but if I am not wrong is not in that release (it's in development server) when do you plan push it to Production?

Comment 23

[Maria Angeles Oteo (:oteo)]

Removing the ni, I am seeing that will be part of 0.14.0 https://github.com/mozilla-services/loop-server/releases (10th Dec) and now Production is 0.13.2