From 895d18b7f217f4cb1a72b99f3c5c52e93ccd93fa Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 9 May 2012 18:13:54 +0200 Subject: [PATCH] Display error if modifying access rights for a ref is forbidden If a user is owner of at least one ref he is able to edit the access rights on a project. If he adds access rights for other refs, these access rights were silently ignored on save. With this change now an error message is displayed to inform the user that he doesn't have permissions to do the update for these refs. In case of such an error the project access screen stays in the edit mode so that the unsaved modifications are not lost. This gets important when the user will be able to propose changes to the access rights through code review. This will be implemented by a follow-up change. Change-Id: Ibb3b89cbae4d7ca44558d45a4df4c6af3de313e2 Signed-off-by: Edwin Kempin --- .../gerrit/common/data/AccessSection.java | 9 ++++ .../google/gerrit/common/data/Permission.java | 20 +++++++++ .../gerrit/common/data/PermissionRule.java | 15 +++++++ .../gerrit/common/data/RefConfigSection.java | 13 ++++++ .../google/gerrit/client/GerritConstants.java | 2 + .../gerrit/client/GerritConstants.properties | 4 +- .../client/admin/ProjectAccessScreen.java | 42 +++++++++++++++++-- .../client/admin/ProjectAccessScreen.ui.xml | 9 ++++ 8 files changed, 110 insertions(+), 4 deletions(-) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/AccessSection.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/AccessSection.java index cd64b0abd0..a9b5e85503 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/AccessSection.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/AccessSection.java @@ -118,4 +118,13 @@ public class AccessSection extends RefConfigSection implements public String toString() { return "AccessSection[" + getName() + "]"; } + + @Override + public boolean equals(final Object obj) { + if (!super.equals(obj) || !(obj instanceof AccessSection)) { + return false; + } + return new HashSet(permissions).equals(new HashSet( + ((AccessSection) obj).permissions)); + } } diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java index 60bf19c0f5..5cb7787505 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java @@ -15,6 +15,7 @@ package com.google.gerrit.common.data; import java.util.ArrayList; +import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -220,4 +221,23 @@ public class Permission implements Comparable { int index = NAMES_LC.indexOf(a.getName().toLowerCase()); return 0 <= index ? index : NAMES_LC.size(); } + + @Override + public boolean equals(final Object obj) { + if (!(obj instanceof Permission)) { + return false; + } + + final Permission other = (Permission) obj; + if (!name.equals(other.name) || exclusiveGroup != other.exclusiveGroup) { + return false; + } + return new HashSet(rules) + .equals(new HashSet(other.rules)); + } + + @Override + public int hashCode() { + return name.hashCode(); + } } diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java index 9b6695e1c0..5960165e27 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java @@ -257,4 +257,19 @@ public class PermissionRule implements Comparable { } return Integer.parseInt(value); } + + @Override + public boolean equals(final Object obj) { + if (!(obj instanceof PermissionRule)) { + return false; + } + final PermissionRule other = (PermissionRule)obj; + return action.equals(other.action) && force == other.force + && min == other.min && max == other.max && group.equals(other.group); + } + + @Override + public int hashCode() { + return group.hashCode(); + } } diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/RefConfigSection.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/RefConfigSection.java index a398c3690c..810e906a3a 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/RefConfigSection.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/RefConfigSection.java @@ -48,4 +48,17 @@ public abstract class RefConfigSection { public void setName(String name) { this.name = name; } + + @Override + public boolean equals(final Object obj) { + if (!(obj instanceof RefConfigSection)) { + return false; + } + return name.equals(((RefConfigSection) obj).name); + } + + @Override + public int hashCode() { + return name.hashCode(); + } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.java index 43afe1ed3f..1b5abdbf21 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.java @@ -99,4 +99,6 @@ public interface GerritConstants extends Constants { String jumpMineWatched(); String jumpMineStarred(); String jumpMineDraftComments(); + + String projectAccessError(); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.properties index 277c380cb2..df31d48f23 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.properties @@ -81,4 +81,6 @@ jumpMine = Go to my dashboard jumpMineWatched = Go to watched changes jumpMineDrafts = Go to drafts jumpMineStarred = Go to starred changes -jumpMineDraftComments = Go to draft comments \ No newline at end of file +jumpMineDraftComments = Go to draft comments + +projectAccessError = You don't have permissions to modify the access rights for the following refs: diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.java index 923a63e3cc..478ea7d9c4 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.java @@ -31,10 +31,14 @@ import com.google.gwt.uibinder.client.UiHandler; import com.google.gwt.user.client.Window; import com.google.gwt.user.client.ui.Button; import com.google.gwt.user.client.ui.HTMLPanel; +import com.google.gwt.user.client.ui.Label; import com.google.gwt.user.client.ui.UIObject; +import com.google.gwt.user.client.ui.VerticalPanel; import com.google.gwtexpui.globalkey.client.NpTextArea; import java.util.Collections; +import java.util.HashSet; +import java.util.Set; public class ProjectAccessScreen extends ProjectScreen { interface Binder extends UiBinder { @@ -59,6 +63,9 @@ public class ProjectAccessScreen extends ProjectScreen { @UiField Button cancel2; + @UiField + VerticalPanel error; + @UiField ProjectAccessEditor accessEditor; @@ -141,7 +148,7 @@ public class ProjectAccessScreen extends ProjectScreen { @UiHandler("commit") void onCommit(ClickEvent event) { - ProjectAccess access = driver.flush(); + final ProjectAccess access = driver.flush(); if (driver.hasErrors()) { Window.alert(Util.C.errorsMustBeFixed()); @@ -161,14 +168,43 @@ public class ProjectAccessScreen extends ProjectScreen { access.getLocal(), // new GerritCallback() { @Override - public void onSuccess(ProjectAccess access) { + public void onSuccess(ProjectAccess newAccess) { enable(true); commitMessage.setText(""); - displayReadOnly(access); + error.clear(); + final Set diffs = getDiffs(access, newAccess); + if (diffs.isEmpty()) { + displayReadOnly(newAccess); + } else { + error.add(new Label(Gerrit.C.projectAccessError())); + for (final String diff : diffs) { + error.add(new Label(diff)); + } + } + } + + private Set getDiffs(ProjectAccess wantedAccess, + ProjectAccess newAccess) { + final HashSet same = + new HashSet(wantedAccess.getLocal()); + final HashSet different = + new HashSet(wantedAccess.getLocal().size() + + newAccess.getLocal().size()); + different.addAll(wantedAccess.getLocal()); + different.addAll(newAccess.getLocal()); + same.retainAll(newAccess.getLocal()); + different.removeAll(same); + + final Set differentNames = new HashSet(); + for (final AccessSection s : different) { + differentNames.add(s.getName()); + } + return differentNames; } @Override public void onFailure(Throwable caught) { + error.clear(); enable(true); super.onFailure(caught); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.ui.xml index 25361599eb..e690a0b189 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.ui.xml @@ -32,6 +32,11 @@ limitations under the License. .commitMessage .gwt-TextArea { margin: 5px 5px 5px 5px; } + .errorMessage { + margin-top: 5px; + margin-bottom: 5px; + color: red; + } @@ -58,6 +63,10 @@ limitations under the License. spellCheck='true' /> + +