addonRepository returns emtpy array for rtamo

VERIFIED FIXED in Firefox 66

Status

defect
VERIFIED FIXED
4 months ago
3 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Dependency tree / graph

Firefox Tracking Flags

(firefox66+ verified, firefox67 verified)

Details

Attachments

(1 attachment)

Assignee

Description

4 months ago

It turns out the kill switch id we use results in an empty array returned from AddonRepository due to the id we send being different from what we get back[1].

Alternate fixes:

  • Wait for 67: switch back to Fetch
    • but make sure there is one central implementation
    • make sure there are no hard coded urls as before
  • Get it into 66: AMO returns the id it received

In the second case this could be handled outside firefox. Looking over the code, nothing in AddonRepository (result from getAddonsByIDs) uses the id beyond the filter mentioned. ActivityStream also does not use the id[2].

[1] https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/toolkit/mozapps/extensions/internal/AddonRepository.jsm#469
[2] https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/browser/components/newtab/lib/OnboardingMessageProvider.jsm#207

Assignee

Comment 1

4 months ago

We could also (if delaying till 67) fix AddonRepository to recognize the rta id.

CCing Stuart and Mat for visibility on the AMO side.

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)

We could also (if delaying till 67) fix AddonRepository to recognize the rta id.

If this is the only blocker and rtamo is otherwise ready for 66, this would probably be a small enough change to uplift to beta.

FWIW I would largely prefer not adding a hack to AMO for this.

(In reply to Mathieu Pillard [:mat] from comment #4)

FWIW I would largely prefer not adding a hack to AMO for this.

I second this, and if an uplift for 66 is possible that sounds like the best solution to me.

Assignee

Comment 6

4 months ago

AddonRepository validates IDs that are returned from AMO. We
need to handle the case where we are using rta prefix for ReturnToAMO.

Assignee

Updated

4 months ago
Assignee: nobody → mixedpuppy
Assignee

Comment 7

4 months ago

I've also manually verified the fix against a download on windows, about:welcome shows me tabby cat!

Comment 9

4 months ago
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d4fca2edcb5
support RTA IDs in AddonRepository r=aswan
Assignee

Comment 10

4 months ago

Comment on attachment 9047102 [details]
Bug 1530816 support RTA IDs in AddonRepository

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: addon recommendations cannot be provided.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: QE has test plans for the entire RTAMO flow.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Limited to AMO api use. Tests coverage is good on this.
  • String changes made/needed: none
Attachment #9047102 - Flags: approval-mozilla-beta?

Comment on attachment 9047102 [details]
Bug 1530816 support RTA IDs in AddonRepository

Support for testing return to AMO.
OK for uplift for beta 12.

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

Comment 12

4 months ago
bugherder
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Comment 13

4 months ago

Verified using the above build for Fx67 using multiple white listed extensions including tabby cat, everything appears fine using this version. Tests were run using Windows 10 x64.

Comment 15

3 months ago

Verified using Firefox 66.0 (20190307095232) on Windows 10 x64 bit. Testing has been done in the same way as on Fx67, no issues have occurred with either white listed or non white listed addons.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.