opendev/gerrit
Code Issues Proposed changes

Fix access removal of added item

Browse Source 
Previously, when removing an added item (of any type) nested inside of
another added type, the removal has no effect. This is because, in the
case of an add, there is no corresponding remove object, as there is
in the case of a modified section.

Example:

Modified access rules with a new ref and rule-2 was removed before
saving.

{
  ...,
  refs/for/new: {
    added: true,
    permissions: {
      permission-1: {
        added: true,
        rules {
          rule-1: {
            added: true,
            action: 'ALLOW"
          },
          rule-2: {
            added: true,
            removed: true,
            action: 'ALLOW"
          }
        }
      }
    }
  }
}

This change fixes the problem by actually removing the item completely
if it was added in the first place, as opposed to keeping a placeholder
that can be undone, which makes sense as both ways allow restoration
of the initial state of the access object.

The new access JSON for this example is as follows:
There is no reason for rule-2 to be in here at all because the server
does not have to remove it (it did not exist before).

{
  ...,
  refs/for/new: {
    added: true,
    permissions: {
      permission-1: {
        added: true,
        rules {
          rule-1: {
            added: true,
            action: 'ALLOW"
          },
        }
      }
    }
  }
}

Bug: Issue 8795
Change-Id: Ief7ebd18f0ad207358b68f504ffe2b0b322340f9
This commit is contained in:
Becky Siegel 2018-04-23 14:44:32 -07:00
parent 712f0ea28e
commit 7c57cf9104
11 changed files with 120 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.html
View File

@ -122,7 +122,8 @@ limitations under the License.
labels="[[labels]]"
section="[[section.id]]"
editing="[[editing]]"
groups="[[groups]]">
groups="[[groups]]"
on-added-permission-removed="_handleAddedPermissionRemoved">
</gr-permission>
</template>
<div id="addPermission">

15
polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.js
View File

@ -23,6 +23,11 @@
* @event access-modified
*/
/**
* Fired when a section that was previously added was removed.
* @event added-section-removed
*/
const GLOBAL_NAME = 'GLOBAL_CAPABILITIES';
// The name that gets automatically input when a new reference is added.
@ -130,6 +135,12 @@
return section.id === 'GLOBAL_CAPABILITIES' ? 'hide' : '';
},
_handleAddedPermissionRemoved(e) {
const index = e.model.index;
this._permissions = this._permissions.slice(0, index).concat(
this._permissions.slice(index + 1, this._permissions.length));
},
_computeLabelOptions(labels) {
const labelOptions = [];
for (const labelName of Object.keys(labels)) {
@ -184,6 +195,10 @@
},
_handleRemoveReference() {
if (this.section.value.added) {
this.dispatchEvent(new CustomEvent('added-section-removed',
{bubbles: true}));
}
this._deleted = true;
this.section.value.deleted = true;
this.dispatchEvent(new CustomEvent('access-modified', {bubbles: true}));

17
polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section_test.html
View File

@ -507,6 +507,23 @@ limitations under the License.
assert.isFalse(element._deleted);
assert.isNotOk(element.section.value.deleted);
});
test('removing an added permission', () => {
element.editing = true;
assert.equal(element._permissions.length, 1);
element.$$('gr-permission').fire('added-permission-removed');
flushAsynchronousOperations();
assert.equal(element._permissions.length, 0);
});
test('remove an added section', () => {
const removeStub = sandbox.stub();
element.addEventListener('added-section-removed', removeStub);
element.editing = true;
element.section.value.added = true;
MockInteractions.tap(element.$.deleteBtn);
assert.isTrue(removeStub.called);
});
});
});
});

3
polygerrit-ui/app/elements/admin/gr-permission/gr-permission.html
View File

@ -115,7 +115,8 @@ limitations under the License.
group-name="[[_computeGroupName(groups, rule.id)]]"
permission="[[permission.id]]"
rule="{{rule}}"
section="[[section]]"></gr-rule-editor>
section="[[section]]"
on-added-rule-removed="_handleAddedRuleRemoved"></gr-rule-editor>
</template>
<div id="addRule">
<gr-autocomplete

15
polygerrit-ui/app/elements/admin/gr-permission/gr-permission.js
View File

@ -25,6 +25,11 @@
* @event access-modified
*/
/**
* Fired when a permission that was previously added was removed.
* @event added-permission-removed
*/
Polymer({
is: 'gr-permission',
@ -117,6 +122,12 @@
}
},
_handleAddedRuleRemoved(e) {
const index = e.model.index;
this._rules = this._rules.slice(0, index)
.concat(this._rules.slice(index + 1, this._rules.length));
},
_handleValueChange() {
this.permission.value.modified = true;
// Allows overall access page to know a change has been made.
@ -124,6 +135,10 @@
},
_handleRemovePermission() {
if (this.permission.value.added) {
this.dispatchEvent(new CustomEvent('added-permission-removed',
{bubbles: true}));
}
this._deleted = true;
this.permission.value.deleted = true;
this.dispatchEvent(new CustomEvent('access-modified', {bubbles: true}));

26
polygerrit-ui/app/elements/admin/gr-permission/gr-permission_test.html
View File

@ -327,11 +327,36 @@ limitations under the License.
assert.equal(Object.keys(element.permission.value.rules).length, 2);
});
test('removing an added rule', () => {
element.name = 'Priority';
element.section = 'refs/*';
element.groups = {};
element.$.groupAutocomplete.text = 'new group name';
assert.equal(element._rules.length, 2);
element.$$('gr-rule-editor').fire('added-rule-removed');
flushAsynchronousOperations();
assert.equal(element._rules.length, 1);
});
test('removing an added permission', () => {
const removeStub = sandbox.stub();
element.addEventListener('added-permission-removed', removeStub);
element.editing = true;
element.name = 'Priority';
element.section = 'refs/*';
element.permission.value.added = true;
MockInteractions.tap(element.$.removeBtn);
assert.isTrue(removeStub.called);
});
test('removing the permission', () => {
element.editing = true;
element.name = 'Priority';
element.section = 'refs/*';
const removeStub = sandbox.stub();
element.addEventListener('added-permission-removed', removeStub);
assert.isFalse(element.$.permission.classList.contains('deleted'));
assert.isFalse(element._deleted);
MockInteractions.tap(element.$.removeBtn);
@ -340,6 +365,7 @@ limitations under the License.
MockInteractions.tap(element.$.undoRemoveBtn);
assert.isFalse(element.$.permission.classList.contains('deleted'));
assert.isFalse(element._deleted);
assert.isFalse(removeStub.called);
});
test('modify a permission', () => {

3
polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html
View File

@ -111,7 +111,8 @@ limitations under the License.
section="{{section}}"
labels="[[_labels]]"
editing="[[_editing]]"
groups="[[_groups]]"></gr-access-section>
groups="[[_groups]]"
on-added-section-removed="_handleAddedSectionRemoved"></gr-access-section>
</template>
<div class="referenceContainer">
<gr-button id="addReferenceBtn"

6
polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js
View File

@ -243,6 +243,12 @@
return inheritsFrom ? 'show' : '';
},
_handleAddedSectionRemoved(e) {
const index = e.model.index;
this._sections = this._sections.slice(0, index)
.concat(this._sections.slice(index + 1, this._sections.length));
},
_handleEditingChanged(editing, editingOld) {
// Ignore when editing gets set initially.
if (!editingOld || editing) { return; }

8
polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html
View File

@ -301,6 +301,14 @@ limitations under the License.
flushAsynchronousOperations();
});
test('removing an added section', () => {
element.editing = true;
assert.equal(element._sections.length, 1);
element.$$('gr-access-section').fire('added-section-removed');
flushAsynchronousOperations();
assert.equal(element._sections.length, 0);
});
test('button visibility for non admin', () => {
assert.equal(getComputedStyle(element.$.saveBtn).display, 'none');
assert.equal(getComputedStyle(element.$.editBtn).display, 'none');

9
polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.js
View File

@ -23,6 +23,11 @@
* @event access-modified
*/
/**
* Fired when a rule that was previously added was removed.
* @event added-rule-removed
*/
const PRIORITY_OPTIONS = [
'BATCH',
'INTERACTIVE',
@ -202,6 +207,10 @@
},
_handleRemoveRule() {
if (this.rule.value.added) {
this.dispatchEvent(new CustomEvent('added-rule-removed',
{bubbles: true}));
}
this._deleted = true;
this.rule.value.deleted = true;
this.dispatchEvent(new CustomEvent('access-modified', {bubbles: true}));

60
polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor_test.html
View File

@ -319,6 +319,7 @@ limitations under the License.
element.section = 'refs/*';
element._setupValues(element.rule);
flushAsynchronousOperations();
element.rule.value.added = true;
});
test('_ruleValues and _originalRuleValues are set correctly', () => {
@ -328,9 +329,9 @@ limitations under the License.
const expectedRuleValue = {
action: 'ALLOW',
force: false,
added: true,
};
assert.deepEqual(element.rule.value, expectedRuleValue);
assert.deepEqual(element._originalRuleValues, expectedRuleValue);
test('values are set correctly', () => {
assert.equal(element.$.action.bindValue, expectedRuleValue.action);
assert.equal(element.$.force.bindValue, expectedRuleValue.action);
@ -346,6 +347,15 @@ limitations under the License.
// The original value should now differ from the rule values.
assert.notDeepEqual(element._originalRuleValues, element.rule.value);
});
test('remove value', () => {
element.editing = true;
const removeStub = sandbox.stub();
element.addEventListener('added-rule-removed', removeStub);
MockInteractions.tap(element.$.removeBtn);
flushAsynchronousOperations();
assert.isTrue(removeStub.called);
});
});
suite('already existing rule with labels', () => {
@ -389,10 +399,13 @@ limitations under the License.
});
test('modify value', () => {
const removeStub = sandbox.stub();
element.addEventListener('added-rule-removed', removeStub);
assert.isNotOk(element.rule.value.modified);
Polymer.dom(element.root).querySelector('#labelMin').bindValue = 1;
flushAsynchronousOperations();
assert.isTrue(element.rule.value.modified);
assert.isFalse(removeStub.called);
// The original value should now differ from the rule values.
assert.notDeepEqual(element._originalRuleValues, element.rule.value);
@ -417,6 +430,7 @@ limitations under the License.
element.section = 'refs/*';
element._setupValues(element.rule);
flushAsynchronousOperations();
element.rule.value.added = true;
});
test('_ruleValues and _originalRuleValues are set correctly', () => {
@ -429,9 +443,9 @@ limitations under the License.
max: element.label.values[element.label.values.length - 1].value,
min: element.label.values[0].value,
action: 'ALLOW',
added: true,
};
assert.deepEqual(element.rule.value, expectedRuleValue);
assert.deepEqual(element._originalRuleValues, expectedRuleValue);
test('values are set correctly', () => {
assert.equal(
element.$.action.bindValue,
@ -507,6 +521,7 @@ limitations under the License.
element.section = 'refs/*';
element._setupValues(element.rule);
flushAsynchronousOperations();
element.rule.value.added = true;
});
test('_ruleValues and _originalRuleValues are set correctly', () => {
@ -516,9 +531,9 @@ limitations under the License.
const expectedRuleValue = {
action: 'ALLOW',
force: false,
added: true,
};
assert.deepEqual(element.rule.value, expectedRuleValue);
assert.deepEqual(element._originalRuleValues, expectedRuleValue);
test('values are set correctly', () => {
assert.equal(element.$.action.bindValue, expectedRuleValue.action);
assert.equal(element.$.force.bindValue, expectedRuleValue.action);
@ -576,44 +591,5 @@ limitations under the License.
assert.notDeepEqual(element._originalRuleValues, element.rule.value);
});
});
suite('new edit rule', () => {
setup(() => {
element.group = 'Group Name';
element.permission = 'editTopicName';
element.rule = {
id: '123',
};
element.section = 'refs/*';
element._setupValues(element.rule);
flushAsynchronousOperations();
});
test('_ruleValues and _originalRuleValues are set correctly', () => {
// Since the element does not already have default values, they should
// be set. The original values should be set to those too.
assert.isNotOk(element.rule.value.modified);
const expectedRuleValue = {
action: 'ALLOW',
force: false,
};
assert.deepEqual(element.rule.value, expectedRuleValue);
assert.deepEqual(element._originalRuleValues, expectedRuleValue);
test('values are set correctly', () => {
assert.equal(element.$.action.bindValue, expectedRuleValue.action);
assert.equal(element.$.force.bindValue, expectedRuleValue.action);
});
});
test('modify value', () => {
assert.isNotOk(element.rule.value.modified);
element.$.force.bindValue = true;
flushAsynchronousOperations();
assert.isTrue(element.rule.value.modified);
// The original value should now differ from the rule values.
assert.notDeepEqual(element._originalRuleValues, element.rule.value);
});
});
});
</script>