Fix changeStatusString logic
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
diff --git a/polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior.html b/polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior.html
index 05b24a6..cf54eac 100644
--- a/polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior.html
+++ b/polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior.html
@@ -116,35 +116,19 @@
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(', ');
},
};
diff --git a/polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior_test.html b/polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior_test.html
index 85964e7..cec5cb3 100644
--- a/polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior_test.html
+++ b/polygerrit-ui/app/behaviors/rest-client-behavior/rest-client-behavior_test.html
@@ -85,11 +85,26 @@
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 @@
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>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.html b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.html
index 0483d23..502a720 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.html
@@ -45,20 +45,6 @@
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');
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 03e2e65..cb304d8 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -341,8 +341,6 @@
--></template><!--
-->)<!--
--></template><!--
- --><span hidden$="[[!_change.work_in_progress]]"> (Work in progress)</span><!--
- --><span>[[_privateChanges(_change)]]</span><!--
-->: [[_change.subject]]
</span>
</div>
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index 4a43f47..d697a4e 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -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)' : '';
},
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index 5d798b8..fb5d88b 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -701,146 +701,6 @@
});
});
- 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}},
