QChris has uploaded a new change for review.
https://gerrit.wikimedia.org/r/62424
Change subject: Fix reviewer email notifications for new patch sets
......................................................................
Fix reviewer email notifications for new patch sets
Since a7c4a3a6de7353e24d7ed112f92893f3144a85af, no emails were sent to
reviewers when a new patch set has been uploaded to a change.
This regression was due to a change when copying labels. While before
the above commit all approvals of a change were returned, only the
approvals of the previous patch set got returned after the above
commit.
Reverting to the previous behaviour would unnecessarily fetch more
rows than needed as most callers do not pick up the returned set.
When uploading a new patch set, we now again fetch all approvals for
the change and can thereby again notify all reviewers. For copying
labels to the new patch set, we reuse this fetched set of approvals
and thereby avoid a further database query.
Submitted-Upstream: Yes
Bug: 48115
Change-Id: Iba1e8585054cdef3e422bd951b0c61aa824b84fc
---
M gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
M gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
2 files changed, 32 insertions(+), 18 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/gerrit refs/changes/24/62424/1
diff --git
a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
index 76c33ab..3fd24f1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -64,29 +64,39 @@
}
/**
- * Moves the PatchSetApprovals to the specified PatchSet on the change from
- * the prior PatchSet, while keeping the vetos.
+ * Copy min/max scores from one patch set to another.
*
* @throws OrmException
- * @return List<PatchSetApproval> The previous approvals
*/
- public static List<PatchSetApproval> copyLabels(ReviewDb db,
- LabelTypes labelTypes,
- PatchSet.Id source,
+ public static void copyLabels(ReviewDb db, LabelTypes labelTypes,
+ PatchSet.Id source, PatchSet.Id dest) throws OrmException {
+ Iterable<PatchSetApproval> sourceApprovals =
+ db.patchSetApprovals().byPatchSet(source);
+ copyLabels(db, labelTypes, sourceApprovals, source, dest);
+ }
+
+ /**
+ * Copy a set's min/max scores from one patch set to another.
+ *
+ * @throws OrmException
+ */
+ public static void copyLabels(ReviewDb db, LabelTypes labelTypes,
+ Iterable<PatchSetApproval> sourceApprovals, PatchSet.Id source,
PatchSet.Id dest) throws OrmException {
List<PatchSetApproval> copied = Lists.newArrayList();
- for (PatchSetApproval a : db.patchSetApprovals().byPatchSet(source)) {
- LabelType type = labelTypes.byLabel(a.getLabelId());
- if (type == null) {
- continue;
- } else if (type.isCopyMinScore() && type.isMaxNegative(a)) {
- copied.add(new PatchSetApproval(dest, a));
- } else if (type.isCopyMaxScore() && type.isMaxPositive(a)) {
- copied.add(new PatchSetApproval(dest, a));
+ for (PatchSetApproval a : sourceApprovals) {
+ if (source.equals(a.getPatchSetId())) {
+ LabelType type = labelTypes.byLabel(a.getLabelId());
+ if (type == null) {
+ continue;
+ } else if (type.isCopyMinScore() && type.isMaxNegative(a)) {
+ copied.add(new PatchSetApproval(dest, a));
+ } else if (type.isCopyMaxScore() && type.isMaxPositive(a)) {
+ copied.add(new PatchSetApproval(dest, a));
+ }
}
}
db.patchSetApprovals().insert(copied);
- return copied;
}
public void addReviewers(ReviewDb db, LabelTypes labelTypes, Change change,
diff --git
a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 9dd5a4f..251ef14 100644
---
a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++
b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -49,6 +49,7 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
@@ -1765,9 +1766,12 @@
mergedIntoRef = mergedInto != null ? mergedInto.getName() : null;
}
- final MailRecipients oldRecipients =
- getRecipientsFromApprovals(ApprovalsUtil.copyLabels(
- db, labelTypes, priorPatchSet, newPatchSet.getId()));
+ List<PatchSetApproval> oldChangeApprovals =
+ db.patchSetApprovals().byChange(change.getId()).toList();
+ final MailRecipients oldRecipients = getRecipientsFromApprovals(
+ oldChangeApprovals);
+ ApprovalsUtil.copyLabels(db, labelTypes, oldChangeApprovals,
+ priorPatchSet, newPatchSet.getId());
approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info,
recipients.getReviewers(), oldRecipients.getAll());
recipients.add(oldRecipients);
--
To view, visit https://gerrit.wikimedia.org/r/62424
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba1e8585054cdef3e422bd951b0c61aa824b84fc
Gerrit-PatchSet: 1
Gerrit-Project: gerrit
Gerrit-Branch: wmf
Gerrit-Owner: QChris <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
