888150 - [MMS][Regression] Header actions are not working properly.

Status: VERIFIED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

A lot of issues here....
STR1:
- Send a SMS to '123123123'
- See the thread
- Tap on the header
- See the list of options
- Tap on 'Call'
EXPECTED:
- You can call to '123123123' from the dialer
CURRENTLY:
- Nothing happens


STR2:
- Send a SMS to '123123123'
- See the thread
- Tap on the header
- See the list of options

EXPECTED:
- 'send message' should be hidden, because you are in your own thread
CURRENTLY:
- 'send messag is there

STR2:
- Send a SMS to '123123123'
- See the thread
- Tap on the header
- See the list of options

EXPECTED:
- 'send message' should be hidden, because you are in your own thread
CURRENTLY:
- 'send message' is there

STR3:
- Send a SMS to a contact 'Manolo'
- See the thread
- Tap on the header

EXPECTED:
- You will go directly to the dialer for calling 'Manolo'
CURRENTLY:
- You see wrong options, and you can not go to dialer directly.

Comment 1

[Borja Salguero [:borjasalguero]]

This functionality is completely broken. Rick, I know you were working on this, could you take a look? Thanks

Comment 2

[Borja Salguero [:borjasalguero]]

Dietrich, this is a completely broken functionality, and a regression as well... leo+?

Comment 3

[Borja Salguero [:borjasalguero]]

Im gonna fix the STR1:

 STR1:
 - Send a SMS to '123ABC'
 - See the thread
 - Tap on the header
 - See the list of options
 - Tap on 'Call'
 EXPECTED:
 - You can call to '123123123' from the dialer
 CURRENTLY:
 - Nothing happens

Comment 4

[Julien Wajsberg [:julienw]]

Ayman, I'd like to be able to go to the contact card when tapping on the thread header, as an additional action in the menu. Do you think it would be a good idea ?

Comment 5

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Ayman, I'd like to be able to go to the contact card when tapping on the
> thread header, as an additional action in the menu. Do you think it would be
> a good idea ?

I see no reason why not, so long as this does not bring risk/extra work to the current build. If not we can defer to 1.2.

Please NeedInfo me to let me know the decision on this so i can update the v1.1 spec accordingly if necessary.

Comment 6

[Julien Wajsberg [:julienw]]

I really think this is a very small work, but I'll let Rick (the assignee) decides if it's worth it now or if we'll do it in 1.2.

Comment 7

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> I really think this is a very small work, but I'll let Rick (the assignee)
> decides if it's worth it now or if we'll do it in 1.2.

ok Julien, referencing comment 4, 5 and 6 needinfo to Rick on this so we can either move forwards with it, or defer to v1.2

Comment 8

[Alex Keybl [:akeybl]]

leo+ for STR1. In what bugs will the other STR be filed in, so that they can be nominated individually?

Comment 9

[Rick Waldron [:rwaldron]]

With:

b2g18@latest
gaia@master


(In reply to Borja Salguero [:borjasalguero] from comment #0)
> A lot of issues here....
> STR1:
> - Send a SMS to '123123123'
> - See the thread
> - Tap on the header
> - See the list of options
> - Tap on 'Call'
> EXPECTED:
> - You can call to '123123123' from the dialer
> CURRENTLY:
> - Nothing happens


This cannot be reproduced.

https://www.dropbox.com/sh/3lr5a5kepnns8z1/rydxqmpucg


> 
> 
> STR2:
> - Send a SMS to '123123123'
> - See the thread
> - Tap on the header
> - See the list of options
> 
> EXPECTED:
> - 'send message' should be hidden, because you are in your own thread
> CURRENTLY:
> - 'send messag is there


"Send Message" will be removed for single participant messages (will still appear in a _sent_ multi-participant message)

> 
> STR3:
> - Send a SMS to a contact 'Manolo'
> - See the thread
> - Tap on the header
> 
> EXPECTED:
> - You will go directly to the dialer for calling 'Manolo'
> CURRENTLY:
> - You see wrong options, and you can not go to dialer directly.

Seems strange to have this behavioral exception, but if that's the spec, then this will be updated.

Comment 10

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Ayman, I'd like to be able to go to the contact card when tapping on the
> thread header, as an additional action in the menu. Do you think it would be
> a good idea ?

This should be easy enough.

Comment 11

[Rick Waldron [:rwaldron]]

(In reply to Rick Waldron from comment #10)
> (In reply to Julien Wajsberg [:julienw] from comment #4)
> > Ayman, I'd like to be able to go to the contact card when tapping on the
> > thread header, as an additional action in the menu. Do you think it would be
> > a good idea ?
> 
> This should be easy enough.

"should", but isn't. I may have missed something, but the Contacts app doesn't appear to have an activity for viewing a contact based on some parameter.

Comment 12

[Corey Frang [:gnarf]]

(In reply to Rick Waldron from comment #11)
> (In reply to Rick Waldron from comment #10)
> > (In reply to Julien Wajsberg [:julienw] from comment #4)
> > > Ayman, I'd like to be able to go to the contact card when tapping on the
> > > thread header, as an additional action in the menu. Do you think it would be
> > > a good idea ?
> > 
> > This should be easy enough.
> 
> "should", but isn't. I may have missed something, but the Contacts app
> doesn't appear to have an activity for viewing a contact based on some
> parameter.

This seems like a good idea.  If we won't be doing that in this bug, can we create bugs to track the feature request?  Is all we need "[Contacts] "view" activity for contact", and then "[SMS] allow "view" contact from header action menu" ?

Comment 13

[Rick Waldron [:rwaldron]]

Attachment: Github Pull Request https://github.com/mozilla-b2g/gaia/pull/10771

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #12)
> (In reply to Rick Waldron from comment #11)
> > (In reply to Rick Waldron from comment #10)
> > > (In reply to Julien Wajsberg [:julienw] from comment #4)
> > > > Ayman, I'd like to be able to go to the contact card when tapping on the
> > > > thread header, as an additional action in the menu. Do you think it would be
> > > > a good idea ?
> > > 
> > > This should be easy enough.
> > 
> > "should", but isn't. I may have missed something, but the Contacts app
> > doesn't appear to have an activity for viewing a contact based on some
> > parameter.
> 
> This seems like a good idea.  If we won't be doing that in this bug, can we
> create bugs to track the feature request?  Is all we need "[Contacts] "view"
> activity for contact", and then "[SMS] allow "view" contact from header
> action menu" ?

I think the "open" activity does just that.

But I'll file another bug for this after all, because we need careful UX decision to be sure that everything feels ok, I don't want to rush for this.

Comment 15

[Julien Wajsberg [:julienw]]

Filed bug 890209 for this specific topic.

Comment 16

[Borja Salguero [:borjasalguero]]

Rick, I will review this tomorrow morning. Sorry for the delay.

Comment 17

[Borja Salguero [:borjasalguero]]

Comment on attachment 771001 [details] [review]
Github Pull Request https://github.com/mozilla-b2g/gaia/pull/10771

Hi Rick,
I've been testing your patch and I found an issue here https://github.com/mozilla-b2g/gaia/pull/10771#issuecomment-20734398 . Please take a look, and once this will be fixed let me know and I will review this with priority. Thanks!

Comment 18

[Rick Waldron [:rwaldron]]

(In reply to Borja Salguero [:borjasalguero] from comment #17)
> Comment on attachment 771001 [details] [review]
> Github Pull Request https://github.com/mozilla-b2g/gaia/pull/10771
> 
> Hi Rick,
> I've been testing your patch and I found an issue here
> https://github.com/mozilla-b2g/gaia/pull/10771#issuecomment-20734398 .
> Please take a look, and once this will be fixed let me know and I will
> review this with priority. Thanks!

Yes, you r+ed the code that broke this: https://github.com/mozilla-b2g/gaia/commit/8b41ee9a80c1d6b3613339e7388035d3fbcc575d#L2R227 

https://bugzilla.mozilla.org/show_bug.cgi?id=880624#c18

Comment 19

[Rick Waldron [:rwaldron]]

Borja, I've fixed all the regressions that bug 880624 created.

Comment 20

[Wayne Chang [:wchang]]

Clear ni? for dietrich from comment 2. Already a leo+ bug.

Comment 21

[Borja Salguero [:borjasalguero]]

Comment on attachment 771001 [details] [review]
Github Pull Request https://github.com/mozilla-b2g/gaia/pull/10771

This is working as expected. R+ from my side! Waiting Francisco for merging this.

Comment 22

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 771001 [details] [review]
Github Pull Request https://github.com/mozilla-b2g/gaia/pull/10771

r+

tried the use cases that were failing, now everything looking awesome.

Thanks a lot Rick!

Comment 23

[Borja Salguero [:borjasalguero]]

https://github.com/mozilla-b2g/gaia/commit/aaafec3c79e94041db7d22d3490f8d4916faeb3a
https://github.com/rwldrn/gaia/commit/ece25d4d32fafa477a928f8c1045b00ece4b292b

R+. Leo+. Merged!

Comment 24

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

Uplifted aaafec3c79e94041db7d22d3490f8d4916faeb3a to:
v1-train: 7fc1f9b4677909ac833817a550bbb21671f7a964

Comment 25

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

v1.1.0hd: 7fc1f9b4677909ac833817a550bbb21671f7a964

Comment 26

[bov]

Tested on Unagi device. 
Build:unagi-ICS.user.manifest.V1-train.Rel0.4.Sprint12.B-332.Gecko-f89d83b.Gaia-6221737
Build ID:20130730104759
Commercial RIL: AU 172

STEPS:
All includes in "Description"
(In reply to Borja Salguero [:borjasalguero] from comment #0)
> A lot of issues here....
> STR1:
> - Send a SMS to '123123123'
> - See the thread
> - Tap on the header
> - See the list of options
> - Tap on 'Call'
> EXPECTED:
> - You can call to '123123123' from the dialer
> CURRENTLY:
> - Nothing happens
> 
> 
> STR2:
> - Send a SMS to '123123123'
> - See the thread
> - Tap on the header
> - See the list of options
> 
> EXPECTED:
> - 'send message' should be hidden, because you are in your own thread
> CURRENTLY:
> - 'send messag is there
> 
> STR3:
> - Send a SMS to a contact 'Manolo'
> - See the thread
> - Tap on the header
> 
> EXPECTED:
> - You will go directly to the dialer for calling 'Manolo'
> CURRENTLY:
> - You see wrong options, and you can not go to dialer directly.

ALL ISSUES/TESTS WORK WELL, AS EXPECTED