opendev/gerrit
Code Issues Proposed changes

Fix changeStatusString logic

Browse Source 
Migrates all changeStatusString computation logic to the behavior,
consolidates tests from the individual elements into the behavior suite.

Change statuses are now a comma separated list of statuses, e.g.
"Merge Conflict, Private, WIP" or "Draft, WIP" -- no more parentheses to
avoid unnecessary complexity and cognitive load.

Also, no longer computes extra bits for closed changes.

Bug: Issue 6795
Change-Id: I36e7875a07ee9e5ff2172328b87c6bc22c3bcaa4
This commit is contained in:
Kasper Nilsson
2017-07-20 12:50:22 -07:00
parent e47666de9d
commit 66c5d9a071
6 changed files with 32 additions and 198 deletions
Download Patch File Download Diff File Expand all files Collapse all files

36
polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior.html
View File

@@ -116,35 +116,19 @@ limitations under the License.
status === this.ChangeStatus.DRAFT;
},
wipOrPrivateStatus(change) {
if (change.work_in_progress && change.is_private) {
return ' (Private) (WIP)';
} else if (change.work_in_progress) {
return ' (WIP)';
} else if (change.is_private) {
return ' (Private)';
}
return '';
},
changeStatusString(change) {
// "Closed" states should take precedence over "open" ones.
const states = [];
if (change.status === this.ChangeStatus.MERGED) {
return 'Merged' + this.wipOrPrivateStatus(change);
states.push('Merged');
} else if (change.status === this.ChangeStatus.ABANDONED) {
states.push('Abandoned');
} else if (!change.mergeable) {
states.push('Merge Conflict');
}
if (change.status === this.ChangeStatus.ABANDONED) {
return 'Abandoned' + this.wipOrPrivateStatus(change);
}
if (change.mergeable === false) {
return 'Merge Conflict' + this.wipOrPrivateStatus(change);
}
if (change.status === this.ChangeStatus.DRAFT) {
return 'Draft' + this.wipOrPrivateStatus(change);
}
if (change.status === this.ChangeStatus.NEW) {
return this.wipOrPrivateStatus(change);
}
return '';
if (change.work_in_progress) { states.push('WIP'); }
if (change.is_private) { states.push('Private'); }
if (change.status === this.ChangeStatus.DRAFT) { states.push('Draft'); }
return states.join(', ');
},
};

25
polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior_test.html
View File

@@ -85,11 +85,26 @@ limitations under the License.
current_revision: 'rev1',
status: 'NEW',
labels: {},
mergeable: true,
};
const status = element.changeStatusString(change);
assert.equal(status, '');
});
test('Merge conflict', () => {
const change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
},
current_revision: 'rev1',
status: 'NEW',
labels: {},
};
const status = element.changeStatusString(change);
assert.equal(status, 'Merge Conflict');
});
test('Merged status', () => {
const change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
@@ -129,25 +144,27 @@ limitations under the License.
is_private: true,
work_in_progress: true,
labels: {},
mergeable: true,
};
const status = element.changeStatusString(change);
assert.equal(status, ' (Private) (WIP)');
assert.equal(status, 'WIP, Private');
});
test('Abandoned status with private and wip', () => {
test('Merge conflict with private and wip', () => {
const change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
},
current_revision: 'rev1',
status: 'ABANDONED',
status: 'NEW',
is_private: true,
work_in_progress: true,
labels: {},
mergeable: false,
};
const status = element.changeStatusString(change);
assert.equal(status, 'Abandoned (Private) (WIP)');
assert.equal(status, 'Merge Conflict, WIP, Private');
});
});
</script>

14
polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.html
View File

@@ -45,20 +45,6 @@ limitations under the License.
element = fixture('basic');
});
test('change status', () => {
const getStatusForChange = function(change) {
element.change = change;
return element.$$('.cell.status').textContent.trim();
};
assert.equal(getStatusForChange({mergeable: true}), '');
assert.equal(getStatusForChange({mergeable: false}), 'Merge Conflict');
assert.equal(getStatusForChange({status: 'NEW'}), '');
assert.equal(getStatusForChange({status: 'MERGED'}), 'Merged');
assert.equal(getStatusForChange({status: 'ABANDONED'}), 'Abandoned');
assert.equal(getStatusForChange({status: 'DRAFT'}), 'Draft');
});
test('computed fields', () => {
assert.equal(element._computeLabelClass({labels: {}}),
'cell label u-gray-background');

2
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
View File

@@ -341,8 +341,6 @@ limitations under the License.
--></template><!--
-->)<!--
--></template><!--
--><span hidden$="[[!_change.work_in_progress]]"> (Work in progress)</span><!--
--><span>[[_privateChanges(_change)]]</span><!--
-->: [[_change.subject]]
</span>
</div>

13
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
View File

@@ -171,7 +171,7 @@
},
_changeStatus: {
type: String,
computed: '_computeChangeStatus(_change, _patchRange.patchNum)',
computed: 'changeStatusString(_change)',
},
_commitCollapsed: {
type: Boolean,
@@ -623,17 +623,6 @@
return Gerrit.Nav.getUrlForChange(change);
},
_computeChangeStatus(change, patchNum) {
let statusString = this.changeStatusString(change);
if (change.status === this.ChangeStatus.NEW) {
const rev = this.getRevisionByPatchNum(change.revisions, patchNum);
if (rev && rev.draft === true) {
statusString = 'Draft';
}
}
return statusString;
},
_privateChanges(change) {
return change.is_private ? ' (Private)' : '';
},

140
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
View File

@@ -701,146 +701,6 @@ limitations under the License.
});
});
test('change status new', () => {
element._changeNum = '1';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: 1,
};
element._change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
},
current_revision: 'rev1',
status: 'NEW',
labels: {},
};
const status = element._computeChangeStatus(element._change, '1');
assert.equal(status, '');
});
test('change status draft', () => {
element._changeNum = '1';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: 1,
};
element._change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
},
current_revision: 'rev1',
status: 'DRAFT',
labels: {},
};
const status = element._computeChangeStatus(element._change, '1');
assert.equal(status, 'Draft');
});
test('change status conflict', () => {
element._changeNum = '1';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: 1,
};
element._change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
},
current_revision: 'rev1',
mergeable: false,
status: 'NEW',
labels: {},
};
const status = element._computeChangeStatus(element._change, '1');
assert.equal(status, 'Merge Conflict');
});
test('change status private', () => {
element._changeNum = '1';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: 1,
};
element._change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
},
current_revision: 'rev1',
status: 'NEW',
labels: {},
is_private: true,
};
const status = element._privateChanges(element._change);
assert.equal(status, ' (Private)');
});
test('change status without private', () => {
element._changeNum = '1';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: 1,
};
element._change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
},
current_revision: 'rev1',
status: 'NEW',
labels: {},
is_private: false,
};
const status = element._privateChanges(element._change);
assert.equal(status, '');
});
test('change status merged', () => {
element._changeNum = '1';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: 1,
};
element._change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
},
current_revision: 'rev1',
status: element.ChangeStatus.MERGED,
labels: {},
};
const status = element._computeChangeStatus(element._change, '1');
assert.equal(status, 'Merged');
});
test('revision status draft', () => {
element._changeNum = '1';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: 1,
};
element._change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
rev2: {
_number: 2,
draft: true,
},
},
current_revision: 'rev1',
status: 'NEW',
labels: {},
};
const status = element._computeChangeStatus(element._change, '2');
assert.equal(status, 'Draft');
});
test('_computeMergedCommitInfo', () => {
const dummyRevs = {
1: {commit: {commit: 1}},