845278 - Regression caused by 843163. Callback missing

Status: RESOLVED FIXED

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

Comment 1

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 2

[Borja Salguero [:borjasalguero]]

We left a var in the previous PR!! :S

Comment 3

[Fernando Campo (:fcampo)]

Comment on attachment 718376 [details]
Pull Request

Fixing regression, no risk on merging (actually, the opposite!)

Comment 4

[Borja Salguero [:borjasalguero]]

https://github.com/mozilla-b2g/gaia/commit/9be4d4bd72f001e60d9a0c787a1f36ccad5157f4
https://github.com/borjasalguero/gaia/commit/4b4f1d44034b772e50e199e2fd6ce0902a92e4f6

Comment 5

[Borja Salguero [:borjasalguero]]

Comment on attachment 718376 [details]
Pull Request

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 #): Regression by 843163 
User impact if declined: SMS It's not working anymore
Testing completed: 
Risk to taking this patch (and alternatives if risky): None. We have to land this one after 843163 in order to have SMS working again & code ready for optimization PR.
String or UUID changes made by this patch:

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Will wait for the answer to the question in bug 845278 before approving this.

Comment 7

[Maria Angeles Oteo (:oteo)]

Already answered Lukas :)

Comment 8

[Maria Angeles Oteo (:oteo)]

Already answered in bug 843163

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 718376 [details]
Pull Request

Approving to go along with bug 843163

Comment 10

[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:
Loading commit times database
  git checkout v1-train
  git cherry-pick -x -m1 9be4d4bd72f001e60d9a0c787a1f36ccad5157f4
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 11

[Julien Wajsberg [:julienw]]

A chain of conflicts leads to Bug 837267, where I asked for approval.

Comment 12

[Julien Wajsberg [:julienw]]

This bug need tef+ because this is a regression to tef+ Bug 843163.

Comment 13

[Julien Wajsberg [:julienw]]

v1-train: 74e15f1721f22035d41167fd4d4b9cd2dbffcfac

Comment 15

[Michael Vines [:m1] [:evilmachines]]

(tef+ per comment 12)

Comment 16

[Alex Keybl [:akeybl]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> v1-train: 74e15f1721f22035d41167fd4d4b9cd2dbffcfac

I imagine uplift to v1.0.1 will also be difficult - can you help jhford uplift there as well?

Comment 17

[Julien Wajsberg [:julienw]]

I asked some help from Borja for Bug 844770 first.

Comment 18

[amiller]

Does not make sense to create a regression issue.

Comment 19

[Borja Salguero [:borjasalguero]]

Fixed https://github.com/mozilla-b2g/gaia/commit/565dbcdb22690a4e6c49093257161264a8d4ad70#apps/sms

Comment 20

[Borja Salguero [:borjasalguero]]

After checking I've realized that was uplifted to v1-train as well. Fixed.

Comment 21

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 22

[Borja Salguero [:borjasalguero]]

There is nothing to check here more than testing than SMS App is working as expected, due to this change it's tied to performance issues.

Comment 23

[John Hammink]

Check that SMS ain't blowing up in smoketest.