Revert "Display the approval table on the Publish Comments screen." This reverts commit 3fc98f99b4bcb87be6c3d37ab92014ee68e1c627. The display was buggy and listed users as "Anonymous Coward" even though they had a Full Name supplied in the Account. I don't have time to debug this myself, but users are unhappy the table is confusing/useless with everyone being called an Anonymous Coward so the change goes out. Bug: issue 1383 CC: Joe Hansche <jhansche@myyearbook.com> Change-Id: Ibe9f70bc001df4b3a04ee2eb4dfa194f75e5b3f0
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchSetPublishDetail.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchSetPublishDetail.java index 3c5c688..075d558 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchSetPublishDetail.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchSetPublishDetail.java
@@ -20,9 +20,6 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.PatchSetInfo; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; import java.util.List; public class PatchSetPublishDetail { @@ -31,8 +28,6 @@ protected Change change; protected List<PatchLineComment> drafts; protected List<PermissionRange> labels; - protected List<ApprovalDetail> approvals; - protected List<SubmitRecord> submitRecords; protected List<PatchSetApproval> given; protected boolean canSubmit; @@ -44,23 +39,6 @@ this.labels = labels; } - public List<ApprovalDetail> getApprovals() { - return approvals; - } - - public void setApprovals(Collection<ApprovalDetail> list) { - approvals = new ArrayList<ApprovalDetail>(list); - Collections.sort(approvals, ApprovalDetail.SORT); - } - - public void setSubmitRecords(List<SubmitRecord> all) { - submitRecords = all; - } - - public List<SubmitRecord> getSubmitRecords() { - return submitRecords; - } - public List<PatchSetApproval> getGiven() { return given; }
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java index 73036ff..20df1df 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
@@ -29,7 +29,6 @@ import com.google.gerrit.common.data.ApprovalType; import com.google.gerrit.common.data.ApprovalTypes; import com.google.gerrit.common.data.ChangeDetail; -import com.google.gerrit.common.data.PatchSetPublishDetail; import com.google.gerrit.common.data.ReviewerResult; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; @@ -134,22 +133,12 @@ return AccountLink.link(accountCache, id); } - void display(PatchSetPublishDetail detail) { - doDisplay(detail.getChange(), detail.getApprovals(), - detail.getSubmitRecords()); - } - void display(ChangeDetail detail) { - doDisplay(detail.getChange(), detail.getApprovals(), - detail.getSubmitRecords()); - } - - private void doDisplay(Change change, List<ApprovalDetail> approvals, - List<SubmitRecord> submitRecords) { - changeId = change.getId(); + changeId = detail.getChange().getId(); reviewerSuggestOracle.setChange(changeId); List<String> columns = new ArrayList<String>(); + List<ApprovalDetail> rows = detail.getApprovals(); final Element missingList = missing.getElement(); while (DOM.getChildCount(missingList) > 0) { @@ -157,16 +146,16 @@ } missing.setVisible(false); - if (submitRecords != null) { + if (detail.getSubmitRecords() != null) { HashSet<String> reportedMissing = new HashSet<String>(); HashMap<Account.Id, ApprovalDetail> byUser = new HashMap<Account.Id, ApprovalDetail>(); - for (ApprovalDetail ad : approvals) { + for (ApprovalDetail ad : detail.getApprovals()) { byUser.put(ad.getAccount(), ad); } - for (SubmitRecord rec : submitRecords) { + for (SubmitRecord rec : detail.getSubmitRecords()) { if (rec.labels == null) { continue; } @@ -211,7 +200,7 @@ missing.setVisible(!reportedMissing.isEmpty()); } else { - for (ApprovalDetail ad : approvals) { + for (ApprovalDetail ad : rows) { for (PatchSetApproval psa : ad.getPatchSetApprovals()) { ApprovalType legacyType = types.byId(psa.getCategoryId()); if (legacyType == null) { @@ -247,13 +236,13 @@ } } - if (approvals.isEmpty()) { + if (rows.isEmpty()) { table.setVisible(false); } else { displayHeader(columns); - table.resizeRows(1 + approvals.size()); - for (int i = 0; i < approvals.size(); i++) { - displayRow(i + 1, approvals.get(i), change, columns); + table.resizeRows(1 + rows.size()); + for (int i = 0; i < rows.size(); i++) { + displayRow(i + 1, rows.get(i), detail.getChange(), columns); } table.setVisible(true); } @@ -261,7 +250,7 @@ addReviewer.setVisible(Gerrit.isSignedIn()); if (Gerrit.getConfig().testChangeMerge() - && !change.isMergeable()) { + && !detail.getChange().isMergeable()) { Element li = DOM.createElement("li"); li.setClassName(Gerrit.RESOURCES.css().missingApproval()); DOM.setInnerText(li, Util.C.messageNeedsRebaseOrHasDependency());
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java index 4705aad..1713331 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java
@@ -64,7 +64,6 @@ private final PatchSet.Id patchSetId; private Collection<ValueRadioButton> approvalButtons; private ChangeDescriptionBlock descBlock; - private ApprovalTable approvals; private Panel approvalPanel; private NpTextArea message; private FlowPanel draftsPanel; @@ -87,9 +86,6 @@ descBlock = new ChangeDescriptionBlock(null); add(descBlock); - approvals = new ApprovalTable(); - add(approvals); - final FormPanel form = new FormPanel(); final FlowPanel body = new FlowPanel(); form.setWidget(body); @@ -278,11 +274,6 @@ if (r.getChange().getStatus().isOpen()) { initApprovals(r, approvalPanel); - - approvals.setAccountInfoCache(r.getAccounts()); - approvals.display(r); - } else { - approvals.setVisible(false); } if (lastState != null && patchSetId.equals(lastState.patchSetId)) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css b/gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css index c5522b3..dc0e171 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css
@@ -955,7 +955,7 @@ margin-top: 5px; } -.approvalTable { +.changeScreen .approvalTable { margin-top: 1em; margin-bottom: 1em; }
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java index bc05c6a..37a1c92 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
@@ -14,14 +14,12 @@ package com.google.gerrit.httpd.rpc.changedetail; -import com.google.gerrit.common.data.ApprovalDetail; import com.google.gerrit.common.data.ApprovalType; import com.google.gerrit.common.data.ApprovalTypes; import com.google.gerrit.common.data.PatchSetPublishDetail; import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.httpd.rpc.Handler; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; @@ -34,8 +32,6 @@ import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; -import com.google.gerrit.server.workflow.CategoryFunction; -import com.google.gerrit.server.workflow.FunctionState; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -53,7 +49,6 @@ private final PatchSetInfoFactory infoFactory; private final ReviewDb db; - private final FunctionState.Factory functionState; private final ChangeControl.Factory changeControlFactory; private final ApprovalTypes approvalTypes; private final AccountInfoCacheFactory aic; @@ -69,13 +64,11 @@ PatchSetPublishDetailFactory(final PatchSetInfoFactory infoFactory, final ReviewDb db, final AccountInfoCacheFactory.Factory accountInfoCacheFactory, - final FunctionState.Factory functionState, final ChangeControl.Factory changeControlFactory, final ApprovalTypes approvalTypes, final IdentifiedUser user, @Assisted final PatchSet.Id patchSetId) { this.infoFactory = infoFactory; this.db = db; - this.functionState = functionState; this.changeControlFactory = changeControlFactory; this.approvalTypes = approvalTypes; this.aic = accountInfoCacheFactory.create(); @@ -137,8 +130,6 @@ int ok = 0; for (SubmitRecord.Label lbl : rec.labels) { - aic.want(lbl.appliedBy); - boolean canMakeOk = false; PermissionRange range = rangeByName.get(lbl.label); if (range != null) { @@ -176,60 +167,12 @@ if (couldSubmit && control.getRefControl().canSubmit()) { detail.setCanSubmit(true); } - - detail.setSubmitRecords(submitRecords); } detail.setLabels(allowed); detail.setGiven(given); detail.setAccounts(aic.create()); - loadApprovals(detail, control); - return detail; } - - private void loadApprovals(final PatchSetPublishDetail detail, - final ChangeControl control) throws OrmException { - final PatchSet.Id psId = detail.getChange().currentPatchSetId(); - final Change.Id changeId = patchSetId.getParentKey(); - final List<PatchSetApproval> allApprovals = - db.patchSetApprovals().byChange(changeId).toList(); - - if (detail.getChange().getStatus().isOpen()) { - final FunctionState fs = functionState.create(control, psId, allApprovals); - - for (final ApprovalType at : approvalTypes.getApprovalTypes()) { - CategoryFunction.forCategory(at.getCategory()).run(at, fs); - } - } - - final boolean canRemoveReviewers = detail.getChange().getStatus().isOpen() // - && control.getCurrentUser() instanceof IdentifiedUser; - final HashMap<Account.Id, ApprovalDetail> ad = - new HashMap<Account.Id, ApprovalDetail>(); - for (PatchSetApproval ca : allApprovals) { - ApprovalDetail d = ad.get(ca.getAccountId()); - if (d == null) { - d = new ApprovalDetail(ca.getAccountId()); - d.setCanRemove(canRemoveReviewers); - ad.put(d.getAccount(), d); - } - if (d.canRemove()) { - d.setCanRemove(control.canRemoveReviewer(ca)); - } - if (ca.getPatchSetId().equals(psId)) { - d.add(ca); - } - } - - final Account.Id owner = detail.getChange().getOwner(); - if (ad.containsKey(owner)) { - // Ensure the owner always sorts to the top of the table - ad.get(owner).sortFirst(); - } - - aic.want(ad.keySet()); - detail.setApprovals(ad.values()); - } }