Merge "Submit Requirements - Update X to red"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 84e68ed..acf65a5 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -4343,7 +4343,7 @@
before it is focefully cancelled.
+
The receive timeout cannot be overriden by setting a higher
-link:user-upload#deadline[deadline] on the git push request.
+link:user-upload.html#deadline[deadline] on the git push request.
+
Default is 4 minutes. If no unit is specified, milliseconds
is assumed.
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index e940b1e..eabee65 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -30,6 +30,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@@ -117,6 +118,16 @@
public boolean verifyCommits = true;
/** Whether to compute and output the diff of the commit history for the backfilled refs. */
public boolean outputDiff = true;
+
+ /** Max number of refs to update in a single {@link BatchRefUpdate}. */
+ public int maxRefsInBatch = 10000;
+ /**
+ * Max number of refs to fix by a single {@link RefsUpdate#backfillProject} run. Since second
+ * run on the same set of refs is a no-op, running with this option in a loop will eventually
+ * fix all refs. Number of executed {@link BatchRefUpdate} depends on {@link #maxRefsInBatch}
+ * option.
+ */
+ public int maxRefsToUpdate = 50000;
}
/** Result of the backfill run for a project. */
@@ -239,15 +250,24 @@
*/
public BackfillResult backfillProject(
Project.NameKey project, Repository repo, RunOptions options) {
+
+ checkState(
+ options.maxRefsInBatch > 0 && options.maxRefsToUpdate > 0,
+ "Expected maxRefsInBatch>0 && <= maxRefsToUpdate>0");
+ checkState(
+ options.maxRefsInBatch <= options.maxRefsToUpdate,
+ "Expected maxRefsInBatch(%s) <= maxRefsToUpdate(%s)",
+ options.maxRefsInBatch,
+ options.maxRefsToUpdate);
BackfillResult result = new BackfillResult();
result.ok = true;
- try (RevWalk revWalk = new RevWalk(repo);
- ObjectInserter ins = newPackInserter(repo)) {
- BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
- bru.setForceRefLog(true);
- bru.setRefLogMessage(CommitRewriter.class.getName(), false);
- bru.setAllowNonFastForwards(true);
+ int refsInUpdate = 0;
+ RefsUpdate refsUpdate = null;
+ try {
for (Ref ref : repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_CHANGES)) {
+ if (result.fixedRefDiff.size() >= options.maxRefsToUpdate) {
+ return result;
+ }
Change.Id changeId = Change.Id.fromRef(ref.getName());
if (changeId == null || !ref.getName().equals(RefNames.changeMetaRef(changeId))) {
continue;
@@ -262,14 +282,26 @@
logger.atWarning().withCause(e).log("Failed to run verification on ref %s", ref);
}
}
+ if (refsUpdate == null) {
+ refsUpdate = RefsUpdate.create(repo);
+ }
ChangeFixProgress changeFixProgress =
- backfillChange(revWalk, ins, ref, accountsInChange, options);
+ backfillChange(refsUpdate, ref, accountsInChange, options);
if (changeFixProgress.anyFixesApplied) {
- bru.addCommand(
- new ReceiveCommand(ref.getObjectId(), changeFixProgress.newTipId, ref.getName()));
+ refsInUpdate++;
+ refsUpdate
+ .batchRefUpdate()
+ .addCommand(
+ new ReceiveCommand(
+ ref.getObjectId(), changeFixProgress.newTipId, ref.getName()));
result.fixedRefDiff.put(ref.getName(), changeFixProgress.commitDiffs);
}
-
+ if (refsInUpdate >= options.maxRefsInBatch
+ || result.fixedRefDiff.size() >= options.maxRefsToUpdate) {
+ processUpdate(options, refsUpdate);
+ refsUpdate = null;
+ refsInUpdate = 0;
+ }
if (!changeFixProgress.isValidAfterFix) {
result.refsStillInvalidAfterFix.add(ref.getName());
}
@@ -278,21 +310,34 @@
result.refsFailedToFix.add(ref.getName());
}
}
-
- if (!bru.getCommands().isEmpty()) {
- if (!options.dryRun) {
- ins.flush();
- RefUpdateUtil.executeChecked(bru, revWalk);
- }
- }
+ processUpdate(options, refsUpdate);
} catch (IOException e) {
- logger.atWarning().withCause(e).log("Failed to fix project %s", project.get());
+ logger.atWarning().log("Failed to fix project %s. Reason: %s", project.get(), e.getMessage());
result.ok = false;
+ } finally {
+ if (refsUpdate != null) {
+ refsUpdate.close();
+ }
}
return result;
}
+ /** Executes a single {@link RefsUpdate#batchRefUpdate}. */
+ private void processUpdate(RunOptions options, @Nullable RefsUpdate refsUpdate)
+ throws IOException {
+ if (refsUpdate == null) {
+ return;
+ }
+ if (!refsUpdate.batchRefUpdate().getCommands().isEmpty()) {
+ if (!options.dryRun) {
+ refsUpdate.inserter().flush();
+ RefUpdateUtil.executeChecked(refsUpdate.batchRefUpdate(), refsUpdate.revWalk());
+ }
+ }
+ refsUpdate.close();
+ }
+
/**
* Retrieves accounts, that are associated with a change (e.g. reviewers, commenters, etc.). These
* accounts are used to verify that commits do not contain user data. See {@link #verifyCommit}
@@ -376,8 +421,7 @@
* ChangeFixProgress#newTipId}.
*/
public ChangeFixProgress backfillChange(
- RevWalk revWalk,
- ObjectInserter inserter,
+ RefsUpdate refsUpdate,
Ref ref,
ImmutableSet<AccountState> accountsInChange,
RunOptions options)
@@ -385,17 +429,17 @@
ObjectId oldTip = ref.getObjectId();
// Walk from the first commit of the branch.
- revWalk.reset();
- revWalk.markStart(revWalk.parseCommit(oldTip));
- revWalk.sort(RevSort.TOPO);
+ refsUpdate.revWalk().reset();
+ refsUpdate.revWalk().markStart(refsUpdate.revWalk().parseCommit(oldTip));
+ refsUpdate.revWalk().sort(RevSort.TOPO);
- revWalk.sort(RevSort.REVERSE);
+ refsUpdate.revWalk().sort(RevSort.REVERSE);
RevCommit originalCommit;
boolean rewriteStarted = false;
ChangeFixProgress changeFixProgress = new ChangeFixProgress(ref.getName());
- while ((originalCommit = revWalk.next()) != null) {
+ while ((originalCommit = refsUpdate.revWalk().next()) != null) {
changeFixProgress.updateAuthorId =
parseIdent(changeFixProgress, originalCommit.getAuthorIdent());
@@ -453,7 +497,8 @@
cb.setEncoding(originalCommit.getEncoding());
byte[] newCommitContent = cb.build();
checkCommitModification(originalCommit, newCommitContent);
- changeFixProgress.newTipId = inserter.insert(Constants.OBJ_COMMIT, newCommitContent);
+ changeFixProgress.newTipId =
+ refsUpdate.inserter().insert(Constants.OBJ_COMMIT, newCommitContent);
// Only compute diff if the content of the commit was actually changed.
if (options.outputDiff && needsFix) {
String diff = computeDiff(originalCommit.getRawBuffer(), newCommitContent);
@@ -1283,4 +1328,33 @@
abstract Optional<String> email();
}
+
+ /**
+ * Objects, needed to fix Refs in a single {@link BatchRefUpdate}. Number of changes in a batch
+ * are limited by {@link RunOptions#maxRefsInBatch}.
+ */
+ @AutoValue
+ abstract static class RefsUpdate implements AutoCloseable {
+ static RefsUpdate create(Repository repo) {
+ RevWalk revWalk = new RevWalk(repo);
+ ObjectInserter inserter = newPackInserter(repo);
+ BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
+ bru.setForceRefLog(true);
+ bru.setRefLogMessage(CommitRewriter.class.getName(), false);
+ bru.setAllowNonFastForwards(true);
+ return new AutoValue_CommitRewriter_RefsUpdate(bru, revWalk, inserter);
+ }
+
+ @Override
+ public void close() {
+ inserter().close();
+ revWalk().close();
+ }
+
+ abstract BatchRefUpdate batchRefUpdate();
+
+ abstract RevWalk revWalk();
+
+ abstract ObjectInserter inserter();
+ }
}
diff --git a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
index 056c7dc..98721fd 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
@@ -24,6 +24,7 @@
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.AttentionSetUpdate.Operation;
@@ -33,6 +34,7 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.git.RefUpdateUtil;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
@@ -48,7 +50,9 @@
import java.sql.Timestamp;
import java.util.Arrays;
import java.util.List;
+import java.util.Map;
import java.util.stream.IntStream;
+import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
@@ -56,6 +60,8 @@
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevSort;
import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -70,6 +76,21 @@
@Before
public void setUp() throws Exception {}
+ @After
+ public void cleanUp() throws Exception {
+ BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
+ bru.setAllowNonFastForwards(true);
+ for (Ref ref : repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_CHANGES)) {
+ Change.Id changeId = Change.Id.fromRef(ref.getName());
+ if (changeId == null || !ref.getName().equals(RefNames.changeMetaRef(changeId))) {
+ continue;
+ }
+ bru.addCommand(new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), ref.getName()));
+ }
+
+ RefUpdateUtil.executeChecked(bru, repo);
+ }
+
@Test
public void validHistoryNoOp() throws Exception {
String tag = "jenkins";
@@ -157,6 +178,138 @@
}
@Test
+ public void numRefs_greater_maxRefsToUpdate_allFixed() throws Exception {
+ int numberOfChanges = 12;
+ ImmutableMap.Builder<String, Ref> refsToOldMetaBuilder = new ImmutableMap.Builder<>();
+ for (int i = 0; i < numberOfChanges; i++) {
+ Change c = newChange();
+ ChangeUpdate update = newUpdate(c, changeOwner);
+ update.setChangeMessage("Change has been successfully merged by " + changeOwner.getName());
+ update.commit();
+ ChangeUpdate updateWithSubject = newUpdate(c, changeOwner);
+ updateWithSubject.setSubjectForCommit("Update with subject");
+ updateWithSubject.commit();
+ String refName = RefNames.changeMetaRef(c.getId());
+ Ref metaRefBeforeRewrite = repo.exactRef(refName);
+ refsToOldMetaBuilder.put(refName, metaRefBeforeRewrite);
+ }
+ ImmutableMap<String, Ref> refsToOldMeta = refsToOldMetaBuilder.build();
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ options.outputDiff = false;
+ options.verifyCommits = false;
+ options.maxRefsInBatch = 10;
+ options.maxRefsToUpdate = 12;
+ BackfillResult backfillResult = rewriter.backfillProject(project, repo, options);
+ assertThat(backfillResult.fixedRefDiff.keySet()).isEqualTo(refsToOldMeta.keySet());
+ for (Map.Entry<String, Ref> refEntry : refsToOldMeta.entrySet()) {
+ Ref metaRefAfterRewrite = repo.exactRef(refEntry.getKey());
+ assertThat(refEntry.getValue()).isNotEqualTo(metaRefAfterRewrite);
+ }
+ }
+
+ @Test
+ public void maxRefsToUpdate_coversAllInvalid_inMultipleBatches() throws Exception {
+ testMaxRefsToUpdate(
+ /*numberOfInvalidChanges=*/ 11,
+ /*numberOfValidChanges=*/ 9,
+ /*maxRefsToUpdate=*/ 12,
+ /*maxRefsInBatch=*/ 2);
+ }
+
+ @Test
+ public void maxRefsToUpdate_coversAllInvalid_inSingleBatch() throws Exception {
+ testMaxRefsToUpdate(
+ /*numberOfInvalidChanges=*/ 11,
+ /*numberOfValidChanges=*/ 9,
+ /*maxRefsToUpdate=*/ 12,
+ /*maxRefsInBatch=*/ 12);
+ }
+
+ @Test
+ public void moreInvalidRefs_thenMaxRefsToUpdate_inMultipleBatches() throws Exception {
+ testMaxRefsToUpdate(
+ /*numberOfInvalidChanges=*/ 11,
+ /*numberOfValidChanges=*/ 9,
+ /*maxRefsToUpdate=*/ 10,
+ /*maxRefsInBatch=*/ 2);
+ }
+
+ @Test
+ public void moreInvalidRefs_thenMaxRefsToUpdate_inSingleBatch() throws Exception {
+ testMaxRefsToUpdate(
+ /*numberOfInvalidChanges=*/ 11,
+ /*numberOfValidChanges=*/ 9,
+ /*maxRefsToUpdate=*/ 10,
+ /*maxRefsInBatch=*/ 10);
+ }
+
+ private void testMaxRefsToUpdate(
+ int numberOfInvalidChanges, int numberOfValidChanges, int maxRefsToUpdate, int maxRefsInBatch)
+ throws Exception {
+ ImmutableMap.Builder<String, ObjectId> expectedFixedRefsToOldMetaBuilder =
+ new ImmutableMap.Builder<>();
+ ImmutableMap.Builder<String, ObjectId> expectedSkippedRefsToOldMetaBuilder =
+ new ImmutableMap.Builder<>();
+ for (int i = 0; i < numberOfValidChanges; i++) {
+ Change c = newChange();
+ ChangeUpdate updateWithSubject = newUpdate(c, changeOwner);
+ updateWithSubject.setSubjectForCommit("Update with subject");
+ updateWithSubject.commit();
+ String refName = RefNames.changeMetaRef(c.getId());
+ Ref metaRefBeforeRewrite = repo.exactRef(refName);
+ expectedSkippedRefsToOldMetaBuilder.put(refName, metaRefBeforeRewrite.getObjectId());
+ }
+ for (int i = 0; i < numberOfInvalidChanges; i++) {
+ Change c = newChange();
+ ChangeUpdate update = newUpdate(c, changeOwner);
+ update.setChangeMessage("Change has been successfully merged by " + changeOwner.getName());
+ update.commit();
+ ChangeUpdate updateWithSubject = newUpdate(c, changeOwner);
+ updateWithSubject.setSubjectForCommit("Update with subject");
+ updateWithSubject.commit();
+ String refName = RefNames.changeMetaRef(c.getId());
+ Ref metaRefBeforeRewrite = repo.exactRef(refName);
+ if (i < maxRefsToUpdate) {
+ expectedFixedRefsToOldMetaBuilder.put(refName, metaRefBeforeRewrite.getObjectId());
+ } else {
+ expectedSkippedRefsToOldMetaBuilder.put(refName, metaRefBeforeRewrite.getObjectId());
+ }
+ }
+ ImmutableMap<String, ObjectId> expectedFixedRefsToOldMeta =
+ expectedFixedRefsToOldMetaBuilder.build();
+ ImmutableMap<String, ObjectId> expectedSkippedRefsToOldMeta =
+ expectedSkippedRefsToOldMetaBuilder.build();
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ options.outputDiff = false;
+ options.verifyCommits = false;
+ options.maxRefsInBatch = maxRefsInBatch;
+ options.maxRefsToUpdate = maxRefsToUpdate;
+ BackfillResult backfillResult = rewriter.backfillProject(project, repo, options);
+ assertThat(backfillResult.fixedRefDiff.keySet()).isEqualTo(expectedFixedRefsToOldMeta.keySet());
+ for (Map.Entry<String, ObjectId> refEntry : expectedFixedRefsToOldMeta.entrySet()) {
+ Ref metaRefAfterRewrite = repo.exactRef(refEntry.getKey());
+ assertThat(refEntry.getValue()).isNotEqualTo(metaRefAfterRewrite.getObjectId());
+ }
+ for (Map.Entry<String, ObjectId> refEntry : expectedSkippedRefsToOldMeta.entrySet()) {
+ Ref metaRefAfterRewrite = repo.exactRef(refEntry.getKey());
+ assertThat(refEntry.getValue()).isEqualTo(metaRefAfterRewrite.getObjectId());
+ }
+ RunOptions secondRunOptions = new RunOptions();
+ secondRunOptions.dryRun = false;
+ secondRunOptions.outputDiff = false;
+ secondRunOptions.verifyCommits = false;
+ secondRunOptions.maxRefsInBatch = maxRefsInBatch;
+ secondRunOptions.maxRefsToUpdate = numberOfInvalidChanges + numberOfValidChanges;
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ int expectedSecondRunResult =
+ numberOfInvalidChanges > maxRefsToUpdate ? numberOfInvalidChanges - maxRefsToUpdate : 0;
+ assertThat(secondRunResult.fixedRefDiff.keySet().size()).isEqualTo(expectedSecondRunResult);
+ }
+
+ @Test
public void fixAuthorIdent() throws Exception {
Change c = newChange();
Timestamp when = TimeUtil.nowTs();
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index f8bdbef..c19be58e 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -301,15 +301,6 @@
@property({type: Boolean})
disableEdit = false;
- @property({type: Boolean})
- disableDiffPrefs = false;
-
- @property({
- type: Boolean,
- computed: '_computeDiffPrefsDisabled(disableDiffPrefs, _loggedIn)',
- })
- _diffPrefsDisabled?: boolean;
-
@property({type: Array})
_commentThreads?: CommentThread[];
@@ -1714,11 +1705,7 @@
if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
-
- if (this._diffPrefsDisabled) {
- return;
- }
-
+ if (!this._loggedIn) return;
e.preventDefault();
this.$.fileList.openDiffPrefs();
}
@@ -2602,10 +2589,6 @@
return currentRevision && revisions && revisions[currentRevision];
}
- _computeDiffPrefsDisabled(disableDiffPrefs: boolean, loggedIn: boolean) {
- return disableDiffPrefs || !loggedIn;
- }
-
/**
* Wrapper for using in the element template and computed properties
*/
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
index d57aca8..0b77bc7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
@@ -537,7 +537,7 @@
patch-num="{{_patchRange.patchNum}}"
base-patch-num="{{_patchRange.basePatchNum}}"
files-expanded="[[_filesExpanded]]"
- diff-prefs-disabled="[[_diffPrefsDisabled]]"
+ diff-prefs-disabled="[[!_loggedIn]]"
on-open-diff-prefs="_handleOpenDiffPrefs"
on-open-download-dialog="_handleOpenDownloadDialog"
on-expand-diffs="_expandAllDiffs"
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index a82fceb..e508c63 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -806,16 +806,11 @@
'open'
);
element._loggedIn = false;
- element.disableDiffPrefs = true;
pressAndReleaseKeyOn(element, 188, null, ',');
assert.isFalse(stub.called);
element._loggedIn = true;
pressAndReleaseKeyOn(element, 188, null, ',');
- assert.isFalse(stub.called);
-
- element.disableDiffPrefs = false;
- pressAndReleaseKeyOn(element, 188, null, ',');
assert.isTrue(stub.called);
});
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
index 8aef3c0..50bb665 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
@@ -126,9 +126,6 @@
@property({type: Object})
diffPrefs?: DiffPreferencesInfo;
- @property({type: Boolean})
- diffPrefsDisabled?: boolean;
-
@property({type: String, notify: true})
diffViewMode?: DiffViewMode;
@@ -175,11 +172,8 @@
return classes.join(' ');
}
- _computePrefsButtonHidden(
- prefs: DiffPreferencesInfo,
- diffPrefsDisabled: boolean
- ) {
- return diffPrefsDisabled || !prefs;
+ _computePrefsButtonHidden(prefs: DiffPreferencesInfo, loggedIn: boolean) {
+ return !loggedIn || !prefs;
}
_fileListActionsVisible(
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts
index 5972393..73d0819 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts
@@ -170,12 +170,12 @@
<gr-diff-mode-selector
id="modeSelect"
mode="{{diffViewMode}}"
- save-on-change="[[!diffPrefsDisabled]]"
+ save-on-change="[[loggedIn]]"
></gr-diff-mode-selector>
<span
id="diffPrefsContainer"
class="hideOnEdit"
- hidden$="[[_computePrefsButtonHidden(diffPrefs, diffPrefsDisabled)]]"
+ hidden$="[[_computePrefsButtonHidden(diffPrefs, loggedIn)]]"
hidden=""
>
<gr-tooltip-content has-tooltip title="Diff preferences">
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js
index c90cfcc..479a9a1 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js
@@ -38,21 +38,11 @@
await flush();
});
- test('Diff preferences hidden when no prefs or diffPrefsDisabled', () => {
- element.diffPrefsDisabled = true;
- flush();
+ test('Diff preferences hidden when no prefs', () => {
assert.isTrue(element.$.diffPrefsContainer.hidden);
- element.diffPrefsDisabled = false;
- flush();
- assert.isTrue(element.$.diffPrefsContainer.hidden);
-
- element.diffPrefsDisabled = true;
element.diffPrefs = {font_size: '12'};
- flush();
- assert.isTrue(element.$.diffPrefsContainer.hidden);
-
- element.diffPrefsDisabled = false;
+ element.loggedIn = true;
flush();
assert.isFalse(element.$.diffPrefsContainer.hidden);
});
@@ -168,7 +158,7 @@
suite('editMode behavior', () => {
setup(() => {
- element.diffPrefsDisabled = false;
+ element.loggedIn = true;
element.diffPrefs = {};
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 295e41f..084f9f6 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -174,15 +174,6 @@
@property({type: Object, notify: true, observer: '_changeViewStateChanged'})
changeViewState: Partial<ChangeViewState> = {};
- @property({type: Boolean})
- disableDiffPrefs = false;
-
- @property({
- type: Boolean,
- computed: '_computeDiffPrefsDisabled(disableDiffPrefs, _loggedIn)',
- })
- _diffPrefsDisabled?: boolean;
-
@property({type: Object})
_patchRange?: PatchRange;
@@ -805,7 +796,7 @@
_handleCommaKey(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
if (this.shortcuts.modifierPressed(e)) return;
- if (this._diffPrefsDisabled) return;
+ if (!this._loggedIn) return;
e.preventDefault();
this.$.diffPreferencesDialog.open();
@@ -1409,11 +1400,8 @@
return dropdownContent;
}
- _computePrefsButtonHidden(
- prefs?: DiffPreferencesInfo,
- prefsDisabled?: boolean
- ) {
- return prefsDisabled || !prefs;
+ _computePrefsButtonHidden(prefs?: DiffPreferencesInfo, loggedIn?: boolean) {
+ return !loggedIn || !prefs;
}
_handleFileChange(e: CustomEvent) {
@@ -1842,10 +1830,6 @@
this.$.diffHost.toggleAllContext();
}
- _computeDiffPrefsDisabled(disableDiffPrefs?: boolean, loggedIn?: boolean) {
- return disableDiffPrefs || !loggedIn;
- }
-
_handleNextUnreviewedFile(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
this._setReviewed(true);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
index b25be5a8..308c353 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
@@ -338,14 +338,14 @@
<span>Diff view:</span>
<gr-diff-mode-selector
id="modeSelect"
- save-on-change="[[!_diffPrefsDisabled]]"
+ save-on-change="[[_loggedIn]]"
mode="{{changeViewState.diffMode}}"
show-tooltip-below=""
></gr-diff-mode-selector>
</div>
<span
id="diffPrefsContainer"
- hidden$="[[_computePrefsButtonHidden(_prefs, _diffPrefsDisabled)]]"
+ hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]"
hidden=""
>
<span class="preferences desktop">
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index 0c7abc8..cc35c3c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -489,10 +489,6 @@
MockInteractions.pressAndReleaseKeyOn(element, 188, null, ',');
assert(showPrefsStub.calledOnce);
- element.disableDiffPrefs = true;
- MockInteractions.pressAndReleaseKeyOn(element, 188, null, ',');
- assert(showPrefsStub.calledOnce);
-
let scrollStub = sinon.stub(element.cursor, 'moveToNextChunk');
MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'n');
assert(scrollStub.calledOnce);
@@ -988,8 +984,7 @@
});
suite('diff prefs hidden', () => {
- test('when no prefs or logged out', () => {
- element.disableDiffPrefs = false;
+ test('whenlogged out', () => {
element._loggedIn = false;
flush();
assert.isTrue(element.$.diffPrefsContainer.hidden);
@@ -1004,21 +999,9 @@
assert.isTrue(element.$.diffPrefsContainer.hidden);
element._loggedIn = true;
- flush();
- assert.isFalse(element.$.diffPrefsContainer.hidden);
- });
-
- test('when disableDiffPrefs is set', () => {
- element._loggedIn = true;
element._prefs = {font_size: '12'};
- element.disableDiffPrefs = false;
flush();
-
assert.isFalse(element.$.diffPrefsContainer.hidden);
- element.disableDiffPrefs = true;
- flush();
-
- assert.isTrue(element.$.diffPrefsContainer.hidden);
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
index 947ef3e..936daed 100644
--- a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
+++ b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.ts
@@ -44,6 +44,7 @@
getVotingRangeOrDefault,
hasNeutralStatus,
hasVoted,
+ valueString,
} from '../../../utils/label-util';
import {appContext} from '../../../services/app-context';
import {ParsedChangeInfo} from '../../../types/types';
@@ -255,7 +256,7 @@
></gr-vote-chip
></gr-account-chip>
${noVoteYet
- ? html`<span class="no-votes">No votes</span>`
+ ? this.renderVoteAbility(reviewer)
: html`${this.renderRemoveVote(reviewer)}`}
</div>`;
}
@@ -283,6 +284,19 @@
</tr>`;
}
+ private renderVoteAbility(reviewer: AccountInfo) {
+ if (this.labelInfo && isDetailedLabelInfo(this.labelInfo)) {
+ const approvalInfo = getApprovalInfo(this.labelInfo, reviewer);
+ if (approvalInfo?.permitted_voting_range) {
+ const {min, max} = approvalInfo?.permitted_voting_range;
+ return html`<span class="no-votes"
+ >Can vote ${valueString(min)}/${valueString(max)}</span
+ >`;
+ }
+ }
+ return html`<span class="no-votes">No votes</span>`;
+ }
+
private renderRemoveVote(reviewer: AccountInfo) {
return html`<gr-tooltip-content has-tooltip title="Remove vote">
<gr-button
