Only log DashboardDisplayed for the default user dashboard.
This is the main view for a user in dashboard and it's performance
characteristcs can be different from repo and custom dashboards, skewing
the metrics.
For example the change query for such dashboard is preloaded using
`IndexPreloadingUtil.java`, but not for the other types of dashboards.
We introduce an extra field in the DashboardViewState. The field can in
most (maybe all) cases be deduced for the fields. But that would add
extra complexity and brittleness to the code. It's easier to populate
the type at the state creation time.
Google-Bug-Id: b/303741140
Release-Notes: skip
Change-Id: I8c73a9c3b1a93144cda228f3597d7635a43f7ce0
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-dashboards/gr-repo-dashboards.ts b/polygerrit-ui/app/elements/admin/gr-repo-dashboards/gr-repo-dashboards.ts
index bff56bdd..e099184 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-dashboards/gr-repo-dashboards.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-dashboards/gr-repo-dashboards.ts
@@ -11,7 +11,10 @@
import {tableStyles} from '../../../styles/gr-table-styles';
import {LitElement, css, html, PropertyValues} from 'lit';
import {customElement, property} from 'lit/decorators.js';
-import {createDashboardUrl} from '../../../models/views/dashboard';
+import {
+ DashboardType,
+ createDashboardUrl,
+} from '../../../models/views/dashboard';
interface DashboardRef {
section: string;
@@ -156,7 +159,7 @@
_getUrl(project?: RepoName, dashboard?: DashboardId) {
if (!project || !dashboard) return '';
- return createDashboardUrl({project, dashboard});
+ return createDashboardUrl({project, type: DashboardType.REPO, dashboard});
}
_computeLoadingClass(loading: boolean) {
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
index f4c5215..00729fd 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
@@ -43,6 +43,7 @@
import {Shortcut} from '../../../services/shortcuts/shortcuts-config';
import {ShortcutController} from '../../lit/shortcut-controller';
import {
+ DashboardType,
dashboardViewModelToken,
DashboardViewState,
} from '../../../models/views/dashboard';
@@ -386,7 +387,7 @@
this.firstTimeLoad = false;
this.loading = true;
- const {project, dashboard, title, user, sections} = this.viewState;
+ const {project, type, dashboard, title, user, sections} = this.viewState;
const dashboardPromise: Promise<UserDashboard | undefined> = project
? this.getRepositoryDashboard(project, dashboard)
@@ -406,7 +407,10 @@
})
.then(() => {
this.maybeShowDraftsBanner();
- this.reporting.dashboardDisplayed();
+ // Only report the metric for the default personal dashboard.
+ if (type === DashboardType.USER && isLoggedInUserDashboard) {
+ this.reporting.dashboardDisplayed();
+ }
})
.catch(err => {
fireTitleChange(title || this.computeTitle(user));
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.ts b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.ts
index b05d970..58a66a6 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.ts
@@ -36,6 +36,7 @@
import {SinonStubbedMember} from 'sinon';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
import {GrButton} from '../../shared/gr-button/gr-button';
+import {DashboardType} from '../../../models/views/dashboard';
suite('gr-dashboard-view tests', () => {
let element: GrDashboardView;
@@ -63,6 +64,7 @@
test('render', async () => {
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.CUSTOM,
user: 'self',
sections: [
{name: 'test1', query: 'test1', hideIfEmpty: true},
@@ -117,6 +119,7 @@
setup(async () => {
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.CUSTOM,
user: 'user',
sections: [
{name: 'test1', query: 'test1', hideIfEmpty: true},
@@ -155,6 +158,7 @@
setup(async () => {
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.CUSTOM,
user: 'self',
sections: [
{name: 'test1', query: 'test1', hideIfEmpty: true},
@@ -167,6 +171,7 @@
test('not dashboard/self', () => {
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
user: 'notself',
dashboard: '' as DashboardId,
};
@@ -178,6 +183,7 @@
element.results = [];
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
user: 'self',
dashboard: '' as DashboardId,
};
@@ -192,6 +198,7 @@
];
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
user: 'self',
dashboard: '' as DashboardId,
};
@@ -212,6 +219,7 @@
assert.isFalse(changeIsOpen(element.results[0].results[0]));
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
user: 'self',
dashboard: '' as DashboardId,
};
@@ -322,6 +330,7 @@
element.loggedInUser = undefined;
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.CUSTOM,
user: 'self',
dashboard: '' as DashboardId,
sections: [
@@ -337,6 +346,7 @@
element.loggedInUser = createAccountDetailWithId(1);
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.CUSTOM,
user: 'self',
dashboard: '' as DashboardId,
sections: [
@@ -353,6 +363,7 @@
test("viewing another user's dashboard omits selfOnly sections", async () => {
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.CUSTOM,
user: 'user',
dashboard: '' as DashboardId,
sections: [
@@ -368,6 +379,7 @@
test('suffixForDashboard is included in getChanges query', async () => {
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.CUSTOM,
dashboard: '' as DashboardId,
sections: [
{name: '', query: '1'},
@@ -508,6 +520,7 @@
test('showNewUserHelp', async () => {
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
};
element.loading = false;
element.showNewUserHelp = false;
@@ -542,6 +555,7 @@
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
dashboard: '' as DashboardId,
user: 'self',
};
@@ -551,6 +565,7 @@
element.loading = false;
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
dashboard: '' as DashboardId,
user: 'user',
};
@@ -559,6 +574,7 @@
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.REPO,
dashboard: '' as DashboardId,
project: 'p' as RepoName,
user: 'user',
@@ -584,6 +600,7 @@
});
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.REPO,
dashboard: 'dashboard' as DashboardId,
project: 'project' as RepoName,
user: '',
@@ -592,6 +609,18 @@
});
test('viewState change triggers dashboardDisplayed()', async () => {
+ getChangesStub.returns(Promise.resolve([[]]));
+ const dashboardDisplayedStub = stubReporting('dashboardDisplayed');
+ element.viewState = {
+ view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
+ user: 'self',
+ };
+ await element.reload();
+ assert.isTrue(dashboardDisplayedStub.calledOnce);
+ });
+
+ test('viewState change does not trigger dashboardDisplayed() for repo', async () => {
stubRestApi('getDashboard').returns(
Promise.resolve({
id: '' as DashboardId,
@@ -609,11 +638,24 @@
const dashboardDisplayedStub = stubReporting('dashboardDisplayed');
element.viewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.REPO,
dashboard: 'dashboard' as DashboardId,
project: 'project' as RepoName,
user: '',
};
await element.reload();
- assert.isTrue(dashboardDisplayedStub.calledOnce);
+ assert.isFalse(dashboardDisplayedStub.calledOnce);
+ });
+
+ test('viewState change does not trigger dashboardDisplayed() for not-self', async () => {
+ getChangesStub.returns(Promise.resolve([]));
+ const dashboardDisplayedStub = stubReporting('dashboardDisplayed');
+ element.viewState = {
+ view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
+ user: 'notself',
+ };
+ await element.reload();
+ assert.isFalse(dashboardDisplayedStub.calledOnce);
});
});
diff --git a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
index 3f99416..d1bba95 100644
--- a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
@@ -15,7 +15,10 @@
import {fontStyles} from '../../../styles/gr-font-styles';
import {LitElement, css, html, PropertyValues} from 'lit';
import {customElement, property} from 'lit/decorators.js';
-import {createDashboardUrl} from '../../../models/views/dashboard';
+import {
+ DashboardType,
+ createDashboardUrl,
+} from '../../../models/views/dashboard';
@customElement('gr-user-header')
export class GrUserHeader extends LitElement {
@@ -145,10 +148,12 @@
if (!accountDetails) return '';
const id = accountDetails._account_id;
- if (id) return createDashboardUrl({user: String(id)});
+ if (id)
+ return createDashboardUrl({type: DashboardType.USER, user: String(id)});
const email = accountDetails.email;
- if (email) return createDashboardUrl({user: email});
+ if (email)
+ return createDashboardUrl({type: DashboardType.USER, user: email});
return '';
}
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index ac35264..1982ee2 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -70,6 +70,7 @@
createDiffUrl,
} from '../../../models/views/change';
import {
+ DashboardType,
DashboardViewModel,
DashboardViewState,
PROJECT_DASHBOARD_ROUTE,
@@ -1020,6 +1021,7 @@
} else {
const state: DashboardViewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
user: ctx.params[0],
};
// Note that router model view must be updated before view models.
@@ -1055,6 +1057,7 @@
const state: DashboardViewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.CUSTOM,
user: 'self',
sections,
title,
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
index 234bf95..50e506b 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -51,6 +51,7 @@
} from '../../../test/test-data-generators';
import {ParsedChangeInfo} from '../../../types/types';
import {ViewState} from '../../../models/views/base';
+import {DashboardType} from '../../../models/views/dashboard';
suite('gr-router tests', () => {
let router: GrRouter;
@@ -591,6 +592,7 @@
// CUSTOM_DASHBOARD: /^\/dashboard\/?$/,
await checkUrlToState('/dashboard?title=Custom Dashboard&a=b&d=e', {
...createDashboardViewState(),
+ type: DashboardType.CUSTOM,
sections: [
{name: 'a', query: 'b'},
{name: 'd', query: 'e'},
@@ -599,6 +601,7 @@
});
await checkUrlToState('/dashboard?a=b&c&d=&=e&foreach=is:open', {
...createDashboardViewState(),
+ type: DashboardType.CUSTOM,
sections: [{name: 'a', query: 'is:open b'}],
title: 'Custom Dashboard',
});
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index 26ecf30..6dfa972 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -66,7 +66,7 @@
import {subscribe} from './lit/subscription-controller';
import {createSearchUrl} from '../models/views/search';
import {createSettingsUrl} from '../models/views/settings';
-import {createDashboardUrl} from '../models/views/dashboard';
+import {DashboardType, createDashboardUrl} from '../models/views/dashboard';
import {userModelToken} from '../models/user/user-model';
import {modalStyles} from '../styles/gr-modal-styles';
import {AdminChildView, createAdminUrl} from '../models/views/admin';
@@ -174,7 +174,9 @@
this.showKeyboardShortcuts()
);
this.shortcuts.addAbstract(Shortcut.GO_TO_USER_DASHBOARD, () =>
- this.getNavigation().setUrl(createDashboardUrl({user: 'self'}))
+ this.getNavigation().setUrl(
+ createDashboardUrl({type: DashboardType.USER, user: 'self'})
+ )
);
this.shortcuts.addAbstract(Shortcut.GO_TO_OPENED_CHANGES, () =>
this.getNavigation().setUrl(createSearchUrl({statuses: ['open']}))
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
index ffc588e..c65a1fe 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
@@ -42,7 +42,10 @@
import {resolve} from '../../../models/dependency';
import {configModelToken} from '../../../models/config/config-model';
import {createSearchUrl} from '../../../models/views/search';
-import {createDashboardUrl} from '../../../models/views/dashboard';
+import {
+ DashboardType,
+ createDashboardUrl,
+} from '../../../models/views/dashboard';
import {fire, fireReload} from '../../../utils/event-util';
import {userModelToken} from '../../../models/user/user-model';
@@ -384,9 +387,15 @@
computeOwnerDashboardLink() {
if (!this.account) return undefined;
if (this.account._account_id)
- return createDashboardUrl({user: `${this.account._account_id}`});
+ return createDashboardUrl({
+ type: DashboardType.USER,
+ user: `${this.account._account_id}`,
+ });
if (this.account.email)
- return createDashboardUrl({user: this.account.email});
+ return createDashboardUrl({
+ type: DashboardType.USER,
+ user: this.account.email,
+ });
return undefined;
}
diff --git a/polygerrit-ui/app/models/views/dashboard.ts b/polygerrit-ui/app/models/views/dashboard.ts
index 918f429..f5fc8b4 100644
--- a/polygerrit-ui/app/models/views/dashboard.ts
+++ b/polygerrit-ui/app/models/views/dashboard.ts
@@ -19,6 +19,7 @@
const dashboard = (ctx.params[1] ?? '') as DashboardId;
const state: DashboardViewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.REPO,
project,
dashboard,
};
@@ -26,8 +27,15 @@
},
};
+export enum DashboardType {
+ USER,
+ REPO,
+ CUSTOM,
+}
+
export interface DashboardViewState extends ViewState {
view: GerritView.DASHBOARD;
+ type: DashboardType;
project?: RepoName;
dashboard?: DashboardId;
user?: string;
diff --git a/polygerrit-ui/app/models/views/dashboard_test.ts b/polygerrit-ui/app/models/views/dashboard_test.ts
index 9509977..47f4868a 100644
--- a/polygerrit-ui/app/models/views/dashboard_test.ts
+++ b/polygerrit-ui/app/models/views/dashboard_test.ts
@@ -11,6 +11,7 @@
import {DashboardId} from '../../types/common';
import {
createDashboardUrl,
+ DashboardType,
DashboardViewState,
PROJECT_DASHBOARD_ROUTE,
} from './dashboard';
@@ -23,6 +24,7 @@
const state: DashboardViewState = {
view: GerritView.DASHBOARD,
+ type: DashboardType.REPO,
project: 'asdf' as RepoName,
dashboard: 'qwer' as DashboardId,
};
@@ -37,21 +39,31 @@
suite('createDashboardUrl()', () => {
test('self dashboard', () => {
- assert.equal(createDashboardUrl({}), '/dashboard/self');
+ assert.equal(
+ createDashboardUrl({type: DashboardType.USER}),
+ '/dashboard/self'
+ );
});
test('baseUrl', () => {
window.CANONICAL_PATH = '/base';
- assert.equal(createDashboardUrl({}).substring(0, 5), '/base');
+ assert.equal(
+ createDashboardUrl({type: DashboardType.USER}).substring(0, 5),
+ '/base'
+ );
window.CANONICAL_PATH = undefined;
});
test('user dashboard', () => {
- assert.equal(createDashboardUrl({user: 'user'}), '/dashboard/user');
+ assert.equal(
+ createDashboardUrl({type: DashboardType.USER, user: 'user'}),
+ '/dashboard/user'
+ );
});
test('custom self dashboard, no title', () => {
const state = {
+ type: DashboardType.CUSTOM,
sections: [
{name: 'section 1', query: 'query 1'},
{name: 'section 2', query: 'query 2'},
@@ -65,6 +77,7 @@
test('custom repo dashboard', () => {
const state = {
+ type: DashboardType.CUSTOM,
sections: [
{name: 'section 1', query: 'query 1 ${project}'},
{name: 'section 2', query: 'query 2 ${repo}'},
@@ -80,6 +93,7 @@
test('custom user dashboard, with title', () => {
const state = {
+ type: DashboardType.CUSTOM,
user: 'user',
sections: [{name: 'name', query: 'query'}],
title: 'custom dashboard',
@@ -92,6 +106,7 @@
test('repo dashboard', () => {
const state = {
+ type: DashboardType.REPO,
project: 'gerrit/repo' as RepoName,
dashboard: 'default:main' as DashboardId,
};
@@ -103,6 +118,7 @@
test('project dashboard (legacy)', () => {
const state = {
+ type: DashboardType.REPO,
project: 'gerrit/project' as RepoName,
dashboard: 'default:main' as DashboardId,
};
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index 38d50d5..d941c9f 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -112,7 +112,7 @@
import {GroupViewState} from '../models/views/group';
import {RepoDetailView, RepoViewState} from '../models/views/repo';
import {AdminChildView, AdminViewState} from '../models/views/admin';
-import {DashboardViewState} from '../models/views/dashboard';
+import {DashboardType, DashboardViewState} from '../models/views/dashboard';
const TEST_DEFAULT_EXPRESSION = 'label:Verified=MAX -label:Verified=MIN';
export const TEST_PROJECT_NAME: RepoName = 'test-project' as RepoName;
@@ -744,6 +744,7 @@
export function createDashboardViewState(): DashboardViewState {
return {
view: GerritView.DASHBOARD,
+ type: DashboardType.USER,
user: 'self',
};
}
diff --git a/polygerrit-ui/app/workers/service-worker-class.ts b/polygerrit-ui/app/workers/service-worker-class.ts
index 03b6b902..260ee57 100644
--- a/polygerrit-ui/app/workers/service-worker-class.ts
+++ b/polygerrit-ui/app/workers/service-worker-class.ts
@@ -16,7 +16,7 @@
getServiceWorkerState,
putServiceWorkerState,
} from './service-worker-indexdb';
-import {createDashboardUrl} from '../models/views/dashboard';
+import {DashboardType, createDashboardUrl} from '../models/views/dashboard';
import {createChangeUrl} from '../models/views/change';
import {noAwait} from '../utils/async-util';
@@ -142,7 +142,7 @@
private showNotificationForDashboard(numOfChangesToNotifyAbout: number) {
const title = `You are in the attention set for ${numOfChangesToNotifyAbout} new changes.`;
- const dashboardUrl = createDashboardUrl({});
+ const dashboardUrl = createDashboardUrl({type: DashboardType.USER});
const data = {url: `${self.location.origin}${dashboardUrl}`};
const icon = `${self.location.origin}/favicon.ico`;
this.ctx.registration.showNotification(title, {data, icon});
