1113605 - [User Story][CONTACTS] Share a contact from contact details screen

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Noemí Freire (:noemi)]

Attachment: [2.0 Contacts] SharevCard v1.pdf

As a user I want to be able to share a contact from contacts details screen

(please refer to "[2.0 Contacts] SharevCard v1.pdf" spec attached)

Comment 1

[Noemí Freire (:noemi)]

Adding to backlog to be properly prioritized. Thanks!

Comment 2

[Jose Manuel Cantera]

We would need the 'share' icon in the Comms theme, as well.

Comment 3

[Jose Manuel Cantera]

Attachment: Patch v1

The proposed patch does not contain tests for the Contacts app, as we would need an integration test and I'm awaiting for the landing of bug 982260. Once that bug lands, I will add the corresponding integration test for contacts app.

thanks

Comment 4

[Jose Manuel Cantera]

Comment on attachment 8545249 [details]
Patch v1

https://github.com/mozilla-b2g/gaia/pull/27240

Comment 5

[Jose Manuel Cantera]

Attachment: Final Pull Request

Comment 6

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

IMO, this shouldn't be a 2.2 blocker as it's a new feature that wasn't planned for 2.2

Comment 7

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

Comment on attachment 8545861 [details] [review]
Final Pull Request

Hi Jose, great work here.

Code wise looking good, just a couple of tiny comments on github.

r- cause saw the following:
- icon is wrong, as it's white while the rest of icons for contacts are green
- since bug 982260 has a r+, please add tests to this bug too, we should not land any code, specially a whole feature without tests.
- I manage to reproduce a weird behavior: share a contact and when in sms click on the attachment and view, that will go back to contact, and I'm able to edit it, canceling the whole sharing end up with the contact edited via a weird flow.

I think we can do a follow up for the 3 point anyway.

Thanks!

Comment 8

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

Forgot to mention, in the gaia try build there are lots of errors in contacts unit tests too

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Just a reminder, 1/12 will be the branch date.
As this is not defined in 2.2 scope at the first place, approval can only be given if it's at very good state and low risk.
I'll keep the feature-b2g nomination for now.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8545861 [details] [review]
Final Pull Request

The message part looks fine, but just like Fransisco said there are lots of failed test that might related to this patch.

Another concern is about the contact/message circular activity and we might have some problem when pointing back to original app. I think we already have a bug for this issue.

Comment 11

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #7)
> Comment on attachment 8545861 [details] [review]
> Final Pull Request
> 
> Hi Jose, great work here.
> 
> Code wise looking good, just a couple of tiny comments on github.

Fixed!

> 
> r- cause saw the following:
> - icon is wrong, as it's white while the rest of icons for contacts are green

Yes, that's why we have needinfo-ed Fang 

> - since bug 982260 has a r+, please add tests to this bug too, we should not
> land any code, specially a whole feature without tests.

done. I've also updated unit tests as well ;)

The failing unit tests have been fixed, too. 

> - I manage to reproduce a weird behavior: share a contact and when in sms
> click on the attachment and view, that will go back to contact, and I'm able
> to edit it, canceling the whole sharing end up with the contact edited via a
> weird flow.

The flow you describe is the normal flow as we always open vcards in edit mode ... 

> 
> I think we can do a follow up for the 3 point anyway.
> 
> Thanks!

Thanks to you!

Comment 12

[Jose Manuel Cantera]

(In reply to Steve Chung [:steveck] from comment #10)
> Comment on attachment 8545861 [details] [review]
> Final Pull Request
> 
> The message part looks fine, but just like Fransisco said there are lots of
> failed test that might related to this patch.

Yep, a missing dependency on the mock DOM tree. Thanks for spotting! 

> 
> Another concern is about the contact/message circular activity and we might
> have some problem when pointing back to original app. I think we already
> have a bug for this issue.

Oh, absolutely, do you have a pointer to the bug?

Comment 13

[Maria Angeles Oteo (:oteo)]

Setting ni to Steve, according to the question asked by Jose Manuel in comment 12

Comment 14

[Steve Chung [:steveck]]

Firstly I thought it might related to bug 931339, but the it seems a default view contact behavior that editing contact is allowed. After discussion with Jenny, she said the the edit button should be disabled if the view contact panel is brought by clicking on the contact attachment(and she already confirmed with Paul).

Hi Jose, I think we will need another view contact activity with proper parameter to avoid contact editing. Do you prefer to address right in this patch or create a follow up for it?

Comment 15

[Jose Manuel Cantera]

(In reply to Steve Chung [:steveck] from comment #14)
> Firstly I thought it might related to bug 931339, but the it seems a default
> view contact behavior that editing contact is allowed. After discussion with
> Jenny, she said the the edit button should be disabled if the view contact
> panel is brought by clicking on the contact attachment(and she already
> confirmed with Paul).
> 
> Hi Jose, I think we will need another view contact activity with proper
> parameter to avoid contact editing. Do you prefer to address right in this
> patch or create a follow up for it?

I think it would be better to create a follow-up as we would need to land this bug before the branching to be done today

Comment 16

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

Comment on attachment 8545861 [details] [review]
Final Pull Request

Just left a nit on github, really tiny thing.

Code wise looking good to me, now with test (thanks a lot for adding them!), as you commented, waiting for the final resources.

Please merge once you have them.

Comment 17

[Jose Manuel Cantera]

https://github.com/mozilla-b2g/gaia/commit/25b310a10360a6063cfe06e7ef3840696314c564

in bug 1120392 we will land the correct icons for the 'share' action

Comment 18

[Fang Shih [:fang]]

I'm sorry for the delay in replying, Just attached the image assets in bug 1120392. Thanks!

Comment 19

[Beatriz Rodríguez [:brg]]

Tested with today build in Flame( Gecko-7a1f501.Gaia-9946a49) there is an option to share contact info in Contact details panel. Thank you everyone for making it happen!

Comment 20

[Jose Manuel Cantera]

Comment on attachment 8545861 [details] [review]
Final Pull Request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Share Contact Feature
[User impact] if declined: High, brand new 2.2 feature will be missing, allowing to share contacts from the Contacts App
[Testing completed]: Yes, unit tests and marionnette tests are provided. 
[Risk to taking this patch] (and alternatives if risky): No-risk patch
[String changes made]: new literal string 'shareContact' 

(Aside note)

We have to request the approval because for some strange reason this bug did not land on the v2.2 branch despite We had agreed to do so. It seems that the branching left it out for some unknown reason. So the approval is just a formality as it was already agreed to have this feature in v2.2

Comment 21

[bhavana bajaj [:bajaj]]

Eric,

Can we assign a QA to help test this enhancement ?

Comment 22

[bhavana bajaj [:bajaj]]

:brg/:oteo, any testing help we can get more your end on this will be super helpful! Just want to make sure we identify any fallouts sooner than later.

Comment 23

[Maria Angeles Oteo (:oteo)]

(In reply to bhavana bajaj [:bajaj] from comment #22)
> :brg/:oteo, any testing help we can get more your end on this will be super
> helpful! Just want to make sure we identify any fallouts sooner than late

Sure, we'll testing on our side and we'll let you know any issue discovered.

Eric, Johan, let me know if you want me to change the QA Contact to someone in Telefonica's team although I think Eric was already doing some testing of the feature and raising an issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1007932#c46)

Comment 24

[Johan Lorenzo [:jlorenzo]]

The Taiwan QA team is responsible for the validation of the 2.2 features on Mozilla's side. I prefer to let the decision in Eric's hands.

Comment 25

[Eric Chang [:ericcc] [:echang]]

Yes, I am working on another related case bug 1007932, so it looks like this one will be granted to 2.2+, right.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/64b1ba4f3ea63969ec3849bf1f49a8027153752f

Comment 28

[Shing Lyu [:shinglyu]]

Attachment: Testcases on MozTrap

Added a test case in MozTrap: https://moztrap.mozilla.org/manage/cases/?filter-id=15417

Comment 29

[Shally Li（leave from Mozilla）]

Per Comment 21 & Comment 25,and this bug had verified in https://bugzilla.mozilla.org/show_bug.cgi?id=1007932#c51 ,clear "verifyme" and modify as "VERIFIED".