Minor improvements to non-personal dashboards
- "Waiting" column is calculated in respect to the user whose dashboard
is shown
- Improving comments in the code
- Sorting of "Your Turn" is similarly done in respect to the user whose
dashboard is shown.
There are 2 related to each other issues which are unaddressed:
- Ideally we would want dashboardUser to have a proper type instead of
string. However the value comes from the Url and can be both Id
(integer), Email (string) and 'self'
- The attention set map used for the sorting and waiting calculation is
keyed by AccountId so when dashboardUser is Email than we would need
to first do a lookup for an Id before we render the column
The Email usecase is rare (only happens if email is manually written in
the url), so we will leave both for the future fix.
Google-Bug-Id: b/280562435
Release-Notes: skip
Change-Id: I697dfd170db6dbb1e96f23a9429bf050b3b1fd69
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
index 8a6f1c6..e656404 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
@@ -75,9 +75,16 @@
@customElement('gr-change-list-item')
export class GrChangeListItem extends LitElement {
- /** The logged-in user's account, or null if no user is logged in. */
+ /** The logged-in user's account, or undefined if no user is logged in. */
@property({type: Object})
- account: AccountInfo | null = null;
+ loggedInUser?: AccountInfo;
+
+ /**
+ * When the list is part of the dashboard, the user for which the dashboard is
+ * generated.
+ */
+ @property({type: String})
+ dashboardUser?: string;
@property({type: Array})
visibleChangeTableColumns?: string[];
@@ -764,9 +771,9 @@
!isServiceUser(r)
);
reviewers.sort((r1, r2) => {
- if (this.account) {
- if (isSelf(r1, this.account)) return -1;
- if (isSelf(r2, this.account)) return 1;
+ if (this.loggedInUser) {
+ if (isSelf(r1, this.loggedInUser)) return -1;
+ if (isSelf(r2, this.loggedInUser)) return 1;
}
if (this.hasAttention(r1) && !this.hasAttention(r2)) return -1;
if (this.hasAttention(r2) && !this.hasAttention(r1)) return 1;
@@ -825,9 +832,15 @@
}
private computeWaiting(): Timestamp | undefined {
- if (!this.account?._account_id || !this.change?.attention_set)
- return undefined;
- return this.change?.attention_set[this.account._account_id]?.last_update;
+ // TODO: dashboardUser comes from DashboardViewState and can be an
+ // Email Address. In this case the attention_set lookup will return
+ // undefined.
+ const userId =
+ this.dashboardUser === 'self'
+ ? this.loggedInUser?._account_id
+ : this.dashboardUser;
+ if (!userId || !this.change?.attention_set) return undefined;
+ return this.change?.attention_set[userId]?.last_update;
}
private computeIsColumnHidden(
@@ -849,7 +862,7 @@
// Don't prevent the default and neither stop bubbling. We just want to
// report the click, but then let the browser handle the click on the link.
- const selfId = (this.account && this.account._account_id) || -1;
+ const selfId = (this.loggedInUser && this.loggedInUser._account_id) || -1;
const ownerId =
(this.change && this.change.owner && this.change.owner._account_id) || -1;
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
index 5e31cc8..25e97af 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
@@ -243,7 +243,9 @@
attSetIds: number[],
expected: number[]
) {
- element.account = userId ? {_account_id: userId as AccountId} : null;
+ element.loggedInUser = userId
+ ? {_account_id: userId as AccountId}
+ : undefined;
element.change = {
...change,
owner: {
@@ -384,7 +386,7 @@
registered_on: '2015-03-12 18:32:08.000000000' as Timestamp,
});
element.showNumber = true;
- element.account = createAccountWithId(1);
+ element.loggedInUser = createAccountWithId(1);
element.config = createServerInfo();
element.change = change;
await element.updateComplete;
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-section/gr-change-list-section.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-section/gr-change-list-section.ts
index 1248a0f..5e99df2 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-section/gr-change-list-section.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-section/gr-change-list-section.ts
@@ -75,7 +75,14 @@
* in.
*/
@property({type: Object})
- account: AccountInfo | undefined = undefined;
+ loggedInUser?: AccountInfo;
+
+ /**
+ * When the list is part of the dashboard, the user for which the dashboard is
+ * generated.
+ */
+ @property({type: String})
+ dashboardUser?: string;
@property({type: String})
usp?: string;
@@ -319,7 +326,8 @@
return html`
<gr-change-list-item
tabindex="0"
- .account=${this.account}
+ .loggedInUser=${this.loggedInUser}
+ .dashboardUser=${this.dashboardUser}
.selected=${selected}
.change=${change}
.sectionName=${this.changeSection.name}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-section/gr-change-list-section_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-section/gr-change-list-section_test.ts
index 63552c7..4ec8491 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-section/gr-change-list-section_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-section/gr-change-list-section_test.ts
@@ -56,7 +56,7 @@
};
element = await fixture<GrChangeListSection>(
html`<gr-change-list-section
- .account=${createAccountDetailWithId(1)}
+ .loggedInUser=${createAccountDetailWithId(1)}
.config=${createServerInfo()}
.visibleChangeTableColumns=${Object.values(ColumnNames)}
.changeSection=${changeSection}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
index 07a27c6..dbca42f 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
@@ -177,7 +177,7 @@
<div ?hidden=${this.loading}>
${this.renderRepoHeader()} ${this.renderUserHeader()}
<gr-change-list
- .account=${this.account}
+ .loggedInUser=${this.account}
.changes=${this.changes}
@toggle-star=${(e: CustomEvent<ChangeStarToggleStarDetail>) => {
this.handleToggleStar(e);
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
index b1f3c2c..38ee01a 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
@@ -84,7 +84,14 @@
* in.
*/
@property({type: Object})
- account: AccountInfo | undefined = undefined;
+ loggedInUser?: AccountInfo;
+
+ /**
+ * When the list is part of the dashboard, the user for which the dashboard is
+ * generated.
+ */
+ @property({type: String})
+ dashboardUser?: string;
@property({type: Array})
changes?: ChangeInfo[];
@@ -262,7 +269,8 @@
.labelNames=${labelNames}
.dynamicHeaderEndpoints=${this.dynamicHeaderEndpoints}
.isCursorMoving=${this.isCursorMoving}
- .account=${this.account}
+ .loggedInUser=${this.loggedInUser}
+ .dashboardUser=${this.dashboardUser}
.selectedIndex=${computeRelativeIndex(
this.selectedIndex,
sectionIndex,
@@ -289,7 +297,7 @@
override willUpdate(changedProperties: PropertyValues) {
if (
- changedProperties.has('account') ||
+ changedProperties.has('loggedInUser') ||
changedProperties.has('preferences') ||
changedProperties.has('config') ||
changedProperties.has('sections')
@@ -340,7 +348,7 @@
this.visibleChangeTableColumns = this.changeTableColumns.filter(col =>
this.isColumnEnabled(col, this.config)
);
- if (this.account && this.preferences) {
+ if (this.loggedInUser && this.preferences) {
this.showNumber = !!this.preferences?.legacycid_in_change_table;
if (
this.preferences?.change_table &&
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.ts
index e201ab4..62a4f7d 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.ts
@@ -51,7 +51,7 @@
time_format: TimeFormat.HHMM_12,
change_table: [],
};
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.config = createServerInfo();
element.sections = [
{
@@ -106,7 +106,7 @@
});
test('show change number disabled when not logged in', async () => {
- element.account = undefined;
+ element.loggedInUser = undefined;
element.preferences = undefined;
element.config = createServerInfo();
await element.updateComplete;
@@ -120,7 +120,7 @@
time_format: TimeFormat.HHMM_12,
change_table: [],
};
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.config = createServerInfo();
await element.updateComplete;
@@ -133,7 +133,7 @@
time_format: TimeFormat.HHMM_12,
change_table: [],
};
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.config = createServerInfo();
await element.updateComplete;
@@ -415,7 +415,7 @@
stubFlags('isEnabled').returns(true);
element = await fixture(html`<gr-change-list></gr-change-list>`);
element.sections = [{results: [{...createChange()}]}];
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.preferences = {
legacycid_in_change_table: true,
time_format: TimeFormat.HHMM_12,
@@ -447,7 +447,7 @@
stubFlags('isEnabled').returns(true);
element = await fixture(html`<gr-change-list></gr-change-list>`);
element.sections = [{results: [{...createChange()}]}];
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.preferences = {
legacycid_in_change_table: true,
time_format: TimeFormat.HHMM_12,
@@ -486,7 +486,7 @@
stubFlags('isEnabled').returns(true);
element = await fixture(html`<gr-change-list></gr-change-list>`);
element.sections = [{results: [{...createChange()}]}];
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.preferences = {
legacycid_in_change_table: true,
time_format: TimeFormat.HHMM_12,
@@ -537,7 +537,7 @@
test('loggedIn and showNumber', async () => {
element.sections = [{results: [{...createChange()}], name: 'a'}];
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.preferences = {
legacycid_in_change_table: false, // sets showNumber false
time_format: TimeFormat.HHMM_12,
@@ -586,7 +586,7 @@
test('garbage columns in preference are not shown', async () => {
// This would only exist if somebody manually updated the config file.
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.preferences = {
legacycid_in_change_table: true,
time_format: TimeFormat.HHMM_12,
@@ -603,7 +603,7 @@
html`<gr-change-list></gr-change-list>`
);
element.sections = [{results: [{...createChange()}]}];
- element.account = {_account_id: 1001 as AccountId};
+ element.loggedInUser = {_account_id: 1001 as AccountId};
element.preferences = {
change_table: [
'Status', // old status
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 cb6d047..f4c5215 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
@@ -79,7 +79,7 @@
protected confirmDeleteModal?: HTMLDialogElement;
@property({type: Object})
- account?: AccountDetailInfo;
+ loggedInUser?: AccountDetailInfo;
@property({type: Object})
preferences?: PreferencesInput;
@@ -126,7 +126,7 @@
subscribe(
this,
() => this.getUserModel().account$,
- x => (this.account = x)
+ x => (this.loggedInUser = x)
);
subscribe(
this,
@@ -286,7 +286,8 @@
${this.renderUserHeader()}
<h1 class="assistive-tech-only">Dashboard</h1>
<gr-change-list
- .account=${this.account}
+ .loggedInUser=${this.loggedInUser}
+ .dashboardUser=${this.viewState?.user}
.preferences=${this.preferences}
.sections=${this.results}
.usp=${'dashboard'}
@@ -392,15 +393,16 @@
: Promise.resolve(
getUserDashboard(user, sections, title || this.computeTitle(user))
);
- // Checking `this.account` to make sure that the user is logged in.
+ // Checking `this.loggedInUser` to make sure that the user is logged in.
// Otherwise sending a query for 'owner:self' will result in an error.
- const checkForNewUser = !project && !!this.account && user === 'self';
+ const isLoggedInUserDashboard =
+ !project && !!this.loggedInUser && user === 'self';
return dashboardPromise
.then(res => {
if (res && res.title) {
fireTitleChange(res.title);
}
- return this.fetchDashboardChanges(res, checkForNewUser);
+ return this.fetchDashboardChanges(res, isLoggedInUserDashboard);
})
.then(() => {
this.maybeShowDraftsBanner();
@@ -423,7 +425,7 @@
*/
fetchDashboardChanges(
res: UserDashboard | undefined,
- checkForNewUser: boolean
+ isLoggedInUserDashboard: boolean
): Promise<void> {
if (!res) {
return Promise.resolve();
@@ -442,7 +444,8 @@
: section.query
);
- if (checkForNewUser) {
+ if (isLoggedInUserDashboard) {
+ // The query to check if the user created any changes yet.
queries.push('owner:self limit:1');
}
}
@@ -453,8 +456,9 @@
if (!changes) {
throw new Error('getChanges returns undefined');
}
- if (checkForNewUser) {
- // Last set of results is not meant for dashboard display.
+ if (isLoggedInUserDashboard) {
+ // Last query ('owner:self limit:1') is only for evaluation if
+ // the user is "New" ie. haven't created any changes yet.
const lastResultSet = changes.pop();
this.showNewUserHelp = lastResultSet!.length === 0;
}
@@ -485,7 +489,12 @@
* And then we want to emphasize the changes where the waiting time is larger.
*/
private maybeSortResults(name: string, results: ChangeInfo[]) {
- const userId = this.account?._account_id;
+ // TODO: viewState?.user can be an Email Address. In this case the
+ // attention_set lookups will return undefined.
+ const userId =
+ this.viewState?.user === 'self'
+ ? this.loggedInUser?._account_id
+ : this.viewState?.user;
const sortedResults = [...results];
if (name === YOUR_TURN.name && userId) {
sortedResults.sort((c1, c2) => {
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 84a3139..b05d970 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
@@ -319,7 +319,7 @@
suite('selfOnly sections', () => {
test('viewing self dashboard includes selfOnly sections', async () => {
- element.account = undefined;
+ element.loggedInUser = undefined;
element.viewState = {
view: GerritView.DASHBOARD,
user: 'self',
@@ -334,7 +334,7 @@
});
test('viewing dashboard when logged in includes owner:self query', async () => {
- element.account = createAccountDetailWithId(1);
+ element.loggedInUser = createAccountDetailWithId(1);
element.viewState = {
view: GerritView.DASHBOARD,
user: 'self',
