Rework SUBM label in LabelId
Use a static method instead of a public LabelId field. LabelId is mutable, so naming it like a constant is misleading at best. At worst, a caller could call SUBMIT.set(newValue) and change the value seen by all callers in the process. (Fortunately no callers were doing this as far as I could tell.) While we're in there, rename methods to refer to this as a "legacy" submit label, as we want to eventually get rid of it in NoteDb. Change-Id: Id7fbddfaac1d625839c66c73231d4e4ba2550c50
This commit is contained in:
		@@ -225,7 +225,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
  private void assertSubmitApproval(PatchSet.Id patchSetId) throws Exception {
 | 
					  private void assertSubmitApproval(PatchSet.Id patchSetId) throws Exception {
 | 
				
			||||||
    PatchSetApproval a = getSubmitter(patchSetId);
 | 
					    PatchSetApproval a = getSubmitter(patchSetId);
 | 
				
			||||||
    assertThat(a.isSubmit()).isTrue();
 | 
					    assertThat(a.isLegacySubmit()).isTrue();
 | 
				
			||||||
    assertThat(a.getValue()).isEqualTo((short) 1);
 | 
					    assertThat(a.getValue()).isEqualTo((short) 1);
 | 
				
			||||||
    assertThat(a.getAccountId()).isEqualTo(admin.id);
 | 
					    assertThat(a.getAccountId()).isEqualTo(admin.id);
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -356,7 +356,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
 | 
				
			|||||||
    PatchSetApproval submitter = approvalsUtil.getSubmitter(
 | 
					    PatchSetApproval submitter = approvalsUtil.getSubmitter(
 | 
				
			||||||
        db, cn, new PatchSet.Id(cn.getChangeId(), psId));
 | 
					        db, cn, new PatchSet.Id(cn.getChangeId(), psId));
 | 
				
			||||||
    assertThat(submitter).isNotNull();
 | 
					    assertThat(submitter).isNotNull();
 | 
				
			||||||
    assertThat(submitter.isSubmit()).isTrue();
 | 
					    assertThat(submitter.isLegacySubmit()).isTrue();
 | 
				
			||||||
    assertThat(submitter.getAccountId()).isEqualTo(admin.getId());
 | 
					    assertThat(submitter.getAccountId()).isEqualTo(admin.getId());
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -20,7 +20,11 @@ import com.google.gwtorm.client.StringKey;
 | 
				
			|||||||
public class LabelId extends StringKey<com.google.gwtorm.client.Key<?>> {
 | 
					public class LabelId extends StringKey<com.google.gwtorm.client.Key<?>> {
 | 
				
			||||||
  private static final long serialVersionUID = 1L;
 | 
					  private static final long serialVersionUID = 1L;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  public static final LabelId SUBMIT = new LabelId("SUBM");
 | 
					  static final String LEGACY_SUBMIT_NAME = "SUBM";
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					  public static LabelId legacySubmit() {
 | 
				
			||||||
 | 
					    return new LabelId(LEGACY_SUBMIT_NAME);
 | 
				
			||||||
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  @Column(id = 1)
 | 
					  @Column(id = 1)
 | 
				
			||||||
  public String id;
 | 
					  public String id;
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -149,8 +149,8 @@ public final class PatchSetApproval {
 | 
				
			|||||||
    return getLabelId().get();
 | 
					    return getLabelId().get();
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  public boolean isSubmit() {
 | 
					  public boolean isLegacySubmit() {
 | 
				
			||||||
    return LabelId.SUBMIT.get().equals(getLabel());
 | 
					    return LabelId.LEGACY_SUBMIT_NAME.equals(getLabel());
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  @Override
 | 
					  @Override
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -321,7 +321,7 @@ public class ApprovalsUtil {
 | 
				
			|||||||
    }
 | 
					    }
 | 
				
			||||||
    PatchSetApproval submitter = null;
 | 
					    PatchSetApproval submitter = null;
 | 
				
			||||||
    for (PatchSetApproval a : approvals) {
 | 
					    for (PatchSetApproval a : approvals) {
 | 
				
			||||||
      if (a.getPatchSetId().equals(c) && a.getValue() > 0 && a.isSubmit()) {
 | 
					      if (a.getPatchSetId().equals(c) && a.getValue() > 0 && a.isLegacySubmit()) {
 | 
				
			||||||
        if (submitter == null
 | 
					        if (submitter == null
 | 
				
			||||||
            || a.getGranted().compareTo(submitter.getGranted()) > 0) {
 | 
					            || a.getGranted().compareTo(submitter.getGranted()) > 0) {
 | 
				
			||||||
          submitter = a;
 | 
					          submitter = a;
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -611,7 +611,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
      for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
 | 
					      for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
 | 
				
			||||||
          ctx.getDb(), ctx.getControl(), psId, user.getAccountId())) {
 | 
					          ctx.getDb(), ctx.getControl(), psId, user.getAccountId())) {
 | 
				
			||||||
        if (a.isSubmit()) {
 | 
					        if (a.isLegacySubmit()) {
 | 
				
			||||||
          continue;
 | 
					          continue;
 | 
				
			||||||
        }
 | 
					        }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -127,7 +127,7 @@ public class LabelNormalizer {
 | 
				
			|||||||
      checkArgument(changeId.equals(ctl.getId()),
 | 
					      checkArgument(changeId.equals(ctl.getId()),
 | 
				
			||||||
          "Approval %s does not match change %s",
 | 
					          "Approval %s does not match change %s",
 | 
				
			||||||
          psa.getKey(), ctl.getChange().getKey());
 | 
					          psa.getKey(), ctl.getChange().getKey());
 | 
				
			||||||
      if (psa.isSubmit()) {
 | 
					      if (psa.isLegacySubmit()) {
 | 
				
			||||||
        unchanged.add(psa);
 | 
					        unchanged.add(psa);
 | 
				
			||||||
        continue;
 | 
					        continue;
 | 
				
			||||||
      }
 | 
					      }
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -253,7 +253,7 @@ public class MergeUtil {
 | 
				
			|||||||
        continue;
 | 
					        continue;
 | 
				
			||||||
      }
 | 
					      }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
      if (a.isSubmit()) {
 | 
					      if (a.isLegacySubmit()) {
 | 
				
			||||||
        // Submit is treated specially, below (becomes committer)
 | 
					        // Submit is treated specially, below (becomes committer)
 | 
				
			||||||
        //
 | 
					        //
 | 
				
			||||||
        if (submitAudit == null
 | 
					        if (submitAudit == null
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -2565,7 +2565,7 @@ public class ReceiveCommits {
 | 
				
			|||||||
                new PatchSetApproval.Key(
 | 
					                new PatchSetApproval.Key(
 | 
				
			||||||
                    change.currentPatchSetId(),
 | 
					                    change.currentPatchSetId(),
 | 
				
			||||||
                    ctx.getUser().getAccountId(),
 | 
					                    ctx.getUser().getAccountId(),
 | 
				
			||||||
                    LabelId.SUBMIT),
 | 
					                    LabelId.legacySubmit()),
 | 
				
			||||||
                    (short) 1, ctx.getWhen());
 | 
					                    (short) 1, ctx.getWhen());
 | 
				
			||||||
          update.putApproval(submitter.getLabel(), submitter.getValue());
 | 
					          update.putApproval(submitter.getLabel(), submitter.getValue());
 | 
				
			||||||
          ctx.getDb().patchSetApprovals().upsert(
 | 
					          ctx.getDb().patchSetApprovals().upsert(
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -301,7 +301,7 @@ public class ReplaceOp extends BatchUpdate.Op {
 | 
				
			|||||||
      for (PatchSetApproval a : approvalsUtil.byPatchSetUser(ctx.getDb(),
 | 
					      for (PatchSetApproval a : approvalsUtil.byPatchSetUser(ctx.getDb(),
 | 
				
			||||||
          ctx.getControl(), priorPatchSetId,
 | 
					          ctx.getControl(), priorPatchSetId,
 | 
				
			||||||
          ctx.getUser().getAccountId())) {
 | 
					          ctx.getUser().getAccountId())) {
 | 
				
			||||||
        if (a.isSubmit()) {
 | 
					        if (a.isLegacySubmit()) {
 | 
				
			||||||
          continue;
 | 
					          continue;
 | 
				
			||||||
        }
 | 
					        }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -337,7 +337,7 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
 | 
				
			|||||||
          new PatchSetApproval.Key(
 | 
					          new PatchSetApproval.Key(
 | 
				
			||||||
              psId,
 | 
					              psId,
 | 
				
			||||||
              ctx.getUser().getAccountId(),
 | 
					              ctx.getUser().getAccountId(),
 | 
				
			||||||
              LabelId.SUBMIT),
 | 
					              LabelId.legacySubmit()),
 | 
				
			||||||
              (short) 1, ctx.getWhen());
 | 
					              (short) 1, ctx.getWhen());
 | 
				
			||||||
    byKey.put(submitter.getKey(), submitter);
 | 
					    byKey.put(submitter.getKey(), submitter);
 | 
				
			||||||
    submitter.setValue((short) 1);
 | 
					    submitter.setValue((short) 1);
 | 
				
			||||||
@@ -373,7 +373,7 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
 | 
				
			|||||||
    // TODO(dborowitz): Don't use a label in notedb; just check when status
 | 
					    // TODO(dborowitz): Don't use a label in notedb; just check when status
 | 
				
			||||||
    // change happened.
 | 
					    // change happened.
 | 
				
			||||||
    for (PatchSetApproval psa : normalized.unchanged()) {
 | 
					    for (PatchSetApproval psa : normalized.unchanged()) {
 | 
				
			||||||
      if (includeUnchanged || psa.isSubmit()) {
 | 
					      if (includeUnchanged || psa.isLegacySubmit()) {
 | 
				
			||||||
        logDebug("Adding submit label " + psa);
 | 
					        logDebug("Adding submit label " + psa);
 | 
				
			||||||
        update.putApprovalFor(
 | 
					        update.putApprovalFor(
 | 
				
			||||||
            psa.getAccountId(), psa.getLabel(), psa.getValue());
 | 
					            psa.getAccountId(), psa.getLabel(), psa.getValue());
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -359,7 +359,7 @@ public class ChangeField {
 | 
				
			|||||||
          Set<String> allApprovals = Sets.newHashSet();
 | 
					          Set<String> allApprovals = Sets.newHashSet();
 | 
				
			||||||
          Set<String> distinctApprovals = Sets.newHashSet();
 | 
					          Set<String> distinctApprovals = Sets.newHashSet();
 | 
				
			||||||
          for (PatchSetApproval a : input.currentApprovals()) {
 | 
					          for (PatchSetApproval a : input.currentApprovals()) {
 | 
				
			||||||
            if (a.getValue() != 0 && !a.isSubmit()) {
 | 
					            if (a.getValue() != 0 && !a.isLegacySubmit()) {
 | 
				
			||||||
              allApprovals.add(formatLabel(a.getLabel(), a.getValue(),
 | 
					              allApprovals.add(formatLabel(a.getLabel(), a.getValue(),
 | 
				
			||||||
                  a.getAccountId()));
 | 
					                  a.getAccountId()));
 | 
				
			||||||
              distinctApprovals.add(formatLabel(a.getLabel(), a.getValue()));
 | 
					              distinctApprovals.add(formatLabel(a.getLabel(), a.getValue()));
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -857,7 +857,7 @@ public class ChangeData {
 | 
				
			|||||||
  public Optional<PatchSetApproval> getSubmitApproval()
 | 
					  public Optional<PatchSetApproval> getSubmitApproval()
 | 
				
			||||||
    throws OrmException {
 | 
					    throws OrmException {
 | 
				
			||||||
    for (PatchSetApproval psa : currentApprovals()) {
 | 
					    for (PatchSetApproval psa : currentApprovals()) {
 | 
				
			||||||
      if (psa.isSubmit()) {
 | 
					      if (psa.isLegacySubmit()) {
 | 
				
			||||||
        return Optional.fromNullable(psa);
 | 
					        return Optional.fromNullable(psa);
 | 
				
			||||||
      }
 | 
					      }
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
 
 | 
				
			|||||||
 Submodule plugins/reviewnotes updated: 47c74e2da9...0ea78c9ca7
									
								
							
		Reference in New Issue
	
	Block a user