840073 - [MMS][User Story] Address recipient by phone number or Contact name

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[Peter Dolanjski [:pdol]]

UCID: Messages-029

User Story:
As a user, I can compose a message to a recipient using their phone number or Contact name for that number

Comment 1

[Casey Yee]

This is currently already implemented.

Flagging Peter for NeedsInfo to confirm that the current implementation fulfills this requirement.

Comment 2

[Rick Waldron [:rwaldron]]

Attachment: gaia-mms-recipient-addressing.jpg

Attachment shows recipient addressing by name and number as currently implemented.

Comment 3

[Peter Dolanjski [:pdol]]

(In reply to Casey Yee [:cyee] from comment #1)
> This is currently already implemented.
> 
> Flagging Peter for NeedsInfo to confirm that the current implementation
> fulfills this requirement.

It does.

Comment 4

[Borja Salguero [:borjasalguero]]

I guess that the modification here is adding more than one recipient, and add the right styles to each recipient right?

Comment 5

[Kev Needham [:kev]]

Yes. The user should be able to add recipents, single or multiple, using either the contact name or mobile number for each recipient.

(In reply to Borja Salguero [:borjasalguero] from comment #4)
> I guess that the modification here is adding more than one recipient, and
> add the right styles to each recipient right?

Comment 6

[Rick Waldron [:rwaldron]]

Attachment: Redirect to https://github.com/mozilla-b2g/gaia/pull/8526

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 722485 [details]
Redirect to https://github.com/mozilla-b2g/gaia/pull/8526

comments added in the PR.

Thanks !

Comment 8

[Dietrich Ayala (:dietrich)]

Per partner and release-driver discussions, marking blocking- until all MMS functionality in bug 849867 is complete, allowing it all to be uplifted at once to avoid SMS bustage.

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 722485 [details]
Redirect to https://github.com/mozilla-b2g/gaia/pull/8526

r=me with the last nits

squash, rebase (hopefully this will go well), push to the PR, wait that travis is green, and then it's ready to be "checkin-needed"-flagged :)

Comment 10

[Rick Waldron [:rwaldron]]

 Good to merge — The Travis build passed (Details)

Comment 11

[Rick Waldron [:rwaldron]]

https://github.com/mozilla-b2g/gaia/pull/8526.patch

Comment 12

[Julien Wajsberg [:julienw]]

master: https://github.com/mozilla-b2g/gaia/commit/2876a652a7e27e12fd7d84d51476de15765549ff

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 722485 [details]
Redirect to https://github.com/mozilla-b2g/gaia/pull/8526

This is not really a MMS specific bug so I'm requesting approval here, because this blocks also other uplifts (like Bug 853788).

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: no real user impact. but there are lots of unit tests covering the new code.
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low, this landed for 3 weeks in master, and there are lots of unit tests for this.
String or UUID changes made by this patch: none

Comment 15

[Maria Angeles Oteo (:oteo)]

We need to land this bug in V1-train beacuse it's blocking now a tef+ bug 847332.
Although, be aware that we don't need it in v1.0.1 only in v1-train (a specific version of 847332 need to be created for v1.0.1)

Apart of it, I think we should have handled this issue in another way and this kind of refactor should be done in another bug.

As Peter has commented several times, this US is for MMS, it's sure that it's already implemented in SMS. But the US was created to check that the same behaviour is happennig when MMS is developed, so when you resolve it, it will seem that we have already checked that it works properly with MMS, and this is not case... Please bear this in mind next time.

Comment 16

[Julien Wajsberg [:julienw]]

(In reply to Maria Angeles Oteo:oteo from comment #15)

> 
> As Peter has commented several times, this US is for MMS, it's sure that
> it's already implemented in SMS. But the US was created to check that the
> same behaviour is happennig when MMS is developed, so when you resolve it,
> it will seem that we have already checked that it works properly with MMS,
> and this is not case... Please bear this in mind next time.

This is a task for QA, we don't need to open bugs to "check that it works properly". Rather, we open bugs when QA find that it doesn't.

Comment 17

[Maria Angeles Oteo (:oteo)]

> 
> This is a task for QA, we don't need to open bugs to "check that it works
> properly". Rather, we open bugs when QA find that it doesn't.

But we don't need to use an existing bug to solve something that is not exactly what that bug is about :)

This was marked by Peter as a [User Story] in order to indicate it's a special bug. Anyway, I am not the one who has decided to follow this approach, talk to Peter if you have any concern about that.

Comment 18

[Rick Waldron [:rwaldron]]

(In reply to Maria Angeles Oteo:oteo from comment #15)
> We need to land this bug in V1-train beacuse it's blocking now a tef+ bug
> 847332.
> Although, be aware that we don't need it in v1.0.1 only in v1-train (a
> specific version of 847332 need to be created for v1.0.1)
> 
> Apart of it, I think we should have handled this issue in another way and
> this kind of refactor should be done in another bug.
> 
> As Peter has commented several times, this US is for MMS, it's sure that
> it's already implemented in SMS. But the US was created to check that the
> same behaviour is happennig when MMS is developed, so when you resolve it,
> it will seem that we have already checked that it works properly with MMS,
> and this is not case... Please bear this in mind next time.

Maria, this is exactly the context in which I completed this user story. I'm working on MMS, therefore this is resolved for MMS. SMS and MMS are the same app.

Comment 19

[Paul Yang [:pyang] (away)]

moztrap #6542

Comment 20

[Maria Angeles Oteo (:oteo)]

(In reply to Rick Waldron from comment #18)
> (In reply to Maria Angeles Oteo:oteo from comment #15)
> > We need to land this bug in V1-train beacuse it's blocking now a tef+ bug
> > 847332.
> > Although, be aware that we don't need it in v1.0.1 only in v1-train (a
> > specific version of 847332 need to be created for v1.0.1)
> > 
> > Apart of it, I think we should have handled this issue in another way and
> > this kind of refactor should be done in another bug.
> > 
> > As Peter has commented several times, this US is for MMS, it's sure that
> > it's already implemented in SMS. But the US was created to check that the
> > same behaviour is happennig when MMS is developed, so when you resolve it,
> > it will seem that we have already checked that it works properly with MMS,
> > and this is not case... Please bear this in mind next time.
> 
> Maria, this is exactly the context in which I completed this user story. I'm
> working on MMS, therefore this is resolved for MMS. SMS and MMS are the same
> app.
Of course, I know it's the same application, but MMS is not ready yet (I think that last night the MMS API was just landed) so I wouldn't say this US is resolved as I have not seen it working in MMS... in fact I have not seen any MMS sent or received yet, I am sure we can see it next week during the ww ;)

Anyway, let's not continue with it, I tried to follow Peter/Dylan indications when at the begining you asked about the sense of not of these US already implemented in SMS, that's all.
I would have created another bug linked to this US to land this patch, and as this US is P1 I would have requested the leo+ for the US and the associated bug so we can land in v1-train all this work easily, but it's up to you, I just wanted to help you for the sake of clarity.
 
Thanks a lot for your work!

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 2876a652a7e27e12fd7d84d51476de15765549ff
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 22

[Julien Wajsberg [:julienw]]

yeah I plan to do it today.

Comment 23

[Julien Wajsberg [:julienw]]

v1-train: 3b6f1407447ab72b3bd3062425eee6683825ebfe