Clearer wording for force option in push permissions
The labels for the force option in push permissions was confusing,
partly because the meaning of the option depends on the selected action.
Clarify the wording by using different labels depending on whether the
action is ALLOW or BLOCK and hide the option when the action is DENY.
Bug: Issue 9094
Change-Id: I111d6a269a6f5e835271ed42ebc6e9c6b8ce315d
diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.html b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.html
index febd446..5d45811 100644
--- a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.html
+++ b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.html
@@ -111,13 +111,13 @@
</a>
<gr-select
id="force"
- class$="[[_computeForceClass(permission)]]"
+ class$="[[_computeForceClass(permission, rule.value.action)]]"
bind-value="{{rule.value.force}}"
on-change="_handleValueChange">
<select disabled$="[[!editing]]">
<template
is="dom-repeat"
- items="[[_computeForceOptions(permission)]]">
+ items="[[_computeForceOptions(permission, rule.value.action)]]">
<option value="[[item.value]]">[[item.name]]</option>
</template>
</select>
diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js
index 4af4952..1893536 100644
--- a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js
+++ b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js
@@ -33,22 +33,24 @@
'INTERACTIVE',
];
- const DROPDOWN_OPTIONS = [
- 'ALLOW',
- 'DENY',
- 'BLOCK',
- ];
+ const Action = {
+ ALLOW: 'ALLOW',
+ DENY: 'DENY',
+ BLOCK: 'BLOCK',
+ };
- const FORCE_PUSH_OPTIONS = [
- {
- name: 'Block all pushes, block force push only',
- value: false,
- },
- {
- name: 'Allow fast-forward only push, allow all pushes',
- value: true,
- },
- ];
+ const DROPDOWN_OPTIONS = [Action.ALLOW, Action.DENY, Action.BLOCK];
+
+ const ForcePushOptions = {
+ ALLOW: [
+ {name: 'Allow pushing but not force pushing', value: false},
+ {name: 'Allow pushing with or without force', value: true},
+ ],
+ BLOCK: [
+ {name: 'Block pushing with or without force', value: false},
+ {name: 'Block force pushing', value: true},
+ ],
+ };
const FORCE_EDIT_OPTIONS = [
{
@@ -117,13 +119,17 @@
this._setOriginalRuleValues(rule.value);
},
- _computeForce(permission) {
- return this.permissionValues.push.id === permission ||
- this.permissionValues.editTopicName.id === permission;
+ _computeForce(permission, action) {
+ if (this.permissionValues.push.id === permission &&
+ action !== Action.DENY) {
+ return true;
+ }
+
+ return this.permissionValues.editTopicName.id === permission;
},
- _computeForceClass(permission) {
- return this._computeForce(permission) ? 'force' : '';
+ _computeForceClass(permission, action) {
+ return this._computeForce(permission, action) ? 'force' : '';
},
_computeGroupPath(group) {
@@ -156,9 +162,15 @@
return classList.join(' ');
},
- _computeForceOptions(permission) {
+ _computeForceOptions(permission, action) {
if (permission === this.permissionValues.push.id) {
- return FORCE_PUSH_OPTIONS;
+ if (action === Action.ALLOW) {
+ return ForcePushOptions.ALLOW;
+ } else if (action === Action.BLOCK) {
+ return ForcePushOptions.BLOCK;
+ } else {
+ return [];
+ }
} else if (permission === this.permissionValues.editTopicName.id) {
return FORCE_EDIT_OPTIONS;
}
@@ -166,6 +178,7 @@
},
_getDefaultRuleValues(permission, label) {
+ const ruleAction = Action.ALLOW;
const value = {};
if (permission === 'priority') {
value.action = PRIORITY_OPTIONS[0];
@@ -173,16 +186,17 @@
} else if (label) {
value.min = label.values[0].value;
value.max = label.values[label.values.length - 1].value;
- } else if (this._computeForce(permission)) {
- value.force = this._computeForceOptions(permission)[0].value;
+ } else if (this._computeForce(permission, ruleAction)) {
+ value.force =
+ this._computeForceOptions(permission, ruleAction)[0].value;
}
value.action = DROPDOWN_OPTIONS[0];
return value;
},
_setDefaultRuleValues() {
- this.set('rule.value',
- this._getDefaultRuleValues(this.permission, this.label));
+ this.set('rule.value', this._getDefaultRuleValues(this.permission,
+ this.label));
},
_computeOptions(permission) {
diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html
index 5b6f947..556568b 100644
--- a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html
+++ b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html
@@ -50,16 +50,16 @@
suite('unit tests', () => {
test('_computeForce, _computeForceClass, and _computeForceOptions',
() => {
- const FORCE_PUSH_OPTIONS = [
- {
- name: 'Block all pushes, block force push only',
- value: false,
- },
- {
- name: 'Allow fast-forward only push, allow all pushes',
- value: true,
- },
- ];
+ const ForcePushOptions = {
+ ALLOW: [
+ {name: 'Allow pushing but not force pushing', value: false},
+ {name: 'Allow pushing with or without force', value: true},
+ ],
+ BLOCK: [
+ {name: 'Block pushing with or without force', value: false},
+ {name: 'Block force pushing', value: true},
+ ],
+ };
const FORCE_EDIT_OPTIONS = [
{
@@ -72,10 +72,26 @@
},
];
let permission = 'push';
- assert.isTrue(element._computeForce(permission));
- assert.equal(element._computeForceClass(permission), 'force');
- assert.deepEqual(element._computeForceOptions(permission),
- FORCE_PUSH_OPTIONS);
+ let action = 'ALLOW';
+ assert.isTrue(element._computeForce(permission, action));
+ assert.equal(element._computeForceClass(permission, action),
+ 'force');
+ assert.deepEqual(element._computeForceOptions(permission, action),
+ ForcePushOptions.ALLOW);
+
+ action = 'BLOCK';
+ assert.isTrue(element._computeForce(permission, action));
+ assert.equal(element._computeForceClass(permission, action),
+ 'force');
+ assert.deepEqual(element._computeForceOptions(permission, action),
+ ForcePushOptions.BLOCK);
+
+ action = 'DENY';
+ assert.isFalse(element._computeForce(permission, action));
+ assert.equal(element._computeForceClass(permission, action), '');
+ assert.equal(
+ element._computeForceOptions(permission, action).length, 0);
+
permission = 'editTopicName';
assert.isTrue(element._computeForce(permission));
assert.equal(element._computeForceClass(permission), 'force');
