Bug 1837097 - Implement "Show less frequently" behavior for Pocket suggestions. r=daisuke

Same as addon suggestions.

Depends on D182632

Differential Revision: https://phabricator.services.mozilla.com/D182606
This commit is contained in:
Drew Willcoxon 2023-07-05 15:47:24 +00:00
parent 5a79fd667d
commit 6ab4467c1a
7 changed files with 435 additions and 226 deletions

View File

@ -189,6 +189,10 @@ const PREF_URLBAR_DEFAULTS = new Map([
// Feature gate pref for Pocket suggestions in the urlbar.
["pocket.featureGate", false],
// The number of times the user has clicked the "Show less frequently" command
// for Pocket suggestions.
["pocket.showLessFrequentlyCount", 0],
// When true, URLs in the user's history that look like search result pages
// are styled to look like search engine results instead of the usual history
// results.
@ -453,6 +457,7 @@ const NIMBUS_DEFAULTS = {
addonsUITreatment: "a",
experimentType: "",
isBestMatchExperiment: false,
pocketShowLessFrequentlyCap: 0,
quickSuggestRemoteSettingsDataType: "data",
quickSuggestScoreMap: null,
recordNavigationalSuggestionTelemetry: false,

View File

@ -57,9 +57,11 @@ export class PocketSuggestions extends BaseFeature {
}
get canShowLessFrequently() {
// TODO (bug 1837097): To be implemented once the "Show less frequently"
// logic is decided.
return false;
let cap =
lazy.UrlbarPrefs.get("pocketShowLessFrequentlyCap") ||
lazy.QuickSuggestRemoteSettings.config.show_less_frequently_cap ||
0;
return !cap || this.showLessFrequentlyCount < cap;
}
enable(enabled) {
@ -144,6 +146,26 @@ export class PocketSuggestions extends BaseFeature {
}
makeResult(queryContext, suggestion, searchString) {
if (!this.isEnabled) {
// The feature is disabled on the client, but Merino may still return
// suggestions anyway, and we filter them out here.
return null;
}
// If the user hasn't clicked the "Show less frequently" command, the
// suggestion can be shown. Otherwise, the suggestion can be shown if the
// user typed more than one word with at least `showLessFrequentlyCount`
// characters after the first word, including spaces.
if (this.showLessFrequentlyCount) {
let spaceIndex = searchString.search(/\s/);
if (
spaceIndex < 0 ||
searchString.length - spaceIndex < this.showLessFrequentlyCount
) {
return null;
}
}
let isBestMatch =
suggestion.is_top_pick &&
lazy.UrlbarPrefs.get("bestMatchEnabled") &&
@ -193,7 +215,7 @@ export class PocketSuggestions extends BaseFeature {
break;
case RESULT_MENU_COMMAND.SHOW_LESS_FREQUENTLY:
queryContext.view.acknowledgeFeedback(result);
this.#incrementShowLessFrequentlyCount();
this.incrementShowLessFrequentlyCount();
break;
}
}
@ -242,7 +264,7 @@ export class PocketSuggestions extends BaseFeature {
return commands;
}
#incrementShowLessFrequentlyCount() {
incrementShowLessFrequentlyCount() {
if (this.canShowLessFrequently) {
lazy.UrlbarPrefs.set(
"pocket.showLessFrequentlyCount",

View File

@ -13,7 +13,7 @@ const REMOTE_SETTINGS_DATA = [
url: "https://example.com/pocket-suggestion",
title: "Pocket Suggestion",
description: "Pocket description",
lowConfidenceKeywords: ["pocket-suggestion"],
lowConfidenceKeywords: ["pocket suggestion"],
highConfidenceKeywords: ["high"],
},
],
@ -47,7 +47,7 @@ add_task(async function basic() {
// Do a search.
await UrlbarTestUtils.promiseAutocompleteResultPopup({
window,
value: "pocket-suggestion",
value: "pocket suggestion",
});
// Check the result.
@ -85,6 +85,132 @@ add_task(async function basic() {
});
});
// Tests the "Show less frequently" command.
add_task(async function resultMenu_showLessFrequently() {
await SpecialPowers.pushPrefEnv({
set: [
["browser.urlbar.pocket.featureGate", true],
["browser.urlbar.pocket.showLessFrequentlyCount", 0],
],
});
const cleanUpNimbus = await UrlbarTestUtils.initNimbusFeature({
pocketShowLessFrequentlyCap: 3,
});
// Sanity check.
Assert.equal(UrlbarPrefs.get("pocketShowLessFrequentlyCap"), 3);
Assert.equal(UrlbarPrefs.get("pocket.showLessFrequentlyCount"), 0);
await doShowLessFrequently({
input: "pocket s",
expected: {
isSuggestionShown: true,
isMenuItemShown: true,
},
});
Assert.equal(UrlbarPrefs.get("pocket.showLessFrequentlyCount"), 1);
await doShowLessFrequently({
input: "pocket s",
expected: {
isSuggestionShown: true,
isMenuItemShown: true,
},
});
Assert.equal(UrlbarPrefs.get("pocket.showLessFrequentlyCount"), 2);
await doShowLessFrequently({
input: "pocket s",
expected: {
isSuggestionShown: true,
isMenuItemShown: true,
},
});
Assert.equal(UrlbarPrefs.get("pocket.showLessFrequentlyCount"), 3);
await doShowLessFrequently({
input: "pocket s",
expected: {
isSuggestionShown: false,
},
});
await doShowLessFrequently({
input: "pocket su",
expected: {
isSuggestionShown: true,
isMenuItemShown: false,
},
});
await cleanUpNimbus();
await SpecialPowers.popPrefEnv();
});
async function doShowLessFrequently({ input, expected }) {
await UrlbarTestUtils.promiseAutocompleteResultPopup({
window,
value: input,
});
if (!expected.isSuggestionShown) {
for (let i = 0; i < UrlbarTestUtils.getResultCount(window); i++) {
const details = await UrlbarTestUtils.getDetailsOfResultAt(window, i);
Assert.notEqual(
details.result.payload.telemetryType,
"pocket",
`Pocket suggestion should be absent (checking index ${i})`
);
}
return;
}
const resultIndex = 1;
const details = await UrlbarTestUtils.getDetailsOfResultAt(
window,
resultIndex
);
Assert.equal(
details.result.payload.telemetryType,
"pocket",
`Pocket suggestion should be present at expected index after ${input} search`
);
// Click the command.
try {
await UrlbarTestUtils.openResultMenuAndClickItem(
window,
"show_less_frequently",
{
resultIndex,
}
);
Assert.ok(expected.isMenuItemShown);
Assert.ok(
gURLBar.view.isOpen,
"The view should remain open clicking the command"
);
Assert.ok(
details.element.row.hasAttribute("feedback-acknowledgment"),
"Row should have feedback acknowledgment after clicking command"
);
} catch (e) {
Assert.ok(!expected.isMenuItemShown);
Assert.ok(
!details.element.row.hasAttribute("feedback-acknowledgment"),
"Row should not have feedback acknowledgment after clicking command"
);
Assert.equal(
e.message,
"Menu item not found for command: show_less_frequently"
);
}
await UrlbarTestUtils.promisePopupClose(window);
}
// Tests the "Not interested" result menu dismissal command.
add_task(async function resultMenu_notInterested() {
await doDismissTest("not_interested");
@ -99,7 +225,7 @@ async function doDismissTest(command) {
// Do a search.
await UrlbarTestUtils.promiseAutocompleteResultPopup({
window,
value: "pocket-suggestion",
value: "pocket suggestion",
});
// Check the result.
@ -189,7 +315,7 @@ async function doDismissTest(command) {
// Do the search again.
await UrlbarTestUtils.promiseAutocompleteResultPopup({
window,
value: "pocket-suggestion",
value: "pocket suggestion",
});
for (let i = 0; i < UrlbarTestUtils.getResultCount(window); i++) {
details = await UrlbarTestUtils.getDetailsOfResultAt(window, i);

View File

@ -225,3 +225,245 @@ async function doMigrateTest({
}
UrlbarPrefs._updatingFirefoxSuggestScenario = false;
}
/**
* Does some "show less frequently" tests where the cap is set in remote
* settings and Nimbus. See `doOneShowLessFrequentlyTest()`. This function
* assumes the matching behavior implemented by the given `BaseFeature` is based
* on matching prefixes of the given keyword starting at the first word. It
* also assumes the `BaseFeature` provides suggestions in remote settings.
*
* @param {object} options
* Options object.
* @param {BaseFeature} options.feature
* The feature that provides the suggestion matched by the searches.
* @param {*} options.expectedResult
* The expected result that should be matched, for searches that are expected
* to match a result. Can also be a function; it's passed the current search
* string and it should return the expected result.
* @param {string} options.showLessFrequentlyCountPref
* The name of the pref that stores the "show less frequently" count being
* tested.
* @param {string} options.nimbusCapVariable
* The name of the Nimbus variable that stores the "show less frequently" cap
* being tested.
* @param {object} options.keyword
* The primary keyword to use during the test. It must contain more than one
* word, and it must have at least two chars after the first space.
*/
async function doShowLessFrequentlyTests({
feature,
expectedResult,
showLessFrequentlyCountPref,
nimbusCapVariable,
keyword,
}) {
// Do some sanity checks on the keyword. Any checks that fail are errors in
// the test. This function assumes
let spaceIndex = keyword.indexOf(" ");
if (spaceIndex < 0) {
throw new Error("keyword must contain a space");
}
if (spaceIndex == 0) {
throw new Error("keyword must not start with a space");
}
if (keyword.length < spaceIndex + 3) {
throw new Error("keyword must have at least two chars after the space");
}
let tests = [
{
showLessFrequentlyCount: 0,
canShowLessFrequently: true,
newSearches: {
[keyword.substring(0, spaceIndex - 1)]: false,
[keyword.substring(0, spaceIndex)]: true,
[keyword.substring(0, spaceIndex + 1)]: true,
[keyword.substring(0, spaceIndex + 2)]: true,
[keyword.substring(0, spaceIndex + 3)]: true,
},
},
{
showLessFrequentlyCount: 1,
canShowLessFrequently: true,
newSearches: {
[keyword.substring(0, spaceIndex)]: false,
},
},
{
showLessFrequentlyCount: 2,
canShowLessFrequently: true,
newSearches: {
[keyword.substring(0, spaceIndex + 1)]: false,
},
},
{
showLessFrequentlyCount: 3,
canShowLessFrequently: false,
newSearches: {
[keyword.substring(0, spaceIndex + 2)]: false,
},
},
{
showLessFrequentlyCount: 3,
canShowLessFrequently: false,
newSearches: {},
},
];
info("Testing 'show less frequently' with cap in remote settings");
await doOneShowLessFrequentlyTest({
tests,
feature,
expectedResult,
showLessFrequentlyCountPref,
rs: {
show_less_frequently_cap: 3,
},
});
// Nimbus should override remote settings.
info("Testing 'show less frequently' with cap in Nimbus and remote settings");
await doOneShowLessFrequentlyTest({
tests,
feature,
expectedResult,
showLessFrequentlyCountPref,
rs: {
show_less_frequently_cap: 10,
},
nimbus: {
[nimbusCapVariable]: 3,
},
});
}
/**
* Does a group of searches, increments the "show less frequently" count, and
* repeats until all groups are done. The cap can be set by remote settings
* config and/or Nimbus.
*
* @param {object} options
* Options object.
* @param {BaseFeature} options.feature
* The feature that provides the suggestion matched by the searches.
* @param {*} options.expectedResult
* The expected result that should be matched, for searches that are expected
* to match a result. Can also be a function; it's passed the current search
* string and it should return the expected result.
* @param {string} options.showLessFrequentlyCountPref
* The name of the pref that stores the "show less frequently" count being
* tested.
* @param {object} options.tests
* An array where each item describes a group of new searches to perform and
* expected state. Each item should look like this:
* `{ showLessFrequentlyCount, canShowLessFrequently, newSearches }`
*
* {number} showLessFrequentlyCount
* The expected value of `showLessFrequentlyCount` before the group of
* searches is performed.
* {boolean} canShowLessFrequently
* The expected value of `canShowLessFrequently` before the group of
* searches is performed.
* {object} newSearches
* An object that maps each search string to a boolean that indicates
* whether the first remote settings suggestion should be triggered by the
* search string. Searches are cumulative: The intended use is to pass a
* large initial group of searches in the first search group, and then each
* following `newSearches` is a diff against the previous.
* @param {object} options.rs
* The remote settings config to set.
* @param {object} options.nimbus
* The Nimbus variables to set.
*/
async function doOneShowLessFrequentlyTest({
feature,
expectedResult,
showLessFrequentlyCountPref,
tests,
rs = {},
nimbus = {},
}) {
// Disable Merino so we trigger only remote settings suggestions. The
// `BaseFeature` is expected to add remote settings suggestions using keywords
// start starting with the first word in each full keyword, but the mock
// Merino server will always return whatever suggestion it's told to return
// regardless of the search string. That means Merino will return a suggestion
// for a keyword that's smaller than the first full word.
UrlbarPrefs.set("quicksuggest.dataCollection.enabled", false);
// Set Nimbus variables and RS config.
let cleanUpNimbus = await UrlbarTestUtils.initNimbusFeature(nimbus);
await QuickSuggestTestUtils.withConfig({
config: rs,
callback: async () => {
let cumulativeSearches = {};
for (let {
showLessFrequentlyCount,
canShowLessFrequently,
newSearches,
} of tests) {
info(
"Starting subtest: " +
JSON.stringify({
showLessFrequentlyCount,
canShowLessFrequently,
newSearches,
})
);
Assert.equal(
feature.showLessFrequentlyCount,
showLessFrequentlyCount,
"showLessFrequentlyCount should be correct initially"
);
Assert.equal(
UrlbarPrefs.get(showLessFrequentlyCountPref),
showLessFrequentlyCount,
"Pref should be correct initially"
);
Assert.equal(
feature.canShowLessFrequently,
canShowLessFrequently,
"canShowLessFrequently should be correct initially"
);
// Merge the current `newSearches` object into the cumulative object.
cumulativeSearches = {
...cumulativeSearches,
...newSearches,
};
for (let [searchString, isExpected] of Object.entries(
cumulativeSearches
)) {
info("Doing search: " + JSON.stringify({ searchString, isExpected }));
let results = [];
if (isExpected) {
results.push(
typeof expectedResult == "function"
? expectedResult(searchString)
: expectedResult
);
}
await check_results({
context: createContext(searchString, {
providers: [UrlbarProviderQuickSuggest.name],
isPrivate: false,
}),
matches: results,
});
}
feature.incrementShowLessFrequentlyCount();
}
},
});
await cleanUpNimbus();
UrlbarPrefs.clear(showLessFrequentlyCountPref);
UrlbarPrefs.set("quicksuggest.dataCollection.enabled", true);
}

View File

@ -481,226 +481,21 @@ add_task(async function merinoIsTopPick() {
});
});
// Tests "show less frequently" with the cap set in remote settings.
add_task(async function showLessFrequently_rs() {
await doShowLessFrequentlyTest({
rs: {
show_less_frequently_cap: 3,
},
tests: [
{
showLessFrequentlyCount: 0,
canShowLessFrequently: true,
searches: {
f: false,
fi: false,
fir: false,
firs: false,
first: true,
t: false,
tw: false,
two: true,
"two ": true,
"two w": true,
"two wo": true,
"two wor": true,
"two word": true,
"two words": true,
a: true,
"a ": true,
"a b": true,
"a b ": true,
"a b c": true,
},
},
{
showLessFrequentlyCount: 1,
canShowLessFrequently: true,
searches: {
first: false,
two: false,
a: false,
},
},
{
showLessFrequentlyCount: 2,
canShowLessFrequently: true,
searches: {
"two ": false,
"a ": false,
},
},
{
showLessFrequentlyCount: 3,
canShowLessFrequently: false,
searches: {
"two w": false,
"a b": false,
},
},
{
showLessFrequentlyCount: 3,
canShowLessFrequently: false,
searches: {},
},
],
// Tests the "show less frequently" behavior.
add_task(async function showLessFrequently() {
await doShowLessFrequentlyTests({
feature: QuickSuggest.getFeature("AddonSuggestions"),
showLessFrequentlyCountPref: "addons.showLessFrequentlyCount",
nimbusCapVariable: "addonsShowLessFrequentlyCap",
expectedResult: makeExpectedResult({
suggestion: REMOTE_SETTINGS_RESULTS[0].attachment[0],
source: "remote-settings",
isTopPick: true,
}),
keyword: "two words",
});
});
// Tests "show less frequently" with the cap set in both Nimbus and remote
// settings. Nimbus should override remote settings.
add_task(async function showLessFrequently_nimbus() {
await doShowLessFrequentlyTest({
nimbus: {
addonsShowLessFrequentlyCap: 3,
},
rs: {
show_less_frequently_cap: 10,
},
tests: [
{
showLessFrequentlyCount: 0,
canShowLessFrequently: true,
searches: {
a: true,
"a ": true,
"a b": true,
"a b ": true,
"a b c": true,
},
},
{
showLessFrequentlyCount: 1,
canShowLessFrequently: true,
searches: {
a: false,
},
},
{
showLessFrequentlyCount: 2,
canShowLessFrequently: true,
searches: {
"a ": false,
},
},
{
showLessFrequentlyCount: 3,
canShowLessFrequently: false,
searches: {
"a b": false,
},
},
{
showLessFrequentlyCount: 3,
canShowLessFrequently: false,
searches: {},
},
],
});
});
/**
* Does a group of searches, increments the `showLessFrequentlyCount`, and
* repeats until all groups are done. The cap can be set by remote settings
* config and/or Nimbus.
*
* @param {object} options
* Options object.
* @param {object} options.tests
* An array where each item describes a group of searches to perform and
* expected state. Each item should look like this:
* `{ showLessFrequentlyCount, canShowLessFrequently, searches }`
*
* {number} showLessFrequentlyCount
* The expected value of `showLessFrequentlyCount` before the group of
* searches is performed.
* {boolean} canShowLessFrequently
* The expected value of `canShowLessFrequently` before the group of
* searches is performed.
* {object} searches
* An object that maps each search string to a boolean that indicates
* whether the first remote settings suggestion should be triggered by the
* search string. `searches` objects are cumulative: The intended use is to
* pass a large initial group of searches in the first search group, and
* then each following `searches` is a diff against the previous.
* @param {object} options.rs
* The remote settings config to set.
* @param {object} options.nimbus
* The Nimbus variables to set.
*/
async function doShowLessFrequentlyTest({ tests, rs = {}, nimbus = {} }) {
// Disable Merino so we trigger only remote settings suggestions.
UrlbarPrefs.set("quicksuggest.dataCollection.enabled", false);
// We'll be testing with the first remote settings suggestion.
let suggestion = REMOTE_SETTINGS_RESULTS[0].attachment[0];
let addonSuggestions = QuickSuggest.getFeature("AddonSuggestions");
// Set Nimbus variables and RS config.
let cleanUpNimbus = await UrlbarTestUtils.initNimbusFeature(nimbus);
await QuickSuggestTestUtils.withConfig({
config: rs,
callback: async () => {
let cumulativeSearches = {};
for (let {
showLessFrequentlyCount,
canShowLessFrequently,
searches,
} of tests) {
Assert.equal(
addonSuggestions.showLessFrequentlyCount,
showLessFrequentlyCount,
"showLessFrequentlyCount should be correct initially"
);
Assert.equal(
UrlbarPrefs.get("addons.showLessFrequentlyCount"),
showLessFrequentlyCount,
"Pref should be correct initially"
);
Assert.equal(
addonSuggestions.canShowLessFrequently,
canShowLessFrequently,
"canShowLessFrequently should be correct initially"
);
// Merge the current `searches` object into the cumulative object.
cumulativeSearches = {
...cumulativeSearches,
...searches,
};
for (let [searchString, isExpected] of Object.entries(
cumulativeSearches
)) {
await check_results({
context: createContext(searchString, {
providers: [UrlbarProviderQuickSuggest.name],
isPrivate: false,
}),
matches: !isExpected
? []
: [
makeExpectedResult({
suggestion,
source: "remote-settings",
isTopPick: true,
}),
],
});
}
addonSuggestions.incrementShowLessFrequentlyCount();
}
},
});
await cleanUpNimbus();
UrlbarPrefs.clear("addons.showLessFrequentlyCount");
UrlbarPrefs.set("quicksuggest.dataCollection.enabled", true);
}
function makeExpectedResult({ suggestion, source, isTopPick }) {
let provider;
let rating;

View File

@ -409,6 +409,18 @@ add_task(async function block() {
});
});
// Tests the "show less frequently" behavior.
add_task(async function showLessFrequently() {
await doShowLessFrequentlyTests({
feature: QuickSuggest.getFeature("PocketSuggestions"),
showLessFrequentlyCountPref: "pocket.showLessFrequentlyCount",
nimbusCapVariable: "pocketShowLessFrequentlyCap",
expectedResult: searchString =>
makeExpectedResult({ searchString, fullKeyword: LOW_KEYWORD }),
keyword: LOW_KEYWORD,
});
});
function makeExpectedResult({
searchString,
fullKeyword = searchString,

View File

@ -239,6 +239,13 @@ urlbar:
description: >-
Feature gate that controls whether all aspects of the Pocket suggestions
feature are exposed to the user.
pocketShowLessFrequentlyCap:
type: int
description: >-
If defined and non-zero, this is the maximum number of times the user
will be able to click the "Show less frequently" command for Pocket
suggestions. If undefined or zero, the user will be able to click the
command without any limit.
quickSuggestAllowPositionInSuggestions:
type: boolean
fallbackPref: browser.urlbar.quicksuggest.allowPositionInSuggestions