1913488 - Uplift Fakespot fixes for 130 beta

Status: RESOLVED FIXED

(Application Services :: Suggest, enhancement)

Description

[Ben Dean-Kawamura [:bdk]]

We should uplift one or both of these changes into the 130 beta so that they will apply to the Fakespot suggest experiment that's launching in 130:

• https://github.com/mozilla/application-services/pull/6342 updates / improves the matching logic. This is pretty much a blocker for the experiment. I think this is low risk, I've been testing the logic locally from Rust and it works for me. There's a chance that there's an issue when integrating this with the desktop code, but this is a new feature so there's a similar risk with the old code. It's also all going to be behind an experiment flag.
• https://github.com/mozilla/application-services/pull/6334 fixes an edge case when ingesting suggestions from remote settings. When we were testing this in nightly, we realized that when old records were deleted on remote settings, they didn't end up deleted in the user's database. I think this is medium risk, mostly just because the change was pretty big. We can workaround the bug with some extra care when we update the remote settings data.

The first question is which of these should be uplifted. The second question is how to uplift.

I think I need to create a new app-services commit that is based of the current commit from beta (8fd08c6f2f8acd38579bd3142fecda9272957b72) and cherry-pick in the new commits. Then I need to vendor in those new commits into the beta branch directly. Nightly is going to get those changes and more when https://phabricator.services.mozilla.com/D219189 is merged, which I expect to do today. Does that process sound right and how can I make a patch that skips moz-central and goes directly to beta?

Comment 1

[Pascal Chevrel:pascalc]

I am on PTO, please contact Dianna net week about it thanks.

Comment 2

[Ben Dean-Kawamura [:bdk]]

Attachment: Bug 1913488 - Uplift application-services changes for 130

• Vendor the app-services changes aimed for uplift to 130
  (https://github.com/mozilla/application-services/pull/6346)
• Copied audit info and test fixes from https://phabricator.services.mozilla.com/D219189

Removed the IsValid check when converting a RustBuffer to an
OwnedRustBuffer. The vendored changes revealed some issues with this
logic. See https://bugzilla.mozilla.org/show_bug.cgi?id=1913183#c4 for
details.

Original Revision: https://phabricator.services.mozilla.com/D219189

Comment 3

[wrocklin]

This is timely and a high priority. Who is owning this? Would be great if both of these can be uplifted in v130 from a product standpoint.

Comment 4

[Ben Dean-Kawamura [:bdk]]

Comment on attachment 9419788 [details]
Bug 1913488 - Uplift application-services changes for 130

Beta/Release Uplift Approval Request

• User impact if declined: The Fakespot suggestion matching code will be worse. This will probably cause the experiment to not launch until v131.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This brings in a decent number of commits, but they essentially only touch the Fakespot experiment code. This experiment is behind a feature flag and so we're relying on manual QA testing rather than nightly users to verify it. Skipping the nightly cycle is not so important for this code. See https://github.com/mozilla/application-services/pull/6346 for more discussion.
• String changes made/needed:
• Is Android affected?: No

Comment 5

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/feadb10b2784