Wesley Huang [:wesley_huang] (EPM) (NI me) Reporter
	Description • 10 years ago

Per the comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=978639#c13

This bugs for derived efforts from new API telephonyCallGroup.hangUp()

	Wesley Huang [:wesley_huang] (EPM) (NI me) Reporter
	Comment 1 • 10 years ago

[Blocking Requested - why for this release]:
This one is GAIA portion to fix bug 978639 which is CAF blocker.

	bhavana bajaj [:bajaj]
	Comment 2 • 10 years ago

Preemptively blocking as this blocks CAF release and needs 978639 to be resolved.

Please let me know if the effort/risk is not manageable to close this in the next few days..

	Doug Sherk (:drs) (inactive)
	Comment 3 • 10 years ago

Considering that bug 978639 doesn't even have a WIP patch posted yet, I'm skeptical of us being able to land the Gaia side in the next few days, which I interpret as meaning by the end of this week.

Having said that, if bug 978639 lands soon, I don't think there's much risk on the Gaia side.

	Doug Sherk (:drs) (inactive)
	Comment 4 • 10 years ago

bug 978639 has an ETA of 10/17. I think we can do this in 3 days. Bhavana, what did you mean by "next few days"?

	bhavana bajaj [:bajaj]
	Comment 5 • 10 years ago

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #4)
> bug 978639 has an ETA of 10/17. I think we can do this in 3 days. Bhavana,
> what did you mean by "next few days"?

Thanks for making the dependency clear. Given this is blocking the partner CS release we needed a clear eta here and a quick fix. I am going to set expectation that once 978639 is resolved we will be able to fix this in a couple of days as you mentioned.

	Tamara Hills [:thills] Assignee
	Comment 6 • 10 years ago

Hi Doug,
I'm working on tests, but wanted to get your feedback.

Also, if you can comment on where I moved the line: setEndConferenceCall.  I put it inside the .then clause, but I could also see that it could be put before the hangUp is called (that's where it was before).  But if the API fails, we could have an incorrect state between UI and what's actually there with the calls(maybe?).

Thanks,

-tamara

	Doug Sherk (:drs) (inactive)
	Comment 7 • 10 years ago

Comment on attachment 8507991 [details] [diff] [review]
Feedback patch

Review of attachment 8507991 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this looks reasonable.

My one concern is that this will run anyways even if `endConferenceCall()` fails:
https://github.com/mozilla-b2g/gaia/blob/bae49d63304b4980c7f17681ba87bec38d6eb8ca/apps/callscreen/js/calls_handler.js#L453

Perhaps we could return the `hangUp()` Promise and chain it instead.

	Tamara Hills [:thills] Assignee
	Comment 8 • 10 years ago

Hi Doug,

Attached is PR updated based on feedback.  I also modified the suite I was working in to use the new spy pattern since I was in there.

Thanks,
-tamara

	Doug Sherk (:drs) (inactive)
	Comment 9 • 10 years ago

Comment on attachment 8508984 [details]
Patch for review

Please see my comments on the PR.

	Tamara Hills [:thills] Assignee
	Comment 10 • 10 years ago

Hi Doug,

The changes are in the second commit.  

Thanks,

-tamara

	Doug Sherk (:drs) (inactive)
	Comment 11 • 10 years ago

Comment on attachment 8509151 [details]
Changes after review

Looks good. I left 3 nits on the PR.

	Doug Sherk (:drs) (inactive)
	Comment 12 • 10 years ago

https://github.com/mozilla-b2g/gaia/commit/2d7386db0496283b10731ba459994b3cff8b6d26

Please request approval for uplift to v2.1.

	Tamara Hills [:thills] Assignee
	Comment 13 • 10 years ago

Comment on attachment 8509151 [details]
Changes after review

This should be landed on 2.1 *after* bug 978639 lands on gecko.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): gap in testing
[User impact] if declined: adherence to standard will be missing.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]:

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

v2.1: https://github.com/mozilla-b2g/gaia/commit/8ad73b2fa8e1019dcb828c572c742fd55f1e0a18

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 10 years ago

I had to revert this because bug 978639 is being backed out.
v2.1: https://github.com/mozilla-b2g/gaia/commit/c97463d61f45513a2123b19610386ddbfc916819

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

v2.1: https://github.com/mozilla-b2g/gaia/commit/81c2596e3282eb096054b894e308b38ef18780ad

Does this need to land on 2.0m too?

	Tamara Hills [:thills] Assignee
	Comment 17 • 10 years ago

Hi Ryan,

Just reading bug 978639 and it looks like we need it on 2.0m as well.

I haven't had a chance to try it out on that branch yet, so would like to do it before we go ahead and merge it.

Thanks,

-tamara

	Doug Sherk (:drs) (inactive)
	Comment 18 • 10 years ago

Comment on attachment 8509151 [details]
Changes after review

See comment 13.

I checked and this shouldn't need anything special to land on 2.0m. The affected code is almost identical.

	sarsenyev
	Comment 19 • 10 years ago

Unable to verify as it's a back-end issue

	bhavana bajaj [:bajaj]
	Comment 20 • 10 years ago

NI kai-zhen for 2.0M landing.

	bhavana bajaj [:bajaj]
	Comment 21 • 10 years ago

Comment on attachment 8509151 [details]
Changes after review

Clearing the approval as kai-zhen should take care of this landing on 2.0M.

	Kai-Zhen Li [:seinlin][:kli]
	Comment 22 • 10 years ago

v2.0m: https://github.com/mozilla-b2g/gaia/commit/94dbf0a1a65736d97342763fd95e672374835990