Feedback: Add hidden logic for assistant logs checkbox
Add a logic for the assistant logs checkbox to be hidden:
- if the FeedbackSource is not from kFeedbackSourceAssistant, or
- the logged in account is not an internal google account.
--gtest_filter=ShowFeedbackPageBrowserTest.*
Test: browser_tests --gtest_filter=OSFeedbackBrowserTest.*;
Bug: b:233078878
Change-Id: Ic51a09d5936b14ba0154cf1c8c4f8c499123008e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3929667
Commit-Queue: Yuhan Yang <yyhyyh@google.com>
Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Xiangdong Kong <xiangdongkong@google.com>
Cr-Commit-Position: refs/heads/main@{#1055091}
diff --git a/ash/webui/os_feedback_ui/mojom/os_feedback_ui.mojom b/ash/webui/os_feedback_ui/mojom/os_feedback_ui.mojom
index e7f02aa..8b283152 100644
--- a/ash/webui/os_feedback_ui/mojom/os_feedback_ui.mojom
+++ b/ash/webui/os_feedback_ui/mojom/os_feedback_ui.mojom
@@ -74,6 +74,8 @@
string? email;
// Whether or not the signed in email is an internal google account.
bool is_internal_account;
+ // Whether or not the feedback app is opened from Assistant.
+ bool from_assistant;
// The URL of the page that this issue was being experienced on.
url.mojom.Url? page_url;
// Extra diagnostics information provided by source CrOS application by
diff --git a/ash/webui/os_feedback_ui/resources/fake_data.js b/ash/webui/os_feedback_ui/resources/fake_data.js
index 47ad2fd..b7ba211 100644
--- a/ash/webui/os_feedback_ui/resources/fake_data.js
+++ b/ash/webui/os_feedback_ui/resources/fake_data.js
@@ -81,6 +81,7 @@
email: 'test.user2@test.com',
pageUrl: {url: 'chrome://tab/'},
isInternalAccount: false,
+ fromAssistant: false,
traceId: 1,
};
@@ -89,6 +90,7 @@
email: '',
pageUrl: {url: ''},
isInternalAccount: false,
+ fromAssistant: false,
traceId: 0,
};
@@ -97,6 +99,7 @@
email: 'test.user@google.com',
pageUrl: {url: 'chrome://tab/'},
isInternalAccount: true,
+ fromAssistant: false,
traceId: 1,
};
diff --git a/ash/webui/os_feedback_ui/resources/feedback_flow.html b/ash/webui/os_feedback_ui/resources/feedback_flow.html
index 3f606c6f..3d5d6a6 100644
--- a/ash/webui/os_feedback_ui/resources/feedback_flow.html
+++ b/ash/webui/os_feedback_ui/resources/feedback_flow.html
@@ -7,6 +7,7 @@
no-help-content-displayed="{{noHelpContentDisplayed_}}">
</search-page>
<share-data-page id="shareDataPage" feedback-context="[[feedbackContext_]]"
+ should-show-assistant-checkbox="[[shouldShowAssistantCheckbox_]]"
should-show-bluetooth-checkbox="[[shouldShowBluetoothCheckbox_]]"
on-continue-click="handleContinueClick_"
on-go-back-click="handleGoBackClick_">
diff --git a/ash/webui/os_feedback_ui/resources/feedback_flow.js b/ash/webui/os_feedback_ui/resources/feedback_flow.js
index f573b04c..776cd15e 100644
--- a/ash/webui/os_feedback_ui/resources/feedback_flow.js
+++ b/ash/webui/os_feedback_ui/resources/feedback_flow.js
@@ -55,6 +55,7 @@
EXTRA_DIAGNOSTICS: 'extra_diagnostics',
CATEGORY_TAG: 'category_tag',
PAGE_URL: 'page_url',
+ FROM_ASSISTANT: 'from_assistant',
};
/**
@@ -181,6 +182,12 @@
*/
this.shouldShowBluetoothCheckbox_;
+ /**
+ * Whether to show the bluetooth Logs checkbox in share data page.
+ * @type {boolean}
+ */
+ this.shouldShowAssistantCheckbox_;
+
/** @private {!FeedbackServiceProviderInterface} */
this.feedbackServiceProvider_ = getFeedbackServiceProvider();
@@ -242,6 +249,9 @@
this.feedbackServiceProvider_.getFeedbackContext().then((response) => {
this.feedbackContext_ = response.feedbackContext;
this.setAdditionalContextFromQueryParams_();
+ this.shouldShowAssistantCheckbox_ = !!this.feedbackContext_ &&
+ this.feedbackContext_.isInternalAccount &&
+ this.feedbackContext_.fromAssistant;
});
window.addEventListener('message', event => {
@@ -331,6 +341,9 @@
if (pageUrl) {
this.feedbackContext_.pageUrl = {url: pageUrl};
}
+ const fromAssistant =
+ params.get(AdditionalContextQueryParam.FROM_ASSISTANT);
+ this.feedbackContext_.fromAssistant = !!fromAssistant;
}
/**
diff --git a/ash/webui/os_feedback_ui/resources/share_data_page.html b/ash/webui/os_feedback_ui/resources/share_data_page.html
index 3427109..fec8b755 100644
--- a/ash/webui/os_feedback_ui/resources/share_data_page.html
+++ b/ash/webui/os_feedback_ui/resources/share_data_page.html
@@ -241,7 +241,8 @@
<label id="sysInfoCheckboxLabel" inner-h-t-m-l="[[sysInfoCheckboxLabel_]]"></label>
</div>
<!-- Assistant Logs (Googler Internal Only) -->
- <div id="assistantLogsContainer" class="checkbox-field-container">
+ <div id="assistantLogsContainer" class="checkbox-field-container"
+ hidden="[[!shouldShowAssistantCheckbox]]">
<cr-checkbox id="assiatantLogsCheckbox" aria-labelledby="assistantLogsLabel" checked>
</cr-checkbox>
<label id="assistantLogsLabel" inner-h-t-m-l="[[assistantLogsCheckboxLabel_]]"></label>
diff --git a/ash/webui/os_feedback_ui/resources/share_data_page.js b/ash/webui/os_feedback_ui/resources/share_data_page.js
index daf8ed3..f78f1b70 100644
--- a/ash/webui/os_feedback_ui/resources/share_data_page.js
+++ b/ash/webui/os_feedback_ui/resources/share_data_page.js
@@ -52,6 +52,8 @@
screenshotUrl: {type: String, readOnly: false, notify: true},
shouldShowBluetoothCheckbox:
{type: Boolean, readOnly: false, notify: true},
+ shouldShowAssistantCheckbox:
+ {type: Boolean, readOnly: false, notify: true},
};
}
@@ -74,6 +76,11 @@
this.shouldShowBluetoothCheckbox;
/**
+ * @type {boolean}
+ */
+ this.shouldShowAssistantCheckbox;
+
+ /**
* @type {string}
* @protected
*/
diff --git a/chrome/browser/feedback/show_feedback_page.cc b/chrome/browser/feedback/show_feedback_page.cc
index fc8a19e..372ce75 100644
--- a/chrome/browser/feedback/show_feedback_page.cc
+++ b/chrome/browser/feedback/show_feedback_page.cc
@@ -50,10 +50,12 @@
#if BUILDFLAG(IS_CHROMEOS_ASH)
constexpr char kExtraDiagnosticsQueryParam[] = "extra_diagnostics";
constexpr char kDescriptionTemplateQueryParam[] = "description_template";
+constexpr char kFromAssistantQueryParam[] = "from_assistant";
constexpr char kCategoryTagParam[] = "category_tag";
constexpr char kPageURLParam[] = "page_url";
constexpr char kQueryParamSeparator[] = "&";
constexpr char kQueryParamKeyValueSeparator[] = "=";
+constexpr char kFromAssistantQueryParamValue[] = "true";
// Concat query parameter with escaped value.
std::string StrCatQueryParam(const std::string query_param,
@@ -66,7 +68,8 @@
GURL BuildFeedbackUrl(const std::string extra_diagnostics,
const std::string description_template,
const std::string category_tag,
- const GURL page_url) {
+ const GURL page_url,
+ bool from_assistant) {
std::vector<std::string> query_params;
if (!extra_diagnostics.empty()) {
@@ -88,6 +91,11 @@
query_params.emplace_back(StrCatQueryParam(kPageURLParam, page_url.spec()));
}
+ if (from_assistant) {
+ query_params.emplace_back(StrCatQueryParam(kFromAssistantQueryParam,
+ kFromAssistantQueryParamValue));
+ }
+
// Use default URL if no extra parameters to be added.
if (query_params.empty()) {
return GURL(ash::kChromeUIOSFeedbackUrl);
@@ -171,8 +179,10 @@
}
if (base::FeatureList::IsEnabled(ash::features::kOsFeedback)) {
ash::SystemAppLaunchParams params{};
- params.url = BuildFeedbackUrl(extra_diagnostics, description_template,
- category_tag, page_url);
+ params.url =
+ BuildFeedbackUrl(extra_diagnostics, description_template, category_tag,
+ page_url, source == kFeedbackSourceAssistant);
+
ash::LaunchSystemWebAppAsync(profile, ash::SystemWebAppType::OS_FEEDBACK,
std::move(params));
return;
diff --git a/chrome/browser/feedback/show_feedback_page_browsertest.cc b/chrome/browser/feedback/show_feedback_page_browsertest.cc
index bed9fa6..cdf4dcf7 100644
--- a/chrome/browser/feedback/show_feedback_page_browsertest.cc
+++ b/chrome/browser/feedback/show_feedback_page_browsertest.cc
@@ -100,6 +100,7 @@
// - `description_template` string.
// - `category_tag` string.
// - `page_url` GURL.
+// - `from_assistant` set true.
IN_PROC_BROWSER_TEST_F(ShowFeedbackPageBrowserTest,
OsFeedbackAdditionalContextAddedToUrl) {
ash::SystemWebAppManager::GetForTest(browser()->profile())
@@ -118,13 +119,16 @@
"&category_tag=",
base::EscapeQueryParamValue(category_tag, /*use_plus=*/false),
"&page_url=",
- base::EscapeQueryParamValue(page_url.spec(), /*use_plus=*/false)}));
+ base::EscapeQueryParamValue(page_url.spec(), /*use_plus=*/false),
+ "&from_assistant=",
+ base::EscapeQueryParamValue("true", /*use_plus=*/false)}));
+
content::TestNavigationObserver navigation_observer(expected_url);
navigation_observer.StartWatchingNewWebContents();
browser()->profile()->GetPrefs()->SetBoolean(prefs::kUserFeedbackAllowed,
true);
- chrome::ShowFeedbackPage(browser(), chrome::kFeedbackSourceBrowserCommand,
+ chrome::ShowFeedbackPage(browser(), chrome::kFeedbackSourceAssistant,
/*description_template=*/description_template,
/*description_placeholder_text=*/unused,
/*category_tag=*/category_tag,
diff --git a/chrome/test/data/webui/chromeos/os_feedback_ui/feedback_flow_test.js b/chrome/test/data/webui/chromeos/os_feedback_ui/feedback_flow_test.js
index 949a61f..2c10444 100644
--- a/chrome/test/data/webui/chromeos/os_feedback_ui/feedback_flow_test.js
+++ b/chrome/test/data/webui/chromeos/os_feedback_ui/feedback_flow_test.js
@@ -115,6 +115,36 @@
verifyRecordExitPathCalled(/*metric_emitted=*/ true, exitPath);
}
+ /**
+ * @private
+ */
+ function testWithInternalAccount() {
+ feedbackServiceProvider = new FakeFeedbackServiceProvider();
+ feedbackServiceProvider.setFakeFeedbackContext(
+ fakeInternalUserFeedbackContext);
+ setFeedbackServiceProviderForTesting(feedbackServiceProvider);
+ }
+
+ /**
+ * @param {boolean} from_assistant
+ * @private
+ */
+ function setFromAssistantFlag(from_assistant) {
+ if (from_assistant) {
+ const queryParams = new URLSearchParams(window.location.search);
+ const from_assistant = 'true';
+ queryParams.set(
+ AdditionalContextQueryParam.FROM_ASSISTANT, from_assistant);
+
+ window.history.replaceState(null, '', '?' + queryParams.toString());
+ } else {
+ window.history.replaceState(
+ null, '',
+ '?' +
+ '');
+ }
+ }
+
// Test that the search page is shown by default.
test('SearchPageIsShownByDefault', async () => {
await initializePage();
@@ -313,10 +343,7 @@
// Test the bluetooth logs will show up if logged with internal account and
// input description is related.
test('ShowBluetoothLogsWithRelatedDescription', async () => {
- feedbackServiceProvider = new FakeFeedbackServiceProvider();
- feedbackServiceProvider.setFakeFeedbackContext(
- fakeInternalUserFeedbackContext);
- setFeedbackServiceProviderForTesting(feedbackServiceProvider);
+ testWithInternalAccount();
await initializePage();
// Check the bluetooth checkbox component hidden when input is not related
@@ -378,6 +405,75 @@
assertFalse(isVisible(bluetoothCheckbox));
});
+ // Test the assistant logs will show up if logged with internal account and
+ // the fromAssistant flag is true.
+ test('ShowAssistantCheckboxWithInternalAccountAndFlagSetTrue', async () => {
+ // Replacing the query string to set the fromAssistant flag as true.
+ setFromAssistantFlag(true);
+ testWithInternalAccount();
+ await initializePage();
+ page.setCurrentStateForTesting(FeedbackFlowState.SHARE_DATA);
+
+ const feedbackContext = getFeedbackContext_();
+ assertTrue(feedbackContext.isInternalAccount);
+ assertTrue(feedbackContext.fromAssistant);
+ // Check the assistant checkbox component visible when input is not
+ // related to bluetooth.
+ const activePage = page.shadowRoot.querySelector('.iron-selected');
+ assertEquals('shareDataPage', activePage.id);
+
+ const assistantCheckbox =
+ activePage.shadowRoot.querySelector('#assistantLogsContainer');
+
+ assertTrue(!!assistantCheckbox);
+ assertTrue(isVisible(assistantCheckbox));
+ });
+
+ // Test the assistant checkbox will not show up to external account user
+ // with fromAssistant flag passed.
+ test('AssistantCheckboxHiddenWithExternalAccount', async () => {
+ // Replacing the query string to set the fromAssistant flag as true.
+ setFromAssistantFlag(true);
+ await initializePage();
+ page.setCurrentStateForTesting(FeedbackFlowState.SHARE_DATA);
+
+ const feedbackContext = getFeedbackContext_();
+ assertFalse(feedbackContext.isInternalAccount);
+ assertTrue(feedbackContext.fromAssistant);
+ let activePage = page.shadowRoot.querySelector('.iron-selected');
+ activePage = page.shadowRoot.querySelector('.iron-selected');
+
+ assertEquals('shareDataPage', activePage.id);
+ const assistantCheckbox =
+ activePage.shadowRoot.querySelector('#assistantLogsContainer');
+ assertTrue(!!assistantCheckbox);
+ assertFalse(isVisible(assistantCheckbox));
+ });
+
+ // Test the assistant logs will not show up if fromAssistant flag is not
+ // passed but logged in with Internal google account.
+ test('AssistantCheckboxHiddenWithoutFlagPassed', async () => {
+ // Replace the current querystring back to default.
+ setFromAssistantFlag(false);
+ // Set Internal Account flag as true.
+ testWithInternalAccount();
+ await initializePage();
+ page.setCurrentStateForTesting(FeedbackFlowState.SHARE_DATA);
+
+ const feedbackContext = getFeedbackContext_();
+ assertTrue(feedbackContext.isInternalAccount);
+ assertFalse(feedbackContext.fromAssistant);
+ // Set input description related to bluetooth.
+ let activePage = page.shadowRoot.querySelector('.iron-selected');
+ activePage = page.shadowRoot.querySelector('.iron-selected');
+
+ assertEquals('shareDataPage', activePage.id);
+ const assistantCheckbox =
+ activePage.shadowRoot.querySelector('#assistantLogsContainer');
+ assertTrue(!!assistantCheckbox);
+ assertFalse(isVisible(assistantCheckbox));
+ });
+
// Test the navigation from confirmation page to search page after the
// send new report button is clicked.
test('NavigateFromConfirmationPageToSearchPage', async () => {
@@ -472,8 +568,8 @@
assertEquals(1, feedbackServiceProvider.getFeedbackContextCallCount());
});
- // Test that the extra diagnostics, category tag and page_url get set
- // when query parameter is non-empty.
+ // Test that the extra diagnostics, category tag, page_url and fromAssistant
+ // flag get set when query parameter is non-empty.
test(
'AdditionalContextParametersProvidedInUrl_FeedbackContext_Matches',
async () => {
@@ -489,6 +585,9 @@
queryParams.set(AdditionalContextQueryParam.CATEGORY_TAG, category_tag);
const page_url = 'some%20page%20url';
queryParams.set(AdditionalContextQueryParam.PAGE_URL, page_url);
+ const from_assistant = 'true';
+ queryParams.set(
+ AdditionalContextQueryParam.FROM_ASSISTANT, from_assistant);
// Replace current querystring with the new one.
window.history.replaceState(null, '', '?' + queryParams.toString());
await initializePage();
@@ -505,6 +604,7 @@
decodeURIComponent(description_template), descriptionElement.value);
assertEquals(
decodeURIComponent(category_tag), feedbackContext.categoryTag);
+ assertTrue(feedbackContext.fromAssistant);
// Set the pageUrl in fake feedback context back to its origin value
// because it's overwritten by the page_url passed from the app.
@@ -532,6 +632,7 @@
assertEquals('', feedbackContext.extraDiagnostics);
assertEquals('', descriptionElement.value);
assertEquals('', feedbackContext.categoryTag);
+ assertFalse(feedbackContext.fromAssistant);
});
/**
