Closed Bug 1519418 Opened 11 months ago Closed 9 months ago

MMA deeplinks do not work on debug variants

Categories

(Firefox for Android :: General, defect, P1, trivial)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox66 + fixed
firefox67 + fixed

People

(Reporter: vlad.baicu, Assigned: vlad.baicu)

Details

Attachments

(3 files)

MMA deeplinks currently do not work on debug variants without overriding a check in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java#162.

This is due to the debug id not being stored in shared preferences -https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java#230.

On debug builds whenever we would attempt to retrieve the value of localUid it
would be null because the LP debug id is never persisted.

Daniel also any concerns with this change?

Flags: needinfo?(dveditz)

no

Flags: needinfo?(dveditz)
Keywords: checkin-needed

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/259e581af979
Retrieve LP debug id for local uid on debug builds. r=petru

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Reopening this as there is a problem with the matching of the local uid and the one received in the intent due to line separators.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/348428783522
Replace line separators from the uid parameter received from the deeplink intent. r=sdaswani

Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 11 months ago10 months ago
Resolution: --- → FIXED

Comment on attachment 9039863 [details]
Bug 1519418 - Replace line separators from the uid parameter received from the deeplink intent. r=sdaswani

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1519418 previous patch

User impact if declined

Patch from bug 1515651 will not work, because 259e581af979 introduces a regression for it. 259e581af979 landed in beta, whereas this patch did not.

Is this code covered by automated tests?

Unknown

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

Once uplifted, attempt to send a push notification with the following action: firefox://open?url=https://www.mozilla.org/ro/firefox/mobile/&uid={{User ID}}, if the notification is opened, the problem is fixed.

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

The uid parameter when extracted, has line separators which does not allow for a proper check with the one stored locally, we are merely removing those from the received value.

String changes made/needed

Attachment #9039863 - Flags: approval-mozilla-beta?

Comment on attachment 9039863 [details]
Bug 1519418 - Replace line separators from the uid parameter received from the deeplink intent. r=sdaswani

OK for uplift to beta 8; part of fix for bug 1515651.

Attachment #9039863 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Tested on the latest 66 Beta 11 and latest Nightly 67.0a1 (2019-02-27). I have received the push notifications but the notification does not open any link.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Patch D16319 attempted to clean the MMA code and allow for easier debugging
of LP deeplinks on dev builds. However it introduced a regression because the
MMA preferences were being accessed by the initializing activity. By making
getDeviceId public and static, calling it from another activity would result
in a null value returned. I have refactored the code to use shared preferences
and remove the dependency on other activities.

Keywords: checkin-needed

Comment on attachment 9048261 [details]
Bug 1519418 - Replace MMA preferences with sharedPreferences.r=JanH

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1519418 first patch
  • User impact if declined: Patch from bug 1515651 will not work.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Send a deeplink from the campaign set up in bug 1515651.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We are reverting to the previous approach used.
  • String changes made/needed:
Attachment #9048261 - Flags: approval-mozilla-beta?

I have submitted an uplift request for beta in order to have this align with the next release mid March as this patch is needed in order to achieve a succesful landing of the patch from bug 1515651.

Priority: P5 → P1

Thanks Vlad, looks like this should also land on mozilla-central so if you can check it in there, we can uplift after it's in nightly.

[Tracking Requested - why for this release]:

The feature added in bug 1515651 is expected by the marketing dept to land on 66. I have submitted the uplift request before landing on nightly so that after it lands, we can confirm it works and uplift afterwards, thus sparing some time.

ok, what we mean by "uplift" is backporting code to beta or release branches which is what we're doing here.
I don't understand - why not also land it on mozilla-central?

Comment on attachment 9048261 [details]
Bug 1519418 - Replace MMA preferences with sharedPreferences.r=JanH

I'm fine with landing this on beta, though we also need it on m-c.

Attachment #9048261 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I think I get what you intended now. You were just putting in the uplift request and this is likely going to land on m-c tomorrow.

Still not seeing this land on m-c - can you land it?

Flags: needinfo?(vlad.baicu)

Seems like the checkin-needed got cleared when it landed on beta, I've reapplied it.

Flags: needinfo?(vlad.baicu)
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 10 months ago9 months ago
Resolution: --- → FIXED
Target Milestone: Firefox 66 → Firefox 67
You need to log in before you can comment on or make changes to this bug.