Merge "PostReview: Fix email for removing an email without Gerrit account as CC"
diff --git a/Documentation/config-project-config.txt b/Documentation/config-project-config.txt
index dfc38c3..7a0e305 100644
--- a/Documentation/config-project-config.txt
+++ b/Documentation/config-project-config.txt
@@ -319,11 +319,64 @@
[[content_merge]]submit.mergeContent::
+
-Defines whether Gerrit will try to
-do a content merge when a path conflict occurs. Valid values are
-'true', 'false', or 'INHERIT'. Default is 'INHERIT'. This option can
-be modified by any project owner through the project console, `Browse`
-> `Repositories` > my/project > `Allow content merges`.
+Defines whether Gerrit will try to do a content merge when a path conflict
+occurs while submitting a change.
++
+A path conflict occurs when the same file has been changed on both sides of a
+merge, e.g. when the same file has been touched in a change and concurrently in
+the target branch.
++
+Doing a content merge means that Gerrit attempts to merge the conflicting file
+contents from both sides of the merge. This is successful if the touched lines
+(plus some surrounding context lines) do not overlap (i.e. both sides touch
+distinct lines).
++
+NOTE: The content merge setting is not relevant when
+link:#fast_forward_only[fast forward only] is configured as the
+link:#submit.action[submit action] because in this case Gerrit will never
+perform a merge, rebase or cherry-pick on submit.
++
+If content merges are disabled, the submit button in the Gerrit web UI is
+disabled, if any path conflict would occur on submitting the change. Users then
+need to rebase the change manually to resolve the path conflict and then get
+the change re-approved so that they can submit it.
++
+NOTE: If only distinct lines have been touched on both sides, rebasing the
+change from the Gerrit UI is sufficient to resolve the path conflict, since the
+rebase action always does the rebase with content merge enabled.
++
+The advantage of enabling content merges on submit is that it makes it less
+likely that change submissions are rejected due to conflicts. Each change
+submission that goes through with content merge, but would be rejected
+otherwise, saves the user from needing to do extra work to get the change
+submitted (rebase the change, get it re-approved and then submit it again).
++
+On the other hand, disabling content merges decreases the chance of breaking
+branches by submitting content merges of incompatible modifications in the same
+file, e.g. a function is removed on one side and a new usage of that function
+is added on the other side. Note, that the chance of breaking a branch by
+incompatible modifications is only reduced, but not eliminated, e.g. even with
+content merges disabled it's possible that a function is removed in one file
+and a new usage of that function is added in another file.
++
+The huge drawback of disabling content merge is that users need to do extra
+work when a change isn't submittable due to a path conflict which could be
+avoided if content merge was enabled (see above). In addition to this, it also
+confuses and frustrates users if a change submission is rejected by Gerrit due
+to a path conflict, but then when they rebase the change manually they do not
+see any conflict (because manual rebases are always done with content merge
+enabled).
++
+In general, the stability gain of disabling content merges is not worth the
+overhead and confusion that this adds for users, which is why disabling content
+merges is highly discouraged.
++
+Valid values are `true`, `false`, or `INHERIT`.
++
+The default is `INHERIT`.
++
+NOTE: Project owners can also set this option in the Gerrit UI:
+`Browse` > `Repositories` > my/repository > `Allow content merges`.
[[submit.action]]submit.action::
+
@@ -342,55 +395,136 @@
[[merge_if_necessary]]
* 'merge if necessary':
+
-If the change being submitted is a strict superset of the destination branch,
-then the branch is fast-forwarded to the change. If not, then a merge commit is
-automatically created. This is identical to the classical `git merge` behavior,
- or `git merge --ff`.
+With this action, when a change is being submitted, Gerrit fast-forwards the
+target branch if possible, and otherwise creates a merge commit automatically.
++
+A fast-forward is possible if the commit that represents the current patch set
+of the change has the current head of the target branch in its parent lineage.
++
+If a fast-forward is not possible, Gerrit automatically creates a merge commit
+that merges the current patch set of the change into the target branch and then
+the target branch is fast-forwarded to the merge commit.
++
+The behavior of this submit action is identical with the classical `git merge`
+behavior, or
+link:https://git-scm.com/docs/git-merge#Documentation/git-merge.txt---ff[git
+merge --ff].
++
+With this submit action the commits that have been reviewed and approved are
+retained in the git history of the target branch. This means, by looking at the
+history of the target branch, you can see for all commits when they were
+originally committed and on which parent commit they were originally based.
[[always_merge]]
[[merge_always]]
* 'merge always':
+
-Always produce a merge commit, even if the change is a strict superset of the
-destination branch. This is identical to the behavior of `git merge --no-ff`,
-and may be useful if the project needs to follow submits with
-`git log --first-parent`.
+With this action, when a change is being submitted, Gerrit always creates a
+merge commit, even if a fast-forward is possible.
++
+This is identical to the behavior of
+link:https://git-scm.com/docs/git-merge#Documentation/git-merge.txt---no-ff[git merge --no-ff].
++
+With this submit action the commits that have been reviewed and approved are
+retained in the git history of the target branch. This means, by looking at the
+history of the target branch, you can see for all commits when they were
+originally committed and on which parent commit they were originally based. In
+addition, from the merge commits you can see when the changes were submitted
+and it's possible to follow submissions with `git log --first-parent`.
[[rebase_if_necessary]]
* 'rebase if necessary':
+
-If the change being submitted is a strict superset of the destination branch,
-then the branch is fast-forwarded to the change. If not, then the change is
-automatically rebased and then the branch is fast-forwarded to the change.
+With this action, when a change is being submitted, Gerrit fast-forwards the
+target branch if possible, and otherwise does a rebase automatically.
+
-When Gerrit tries to do a merge, by default the merge will only succeed if
-there is no path conflict. A path conflict occurs when the same file has also
-been changed on the other side of the merge.
+A fast-forward is possible if the commit that represents the current patch set
+of the change has the current head of the target branch in its parent lineage.
++
+If a fast-forward is not possible, Gerrit automatically rebases the current
+patch set of the change on top of the current head of the target branch and
+then the target branch is fast-forwarded to the rebased commit.
++
+With this submit action, when a rebase is performed, the original commits that
+have been reviewed and approved do not become part of the target branch's
+history. This means the information on when the original commits were committed
+and on which parent they were based is not retained in the branch history.
++
+Using this submit action results in a linear history of the target branch,
+unless merge commits are being submitted. For some people this is an advantage
+since they find the linear history easier to read.
++
+NOTE: Rebasing merge commits is not supported. If a change with a merge commit
+is submitted the link:#merge_if_necessary[merge if necessary] submit action is
+applied.
++
+When rebasing the patchset, Gerrit automatically appends onto the end of the
+commit message a short summary of the change's approvals, and a URL link back
+to the change in the web UI (see link:#submit-footers[below]). If a fast-forward
+is done no footers are added.
[[rebase_always]]
* 'rebase always':
+
-Basically, the same as `rebase if necessary`, but it creates a new patchset
-even if fast forward is possible and like `cherry pick` it ensures footers such
-as `Change-Id`, `Reviewed-On`, and others are present in resulting commit that
-is merged.
+With this action, when a change is being submitted, Gerrit always does a
+rebase, even if a fast-forward is possible.
+
-Thus, `rebase always` can be considered similar to `cherry pick`, but with the
-important distinction that `rebase always` does not ignore dependencies.
+With this submit action, the original commits that have been reviewed and
+approved do not become part of the target branch's history. This means the
+information on when the original commits were committed and on which parent
+they were based is not retained in the branch history.
++
+Using this submit action results in a linear history of the target branch,
+unless merge commits are being submitted. For some people this is an advantage
+since they find the linear history easier to read.
++
+NOTE: Rebasing merge commits is not supported. If a change with a merge commit
+is submitted the link:#merge_if_necessary[merge if necessary] submit action is
+applied.
++
+When rebasing the patchset, Gerrit automatically appends onto the end of the
+commit message a short summary of the change's approvals, and a URL link back
+to the change in the web UI (see link:#submit-footers[below]).
++
+The footers that are added are exactly the same footers that are also added by
+the link:cherry_pick[cherry pick] action. Thus, the `rebase always` action can
+be considered similar to the `cherry pick` action, but with the important
+distinction that `rebase always` does not ignore dependencies, which is why
+using the `rebase always` action should be preferred over the `cherry pick`
+submit action.
[[fast_forward_only]]
-* 'fast forward only':
+* 'fast forward only' (usage generally not recommended):
+
-With this action Gerrit does not create merge commits on submitting a change.
-Merge commits may still be submitted, but they must be created on the client
-prior to uploading to Gerrit for review.
+With this action a change can only be submitted if at submit time the target
+branch can be fast-forwarded to the commit that represents the current patch
+set of the change. This means in order for a change to be submittable its
+current patch set must have the current head of the target branch in its parent
+lineage.
+
-To submit a change, the change must be a strict superset of the destination
-branch. That is, the change must already contain the tip of the destination
-branch at submit time.
+The advantage of using this action is that the target branch is always updated
+to the exact commit that has been reviewed and approved. In particular, if CI
+verification is configured, this means that the CI verified the exact commit to
+which the target branch is being fast-forwarded on submit (assuming no approval
+copying is configured for CI votes).
++
+The huge drawback of using this action is that whenever one change is submitted
+all other open changes for the same branch, that are not successors of the
+submitted change, become non-submittable, since the target branch can no longer
+be fast-forwarded to their current patch sets. Making these changes submittable
+again requires rebasing and re-approving/re-verifying them. For most projects
+this causes an unreasonable amount of overhead that doesn't justify the
+stability gain by verifying exact commits so that using this submit action is
+generally discouraged. Using this action should only be considered for projects
+that have a low frequency of changes and that have special requirements to
+never break any target branch.
++
+NOTE: With this submit action Gerrit does not create merge commits on
+submitting a change, but merge commits that are created on the client, prior to
+uploading to Gerrit for review, may still be submitted.
[[cherry_pick]]
-* 'cherry pick' (usage not recommended, user link:#rebase_always[rebase always]
+* 'cherry pick' (usage not recommended, use link:#rebase_always[rebase always]
instead):
+
With this submit action Gerrit always performs a cherry pick of the current
@@ -416,7 +550,7 @@
+
When cherry picking the patchset, Gerrit automatically appends onto the end of
the commit message a short summary of the change's approvals, and a URL link
-back to the change on the web.
+back to the change in the web UI (see link:#submit-footers[below]).
+
Using this submit action is not recommended because it ignores change
dependencies, instead link:#rebase_always[rebase always] should be used which
@@ -436,6 +570,49 @@
+
If `submit.action` is not set, the default is 'merge if necessary'.
+
+NOTE: The different submit actions are also described in the
+link:https://docs.google.com/presentation/d/1C73UgQdzZDw0gzpaEqIC6SPujZJhqamyqO1XOHjH-uk/edit#slide=id.g4d6c16487b_1_800[Gerrit - Concepts and Workflows]
+presentation, where their behavior is visualized by git commit graphs.
++
+NOTE: If Gerrit performs a merge, rebase or cherry-pick as part of the
+change submission (true for all submit actions, except for
+link:#fast_forward_only[fast forward only]), it is controlled by the
+link:#submit.mergeContent[mergeContent] setting whether a content merge is
+performed when there is a path conflict.
++
+NOTE: If Gerrit performs a merge, rebase or cherry-pick as part of the
+change submission (true for all submit actions, except for
+link:#fast_forward_only[fast forward only]), it can be that trying to submit
+a change would fail due to Git conflicts (if the same lines were modified
+concurrently, or if link:#submit.mergeContent[mergeContent] is disabled also if
+the same files were modified concurrently). In this case the submit button in
+the Gerrit web UI is disabled and a tooltip on the disabled submit button
+informs about the change being non-mergeable.
++
+[[submit-footers]]
+--
+NOTE: If Gerrit performs a rebase or cherry-pick as part of the change
+submission (true for link:#rebase_if_necessary[rebase if necessary],
+link:#rebase_always[rebase always] and link:#cherry_pick[cherry pick]) Gerrit
+inserts additional footers into the commit message of the newly created
+commit: +
+ +
+* `Change-Id: <change-id>` (only if this footer is not already present, see
+ link:user-changeid.html[Change-Id]) +
+* `Reviewed-on: <change-url>` (links to the change in Gerrit where this commit
+ was reviewed) +
+* `Reviewed-by: <reviewer>` (once for every reviewer with a positive
+ `Code-Review` vote) +
+* `Tested-by: <reviewer>` (once for every reviewer with a positive `Verified`
+ vote) +
+* `<label-name>: <reviewer>` (once for every reviewer with a positive vote on
+ any other label) +
+ +
+In addition, plugins that implement a
+link:dev-plugins.html#change-message-modifier[Change Message Modifier] may add
+additional custom footers.
+--
++
NOTE: For the value of `submit.action` in `project.config` use the exact
spelling as given above, e.g. 'merge if necessary' (without the single quotes,
but with the spaces).
diff --git a/Documentation/dev-roles.txt b/Documentation/dev-roles.txt
index cecaedc..d8f7e11 100644
--- a/Documentation/dev-roles.txt
+++ b/Documentation/dev-roles.txt
@@ -66,11 +66,12 @@
link:https://groups.google.com/d/forum/repo-discuss[repo-discuss,role=external,window=_blank]
mailing list):
-* become member of the `gerrit-verifiers` group, which allows to:
+* become member of the `gerrit-trusted-contributors` group, which allows to:
** vote on the `Verified` and `Code-Style` labels
** edit hashtags on all changes
** edit topics on all open changes
** abandon changes
+** revert changes
* approve posts to the
link:https://groups.google.com/d/forum/repo-discuss[repo-discuss,role=external,window=_blank]
mailing list
@@ -106,14 +107,14 @@
have. In addition they have signed a link:dev-cla.html[contributor
license agreement] which enables them to push changes.
-Regular contributors can ask to be added to the `gerrit-verifiers`
+Regular contributors can ask to be added to the `gerrit-trusted-contributors`
group, which allows to:
* add patch sets to changes of other users
* propose project config changes (push changes for the
`refs/meta/config` branch
-Being member of the `gerrit-verifiers` group includes further
+Being member of the `gerrit-trusted-contributors` group includes further
permissions (see link:#supporter[supporter] section above).
It's highly appreciated if contributors engage in code reviews,
diff --git a/Documentation/pg-plugin-endpoints.txt b/Documentation/pg-plugin-endpoints.txt
index f2a72f1..41f544d 100644
--- a/Documentation/pg-plugin-endpoints.txt
+++ b/Documentation/pg-plugin-endpoints.txt
@@ -141,6 +141,11 @@
=== header-title
This endpoint wraps the title-text in the application header.
+=== cherrypick-main
+This endpoint is located in the cherrypick dialog. It has two slots `top`
+and `bottom` and `changes` as a parameter with the list of changes (or
+just the one change) to be cherrypicked.
+
=== confirm-revert-change
This endpoint is inside the confirm revert dialog. By default it displays a
generic confirmation message regarding reverting the change. Plugins may add
diff --git a/Documentation/user-notify.txt b/Documentation/user-notify.txt
index 20ad07c..24c35f0 100644
--- a/Documentation/user-notify.txt
+++ b/Documentation/user-notify.txt
@@ -18,8 +18,8 @@
[[user]]
== User Level Settings
-Individual users can configure email subscriptions by editing
-watched projects through Settings > Watched Projects with the web UI.
+Individual users can configure email subscriptions by editing their
+notifications in the Web UI under `Settings` > `Notifications`.
Specific projects may be watched, or the special project
`All-Projects` can be watched to watch all projects that
diff --git a/java/com/google/gerrit/server/index/group/GroupField.java b/java/com/google/gerrit/server/index/group/GroupField.java
index 775bd5e..dd6aa3b 100644
--- a/java/com/google/gerrit/server/index/group/GroupField.java
+++ b/java/com/google/gerrit/server/index/group/GroupField.java
@@ -16,11 +16,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.index.FieldDef.exact;
-import static com.google.gerrit.index.FieldDef.fullText;
-import static com.google.gerrit.index.FieldDef.integer;
-import static com.google.gerrit.index.FieldDef.prefix;
import static com.google.gerrit.index.FieldDef.storedOnly;
-import static com.google.gerrit.index.FieldDef.timestamp;
import com.google.common.base.MoreObjects;
import com.google.gerrit.entities.Account;
@@ -28,6 +24,7 @@
import com.google.gerrit.entities.InternalGroup;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.IndexedField;
import com.google.gerrit.index.SchemaUtil;
import java.sql.Timestamp;
import org.eclipse.jgit.lib.ObjectId;
@@ -40,49 +37,88 @@
*/
public class GroupField {
/** Legacy group ID. */
- public static final FieldDef<InternalGroup, Integer> ID =
- integer("id").build(g -> g.getId().get());
+ public static final IndexedField<InternalGroup, Integer> ID_FIELD =
+ IndexedField.<InternalGroup>integerBuilder("Id").required().build(g -> g.getId().get());
+
+ public static final IndexedField<InternalGroup, Integer>.SearchSpec ID_FIELD_SPEC =
+ ID_FIELD.integer("id");
/** Group UUID. */
public static final FieldDef<InternalGroup, String> UUID =
exact("uuid").stored().build(g -> g.getGroupUUID().get());
/** Group owner UUID. */
- public static final FieldDef<InternalGroup, String> OWNER_UUID =
- exact("owner_uuid").build(g -> g.getOwnerGroupUUID().get());
+ public static final IndexedField<InternalGroup, String> OWNER_UUID_FIELD =
+ IndexedField.<InternalGroup>stringBuilder("OwnerUUID")
+ .required()
+ .build(g -> g.getOwnerGroupUUID().get());
+
+ public static final IndexedField<InternalGroup, String>.SearchSpec OWNER_UUID_SPEC =
+ OWNER_UUID_FIELD.exact("owner_uuid");
/** Timestamp indicating when this group was created. */
// TODO(issue-15518): Migrate type for timestamp index fields from Timestamp to Instant
- public static final FieldDef<InternalGroup, Timestamp> CREATED_ON =
- timestamp("created_on").build(internalGroup -> Timestamp.from(internalGroup.getCreatedOn()));
+ public static final IndexedField<InternalGroup, Timestamp> CREATED_ON_FIELD =
+ IndexedField.<InternalGroup>timestampBuilder("CreatedOn")
+ .required()
+ .build(internalGroup -> Timestamp.from(internalGroup.getCreatedOn()));
+
+ public static final IndexedField<InternalGroup, Timestamp>.SearchSpec CREATED_ON_SPEC =
+ CREATED_ON_FIELD.timestamp("created_on");
/** Group name. */
- public static final FieldDef<InternalGroup, String> NAME =
- exact("name").build(InternalGroup::getName);
+ public static final IndexedField<InternalGroup, String> NAME_FIELD =
+ IndexedField.<InternalGroup>stringBuilder("Name")
+ .required()
+ .size(200)
+ .build(InternalGroup::getName);
+
+ public static final IndexedField<InternalGroup, String>.SearchSpec NAME_SPEC =
+ NAME_FIELD.exact("name");
/** Prefix match on group name parts. */
- public static final FieldDef<InternalGroup, Iterable<String>> NAME_PART =
- prefix("name_part").buildRepeatable(g -> SchemaUtil.getNameParts(g.getName()));
+ public static final IndexedField<InternalGroup, Iterable<String>> NAME_PART_FIELD =
+ IndexedField.<InternalGroup>iterableStringBuilder("NamePart")
+ .required()
+ .size(200)
+ .build(g -> SchemaUtil.getNameParts(g.getName()));
+
+ public static final IndexedField<InternalGroup, Iterable<String>>.SearchSpec NAME_PART_SPEC =
+ NAME_PART_FIELD.prefix("name_part");
/** Group description. */
- public static final FieldDef<InternalGroup, String> DESCRIPTION =
- fullText("description").build(InternalGroup::getDescription);
+ public static final IndexedField<InternalGroup, String> DESCRIPTION_FIELD =
+ IndexedField.<InternalGroup>stringBuilder("Description").build(InternalGroup::getDescription);
+
+ public static final IndexedField<InternalGroup, String>.SearchSpec DESCRIPTION_SPEC =
+ DESCRIPTION_FIELD.fullText("description");
/** Whether the group is visible to all users. */
- public static final FieldDef<InternalGroup, String> IS_VISIBLE_TO_ALL =
- exact("is_visible_to_all").build(g -> g.isVisibleToAll() ? "1" : "0");
+ public static final IndexedField<InternalGroup, String> IS_VISIBLE_TO_ALL_FIELD =
+ IndexedField.<InternalGroup>stringBuilder("IsVisibleToAll")
+ .required()
+ .size(1)
+ .build(g -> g.isVisibleToAll() ? "1" : "0");
- public static final FieldDef<InternalGroup, Iterable<Integer>> MEMBER =
- integer("member")
- .buildRepeatable(
- g -> g.getMembers().stream().map(Account.Id::get).collect(toImmutableList()));
+ public static final IndexedField<InternalGroup, String>.SearchSpec IS_VISIBLE_TO_ALL_SPEC =
+ IS_VISIBLE_TO_ALL_FIELD.exact("is_visible_to_all");
- public static final FieldDef<InternalGroup, Iterable<String>> SUBGROUP =
- exact("subgroup")
- .buildRepeatable(
+ public static final IndexedField<InternalGroup, Iterable<Integer>> MEMBER_FIELD =
+ IndexedField.<InternalGroup>iterableIntegerBuilder("Member")
+ .build(g -> g.getMembers().stream().map(Account.Id::get).collect(toImmutableList()));
+
+ public static final IndexedField<InternalGroup, Iterable<Integer>>.SearchSpec MEMBER_SPEC =
+ MEMBER_FIELD.integer("member");
+
+ public static final IndexedField<InternalGroup, Iterable<String>> SUBGROUP_FIELD =
+ IndexedField.<InternalGroup>iterableStringBuilder("Subgroup")
+ .build(
g ->
g.getSubgroups().stream().map(AccountGroup.UUID::get).collect(toImmutableList()));
+ public static final IndexedField<InternalGroup, Iterable<String>>.SearchSpec SUBGROUP_SPEC =
+ SUBGROUP_FIELD.exact("subgroup");
+
/** ObjectId of HEAD:refs/groups/<UUID>. */
public static final FieldDef<InternalGroup, byte[]> REF_STATE =
storedOnly("ref_state")
diff --git a/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java b/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
index 5527345..2fba6ab 100644
--- a/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
@@ -16,7 +16,9 @@
import static com.google.gerrit.index.SchemaUtil.schema;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.InternalGroup;
+import com.google.gerrit.index.IndexedField;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.SchemaDefinitions;
@@ -31,17 +33,27 @@
static final Schema<InternalGroup> V5 =
schema(
/* version= */ 5,
- GroupField.CREATED_ON,
- GroupField.DESCRIPTION,
- GroupField.ID,
- GroupField.IS_VISIBLE_TO_ALL,
- GroupField.MEMBER,
- GroupField.NAME,
- GroupField.NAME_PART,
- GroupField.OWNER_UUID,
- GroupField.REF_STATE,
- GroupField.SUBGROUP,
- GroupField.UUID);
+ ImmutableList.of(GroupField.REF_STATE, GroupField.UUID),
+ ImmutableList.of(
+ GroupField.CREATED_ON_FIELD,
+ GroupField.DESCRIPTION_FIELD,
+ GroupField.ID_FIELD,
+ GroupField.IS_VISIBLE_TO_ALL_FIELD,
+ GroupField.MEMBER_FIELD,
+ GroupField.NAME_FIELD,
+ GroupField.NAME_PART_FIELD,
+ GroupField.OWNER_UUID_FIELD,
+ GroupField.SUBGROUP_FIELD),
+ ImmutableList.<IndexedField<InternalGroup, ?>.SearchSpec>of(
+ GroupField.CREATED_ON_SPEC,
+ GroupField.DESCRIPTION_SPEC,
+ GroupField.ID_FIELD_SPEC,
+ GroupField.IS_VISIBLE_TO_ALL_SPEC,
+ GroupField.MEMBER_SPEC,
+ GroupField.NAME_SPEC,
+ GroupField.NAME_PART_SPEC,
+ GroupField.OWNER_UUID_SPEC,
+ GroupField.SUBGROUP_SPEC));
// Bump Lucene version requires reindexing
@Deprecated static final Schema<InternalGroup> V6 = schema(V5);
diff --git a/java/com/google/gerrit/server/query/group/GroupPredicates.java b/java/com/google/gerrit/server/query/group/GroupPredicates.java
index 8a2dc8d..8d935d6 100644
--- a/java/com/google/gerrit/server/query/group/GroupPredicates.java
+++ b/java/com/google/gerrit/server/query/group/GroupPredicates.java
@@ -17,7 +17,7 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.InternalGroup;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.IndexPredicate;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.server.index.group.GroupField;
@@ -26,7 +26,7 @@
/** Utility class to create predicates for group index queries. */
public class GroupPredicates {
public static Predicate<InternalGroup> id(AccountGroup.Id groupId) {
- return new GroupPredicate(GroupField.ID, groupId.toString());
+ return new GroupPredicate(GroupField.ID_FIELD_SPEC, groupId.toString());
}
public static Predicate<InternalGroup> uuid(AccountGroup.UUID uuid) {
@@ -35,42 +35,42 @@
public static Predicate<InternalGroup> description(String description) {
return new GroupPredicate(
- GroupField.DESCRIPTION, GroupQueryBuilder.FIELD_DESCRIPTION, description);
+ GroupField.DESCRIPTION_SPEC, GroupQueryBuilder.FIELD_DESCRIPTION, description);
}
public static Predicate<InternalGroup> inname(String name) {
return new GroupPredicate(
- GroupField.NAME_PART, GroupQueryBuilder.FIELD_INNAME, name.toLowerCase(Locale.US));
+ GroupField.NAME_PART_SPEC, GroupQueryBuilder.FIELD_INNAME, name.toLowerCase(Locale.US));
}
public static Predicate<InternalGroup> name(String name) {
- return new GroupPredicate(GroupField.NAME, name);
+ return new GroupPredicate(GroupField.NAME_SPEC, name);
}
public static Predicate<InternalGroup> owner(AccountGroup.UUID ownerUuid) {
return new GroupPredicate(
- GroupField.OWNER_UUID, GroupQueryBuilder.FIELD_OWNER, ownerUuid.get());
+ GroupField.OWNER_UUID_SPEC, GroupQueryBuilder.FIELD_OWNER, ownerUuid.get());
}
public static Predicate<InternalGroup> isVisibleToAll() {
- return new GroupPredicate(GroupField.IS_VISIBLE_TO_ALL, "1");
+ return new GroupPredicate(GroupField.IS_VISIBLE_TO_ALL_SPEC, "1");
}
public static Predicate<InternalGroup> member(Account.Id memberId) {
- return new GroupPredicate(GroupField.MEMBER, memberId.toString());
+ return new GroupPredicate(GroupField.MEMBER_SPEC, memberId.toString());
}
public static Predicate<InternalGroup> subgroup(AccountGroup.UUID subgroupUuid) {
- return new GroupPredicate(GroupField.SUBGROUP, subgroupUuid.get());
+ return new GroupPredicate(GroupField.SUBGROUP_SPEC, subgroupUuid.get());
}
/** Predicate that is mapped to a field in the group index. */
static class GroupPredicate extends IndexPredicate<InternalGroup> {
- GroupPredicate(FieldDef<InternalGroup, ?> def, String value) {
+ GroupPredicate(SchemaField<InternalGroup, ?> def, String value) {
super(def, value);
}
- GroupPredicate(FieldDef<InternalGroup, ?> def, String name, String value) {
+ GroupPredicate(SchemaField<InternalGroup, ?> def, String name, String value) {
super(def, name, value);
}
}
diff --git a/polygerrit-ui/app/api/rest-api.ts b/polygerrit-ui/app/api/rest-api.ts
index 5ec0836..ab33eed 100644
--- a/polygerrit-ui/app/api/rest-api.ts
+++ b/polygerrit-ui/app/api/rest-api.ts
@@ -407,7 +407,6 @@
cherry_pick_of_change?: NumericChangeId;
cherry_pick_of_patch_set?: RevisionPatchSetNum;
contains_git_conflicts?: boolean;
- internalHost?: string; // TODO(TS): provide an explanation what is its
submit_requirements?: SubmitRequirementResultInfo[];
submit_records?: SubmitRecordInfo[];
}
@@ -1180,3 +1179,19 @@
status: LabelStatus;
appliedBy: AccountInfo;
}
+
+/**
+ * Represent a file in a base64 encoding; GrRestApiInterface returns
+ * it from some methods
+ */
+export declare interface Base64FileContent {
+ content: string | null;
+ type: string | null;
+ ok: true;
+}
+
+export function isBase64FileContent(
+ res: Response | Base64FileContent
+): res is Base64FileContent {
+ return (res as Base64FileContent).ok;
+}
diff --git a/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.ts b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.ts
index 4b62710..29f06fd 100644
--- a/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.ts
+++ b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section.ts
@@ -179,7 +179,7 @@
class=${this.section?.id === GLOBAL_NAME ? 'global' : ''}
@click=${this.editReference}
>
- <gr-icon id="icon" icon="edit"></gr-icon>
+ <gr-icon id="icon" icon="edit" filled small></gr-icon>
</gr-button>
</div>
<iron-input
diff --git a/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section_test.ts b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section_test.ts
index 1048dde..b8cee66 100644
--- a/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-access-section/gr-access-section_test.ts
@@ -84,7 +84,7 @@
role="button"
tabindex="0"
>
- <gr-icon icon="edit" id="icon"></gr-icon>
+ <gr-icon icon="edit" id="icon" small filled></gr-icon>
</gr-button>
</div>
<iron-input class="editRefInput">
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
index 8910e27..33129f6 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
@@ -477,20 +477,16 @@
ColumnNames.REPO,
this.visibleChangeTableColumns
)
- )
+ ) {
return;
+ }
+ const repo = this.change?.project ?? '';
return html`
<td class="cell repo">
- <a class="fullRepo" href=${this.computeRepoUrl()}>
- ${this.computeRepoDisplay()}
- </a>
- <a
- class="truncatedRepo"
- href=${this.computeRepoUrl()}
- title=${this.computeRepoDisplay()}
- >
- ${this.computeTruncatedRepoDisplay()}
+ <a class="fullRepo" href=${this.computeRepoUrl()}> ${repo} </a>
+ <a class="truncatedRepo" href=${this.computeRepoUrl()} title=${repo}>
+ ${truncatePath(repo, 2)}
</a>
</td>
`;
@@ -655,11 +651,7 @@
private computeRepoUrl() {
if (!this.change) return '';
- return GerritNav.getUrlForProjectChanges(
- this.change.project,
- true,
- this.change.internalHost
- );
+ return GerritNav.getUrlForProjectChanges(this.change.project, true);
}
private computeRepoBranchURL() {
@@ -667,36 +659,13 @@
return GerritNav.getUrlForBranch(
this.change.branch,
this.change.project,
- undefined,
- this.change.internalHost
+ undefined
);
}
private computeTopicURL() {
if (!this.change?.topic) return '';
- return GerritNav.getUrlForTopic(
- this.change.topic,
- this.change.internalHost
- );
- }
-
- /**
- * Computes the display string for the project column. If there is a host
- * specified in the change detail, the string will be prefixed with it.
- *
- * @param truncate whether or not the project name should be
- * truncated. If this value is truthy, the name will be truncated.
- *
- * private but used in test
- */
- computeRepoDisplay() {
- if (!this.change?.project) return '';
- let str = '';
- if (this.change.internalHost) {
- str += this.change.internalHost + '/';
- }
- str += this.change.project;
- return str;
+ return GerritNav.getUrlForTopic(this.change.topic);
}
private toggleCheckbox() {
@@ -706,19 +675,6 @@
}
// private but used in test
- computeTruncatedRepoDisplay() {
- if (!this.change?.project) {
- return '';
- }
- let str = '';
- if (this.change.internalHost) {
- str += this.change.internalHost + '/';
- }
- str += truncatePath(this.change.project, 2);
- return str;
- }
-
- // private but used in test
computeSizeTooltip() {
if (
!this.change ||
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
index 50e6c2d..ed8c69f 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.ts
@@ -53,7 +53,6 @@
const account = createAccountWithId();
const change: ChangeInfo = {
...createChange(),
- internalHost: 'host',
project: 'a/test/repo' as RepoName,
topic: 'test-topic' as TopicName,
branch: 'test-branch' as BranchName,
@@ -335,18 +334,13 @@
assert.deepEqual(navStub.getUrlForProjectChanges.lastCall.args, [
change.project,
true,
- change.internalHost,
]);
assert.deepEqual(navStub.getUrlForBranch.lastCall.args, [
change.branch,
change.project,
undefined,
- change.internalHost,
]);
- assert.deepEqual(navStub.getUrlForTopic.lastCall.args, [
- change.topic,
- change.internalHost,
- ]);
+ assert.deepEqual(navStub.getUrlForTopic.lastCall.args, [change.topic!]);
});
test('clicking item navigates to change', async () => {
@@ -361,16 +355,6 @@
assert.isTrue(navStub.navigateToChange.calledWithExactly(change));
});
- test('computeRepoDisplay', () => {
- element.change = {...change};
- assert.equal(element.computeRepoDisplay(), 'host/a/test/repo');
- assert.equal(element.computeTruncatedRepoDisplay(), 'host/…/test/repo');
- delete change.internalHost;
- element.change = {...change};
- assert.equal(element.computeRepoDisplay(), 'a/test/repo');
- assert.equal(element.computeTruncatedRepoDisplay(), '…/test/repo');
- });
-
test('renders', async () => {
element.showStar = true;
element.showNumber = true;
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow_test.ts
index 5a4a90c..ef48323 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow_test.ts
@@ -891,8 +891,6 @@
);
assert.deepEqual(reviewerList.accounts[1], {
- _group: true,
- _pendingAdd: true,
confirmed: true,
id: '5' as GroupId,
name: 'large-group',
@@ -933,8 +931,6 @@
getComputedStyle(confirmDialog).getPropertyValue('display') === 'none'
);
assert.deepEqual(reviewerList.accounts[1], {
- _group: true,
- _pendingAdd: true,
id: '5' as GroupId,
name: 'small-group',
});
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
index cfb5602..a9f57f5 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
@@ -20,7 +20,6 @@
import '../gr-submit-requirements/gr-submit-requirements';
import '../gr-commit-info/gr-commit-info';
import '../gr-reviewer-list/gr-reviewer-list';
-import '../../shared/gr-account-list/gr-account-list';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
import {
ChangeStatus,
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
index e6b9058..8bebf58 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
@@ -231,34 +231,40 @@
Cherry Pick Change to Another Branch
</div>
<div class="main" slot="main">
- ${when(this.showCherryPickTopic, () =>
- this.renderCherrypickTopicLayout()
- )}
- <label for="branchInput"> Cherry Pick to branch </label>
- <gr-autocomplete
- id="branchInput"
- .text=${this.branch}
- .query=${this.query}
- placeholder="Destination branch"
- @text-changed=${(e: BindValueChangeEvent) =>
- (this.branch = e.detail.value as BranchName)}
- >
- </gr-autocomplete>
- ${when(
- this.invalidBranch,
- () => html`
- <span class="error"
- >Branch name cannot contain space or commas.</span
- >
- `
- )}
- ${choose(this.cherryPickType, [
- [
- CherryPickType.SINGLE_CHANGE,
- () => this.renderCherrypickSingleChangeInputs(),
- ],
- [CherryPickType.TOPIC, () => this.renderCherrypickTopicTable()],
- ])}
+ <gr-endpoint-decorator name="cherrypick-main">
+ <gr-endpoint-param name="changes" .value=${this.changes}>
+ </gr-endpoint-param>
+ <gr-endpoint-slot name="top"></gr-endpoint-slot>
+ ${when(this.showCherryPickTopic, () =>
+ this.renderCherrypickTopicLayout()
+ )}
+ <label for="branchInput"> Cherry Pick to branch </label>
+ <gr-autocomplete
+ id="branchInput"
+ .text=${this.branch}
+ .query=${this.query}
+ placeholder="Destination branch"
+ @text-changed=${(e: BindValueChangeEvent) =>
+ (this.branch = e.detail.value as BranchName)}
+ >
+ </gr-autocomplete>
+ ${when(
+ this.invalidBranch,
+ () => html`
+ <span class="error"
+ >Branch name cannot contain space or commas.</span
+ >
+ `
+ )}
+ ${choose(this.cherryPickType, [
+ [
+ CherryPickType.SINGLE_CHANGE,
+ () => this.renderCherrypickSingleChangeInputs(),
+ ],
+ [CherryPickType.TOPIC, () => this.renderCherrypickTopicTable()],
+ ])}
+ <gr-endpoint-slot name="bottom"></gr-endpoint-slot>
+ </gr-endpoint-decorator>
</div>
</gr-dialog>
`;
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
index fbb6839..7933896 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
@@ -60,29 +60,34 @@
Cherry Pick Change to Another Branch
</div>
<div class="main" slot="main">
- <label for="branchInput"> Cherry Pick to branch </label>
- <gr-autocomplete id="branchInput" placeholder="Destination branch">
- </gr-autocomplete>
- <label for="baseInput">
- Provide base commit sha1 for cherry-pick
- </label>
- <iron-input>
- <input
- id="baseCommitInput"
- is="iron-input"
- maxlength="40"
- placeholder="(optional)"
- />
- </iron-input>
- <label for="messageInput"> Cherry Pick Commit Message </label>
- <iron-autogrow-textarea
- aria-disabled="false"
- autocomplete="on"
- class="message"
- id="messageInput"
- rows="4"
- >
- </iron-autogrow-textarea>
+ <gr-endpoint-decorator name="cherrypick-main">
+ <gr-endpoint-param name="changes"> </gr-endpoint-param>
+ <gr-endpoint-slot name="top"> </gr-endpoint-slot>
+ <label for="branchInput"> Cherry Pick to branch </label>
+ <gr-autocomplete id="branchInput" placeholder="Destination branch">
+ </gr-autocomplete>
+ <label for="baseInput">
+ Provide base commit sha1 for cherry-pick
+ </label>
+ <iron-input>
+ <input
+ id="baseCommitInput"
+ is="iron-input"
+ maxlength="40"
+ placeholder="(optional)"
+ />
+ </iron-input>
+ <label for="messageInput"> Cherry Pick Commit Message </label>
+ <iron-autogrow-textarea
+ aria-disabled="false"
+ autocomplete="on"
+ class="message"
+ id="messageInput"
+ rows="4"
+ >
+ </iron-autogrow-textarea>
+ <gr-endpoint-slot name="bottom"></gr-endpoint-slot>
+ </gr-endpoint-decorator>
</div>
</gr-dialog>
`);
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 731bcd0..ccdcc15 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -27,15 +27,14 @@
} from '../../../constants/constants';
import {
accountOrGroupKey,
- isReviewerOrCC,
- mapReviewer,
+ isAccountNewlyAdded,
removeServiceUsers,
+ toReviewInput,
} from '../../../utils/account-util';
import {IronA11yAnnouncer} from '@polymer/iron-a11y-announcer/iron-a11y-announcer';
import {TargetElement} from '../../../api/plugin';
import {FixIronA11yAnnouncer} from '../../../types/types';
import {
- AccountAddition,
AccountInfoInput,
AccountInput,
AccountInputDetail,
@@ -71,6 +70,7 @@
areSetsEqual,
assertIsDefined,
containsAll,
+ difference,
queryAndAssert,
} from '../../../utils/common-util';
import {CommentThread, isUnresolved} from '../../../utils/comment-util';
@@ -108,6 +108,7 @@
import {customElement, property, state, query} from 'lit/decorators';
import {subscribe} from '../../lit/subscription-controller';
import {configModelToken} from '../../../models/config/config-model';
+import {hasHumanReviewer, isOwner} from '../../../utils/change-util';
const STORAGE_DEBOUNCE_INTERVAL_MS = 400;
@@ -321,9 +322,6 @@
@state()
isResolvedPatchsetLevelComment = true;
- @state()
- allReviewers: (AccountInfo | GroupInfo)[] = [];
-
private readonly restApiService: RestApiService =
getAppContext().restApiService;
@@ -493,6 +491,10 @@
}
.attention .edit-attention-button gr-icon {
color: inherit;
+ /* The line-height:26px hack (see below) requires us to do this.
+ Normally the gr-icon would account for a proper positioning
+ within the standard line-height:20px context. */
+ top: 5px;
}
.attention a,
.attention-detail a {
@@ -659,9 +661,6 @@
if (changedProperties.has('draftCommentThreads')) {
this.handleHeightChanged();
}
- if (changedProperties.has('reviewers')) {
- this.computeAllReviewers();
- }
if (changedProperties.has('sendDisabled')) {
this.sendDisabledChanged();
}
@@ -700,7 +699,7 @@
name="change"
.value=${this.change}
></gr-endpoint-param>
- <gr-endpoint-param name="reviewers" .value=${this.allReviewers}>
+ <gr-endpoint-param name="reviewers" .value=${[...this.reviewers]}>
</gr-endpoint-param>
${this.renderReviewerList()}
<gr-endpoint-slot name="below"></gr-endpoint-slot>
@@ -747,6 +746,7 @@
id="reviewers"
.accounts=${this.getAccountListCopy(this.reviewers)}
.change=${this.change}
+ .reviewerState=${ReviewerState.REVIEWER}
@account-added=${this.accountAdded}
@accounts-changed=${this.handleReviewersChanged}
.removableValues=${this.change?.removable_reviewers}
@@ -773,6 +773,8 @@
<gr-account-list
id="ccs"
.accounts=${this.getAccountListCopy(this.ccs)}
+ .change=${this.change}
+ .reviewerState=${ReviewerState.CC}
@account-added=${this.accountAdded}
@accounts-changed=${this.handleCcsChanged}
.removableValues=${this.change?.removable_reviewers}
@@ -969,8 +971,10 @@
role="button"
tabindex="0"
>
- <gr-icon icon="edit" filled></gr-icon>
- Modify
+ <div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ <span>Modify</span>
+ </div>
</gr-button>
</gr-tooltip-content>
</div>
@@ -1047,7 +1051,7 @@
<div class="peopleList">
<div class="peopleListLabel">Reviewers</div>
<div class="peopleListValues">
- ${this.removeServiceUsers(this.reviewers).map(
+ ${removeServiceUsers(this.reviewers).map(
account => html`
<gr-account-label
.account=${account}
@@ -1069,7 +1073,7 @@
<div class="peopleList">
<div class="peopleListLabel">CC</div>
<div class="peopleListValues">
- ${this.removeServiceUsers(this.ccs).map(
+ ${removeServiceUsers(this.ccs).map(
account => html`
<gr-account-label
.account=${account}
@@ -1281,42 +1285,32 @@
return isResolvedPatchsetLevelComment ? 'resolved' : 'unresolved';
}
- computeReviewers(change: ChangeInfo) {
+ computeReviewers() {
const reviewers: ReviewerInput[] = [];
- const addToReviewInput = (
- additions: AccountAddition[],
- state?: ReviewerState
- ) => {
- additions.forEach(addition => {
- const reviewer = mapReviewer(addition);
- if (state) reviewer.state = state;
- reviewers.push(reviewer);
- });
- };
- addToReviewInput(this.reviewersList!.additions(), ReviewerState.REVIEWER);
- addToReviewInput(this.ccsList!.additions(), ReviewerState.CC);
- addToReviewInput(
- this.reviewersList!.removals().filter(
- r =>
- isReviewerOrCC(change, r) &&
- // ignore removal from reviewer request if being added to CC
- !this.ccsList!.additions().some(
- account => mapReviewer(account).reviewer === mapReviewer(r).reviewer
- )
- ),
- ReviewerState.REMOVED
+ const reviewerAdditions = this.reviewersList?.additions() ?? [];
+ reviewers.push(
+ ...reviewerAdditions.map(v => toReviewInput(v, ReviewerState.REVIEWER))
);
- addToReviewInput(
- this.ccsList!.removals().filter(
- r =>
- isReviewerOrCC(change, r) &&
- // ignore removal from CC request if being added as reviewer
- !this.reviewersList!.additions().some(
- account => mapReviewer(account).reviewer === mapReviewer(r).reviewer
- )
- ),
- ReviewerState.REMOVED
- );
+
+ const ccAdditions = this.ccsList?.additions() ?? [];
+ reviewers.push(...ccAdditions.map(v => toReviewInput(v, ReviewerState.CC)));
+
+ // ignore removal from reviewer request if being added as CC
+ let removals = difference(
+ this.reviewersList?.removals() ?? [],
+ ccAdditions,
+ (a, b) => accountOrGroupKey(a) === accountOrGroupKey(b)
+ ).map(v => toReviewInput(v, ReviewerState.REMOVED));
+ reviewers.push(...removals);
+
+ // ignore removal from CC request if being added as reviewer
+ removals = difference(
+ this.ccsList?.removals() ?? [],
+ reviewerAdditions,
+ (a, b) => accountOrGroupKey(a) === accountOrGroupKey(b)
+ ).map(v => toReviewInput(v, ReviewerState.REMOVED));
+ reviewers.push(...removals);
+
return reviewers;
}
@@ -1367,7 +1361,7 @@
}
assertIsDefined(this.change, 'change');
- reviewInput.reviewers = this.computeReviewers(this.change);
+ reviewInput.reviewers = this.computeReviewers();
this.disabled = true;
const errFn = (r?: Response | null) => this.handle400Error(r);
@@ -1422,19 +1416,9 @@
}
chooseFocusTarget() {
- // If we are the owner and the reviewers field is empty, focus on that.
- if (
- this.account &&
- this.change &&
- this.change.owner &&
- this.account._account_id === this.change.owner._account_id &&
- (!this.reviewers || this.reviewers?.length === 0)
- ) {
- return FocusTarget.REVIEWERS;
- }
-
- // Default to BODY.
- return FocusTarget.BODY;
+ if (!isOwner(this.change, this.account)) return FocusTarget.BODY;
+ if (hasHumanReviewer(this.change)) return FocusTarget.BODY;
+ return FocusTarget.REVIEWERS;
}
isOwner(account?: AccountInfo, change?: ChangeInfo) {
@@ -1624,7 +1608,11 @@
);
this.reviewers
.filter(r => isAccount(r))
- .filter(r => r._pendingAdd || (this.canBeStarted && isOwner))
+ .filter(
+ r =>
+ isAccountNewlyAdded(r, ReviewerState.REVIEWER, this.change) ||
+ (this.canBeStarted && isOwner)
+ )
.filter(notIsReviewerAndHasDraftOrLabel)
.forEach(r => newAttention.add((r as AccountInfo)._account_id!));
// Add owner and uploader, if someone else replies.
@@ -1743,10 +1731,6 @@
return removeServiceUsers(allAccounts.filter(isAccount));
}
- removeServiceUsers(accounts: AccountInfo[]) {
- return removeServiceUsers(accounts);
- }
-
computeUploader() {
if (
!this.change?.current_revision ||
@@ -1816,7 +1800,6 @@
})
);
queryAndAssert<GrTextarea>(this, 'gr-textarea').closeDropdown();
- this.reviewersList?.clearPendingRemovals();
this.rebuildReviewerArrays();
}
@@ -2102,10 +2085,6 @@
}
this.reporting.reportInteraction('attention-set-actions', {actions});
}
-
- computeAllReviewers() {
- this.allReviewers = [...this.reviewers];
- }
}
declare global {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 95cdba91..67190de 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -28,6 +28,7 @@
createCommentThread,
createDraft,
createRevision,
+ createServiceUserWithId,
} from '../../../test/test-data-generators';
import {
pressAndReleaseKeyOn,
@@ -281,8 +282,10 @@
role="button"
tabindex="0"
>
- <gr-icon icon="edit" filled></gr-icon>
- Modify
+ <div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ <span>Modify</span>
+ </div>
</gr-button>
</gr-tooltip-content>
</div>
@@ -897,8 +900,8 @@
await element.updateComplete;
element.reviewers = [
- {_account_id: 1 as AccountId, _pendingAdd: true},
- {_account_id: 2 as AccountId, _pendingAdd: true},
+ {_account_id: 1 as AccountId},
+ {_account_id: 2 as AccountId},
];
element.ccs = [];
element.draftCommentThreads = [];
@@ -927,6 +930,12 @@
owner: {_account_id: 1 as AccountId},
status: ChangeStatus.NEW,
attention_set: {},
+ reviewers: {
+ [ReviewerState.REVIEWER]: [
+ {...createAccountWithId(2)},
+ {...createAccountWithId(3)},
+ ],
+ },
};
// let rebuildReviewers triggered by change update finish running
await element.updateComplete;
@@ -1340,14 +1349,8 @@
);
// No reviewer/CC should have been added.
- assert.equal(
- queryAndAssert<GrAccountList>(element, '#ccs').additions().length,
- 0
- );
- assert.equal(
- queryAndAssert<GrAccountList>(element, '#reviewers').additions().length,
- 0
- );
+ assert.equal(element.ccsList?.additions().length, 0);
+ assert.equal(element.reviewersList?.additions().length, 0);
// Reopen confirmation dialog.
observer = overlayObserver('opened');
@@ -1377,17 +1380,11 @@
isVisible(queryAndAssert(element, 'reviewerConfirmationOverlay'))
);
const additions = cc
- ? queryAndAssert<GrAccountList>(element, '#ccs').additions()
- : queryAndAssert<GrAccountList>(element, '#reviewers').additions();
+ ? element.ccsList?.additions()
+ : element.reviewersList?.additions();
assert.deepEqual(additions, [
{
- group: {
- id: 'id' as GroupId,
- name: 'name' as GroupName,
- confirmed: true,
- _group: true,
- _pendingAdd: true,
- },
+ name: 'name' as GroupName,
},
]);
@@ -1628,28 +1625,22 @@
test('chooseFocusTarget', () => {
element.account = undefined;
- assert.strictEqual(element.chooseFocusTarget(), element.FocusTarget.BODY);
+ assert.equal(element.chooseFocusTarget(), element.FocusTarget.BODY);
- element.account = {_account_id: 1 as AccountId};
- assert.strictEqual(element.chooseFocusTarget(), element.FocusTarget.BODY);
+ element.account = element.change!.owner;
+ assert.equal(element.chooseFocusTarget(), element.FocusTarget.REVIEWERS);
- element.change!.owner = {_account_id: 2 as AccountId};
- assert.strictEqual(element.chooseFocusTarget(), element.FocusTarget.BODY);
+ element.change!.reviewers.REVIEWER = [createAccountWithId(314)];
+ assert.equal(element.chooseFocusTarget(), element.FocusTarget.BODY);
- element.change!.owner._account_id = 1 as AccountId;
- assert.strictEqual(
- element.chooseFocusTarget(),
- element.FocusTarget.REVIEWERS
- );
+ element.change!.reviewers.REVIEWER = [createServiceUserWithId(314)];
+ assert.equal(element.chooseFocusTarget(), element.FocusTarget.REVIEWERS);
- element.reviewers = [];
- assert.strictEqual(
- element.chooseFocusTarget(),
- element.FocusTarget.REVIEWERS
- );
-
- element.reviewers.push({});
- assert.strictEqual(element.chooseFocusTarget(), element.FocusTarget.BODY);
+ element.change!.reviewers.REVIEWER = [
+ createAccountWithId(314),
+ createServiceUserWithId(314),
+ ];
+ assert.equal(element.chooseFocusTarget(), element.FocusTarget.BODY);
});
test('only send labels that have changed', async () => {
@@ -2003,6 +1994,8 @@
[ReviewerState.REVIEWER]: [{_account_id: reviewer1._account_id}],
};
+ await element.updateComplete;
+
const mutations: ReviewerInput[] = [];
stubSaveReview((review: ReviewInput) => {
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
index 61e16b0..163ec49 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
@@ -73,15 +73,14 @@
}
.addReviewer gr-icon {
color: inherit;
- font-size: 18px;
}
.controlsContainer {
display: inline-block;
}
gr-button.addReviewer {
- --gr-button-padding: 1px 0px;
vertical-align: top;
- top: 1px;
+ --gr-button-padding: var(--spacing-s);
+ --margin: calc(0px - var(--spacing-s));
}
gr-button {
line-height: var(--line-height-normal);
@@ -117,7 +116,10 @@
class="addReviewer"
@click=${this.handleAddTap}
title=${this.ccsOnly ? 'Add CC' : 'Add reviewer'}
- ><gr-icon icon="edit" filled></gr-icon>
+ >
+ <div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ </div>
</gr-button>
</div>
</div>
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.ts b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.ts
index 89bb2fd..9f8aada 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.ts
@@ -39,7 +39,9 @@
tabindex="0"
title="Add reviewer"
>
- <gr-icon icon="edit" filled></gr-icon>
+ <div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ </div>
</gr-button>
</div>
</div>
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-action.ts b/polygerrit-ui/app/elements/checks/gr-checks-action.ts
index 3564ec9..d7373de 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-action.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-action.ts
@@ -6,7 +6,7 @@
import {LitElement, css, html} from 'lit';
import {customElement, property} from 'lit/decorators';
import {Action} from '../../api/checks';
-import {checkRequiredProperty} from '../../utils/common-util';
+import {assertIsDefined} from '../../utils/common-util';
import {resolve} from '../../models/dependency';
import {checksModelToken} from '../../models/checks/checks-model';
@customElement('gr-checks-action')
@@ -25,7 +25,7 @@
override connectedCallback() {
super.connectedCallback();
- checkRequiredProperty(this.action, 'action');
+ assertIsDefined(this.action, 'action');
}
static override get styles() {
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 202c502..edefdd1 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -33,7 +33,7 @@
secondaryLinks,
tooltipForLink,
} from '../../models/checks/checks-util';
-import {assertIsDefined, check} from '../../utils/common-util';
+import {assertIsDefined, assert} from '../../utils/common-util';
import {modifierPressed, toggleClass, whenVisible} from '../../utils/dom-util';
import {durationString} from '../../utils/date-util';
import {charsOnly} from '../../utils/string-util';
@@ -1201,7 +1201,7 @@
private onPatchsetSelected(e: CustomEvent<{value: string}>) {
const patchset = Number(e.detail.value);
- check(!isNaN(patchset), 'selected patchset must be a number');
+ assert(!isNaN(patchset), 'selected patchset must be a number');
this.getChecksModel().setPatchset(patchset as PatchSetNumber);
}
diff --git a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts
index 3942086..63338db 100644
--- a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts
+++ b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts
@@ -157,7 +157,6 @@
// TODO(TS): Define more precise type (enum?)
statuses?: string[];
hashtag?: string;
- host?: string;
owner?: string;
}
@@ -169,7 +168,6 @@
patchNum?: RevisionPatchSetNum;
basePatchNum?: BasePatchSetNum;
edit?: boolean;
- host?: string;
messageHash?: string;
commentId?: UrlEncodedCommentId;
forceReload?: boolean;
@@ -456,49 +454,34 @@
/**
* @param openOnly When true, only search open changes in the project.
- * @param host The host in which to search.
*/
- getUrlForProjectChanges(
- project: RepoName,
- openOnly?: boolean,
- host?: string
- ) {
+ getUrlForProjectChanges(project: RepoName, openOnly?: boolean) {
return this._getUrlFor({
view: GerritView.SEARCH,
project,
statuses: openOnly ? ['open'] : [],
- host,
});
},
/**
* @param status The status to search.
- * @param host The host in which to search.
*/
- getUrlForBranch(
- branch: BranchName,
- project: RepoName,
- status?: string,
- host?: string
- ) {
+ getUrlForBranch(branch: BranchName, project: RepoName, status?: string) {
return this._getUrlFor({
view: GerritView.SEARCH,
branch,
project,
statuses: status ? [status] : undefined,
- host,
});
},
/**
* @param topic The name of the topic.
- * @param host The host in which to search.
*/
- getUrlForTopic(topic: TopicName, host?: string) {
+ getUrlForTopic(topic: TopicName) {
return this._getUrlFor({
view: GerritView.SEARCH,
topic,
- host,
});
},
@@ -543,7 +526,7 @@
* @param basePatchNum The string PARENT can be used for none.
*/
getUrlForChange(
- change: Pick<ChangeInfo, '_number' | 'project' | 'internalHost'>,
+ change: Pick<ChangeInfo, '_number' | 'project'>,
options: ChangeUrlParams = {}
) {
let {
@@ -567,7 +550,6 @@
patchNum,
basePatchNum,
edit: isEdit,
- host: change.internalHost || undefined,
messageHash,
forceReload,
openReplyDialog,
@@ -600,7 +582,7 @@
* and re-render everything.
*/
navigateToChange(
- change: Pick<ChangeInfo, '_number' | 'project' | 'internalHost'>,
+ change: Pick<ChangeInfo, '_number' | 'project'>,
options: NavigateToChangeParams = {}
) {
const {
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
index 5296f13..6e48231 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
@@ -12,15 +12,22 @@
import {
NumericChangeId,
EDIT,
- FixId,
FixSuggestionInfo,
PatchSetNum,
RobotId,
BasePatchSetNum,
+ FilePathToDiffInfoMap,
} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
import {GrOverlay} from '../../shared/gr-overlay/gr-overlay';
-import {isRobot} from '../../../utils/comment-util';
+import {
+ createUserFixSuggestion,
+ getContentInCommentRange,
+ getUserSuggestion,
+ hasUserSuggestion,
+ isRobot,
+ USER_SUGGEST_EDIT_FIX_ID,
+} from '../../../utils/comment-util';
import {OpenFixPreviewEvent} from '../../../types/events';
import {getAppContext} from '../../../services/app-context';
import {fireCloseFixPreview, fireEvent} from '../../../utils/event-util';
@@ -31,6 +38,8 @@
import {customElement, property, query, state} from 'lit/decorators';
import {sharedStyles} from '../../../styles/shared-styles';
import {subscribe} from '../../lit/subscription-controller';
+import {isBase64FileContent} from '../../../api/rest-api';
+import {assertIsDefined} from '../../../utils/common-util';
interface FilePreview {
filepath: string;
@@ -210,15 +219,32 @@
* @return Promise that resolves either when all
* preview diffs are fetched or no fix suggestions in custom event detail.
*/
- open(e: OpenFixPreviewEvent) {
+ async open(e: OpenFixPreviewEvent) {
const detail = e.detail;
const comment = detail.comment;
- if (!detail.patchNum || !comment || !isRobot(comment)) {
- return Promise.resolve();
+ if (comment && hasUserSuggestion(comment)) {
+ const replacement = getUserSuggestion(comment);
+ if (!replacement) throw new Error('User suggestion is malformatted');
+ // TODO(milutin): This should belongs into service/model.
+ const file = await this.restApiService.getFileContent(
+ this.changeNum!,
+ comment.path!,
+ comment.patch_set!
+ );
+ assertIsDefined(file, 'file');
+ if (!isBase64FileContent(file))
+ throw new Error('Cannot retrieve file content');
+ assertIsDefined(file.content, 'content');
+ const line = getContentInCommentRange(file.content, comment);
+ this.fixSuggestions = createUserFixSuggestion(comment, line, replacement);
+ } else {
+ if (!detail.patchNum || !comment || !isRobot(comment)) {
+ return Promise.resolve();
+ }
+ this.fixSuggestions = comment.fix_suggestions;
+ this.robotId = comment.robot_id;
}
this.patchNum = detail.patchNum;
- this.fixSuggestions = comment.fix_suggestions;
- this.robotId = comment.robot_id;
if (!this.fixSuggestions || !this.fixSuggestions.length) {
return Promise.resolve();
}
@@ -233,30 +259,42 @@
});
}
- private showSelectedFixSuggestion(fixSuggestion: FixSuggestionInfo) {
+ private async showSelectedFixSuggestion(fixSuggestion: FixSuggestionInfo) {
this.currentFix = fixSuggestion;
- return this.fetchFixPreview(fixSuggestion.fix_id);
+ await this.fetchFixPreview(fixSuggestion);
}
- private fetchFixPreview(fixId: FixId) {
+ private async fetchFixPreview(fixSuggestion: FixSuggestionInfo) {
if (!this.changeNum || !this.patchNum) {
return Promise.reject(
new Error('Both patchNum and changeNum must be set')
);
}
- return this.restApiService
- .getRobotCommentFixPreview(this.changeNum, this.patchNum, fixId)
- .then(res => {
- if (res) {
- this.currentPreviews = Object.keys(res).map(key => {
- return {filepath: key, preview: res[key]};
- });
- }
- })
- .catch(err => {
- this.close(false);
- throw err;
- });
+ let res: FilePathToDiffInfoMap | undefined;
+ try {
+ if (fixSuggestion.fix_id === USER_SUGGEST_EDIT_FIX_ID) {
+ res = await this.restApiService.getFixPreview(
+ this.changeNum,
+ this.patchNum,
+ fixSuggestion.replacements
+ );
+ } else {
+ res = await this.restApiService.getRobotCommentFixPreview(
+ this.changeNum,
+ this.patchNum,
+ fixSuggestion.fix_id
+ );
+ }
+ if (res) {
+ this.currentPreviews = Object.keys(res).map(key => {
+ return {filepath: key, preview: res![key]};
+ });
+ }
+ } catch (e) {
+ this.close(false);
+ throw e;
+ }
+ return res;
}
private overridePartialDiffPrefs() {
@@ -328,11 +366,20 @@
throw new Error('Not all required properties are set.');
}
this.isApplyFixLoading = true;
- const res = await this.restApiService.applyFixSuggestion(
- changeNum,
- patchNum,
- this.currentFix.fix_id
- );
+ let res;
+ if (this.fixSuggestions?.[0].fix_id === USER_SUGGEST_EDIT_FIX_ID) {
+ res = await this.restApiService.applyFixSuggestion(
+ changeNum,
+ patchNum,
+ this.fixSuggestions[0].replacements
+ );
+ } else {
+ res = await this.restApiService.applyRobotFixSuggestion(
+ changeNum,
+ patchNum,
+ this.currentFix.fix_id
+ );
+ }
if (res && res.ok) {
GerritNav.navigateToChange(change, {
patchNum: EDIT,
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
index e29731b..8d2b81e 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
@@ -256,9 +256,9 @@
});
test('apply fix button should call apply, navigate to change view and fire close', async () => {
- const applyFixSuggestionStub = stubRestApi('applyFixSuggestion').returns(
- Promise.resolve(new Response(null, {status: 200}))
- );
+ const applyRobotFixSuggestionStub = stubRestApi(
+ 'applyRobotFixSuggestion'
+ ).returns(Promise.resolve(new Response(null, {status: 200})));
const navigateToChangeStub = sinon.stub(GerritNav, 'navigateToChange');
element.currentFix = createFixSuggestionInfo('123');
@@ -271,7 +271,7 @@
await element.handleApplyFix(new CustomEvent('confirm'));
sinon.assert.calledOnceWithExactly(
- applyFixSuggestionStub,
+ applyRobotFixSuggestionStub,
element.change!._number,
2 as PatchSetNum,
'123'
@@ -296,15 +296,15 @@
});
test('should not navigate to change view if incorect reponse', async () => {
- const applyFixSuggestionStub = stubRestApi('applyFixSuggestion').returns(
- Promise.resolve(new Response(null, {status: 500}))
- );
+ const applyRobotFixSuggestionStub = stubRestApi(
+ 'applyRobotFixSuggestion'
+ ).returns(Promise.resolve(new Response(null, {status: 500})));
const navigateToChangeStub = sinon.stub(GerritNav, 'navigateToChange');
element.currentFix = createFixSuggestionInfo('fix_123');
await element.handleApplyFix(new CustomEvent('confirm'));
sinon.assert.calledWithExactly(
- applyFixSuggestionStub,
+ applyRobotFixSuggestionStub,
element.change!._number,
2 as PatchSetNum,
'fix_123'
@@ -325,7 +325,7 @@
});
test('server-error should throw for failed apply call', async () => {
- stubRestApi('applyFixSuggestion').returns(
+ stubRestApi('applyRobotFixSuggestion').returns(
Promise.reject(new Error('backend error'))
);
const navigateToChangeStub = sinon.stub(GerritNav, 'navigateToChange');
diff --git a/polygerrit-ui/app/elements/gr-app.ts b/polygerrit-ui/app/elements/gr-app.ts
index f63b50e..119cd5a 100644
--- a/polygerrit-ui/app/elements/gr-app.ts
+++ b/polygerrit-ui/app/elements/gr-app.ts
@@ -67,7 +67,8 @@
}
if (!this.serviceWorkerInstaller) {
this.serviceWorkerInstaller = new ServiceWorkerInstaller(
- appContext.flagsService
+ appContext.flagsService,
+ appContext.userModel
);
this.serviceWorkerInstaller.init();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
index 5c0c6d6..9e27013 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
@@ -155,11 +155,12 @@
/* This negates the 4px horizontal padding, which we appreciate as a
larger click target, but which we don't want to consume space. :-) */
margin: 0 -4px 0 -4px;
+ --gr-button-padding: 0 var(--spacing-xs);
vertical-align: top;
}
gr-icon.attention {
color: var(--deemphasized-text-color);
- font-size: 12px;
+ transform: scaleX(0.8);
}
.name {
display: inline-block;
@@ -235,11 +236,16 @@
this.selected,
this._selfAccount
)}
- ><gr-icon
- icon="label_important"
- filled
- class="attention"
- ></gr-icon>
+ >
+ <div>
+ <gr-icon
+ icon="label_important"
+ filled
+ small
+ class="attention"
+ >
+ </gr-icon>
+ </div>
</gr-button>
</gr-tooltip-content>`
: ''}
diff --git a/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.ts b/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.ts
index 1fa21e5..c8a2995 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.ts
@@ -15,12 +15,16 @@
SuggestedReviewerGroupInfo,
SuggestedReviewerAccountInfo,
SuggestedReviewerInfo,
+ isGroup,
} from '../../../types/common';
import {ReviewerSuggestionsProvider} from '../../../scripts/gr-reviewer-suggestions-provider/gr-reviewer-suggestions-provider';
import {GrAccountEntry} from '../gr-account-entry/gr-account-entry';
import {GrAccountChip} from '../gr-account-chip/gr-account-chip';
import {fire, fireAlert} from '../../../utils/event-util';
-import {accountOrGroupKey} from '../../../utils/account-util';
+import {
+ accountOrGroupKey,
+ isAccountNewlyAdded,
+} from '../../../utils/account-util';
import {LitElement, css, html, PropertyValues} from 'lit';
import {customElement, property, query, state} from 'lit/decorators';
import {sharedStyles} from '../../../styles/shared-styles';
@@ -31,9 +35,10 @@
GrAutocomplete,
} from '../gr-autocomplete/gr-autocomplete';
import {ValueChangedEvent} from '../../../types/events';
-import {queryAndAssert} from '../../../utils/common-util';
+import {difference, queryAndAssert} from '../../../utils/common-util';
import {PaperInputElement} from '@polymer/paper-input/paper-input';
import {IronInputElement} from '@polymer/iron-input';
+import {ReviewerState} from '../../../api/rest-api';
const VALID_EMAIL_ALERT = 'Please input a valid email.';
@@ -74,37 +79,18 @@
// Internal input type with account info
export interface AccountInfoInput extends AccountInfo {
- _group?: boolean;
_account?: boolean;
- _pendingAdd?: boolean;
confirmed?: boolean;
}
// Internal input type with group info
export interface GroupInfoInput extends GroupInfo {
- _group?: boolean;
_account?: boolean;
- _pendingAdd?: boolean;
confirmed?: boolean;
}
-function isAccountInfoInput(x: AccountInput): x is AccountInfoInput {
- const input = x as AccountInfoInput;
- return !!input._account || !!input._account_id || !!input.email;
-}
-
-function isGroupInfoInput(x: AccountInput): x is GroupInfoInput {
- const input = x as GroupInfoInput;
- return !!input._group || !!input.id;
-}
-
export type AccountInput = AccountInfoInput | GroupInfoInput;
-export interface AccountAddition {
- account?: AccountInfoInput;
- group?: GroupInfoInput;
-}
-
@customElement('gr-account-list')
export class GrAccountList extends LitElement {
/**
@@ -129,6 +115,9 @@
@property({type: Boolean})
disabled = false;
+ @property({type: String})
+ reviewerState?: ReviewerState;
+
/**
* Returns suggestions and convert them to list item
*/
@@ -164,8 +153,6 @@
private readonly reporting = getAppContext().reportingService;
- private pendingRemoval: Set<AccountInput> = new Set();
-
constructor() {
super();
this.querySuggestions = input => this.getSuggestions(input);
@@ -190,7 +177,7 @@
.group {
--account-label-suffix: ' (group)';
}
- .pendingAdd {
+ .newlyAdded {
font-style: italic;
}
.list {
@@ -209,8 +196,12 @@
.account=${account}
.change=${this.change}
class=${classMap({
- group: !!account._group,
- pendingAdd: !!account._pendingAdd,
+ group: isGroup(account),
+ newlyAdded: isAccountNewlyAdded(
+ account,
+ this.reviewerState,
+ this.change
+ ),
})}
?removable=${this.computeRemovable(account)}
@keydown=${this.handleChipKeydown}
@@ -284,8 +275,7 @@
let group;
let itemTypeAdded = 'unknown';
if (isAccountObject(item)) {
- account = {...item.account, _pendingAdd: true};
- this.removeFromPendingRemoval(account);
+ account = {...item.account};
this.accounts.push(account);
itemTypeAdded = 'account';
} else if (isSuggestedReviewerGroupInfo(item)) {
@@ -293,9 +283,8 @@
this.pendingConfirmation = item;
return;
}
- group = {...item.group, _pendingAdd: true, _group: true};
+ group = {...item.group};
this.accounts.push(group);
- this.removeFromPendingRemoval(group);
itemTypeAdded = 'group';
} else if (this.allowAnyInput) {
if (!item.includes('@')) {
@@ -305,9 +294,8 @@
fireAlert(this, VALID_EMAIL_ALERT);
return false;
} else {
- account = {email: item as EmailAddress, _pendingAdd: true};
+ account = {email: item as EmailAddress};
this.accounts.push(account);
- this.removeFromPendingRemoval(account);
itemTypeAdded = 'email';
}
}
@@ -323,8 +311,6 @@
this.accounts.push({
...group,
confirmed: true,
- _pendingAdd: true,
- _group: true,
});
this.pendingConfirmation = null;
fire(this, 'accounts-changed', {value: this.accounts});
@@ -332,18 +318,6 @@
}
// private but used in test
- computeChipClass(account: AccountInput) {
- const classes = [];
- if (account._group) {
- classes.push('group');
- }
- if (account._pendingAdd) {
- classes.push('pendingAdd');
- }
- return classes.join(' ');
- }
-
- // private but used in test
computeRemovable(account: AccountInput) {
if (this.readonly) {
return false;
@@ -357,7 +331,7 @@
return true;
}
}
- return !!account._pendingAdd;
+ return isAccountNewlyAdded(account, this.reviewerState, this.change);
}
return true;
}
@@ -375,7 +349,6 @@
for (let i = 0; i < this.accounts.length; i++) {
if (accountOrGroupKey(toRemove) === accountOrGroupKey(this.accounts[i])) {
this.accounts.splice(i, 1);
- this.pendingRemoval.add(toRemove);
this.reporting.reportInteraction(`Remove from ${this.id}`);
this.requestUpdate();
fire(this, 'accounts-changed', {value: this.accounts.slice()});
@@ -473,37 +446,19 @@
return wasSubmitted;
}
- additions(): AccountAddition[] {
- return this.accounts
- .filter(account => account._pendingAdd)
- .map(account => {
- if (isGroupInfoInput(account)) {
- return {group: account};
- } else if (isAccountInfoInput(account)) {
- return {account};
- } else {
- throw new Error('AccountInput must be either Account or Group.');
- }
- });
+ additions(): (AccountInfoInput | GroupInfoInput)[] {
+ if (!this.change) return [];
+ return this.accounts.filter(account =>
+ isAccountNewlyAdded(account, this.reviewerState, this.change)
+ );
}
- removals(): AccountAddition[] {
- return Array.from(this.pendingRemoval).map(account => {
- if (isGroupInfoInput(account)) {
- return {group: account};
- } else if (isAccountInfoInput(account)) {
- return {account};
- } else {
- throw new Error('AccountInput must be either Account or Group.');
- }
- });
- }
-
- private removeFromPendingRemoval(account: AccountInput) {
- this.pendingRemoval.delete(account);
- }
-
- clearPendingRemovals() {
- this.pendingRemoval.clear();
+ removals(): AccountInfo[] {
+ if (!this.reviewerState) return [];
+ return difference(
+ this.change?.reviewers[this.reviewerState] ?? [],
+ this.accounts,
+ (a, b) => accountOrGroupKey(a) === accountOrGroupKey(b)
+ );
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list_test.ts b/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list_test.ts
index 21b9e59..873b6cf 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list_test.ts
@@ -28,6 +28,8 @@
GrAutocomplete,
} from '../gr-autocomplete/gr-autocomplete';
import {GrAccountEntry} from '../gr-account-entry/gr-account-entry';
+import {createChange} from '../../../test/test-data-generators';
+import {ReviewerState} from '../../../api/rest-api';
const basicFixture = fixtureFromElement('gr-account-list');
@@ -65,7 +67,6 @@
const groupId = `group${++_nextAccountId}`;
return {
id: groupId as GroupId,
- _group: true,
name: 'abcd' as GroupName,
};
};
@@ -94,6 +95,9 @@
element = basicFixture.instantiate();
element.accounts = [existingAccount1, existingAccount2];
+ element.reviewerState = ReviewerState.REVIEWER;
+ element.change = {...createChange()};
+ element.change.reviewers[ReviewerState.REVIEWER] = [...element.accounts];
suggestionsProvider = new MockSuggestionsProvider();
element.suggestionsProvider = suggestionsProvider;
await element.updateComplete;
@@ -130,18 +134,18 @@
// Existing accounts are listed.
let chips = getChips();
assert.equal(chips.length, 2);
- assert.isFalse(chips[0].classList.contains('pendingAdd'));
- assert.isFalse(chips[1].classList.contains('pendingAdd'));
+ assert.isFalse(chips[0].classList.contains('newlyAdded'));
+ assert.isFalse(chips[1].classList.contains('newlyAdded'));
- // New accounts are added to end with pendingAdd class.
+ // New accounts are added to end with newlyAdded class.
const newAccount = makeAccount();
handleAdd({account: newAccount, count: 1});
await element.updateComplete;
chips = getChips();
assert.equal(chips.length, 3);
- assert.isFalse(chips[0].classList.contains('pendingAdd'));
- assert.isFalse(chips[1].classList.contains('pendingAdd'));
- assert.isTrue(chips[2].classList.contains('pendingAdd'));
+ assert.isFalse(chips[0].classList.contains('newlyAdded'));
+ assert.isFalse(chips[1].classList.contains('newlyAdded'));
+ assert.isTrue(chips[2].classList.contains('newlyAdded'));
// Removed accounts are taken out of the list.
element.dispatchEvent(
@@ -154,8 +158,8 @@
await element.updateComplete;
chips = getChips();
assert.equal(chips.length, 2);
- assert.isFalse(chips[0].classList.contains('pendingAdd'));
- assert.isTrue(chips[1].classList.contains('pendingAdd'));
+ assert.isFalse(chips[0].classList.contains('newlyAdded'));
+ assert.isTrue(chips[1].classList.contains('newlyAdded'));
// Invalid remove is ignored.
element.dispatchEvent(
@@ -175,16 +179,16 @@
await element.updateComplete;
chips = getChips();
assert.equal(chips.length, 1);
- assert.isFalse(chips[0].classList.contains('pendingAdd'));
+ assert.isFalse(chips[0].classList.contains('newlyAdded'));
- // New groups are added to end with pendingAdd and group classes.
+ // New groups are added to end with newlyAdded and group classes.
const newGroup = makeGroup();
handleAdd({group: newGroup, confirm: false, count: 1});
await element.updateComplete;
chips = getChips();
assert.equal(chips.length, 2);
assert.isTrue(chips[1].classList.contains('group'));
- assert.isTrue(chips[1].classList.contains('pendingAdd'));
+ assert.isTrue(chips[1].classList.contains('newlyAdded'));
// Removed groups are taken out of the list.
element.dispatchEvent(
@@ -197,7 +201,7 @@
await element.updateComplete;
chips = getChips();
assert.equal(chips.length, 1);
- assert.isFalse(chips[0].classList.contains('pendingAdd'));
+ assert.isFalse(chips[0].classList.contains('newlyAdded'));
});
test('getSuggestions uses filter correctly', () => {
@@ -260,20 +264,8 @@
});
});
- test('computeChipClass', () => {
- const account = makeAccount() as AccountInfoInput;
- assert.equal(element.computeChipClass(account), '');
- account._pendingAdd = true;
- assert.equal(element.computeChipClass(account), 'pendingAdd');
- account._group = true;
- assert.equal(element.computeChipClass(account), 'group pendingAdd');
- account._pendingAdd = false;
- assert.equal(element.computeChipClass(account), 'group');
- });
-
test('computeRemovable', async () => {
const newAccount = makeAccount() as AccountInfoInput;
- newAccount._pendingAdd = true;
element.readonly = false;
element.removableValues = [];
element.updateComplete;
@@ -320,7 +312,7 @@
assert.isTrue(element.submitEntryText());
assert.isTrue(clearStub.called);
assert.equal(
- element.additions()[0].account?.email,
+ (element.additions()[0] as AccountInfo)?.email,
'test@test' as EmailAddress
);
});
@@ -335,18 +327,11 @@
assert.deepEqual(element.additions(), [
{
- account: {
- _account_id: newAccount._account_id,
- _pendingAdd: true,
- },
+ _account_id: newAccount._account_id,
},
{
- group: {
- id: newGroup.id,
- _group: true,
- _pendingAdd: true,
- name: 'abcd' as GroupName,
- },
+ id: newGroup.id,
+ name: 'abcd' as GroupName,
},
]);
});
@@ -356,7 +341,7 @@
assert.deepEqual(element.additions(), []);
const group = makeGroup();
- const reviewer = {
+ const reviewer: RawAccountInput = {
group,
count: 10,
confirm: true,
@@ -370,13 +355,9 @@
assert.isNull(element.pendingConfirmation);
assert.deepEqual(element.additions(), [
{
- group: {
- id: group.id,
- _group: true,
- _pendingAdd: true,
- confirmed: true,
- name: 'abcd' as GroupName,
- },
+ id: group.id,
+ name: 'abcd' as GroupName,
+ confirmed: true,
},
]);
});
diff --git a/polygerrit-ui/app/elements/shared/gr-change-status/gr-change-status.ts b/polygerrit-ui/app/elements/shared/gr-change-status/gr-change-status.ts
index 954a606..cbc1428 100644
--- a/polygerrit-ui/app/elements/shared/gr-change-status/gr-change-status.ts
+++ b/polygerrit-ui/app/elements/shared/gr-change-status/gr-change-status.ts
@@ -121,12 +121,9 @@
padding: 0;
}
:host(:not([flat])) .chip,
- .icon {
+ :host(:not([flat])) .chip gr-icon {
color: var(--status-text-color);
}
- gr-icon {
- font-size: 18px;
- }
`,
];
}
@@ -158,7 +155,7 @@
<div class="chip" aria-label="Label: ${this.status}">
${this.computeStatusString()}
${this.showResolveIcon()
- ? html`<gr-icon icon="edit" filled></gr-icon>`
+ ? html`<gr-icon icon="edit" filled small></gr-icon>`
: ''}
</div>
</a>
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 2d6c2b5..456cdff 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -17,7 +17,7 @@
import '../gr-confirm-delete-comment-dialog/gr-confirm-delete-comment-dialog';
import '../gr-account-label/gr-account-label';
import {getAppContext} from '../../../services/app-context';
-import {css, html, LitElement, PropertyValues} from 'lit';
+import {css, html, LitElement, nothing, PropertyValues} from 'lit';
import {customElement, property, query, state} from 'lit/decorators';
import {resolve} from '../../../models/dependency';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
@@ -34,15 +34,18 @@
import {
Comment,
DraftInfo,
+ getContentInCommentRange,
+ hasUserSuggestion,
isDraftOrUnsaved,
isRobot,
isUnsaved,
+ USER_SUGGESTION_START_PATTERN,
} from '../../../utils/comment-util';
import {
OpenFixPreviewEventDetail,
ValueChangedEvent,
} from '../../../types/events';
-import {fire, fireEvent} from '../../../utils/event-util';
+import {fire, fireAlert, fireEvent} from '../../../utils/event-util';
import {assertIsDefined} from '../../../utils/common-util';
import {Key, Modifier} from '../../../utils/dom-util';
import {commentsModelToken} from '../../../models/comments/comments-model';
@@ -51,13 +54,15 @@
import {ShortcutController} from '../../lit/shortcut-controller';
import {classMap} from 'lit/directives/class-map';
import {LineNumber} from '../../../api/diff';
-import {CommentSide} from '../../../constants/constants';
+import {CommentSide, SpecialFilePath} from '../../../constants/constants';
import {getRandomInt} from '../../../utils/math-util';
import {Subject} from 'rxjs';
import {debounceTime} from 'rxjs/operators';
import {configModelToken} from '../../../models/config/config-model';
import {changeModelToken} from '../../../models/change/change-model';
import {Interaction} from '../../../constants/reporting';
+import {KnownExperimentId} from '../../../services/flags/flags';
+import {isBase64FileContent} from '../../../api/rest-api';
const UNSAVED_MESSAGE = 'Unable to save draft';
@@ -199,6 +204,8 @@
private readonly reporting = getAppContext().reportingService;
+ private readonly flagsService = getAppContext().flagsService;
+
private readonly getChangeModel = resolve(this, changeModelToken);
// Private but used in tests.
@@ -476,7 +483,7 @@
<div class="body">
${this.renderRobotAuthor()} ${this.renderEditingTextarea()}
${this.renderCommentMessage()} ${this.renderHumanActions()}
- ${this.renderRobotActions()}
+ ${this.renderRobotActions()} ${this.renderSuggestEditActions()}
</div>
</div>
${this.renderConfirmDialog()}
@@ -702,13 +709,49 @@
return html`
<div class="rightActions">
${this.autoSaving ? html`. ` : ''}
- ${this.renderDiscardButton()} ${this.renderEditButton()}
+ ${this.renderDiscardButton()} ${this.renderSuggestEditButton()}
+ ${this.renderPreviewSuggestEditButton()} ${this.renderEditButton()}
${this.renderCancelButton()} ${this.renderSaveButton()}
${this.renderCopyLinkIcon()}
</div>
`;
}
+ private renderPreviewSuggestEditButton() {
+ if (!this.flagsService.isEnabled(KnownExperimentId.SUGGEST_EDIT)) {
+ return nothing;
+ }
+ assertIsDefined(this.comment, 'comment');
+ if (!hasUserSuggestion(this.comment)) return nothing;
+ return html`
+ <gr-button
+ link
+ secondary
+ class="action show-fix"
+ ?disabled=${this.saving}
+ @click=${this.handleShowFix}
+ >
+ Preview Fix
+ </gr-button>
+ `;
+ }
+
+ private renderSuggestEditButton() {
+ if (!this.flagsService.isEnabled(KnownExperimentId.SUGGEST_EDIT)) {
+ return nothing;
+ }
+ assertIsDefined(this.comment, 'comment');
+ if (hasUserSuggestion(this.comment)) return nothing;
+ // TODO(milutin): remove this check once suggesting on commit message is
+ // fixed. Currently diff line doesn't match commit message line, because
+ // of metadata in diff, which aren't in content api request.
+ if (this.comment.path === SpecialFilePath.COMMIT_MESSAGE) return nothing;
+ // TODO(milutin): do not show for author/owner
+ return html`<gr-button link class="action" @click=${this.createSuggestEdit}
+ >Suggest Fix</gr-button
+ >`;
+ }
+
private renderDiscardButton() {
if (this.editing) return;
return html`<gr-button
@@ -773,6 +816,22 @@
`;
}
+ private renderSuggestEditActions() {
+ if (!this.flagsService.isEnabled(KnownExperimentId.SUGGEST_EDIT)) {
+ return nothing;
+ }
+ if (
+ !this.account ||
+ isRobot(this.comment) ||
+ isDraftOrUnsaved(this.comment)
+ ) {
+ return nothing;
+ }
+ return html`
+ <div class="robotActions">${this.renderPreviewSuggestEditButton()}</div>
+ `;
+ }
+
private renderShowFixButton() {
if (!(this.comment as RobotCommentInfo)?.fix_suggestions) return;
return html`
@@ -947,6 +1006,27 @@
fire(this, 'open-fix-preview', this.getEventPayload());
}
+ async createSuggestEdit() {
+ assertIsDefined(this.comment, 'comment');
+ assertIsDefined(this.changeNum, 'changeNum');
+ // TODO(milutin): showing a toast while the file is being loaded.
+ const file = await this.restApiService.getFileContent(
+ this.changeNum,
+ this.comment.path!,
+ this.comment.patch_set!
+ );
+ if (!file || !isBase64FileContent(file) || !file.content) {
+ fireAlert(this, 'Cannot create suggestion for this file');
+ return;
+ }
+ const line = getContentInCommentRange(file.content, this.comment);
+ if (!line) {
+ fireAlert(this, 'Cannot create suggestion for comment selection');
+ return;
+ }
+ this.messageText += `${USER_SUGGESTION_START_PATTERN}${line}${'\n```'}`;
+ }
+
// private, but visible for testing
cancel() {
assertIsDefined(this.comment, 'comment');
diff --git a/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard.ts b/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard.ts
index 2b62b34..cdd4c4c 100644
--- a/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard.ts
+++ b/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard.ts
@@ -10,8 +10,9 @@
import {classMap} from 'lit/directives/class-map';
import {ifDefined} from 'lit/directives/if-defined';
import {LitElement, css, html} from 'lit';
-import {customElement, property} from 'lit/decorators';
+import {customElement, property, query} from 'lit/decorators';
import {GrButton} from '../gr-button/gr-button';
+import {GrIcon} from '../gr-icon/gr-icon';
const COPY_TIMEOUT_MS = 1000;
@@ -34,6 +35,9 @@
@property({type: Boolean})
hideInput = false;
+ @query('#icon')
+ iconEl!: GrIcon;
+
static override get styles() {
return [
css`
@@ -55,17 +59,6 @@
line-height: var(--line-height-mono);
width: 100%;
}
- /*
- * Typically icons are 20px, which is the normal line-height.
- * The copy icon is too prominent at 20px, so we choose 16px
- * here, but add 2x2px padding below, so the entire
- * component should still fit nicely into a normal inline
- * layout flow.
- */
- #icon {
- height: 16px;
- width: 16px;
- }
gr-icon {
color: var(--deemphasized-text-color);
}
@@ -107,7 +100,7 @@
@click=${this._copyToClipboard}
aria-label="Click to copy to clipboard"
>
- <gr-icon id="icon" icon="content_copy"></gr-icon>
+ <gr-icon id="icon" icon="content_copy" small></gr-icon>
</gr-button>
</gr-tooltip-content>
</div>
@@ -130,12 +123,8 @@
this.text = queryAndAssert<HTMLInputElement>(this, '#input').value;
assertIsDefined(this.text, 'text');
- this.iconEl.innerText = 'check';
+ this.iconEl.icon = 'check';
navigator.clipboard.writeText(this.text);
- setTimeout(() => (this.iconEl.innerText = 'content_copy'), COPY_TIMEOUT_MS);
- }
-
- private get iconEl(): HTMLSpanElement {
- return queryAndAssert<HTMLSpanElement>(this, '#icon');
+ setTimeout(() => (this.iconEl.icon = 'content_copy'), COPY_TIMEOUT_MS);
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard_test.ts b/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard_test.ts
index 7b441d3..b10f488 100644
--- a/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard_test.ts
@@ -43,7 +43,7 @@
role="button"
tabindex="0"
>
- <gr-icon icon="content_copy" id="icon"></gr-icon>
+ <gr-icon icon="content_copy" id="icon" small></gr-icon>
</gr-button>
</gr-tooltip-content>
</div>
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
index ac910ea..5e18afe 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
+++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
@@ -25,6 +25,7 @@
import {nothing} from 'lit';
import {classMap} from 'lit/directives/class-map';
import {when} from 'lit/directives/when';
+import {fontStyles} from '../../../styles/gr-font-styles';
const RESTORED_MESSAGE = 'Content restored from a previous edit.';
const STORAGE_DEBOUNCE_INTERVAL_MS = 400;
@@ -110,6 +111,7 @@
static override get styles() {
return [
sharedStyles,
+ fontStyles,
css`
:host {
display: block;
@@ -166,7 +168,6 @@
}
.show-all-container gr-icon {
color: inherit;
- font-size: 18px;
}
.cancel-button {
margin-right: var(--spacing-l);
@@ -175,8 +176,6 @@
margin-right: var(--spacing-xs);
}
gr-button {
- font-family: var(--font-family);
- line-height: var(--line-height-normal);
padding: var(--spacing-xs);
}
`,
@@ -233,7 +232,7 @@
return nothing;
return html`
- <div class="show-all-container">
+ <div class="show-all-container font-normal">
${when(
this.commitCollapsible && !this.editing,
() => html`
@@ -242,15 +241,17 @@
class="show-all-button"
@click=${this.toggleCommitCollapsed}
>
- ${when(
- !this.commitCollapsed,
- () => html`<gr-icon icon="expand_less"></gr-icon>`
- )}
- ${when(
- this.commitCollapsed,
- () => html`<gr-icon icon="expand_more"></gr-icon>`
- )}
- ${this.commitCollapsed ? 'Show all' : 'Show less'}
+ <div>
+ ${when(
+ !this.commitCollapsed,
+ () => html`<gr-icon icon="expand_less" small></gr-icon>`
+ )}
+ ${when(
+ this.commitCollapsed,
+ () => html`<gr-icon icon="expand_more" small></gr-icon>`
+ )}
+ <span>${this.commitCollapsed ? 'Show all' : 'Show less'}</span>
+ </div>
</gr-button>
<div class="flex-space"></div>
`
@@ -263,7 +264,10 @@
class="edit-commit-message"
title="Edit commit message"
@click=${this.handleEditCommitMessage}
- ><gr-icon icon="edit" filled></gr-icon> Edit</gr-button
+ ><div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ <span>Edit</span>
+ </div></gr-button
>
`
)}
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.ts b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.ts
index ed09c49..d2663ed 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.ts
@@ -27,7 +27,7 @@
<div class="collapsed viewer">
<slot> </slot>
</div>
- <div class="show-all-container">
+ <div class="show-all-container font-normal">
<gr-button
aria-disabled="false"
class="show-all-button"
@@ -35,8 +35,10 @@
role="button"
tabindex="0"
>
- <gr-icon icon="expand_more"></gr-icon>
- Show all
+ <div>
+ <gr-icon icon="expand_more" small></gr-icon>
+ <span>Show all</span>
+ </div>
</gr-button>
<div class="flex-space"></div>
<gr-button
@@ -47,8 +49,10 @@
tabindex="0"
title="Edit commit message"
>
- <gr-icon icon="edit" filled></gr-icon>
- Edit
+ <div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ <span>Edit</span>
+ </div>
</gr-button>
</div>
<gr-endpoint-slot name="above-actions"> </gr-endpoint-slot>
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
index 231b6fd..19cda0f 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
+++ b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
@@ -144,10 +144,10 @@
}
gr-button gr-icon {
color: inherit;
- font-size: 18px;
}
gr-button.pencil {
- --gr-button-padding: 0px 0px;
+ --gr-button-padding: var(--spacing-s);
+ --margin: calc(0px - var(--spacing-s));
}
`,
];
@@ -188,8 +188,11 @@
class="pencil ${this.computeLabelClass()}"
@click=${this.showDropdown}
title=${this.computeLabel()}
- ><gr-icon icon="edit" filled></gr-icon
- ></gr-button>`;
+ >
+ <div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ </div>
+ </gr-button>`;
} else {
return html`<label
class=${this.computeLabelClass()}
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
index d3b16c8..963c65f 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
@@ -128,9 +128,7 @@
top: 2px;
}
gr-icon.attentionIcon {
- font-size: 14px;
- position: relative;
- top: 3px;
+ transform: scaleX(0.8);
}
gr-icon.linkIcon {
font-size: var(--line-height-normal, 20px);
@@ -270,6 +268,7 @@
<gr-icon
icon="label_important"
filled
+ small
class="attentionIcon"
></gr-icon>
<span> ${this.computePronoun()} turn to take action. </span>
diff --git a/polygerrit-ui/app/elements/shared/gr-icon/gr-icon.ts b/polygerrit-ui/app/elements/shared/gr-icon/gr-icon.ts
index 00f8ae9..cc6231a 100644
--- a/polygerrit-ui/app/elements/shared/gr-icon/gr-icon.ts
+++ b/polygerrit-ui/app/elements/shared/gr-icon/gr-icon.ts
@@ -21,7 +21,8 @@
* `icon`.
*
* @attr {String} icon - the icon to display
- * @attr {Boolean} filled - whether the icon should be filled.
+ * @attr {Boolean} filled - whether the icon should be filled
+ * @attr {Boolean} small - whether the icon should be smaller than usual
*/
@customElement('gr-icon')
export class GrIcon extends LitElement {
@@ -51,6 +52,11 @@
font-variation-settings: 'FILL' 0;
vertical-align: top;
}
+ :host([small]) {
+ font-size: 16px;
+ position: relative;
+ top: 2px;
+ }
:host([filled]) {
font-variation-settings: 'FILL' 1;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
index 583a5d1..23d0673 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
@@ -119,8 +119,6 @@
private readonly restApiService = getAppContext().restApiService;
- private disableEnterKeyForSelectingSuggestion = false;
-
private changeNum?: NumericChangeId;
/** Called in disconnectedCallback. */
@@ -297,7 +295,7 @@
// private but used in test
closeDropdown() {
- if (this.isMentionsDropdownActive()) {
+ if (this.isMentionsDropdownActive(this.text)) {
this.mentionsSuggestions?.close();
} else {
this.emojiSuggestions?.close();
@@ -350,7 +348,6 @@
e.stopPropagation();
this.getVisibleDropdown().cursorUp();
this.textarea!.textarea.focus();
- this.disableEnterKeyForSelectingSuggestion = false;
}
private handleDownKey(e: KeyboardEvent) {
@@ -361,7 +358,6 @@
e.stopPropagation();
this.getVisibleDropdown().cursorDown();
this.textarea!.textarea.focus();
- this.disableEnterKeyForSelectingSuggestion = false;
}
private handleTabKey(e: KeyboardEvent) {
@@ -369,7 +365,7 @@
// has only typed ':'.
if (
!this.isDropdownVisible() ||
- this.disableEnterKeyForSelectingSuggestion
+ (this.isEmojiDropdownActive() && this.currentSearchString === '')
) {
return;
}
@@ -384,7 +380,7 @@
// has only typed ':'. Also make sure that shortcuts aren't clobbered.
if (
!this.isDropdownVisible() ||
- this.disableEnterKeyForSelectingSuggestion
+ (this.isEmojiDropdownActive() && this.currentSearchString === '')
) {
this.indent(e);
return;
@@ -512,9 +508,9 @@
}
}
- private isMentionsDropdownActive() {
+ private isMentionsDropdownActive(text: string) {
return (
- this.specialCharIndex !== null && this.text[this.specialCharIndex] === '@'
+ this.specialCharIndex !== null && text[this.specialCharIndex] === '@'
);
}
@@ -548,10 +544,19 @@
const text = e.detail.value ?? '';
- if (!this.isMentionsDropdownActive()) {
- if (charAtCursor === ':' && this.specialCharIndex === null) {
+ if (this.flagsService.isEnabled(KnownExperimentId.MENTION_USERS)) {
+ // specialCharIndex needs to be assigned before isMentionsDropdownActive
+ // is called
+ if (charAtCursor === '@' && this.specialCharIndex === null) {
this.specialCharIndex = this.getSpecialCharIndex(text);
}
+ }
+ if (charAtCursor === ':' && this.specialCharIndex === null) {
+ this.specialCharIndex = this.getSpecialCharIndex(text);
+ }
+
+ // this.text does not contain newly typed character yet
+ if (!this.isMentionsDropdownActive(text)) {
if (this.specialCharIndex !== null) {
this.openOrResetDropdown(
this.emojiSuggestions!,
@@ -566,9 +571,6 @@
if (!this.flagsService.isEnabled(KnownExperimentId.MENTION_USERS)) return;
- if (charAtCursor === '@' && this.specialCharIndex === null) {
- this.specialCharIndex = this.getSpecialCharIndex(text);
- }
if (this.specialCharIndex !== null) {
this.openOrResetDropdown(
this.mentionsSuggestions!,
@@ -606,13 +608,11 @@
determineEmojiSuggestions(suggestionsText: string) {
if (!suggestionsText.length) {
this.formatSuggestions(ALL_SUGGESTIONS);
- this.disableEnterKeyForSelectingSuggestion = true;
} else {
const matches = ALL_SUGGESTIONS.filter(suggestion =>
suggestion.match.includes(suggestionsText)
).slice(0, MAX_ITEMS_DROPDOWN);
this.formatSuggestions(matches);
- this.disableEnterKeyForSelectingSuggestion = false;
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
index 0b3bb01..d8b4ad3 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
@@ -161,6 +161,7 @@
test('emoji dropdown does not open if mention dropdown is open', async () => {
const listenerStub = sinon.stub();
element.addEventListener('bind-value-changed', listenerStub);
+ const resetSpy = sinon.spy(element, 'resetDropdown');
stubRestApi('getSuggestedAccounts').returns(
Promise.resolve([
createAccountWithEmail('abc@google.com'),
@@ -182,6 +183,8 @@
await waitUntil(() => element.mentions.length > 0);
await element.updateComplete;
+ assert.isFalse(resetSpy.called);
+
assert.isTrue(element.emojiSuggestions!.isHidden);
assert.isFalse(element.mentionsSuggestions!.isHidden);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
index eac76da..5a34ae3 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
@@ -6,7 +6,7 @@
import {BLANK_LINE, GrDiffLine, GrDiffLineType} from './gr-diff-line';
import {LineRange, Side} from '../../../api/diff';
import {LineNumber} from './gr-diff-line';
-import {assertIsDefined, check} from '../../../utils/common-util';
+import {assertIsDefined, assert} from '../../../utils/common-util';
import {untilRendered} from '../../../utils/dom-util';
export enum GrDiffGroupType {
@@ -267,7 +267,7 @@
};
} else {
assertIsDefined(options.lines);
- check(options.lines.length > 0, 'diff group must have lines');
+ assert(options.lines.length > 0, 'diff group must have lines');
for (const line of options.lines) {
this.addLine(line);
}
diff --git a/polygerrit-ui/app/services/flags/flags.ts b/polygerrit-ui/app/services/flags/flags.ts
index 54f1a01..c1a63a7 100644
--- a/polygerrit-ui/app/services/flags/flags.ts
+++ b/polygerrit-ui/app/services/flags/flags.ts
@@ -19,6 +19,7 @@
BULK_ACTIONS = 'UiFeature__bulk_actions_dashboard',
DIFF_RENDERING_LIT = 'UiFeature__diff_rendering_lit',
PUSH_NOTIFICATIONS = 'UiFeature__push_notifications',
+ SUGGEST_EDIT = 'UiFeature__suggest_edit',
RELATED_CHANGES_SUBMITTABILITY = 'UiFeature__related_changes_submittability',
MENTION_USERS = 'UiFeature__mention_users',
}
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
index 20493a3..16aa13a 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
@@ -119,6 +119,7 @@
TagInput,
TopMenuEntryInfo,
UrlEncodedCommentId,
+ FixReplacementInfo,
} from '../../types/common';
import {
DiffInfo,
@@ -2092,6 +2093,23 @@
});
}
+ getFixPreview(
+ changeNum: NumericChangeId,
+ patchNum: PatchSetNum,
+ fixReplacementInfos: FixReplacementInfo[]
+ ): Promise<FilePathToDiffInfoMap | undefined> {
+ return this._getChangeURLAndSend({
+ method: HttpMethod.POST,
+ changeNum,
+ patchNum,
+ endpoint: '/fix:preview',
+ reportEndpointAsId: true,
+ headers: {Accept: 'application/json'},
+ parseResponse: true,
+ body: {fix_replacement_infos: fixReplacementInfos},
+ }) as Promise<FilePathToDiffInfoMap | undefined>;
+ }
+
getRobotCommentFixPreview(
changeNum: NumericChangeId,
patchNum: PatchSetNum,
@@ -2108,6 +2126,22 @@
applyFixSuggestion(
changeNum: NumericChangeId,
patchNum: PatchSetNum,
+ fixReplacementInfos: FixReplacementInfo[]
+ ): Promise<Response> {
+ return this._getChangeURLAndSend({
+ method: HttpMethod.POST,
+ changeNum,
+ patchNum,
+ endpoint: '/fix:apply',
+ reportEndpointAsId: true,
+ headers: {Accept: 'application/json'},
+ body: {fix_replacement_infos: fixReplacementInfos},
+ });
+ }
+
+ applyRobotFixSuggestion(
+ changeNum: NumericChangeId,
+ patchNum: PatchSetNum,
fixId: string
): Promise<Response> {
return this._getChangeURLAndSend({
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
index 14b3ae5..ad574b6 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
@@ -41,6 +41,7 @@
FileNameToFileInfoMap,
FilePathToDiffInfoMap,
FixId,
+ FixReplacementInfo,
GitRef,
GpgKeyId,
GpgKeyInfo,
@@ -662,15 +663,45 @@
*/
awaitPendingDiffDrafts(): Promise<void>;
+ /**
+ * Preview Stored Fix
+ * Gets the diffs of all files for a certain {fix-id} associated with apply fix.
+ * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#preview-stored-fix
+ */
getRobotCommentFixPreview(
changeNum: NumericChangeId,
patchNum: PatchSetNum,
fixId: FixId
): Promise<FilePathToDiffInfoMap | undefined>;
+ /**
+ * Preview Provided fix
+ * Gets the diffs of all files for a provided fix replacements infos
+ * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#preview-provided-fix
+ */
+ getFixPreview(
+ changeNum: NumericChangeId,
+ patchNum: PatchSetNum,
+ fixReplacementInfos: FixReplacementInfo[]
+ ): Promise<FilePathToDiffInfoMap | undefined>;
+
+ /**
+ * Apply Provided Fix
+ * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#apply-provided-fix
+ */
applyFixSuggestion(
changeNum: NumericChangeId,
patchNum: PatchSetNum,
+ fixReplacementInfos: FixReplacementInfo[]
+ ): Promise<Response>;
+
+ /**
+ * Apply Stored Fix
+ * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#apply-stored-fix
+ */
+ applyRobotFixSuggestion(
+ changeNum: NumericChangeId,
+ patchNum: PatchSetNum,
fixId: string
): Promise<Response>;
diff --git a/polygerrit-ui/app/services/service-worker-installer.ts b/polygerrit-ui/app/services/service-worker-installer.ts
index 6f889e8..3af79a3 100644
--- a/polygerrit-ui/app/services/service-worker-installer.ts
+++ b/polygerrit-ui/app/services/service-worker-installer.ts
@@ -6,6 +6,8 @@
import {FlagsService, KnownExperimentId} from './flags/flags';
import {registerServiceWorker} from '../utils/worker-util';
+import {UserModel} from '../models/user/user-model';
+import {AccountDetailInfo} from '../api/rest-api';
/** Type of incoming messages for ServiceWorker. */
export enum ServiceWorkerMessageType {
@@ -17,7 +19,14 @@
export class ServiceWorkerInstaller {
initialized = false;
- constructor(private readonly flagsService: FlagsService) {}
+ account?: AccountDetailInfo;
+
+ constructor(
+ private readonly flagsService: FlagsService,
+ private readonly userModel: UserModel
+ ) {
+ this.userModel.account$.subscribe(acc => (this.account = acc));
+ }
async init() {
if (this.initialized) return;
@@ -44,6 +53,7 @@
this.startTriggerTimer();
navigator.serviceWorker.controller?.postMessage({
type: ServiceWorkerMessageType.TRIGGER_NOTIFICATIONS,
+ account: this.account,
});
}, TRIGGER_NOTIFICATION_UPDATES_MS);
}
diff --git a/polygerrit-ui/app/services/service-worker-installer_test.ts b/polygerrit-ui/app/services/service-worker-installer_test.ts
index 9de8690..d6b0c02 100644
--- a/polygerrit-ui/app/services/service-worker-installer_test.ts
+++ b/polygerrit-ui/app/services/service-worker-installer_test.ts
@@ -12,8 +12,12 @@
setup(() => {
const flagsService = getAppContext().flagsService;
+ const userModel = getAppContext().userModel;
sinon.stub(flagsService, 'isEnabled').returns(true);
- serviceWorkerInstaller = new ServiceWorkerInstaller(flagsService);
+ serviceWorkerInstaller = new ServiceWorkerInstaller(
+ flagsService,
+ userModel
+ );
});
test('init', () => {
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index 3c4bb62..492bd8d 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -93,6 +93,9 @@
applyFixSuggestion(): Promise<Response> {
return Promise.resolve(new Response());
},
+ applyRobotFixSuggestion(): Promise<Response> {
+ return Promise.resolve(new Response());
+ },
awaitPendingDiffDrafts(): Promise<void> {
return Promise.resolve();
},
@@ -397,6 +400,9 @@
getReviewedFiles(): Promise<string[] | undefined> {
return Promise.resolve([]);
},
+ getFixPreview(): Promise<FilePathToDiffInfoMap | undefined> {
+ return Promise.resolve({});
+ },
getRobotCommentFixPreview(): Promise<FilePathToDiffInfoMap | undefined> {
return Promise.resolve({});
},
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index e7e85c2..81ca830 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -72,6 +72,7 @@
} from '../types/common';
import {
AccountsVisibility,
+ AccountTag,
AppTheme,
AuthType,
ChangeStatus,
@@ -188,6 +189,13 @@
};
}
+export function createServiceUserWithId(id = 5): AccountInfo {
+ return {
+ ...createAccountWithId(id),
+ tags: [AccountTag.SERVICE_USER],
+ };
+}
+
export function createAccountDetailWithId(id = 5): AccountDetailInfo {
return {
_account_id: id as AccountId,
diff --git a/polygerrit-ui/app/tsconfig.json b/polygerrit-ui/app/tsconfig.json
index b799e58..8101b22 100644
--- a/polygerrit-ui/app/tsconfig.json
+++ b/polygerrit-ui/app/tsconfig.json
@@ -46,7 +46,7 @@
"lib": [
"dom",
"dom.iterable",
- "es2019",
+ "es2020",
"webworker"
],
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 77b0305..0e05828 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -115,6 +115,7 @@
WebLinkInfo,
isDetailedLabelInfo,
isQuickLabelInfo,
+ Base64FileContent,
} from '../api/rest-api';
import {DiffInfo, IgnoreWhitespaceType} from './diff';
@@ -128,6 +129,7 @@
ApprovalInfo,
AuthInfo,
AvatarInfo,
+ Base64FileContent,
BasePatchSetNum,
BranchName,
BrandType,
@@ -923,16 +925,6 @@
}
/**
- * Represent a file in a base64 encoding; GrRestApiInterface returns it from some
- * methods
- */
-export interface Base64FileContent {
- content: string | null;
- type: string | null;
- ok: true;
-}
-
-/**
* The WatchedProjectsInfo entity contains information about a project watch for a user
* https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#project-watch-info
*/
diff --git a/polygerrit-ui/app/utils/account-util.ts b/polygerrit-ui/app/utils/account-util.ts
index de2c20b..012dc1c 100644
--- a/polygerrit-ui/app/utils/account-util.ts
+++ b/polygerrit-ui/app/utils/account-util.ts
@@ -19,13 +19,13 @@
} from '../types/common';
import {AccountTag, ReviewerState} from '../constants/constants';
import {assertNever, hasOwnProperty} from './common-util';
-import {AccountAddition} from '../elements/shared/gr-account-list/gr-account-list';
import {getDisplayName} from './display-name-util';
import {getApprovalInfo} from './label-util';
import {RestApiService} from '../services/gr-rest-api/gr-rest-api';
export const ACCOUNT_TEMPLATE_REGEX = '<GERRIT_ACCOUNT_(\\d+)>';
const SUGGESTIONS_LIMIT = 15;
+// https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address
export const MENTIONS_REGEX =
/(?:^|\s)@([a-zA-Z0-9.!#$%&'*+=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*)(?:\s+|$)/g;
@@ -35,30 +35,6 @@
throw new Error('Account has neither _account_id nor email.');
}
-export function mapReviewer(addition: AccountAddition): ReviewerInput {
- if (addition.account) {
- return {reviewer: accountKey(addition.account)};
- }
- if (addition.group) {
- const reviewer = decodeURIComponent(addition.group.id) as GroupId;
- const confirmed = addition.group.confirmed;
- return {reviewer, confirmed};
- }
- throw new Error('Reviewer must be either an account or a group.');
-}
-
-export function isReviewerOrCC(
- change: ChangeInfo,
- reviewerAddition: AccountAddition
-): boolean {
- const reviewers = [
- ...(change.reviewers[ReviewerState.CC] ?? []),
- ...(change.reviewers[ReviewerState.REVIEWER] ?? []),
- ];
- const reviewer = mapReviewer(reviewerAddition);
- return reviewers.some(r => accountOrGroupKey(r) === reviewer.reviewer);
-}
-
export function isServiceUser(account?: AccountInfo): boolean {
return !!account?.tags?.includes(AccountTag.SERVICE_USER);
}
@@ -81,6 +57,18 @@
assertNever(entry, 'entry must be account or group');
}
+export function isAccountNewlyAdded(
+ account: AccountInfo | GroupInfo,
+ state?: ReviewerState,
+ change?: ChangeInfo
+) {
+ if (!change || !state) return false;
+ const accounts = [...(change.reviewers[state] ?? [])];
+ return !accounts.some(
+ a => accountOrGroupKey(a) === accountOrGroupKey(account)
+ );
+}
+
export function uniqueDefinedAvatar(
account: AccountInfo,
index: number,
@@ -213,5 +201,26 @@
*/
export function extractMentionedEmails(text?: string): EmailAddress[] {
if (!text) return [];
- return [...text.matchAll(MENTIONS_REGEX)].map(m => m[1] as EmailAddress);
+ let match;
+ const emails = [];
+ while ((match = MENTIONS_REGEX.exec(text))) {
+ emails.push(match[1] as EmailAddress);
+ }
+ return emails;
+}
+
+export function toReviewInput(
+ account: AccountInfo | GroupInfo,
+ state: ReviewerState
+): ReviewerInput {
+ if (isAccount(account)) {
+ return {
+ reviewer: accountKey(account),
+ state,
+ };
+ } else if (isGroup(account)) {
+ const reviewer = decodeURIComponent(account.id) as GroupId;
+ return {reviewer, state};
+ }
+ throw new Error('Must be either an account or a group.');
}
diff --git a/polygerrit-ui/app/utils/account-util_test.ts b/polygerrit-ui/app/utils/account-util_test.ts
index 5c58d5a..68fa386 100644
--- a/polygerrit-ui/app/utils/account-util_test.ts
+++ b/polygerrit-ui/app/utils/account-util_test.ts
@@ -81,6 +81,9 @@
text = '@abcgoogle.com';
assert.deepEqual(extractMentionedEmails(text), []);
+ text = '@abc@googlecom';
+ assert.deepEqual(extractMentionedEmails(text), ['abc@googlecom']);
+
// with newline before email
text = '\n\n\n random text \n\n@abc@google.com';
assert.deepEqual(extractMentionedEmails(text), ['abc@google.com']);
diff --git a/polygerrit-ui/app/utils/change-util.ts b/polygerrit-ui/app/utils/change-util.ts
index d6d2f7d..cf0ff53 100644
--- a/polygerrit-ui/app/utils/change-util.ts
+++ b/polygerrit-ui/app/utils/change-util.ts
@@ -14,6 +14,7 @@
} from '../types/common';
import {ParsedChangeInfo} from '../types/types';
import {ChangeStates} from '../elements/shared/gr-change-status/gr-change-status';
+import {isServiceUser} from './account-util';
// This can be wrong! See WARNING above
interface ChangeStatusesOptions {
@@ -249,6 +250,11 @@
);
}
+export function hasHumanReviewer(change?: ChangeInfo): boolean {
+ const reviewers = change?.reviewers.REVIEWER ?? [];
+ return reviewers.some(r => !isServiceUser(r));
+}
+
export function isRemovableReviewer(
change?: ChangeInfo,
reviewer?: AccountInfo
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index c5cbe64..c187a50 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -20,6 +20,8 @@
AccountDetailInfo,
ChangeMessageInfo,
VotingRangeInfo,
+ FixSuggestionInfo,
+ FixId,
} from '../types/common';
import {CommentSide, SpecialFilePath} from '../constants/constants';
import {parseDate} from './date-util';
@@ -492,3 +494,55 @@
unsaved: isUnsaved(comment),
};
}
+
+export const USER_SUGGESTION_START_PATTERN = '```suggestion\n';
+export const USER_SUGGEST_EDIT_FIX_ID = 'user_suggestion' as FixId;
+
+export function hasUserSuggestion(comment: Comment) {
+ return comment.message?.includes(USER_SUGGESTION_START_PATTERN) ?? false;
+}
+
+export function getUserSuggestion(comment: Comment) {
+ if (!comment.message) return;
+ const start =
+ comment.message.indexOf(USER_SUGGESTION_START_PATTERN) +
+ USER_SUGGESTION_START_PATTERN.length;
+ const end = comment.message.indexOf('\n```', start);
+ return comment.message.substring(start, end);
+}
+
+/**
+ * Currently it works only on 1 line.
+ * TODO(milutin): Extend for multiline comments
+ */
+export function getContentInCommentRange(
+ fileContent: string,
+ comment: Comment
+) {
+ return fileContent.split('\n')[comment.line! - 1];
+}
+
+export function createUserFixSuggestion(
+ comment: Comment,
+ line: string,
+ replacement: string
+): FixSuggestionInfo[] {
+ return [
+ {
+ fix_id: USER_SUGGEST_EDIT_FIX_ID,
+ description: 'User suggestion',
+ replacements: [
+ {
+ path: comment.path!,
+ range: {
+ start_line: comment.line!,
+ start_character: 0,
+ end_line: comment.line!,
+ end_character: line.length,
+ },
+ replacement,
+ },
+ ],
+ },
+ ];
+}
diff --git a/polygerrit-ui/app/utils/comment-util_test.ts b/polygerrit-ui/app/utils/comment-util_test.ts
index bb9c455..dd57042 100644
--- a/polygerrit-ui/app/utils/comment-util_test.ts
+++ b/polygerrit-ui/app/utils/comment-util_test.ts
@@ -9,6 +9,12 @@
getPatchRangeForCommentUrl,
createCommentThreads,
sortComments,
+ USER_SUGGESTION_START_PATTERN,
+ hasUserSuggestion,
+ getUserSuggestion,
+ getContentInCommentRange,
+ createUserFixSuggestion,
+ USER_SUGGEST_EDIT_FIX_ID,
} from './comment-util';
import {createComment, createCommentThread} from '../test/test-data-generators';
import {CommentSide} from '../constants/constants';
@@ -223,4 +229,58 @@
assert.equal(createCommentThreads(comments).length, 2);
});
});
+
+ test('hasUserSuggestion', () => {
+ const comment = {
+ ...createComment(),
+ message: `${USER_SUGGESTION_START_PATTERN}${'test'}${'\n```'}`,
+ };
+ assert.isTrue(hasUserSuggestion(comment));
+ });
+
+ test('getUserSuggestion', () => {
+ const suggestion = 'test';
+ const comment = {
+ ...createComment(),
+ message: `${USER_SUGGESTION_START_PATTERN}${suggestion}${'\n```'}`,
+ };
+ assert.equal(getUserSuggestion(comment), suggestion);
+ });
+
+ test('getContentInCommentRange', () => {
+ const comment = {
+ ...createComment(),
+ line: 1,
+ };
+ const content = 'line1\nline2\nline3';
+ assert.equal(getContentInCommentRange(content, comment), 'line1');
+ });
+
+ test('createUserFixSuggestion', () => {
+ const comment = {
+ ...createComment(),
+ line: 1,
+ path: 'abc.txt',
+ };
+ const line = 'lane1';
+ const replacement = 'line1';
+ assert.deepEqual(createUserFixSuggestion(comment, line, replacement), [
+ {
+ fix_id: USER_SUGGEST_EDIT_FIX_ID,
+ description: 'User suggestion',
+ replacements: [
+ {
+ path: 'abc.txt',
+ range: {
+ start_line: 1,
+ start_character: 0,
+ end_line: 1,
+ end_character: line.length,
+ },
+ replacement,
+ },
+ ],
+ },
+ ]);
+ });
});
diff --git a/polygerrit-ui/app/utils/common-util.ts b/polygerrit-ui/app/utils/common-util.ts
index fe86d91..0bae49a 100644
--- a/polygerrit-ui/app/utils/common-util.ts
+++ b/polygerrit-ui/app/utils/common-util.ts
@@ -38,7 +38,7 @@
/**
* Throws an error with the provided error message if the condition is false.
*/
-export function check(
+export function assert(
condition: boolean,
errorMessage: string
): asserts condition {
@@ -48,28 +48,6 @@
/**
* Throws an error if the property is not defined.
*/
-export function checkProperty(
- condition: boolean,
- propertyName: string
-): asserts condition {
- check(condition, `missing required property '${propertyName}'`);
-}
-
-/**
- * Throws an error if the property is not defined.
- */
-export function checkRequiredProperty<T>(
- property: T,
- propertyName: string
-): asserts property is NonNullable<T> {
- if (property === undefined || property === null) {
- throw new Error(`Required property '${propertyName}' not set.`);
- }
-}
-
-/**
- * Throws an error if the property is not defined.
- */
export function assertIsDefined<T>(
val: T,
variableName = 'variable'
@@ -168,3 +146,14 @@
result.filter(t => array.find(u => compareBy(t, u)))
);
}
+
+/**
+ * Returns the elements that are present in A but not present in B.
+ */
+export function difference<T>(
+ a: T[],
+ b: T[],
+ compareBy: (t: T, u: T) => boolean = (t, u) => t === u
+): T[] {
+ return a.filter(aVal => !b.some(bVal => compareBy(aVal, bVal)));
+}
diff --git a/polygerrit-ui/app/utils/common-util_test.ts b/polygerrit-ui/app/utils/common-util_test.ts
index e07401b..c9ba060 100644
--- a/polygerrit-ui/app/utils/common-util_test.ts
+++ b/polygerrit-ui/app/utils/common-util_test.ts
@@ -9,6 +9,7 @@
areSetsEqual,
containsAll,
intersection,
+ difference,
} from './common-util';
suite('common-util tests', () => {
@@ -88,4 +89,11 @@
[foo1]
);
});
+
+ test('difference', () => {
+ assert.deepEqual(difference([1, 2, 3], []), [1, 2, 3]);
+ assert.deepEqual(difference([1, 2, 3], [2, 3, 4]), [1]);
+ assert.deepEqual(difference([1, 2, 3], [1, 2, 3]), []);
+ assert.deepEqual(difference([1, 2, 3], [4, 5, 6]), [1, 2, 3]);
+ });
});
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index f8ba4c3..515f2e7 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -9,7 +9,7 @@
RevisionPatchSetNum,
} from '../types/common';
import {EditRevisionInfo, ParsedChangeInfo} from '../types/types';
-import {check} from './common-util';
+import {assert} from './common-util';
/**
* @license
@@ -261,7 +261,7 @@
if (latest === EDIT) {
latest = allPatchSets[1].num;
}
- check(isNumber(latest), 'Latest patchset cannot be EDIT or PARENT.');
+ assert(isNumber(latest), 'Latest patchset cannot be EDIT or PARENT.');
return latest;
}
diff --git a/polygerrit-ui/app/utils/service-worker-util.ts b/polygerrit-ui/app/utils/service-worker-util.ts
new file mode 100644
index 0000000..3cf0dce
--- /dev/null
+++ b/polygerrit-ui/app/utils/service-worker-util.ts
@@ -0,0 +1,29 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import {AccountDetailInfo} from '../api/rest-api';
+import {ParsedChangeInfo} from '../types/types';
+import {parseDate} from './date-util';
+
+/**
+ * Filter changes that had change in attention set after last round
+ * of notifications. Filter out changes we already notified about.
+ */
+export function filterAttentionChangesAfter(
+ changes: ParsedChangeInfo[],
+ account: AccountDetailInfo,
+ latestUpdateTimestampMs?: number
+) {
+ if (!latestUpdateTimestampMs) return changes;
+ return changes.filter(change => {
+ const attention_set = change.attention_set![account._account_id!];
+ if (!attention_set.last_update) return false;
+ const lastUpdateTimestampMs = parseDate(
+ attention_set.last_update
+ ).valueOf();
+ return latestUpdateTimestampMs < lastUpdateTimestampMs;
+ });
+}
diff --git a/polygerrit-ui/app/utils/service-worker-util_test.ts b/polygerrit-ui/app/utils/service-worker-util_test.ts
new file mode 100644
index 0000000..0caea3b
--- /dev/null
+++ b/polygerrit-ui/app/utils/service-worker-util_test.ts
@@ -0,0 +1,48 @@
+/**
+ * @license
+ * Copyright 2017 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import {Timestamp} from '../api/rest-api';
+import '../test/common-test-setup-karma';
+import {
+ createAccountDetailWithId,
+ createParsedChange,
+} from '../test/test-data-generators';
+import {ParsedChangeInfo} from '../types/types';
+import {parseDate} from './date-util';
+import {filterAttentionChangesAfter} from './service-worker-util';
+
+suite('service worker util tests', () => {
+ test('filterAttentionChangesAfter', () => {
+ const account = createAccountDetailWithId();
+ const changeBefore: ParsedChangeInfo = {
+ ...createParsedChange(),
+ attention_set: {
+ [`${account._account_id}`]: {
+ account,
+ last_update: '2016-01-12 20:24:49.000000000' as Timestamp,
+ },
+ },
+ };
+ const changeAfter: ParsedChangeInfo = {
+ ...createParsedChange(),
+ attention_set: {
+ [`${account._account_id}`]: {
+ account,
+ last_update: '2016-01-12 20:24:51.000000000' as Timestamp,
+ },
+ },
+ };
+ const changes = [changeBefore, changeAfter];
+
+ const filteredChanges = filterAttentionChangesAfter(
+ changes,
+ account,
+ parseDate('2016-01-12 20:24:50.000000000' as Timestamp).valueOf()
+ );
+
+ assert.equal(filteredChanges.length, 1);
+ assert.equal(filteredChanges[0], changeAfter);
+ });
+});
diff --git a/polygerrit-ui/app/workers/service-worker-class.ts b/polygerrit-ui/app/workers/service-worker-class.ts
new file mode 100644
index 0000000..966b6b0
--- /dev/null
+++ b/polygerrit-ui/app/workers/service-worker-class.ts
@@ -0,0 +1,56 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import {ParsedChangeInfo} from '../types/types';
+import {getReason} from '../utils/attention-set-util';
+import {readResponsePayload} from '../elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
+import {filterAttentionChangesAfter} from '../utils/service-worker-util';
+import {AccountDetailInfo} from '../api/rest-api';
+import {TRIGGER_NOTIFICATION_UPDATES_MS} from '../services/service-worker-installer';
+
+export class ServiceWorker {
+ constructor(private ctx: ServiceWorkerGlobalScope) {}
+
+ latestUpdateTimestampMs?: number;
+
+ showNotification(change: ParsedChangeInfo, account: AccountDetailInfo) {
+ const body = getReason(undefined, account, change);
+ // TODO(milutin): Implement event.action that
+ // focus on firstWindowClient and open change there.
+ // TODO(milutin): Add gerrit host icon
+ this.ctx.registration.showNotification(change.subject, {body});
+ }
+
+ async getChangesToNotify(account: AccountDetailInfo) {
+ // We throttle polling, since there can be many clients triggerring
+ // always only one service worker.
+ if (this.latestUpdateTimestampMs) {
+ const durationFromLatestUpdateMS =
+ Date.now() - this.latestUpdateTimestampMs;
+ if (durationFromLatestUpdateMS < TRIGGER_NOTIFICATION_UPDATES_MS) {
+ return [];
+ }
+ }
+ const changes = await this.getLatestAttentionSetChanges();
+ const latestAttentionChanges = filterAttentionChangesAfter(
+ changes,
+ account,
+ this.latestUpdateTimestampMs
+ );
+ this.latestUpdateTimestampMs = Date.now();
+ return latestAttentionChanges;
+ }
+
+ async getLatestAttentionSetChanges(): Promise<ParsedChangeInfo[]> {
+ // TODO(milutin): Implement more generic query builder
+ const response = await fetch(
+ '/changes/?O=1000081&S=0&n=25&q=attention%3Aself'
+ );
+ const payload = await readResponsePayload(response);
+ const changes = payload.parsed as unknown as ParsedChangeInfo[] | undefined;
+ return changes ?? [];
+ }
+}
diff --git a/polygerrit-ui/app/workers/service-worker-class_test.ts b/polygerrit-ui/app/workers/service-worker-class_test.ts
new file mode 100644
index 0000000..ecfef5e
--- /dev/null
+++ b/polygerrit-ui/app/workers/service-worker-class_test.ts
@@ -0,0 +1,50 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import {Timestamp} from '../api/rest-api';
+import '../test/common-test-setup-karma';
+import {
+ createAccountDetailWithId,
+ createParsedChange,
+} from '../test/test-data-generators';
+import {ParsedChangeInfo} from '../types/types';
+import {parseDate} from '../utils/date-util';
+import {ServiceWorker} from './service-worker-class';
+
+suite('service worker class tests', () => {
+ let serviceWorker: ServiceWorker;
+
+ setup(() => {
+ const moctCtx = {
+ registration: {
+ showNotification: () => {},
+ },
+ } as {} as ServiceWorkerGlobalScope;
+ serviceWorker = new ServiceWorker(moctCtx);
+ });
+
+ test('notify about attention in t2 when time is t3', async () => {
+ const t1 = parseDate('2016-01-12 20:20:00' as Timestamp).getTime();
+ const t2 = '2016-01-12 20:30:00' as Timestamp;
+ const t3 = parseDate('2016-01-12 20:40:00' as Timestamp).getTime();
+ serviceWorker.latestUpdateTimestampMs = t1;
+ const account = createAccountDetailWithId();
+ const change: ParsedChangeInfo = {
+ ...createParsedChange(),
+ attention_set: {
+ [`${account._account_id}`]: {
+ account,
+ last_update: t2,
+ },
+ },
+ };
+ sinon.useFakeTimers(t3);
+ sinon
+ .stub(serviceWorker, 'getLatestAttentionSetChanges')
+ .returns(Promise.resolve([change]));
+ const changes = await serviceWorker.getChangesToNotify(account);
+ assert.equal(changes[0], change);
+ });
+});
diff --git a/polygerrit-ui/app/workers/service-worker.ts b/polygerrit-ui/app/workers/service-worker.ts
index e219946..ca1f5dc 100644
--- a/polygerrit-ui/app/workers/service-worker.ts
+++ b/polygerrit-ui/app/workers/service-worker.ts
@@ -4,12 +4,9 @@
* SPDX-License-Identifier: Apache-2.0
*/
-import {readResponsePayload} from '../elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
-import {
- ServiceWorkerMessageType,
- TRIGGER_NOTIFICATION_UPDATES_MS,
-} from '../services/service-worker-installer';
-import {ParsedChangeInfo} from '../types/types';
+import {AccountDetailInfo} from '../api/rest-api';
+import {ServiceWorkerMessageType} from '../services/service-worker-installer';
+import {ServiceWorker} from './service-worker-class';
/**
* `self` is for a worker what `window` is for the web app. It is called
@@ -20,49 +17,20 @@
ctx.addEventListener('message', async event => {
if (event.data?.type !== ServiceWorkerMessageType.TRIGGER_NOTIFICATIONS) {
- throw new Error(`Unsupported event type: ${event.data?.type}`);
+ // Only this notification message type exists, but we do not throw error.
+ return;
}
- const changes = await serviceWorker.getLatestAttentionSetChange();
+ const account = event.data?.account as AccountDetailInfo | undefined;
+ // Message always contains account, but we do not throw error.
+ if (!account?._account_id) return;
+ const latestAttentionChanges = await serviceWorker.getChangesToNotify(
+ account
+ );
// TODO(milutin): Implement handling more than 1 change
- if (changes && changes.length > 0) {
- serviceWorker.showNotification(changes[0]);
+ if (latestAttentionChanges && latestAttentionChanges.length > 0) {
+ serviceWorker.showNotification(latestAttentionChanges[0], account);
}
});
-class ServiceWorker {
- latestUpdateTimestampMs?: number;
-
- showNotification(change: ParsedChangeInfo) {
- // TODO(milutin): Replace with getReason from attention-set-util.
- // For get Reason you will need AccountInfo.
- const body = 'Administrator replied on the change';
- // TODO(milutin): Implement event.action that
- // focus on firstWindowClient and open change there.
- // TODO(milutin): Add gerrit host icon
- ctx.registration.showNotification(change.subject, {body});
- }
-
- async getLatestAttentionSetChange(): Promise<ParsedChangeInfo[]> {
- // We throttle polling, since there can be many clients triggerring
- // always only one service worker.
- if (this.latestUpdateTimestampMs) {
- const durationFromLatestUpdateMS =
- Date.now() - this.latestUpdateTimestampMs;
- if (durationFromLatestUpdateMS < TRIGGER_NOTIFICATION_UPDATES_MS) {
- return [];
- }
- }
- this.latestUpdateTimestampMs = Date.now();
- // TODO(milutin): Implement more generic query builder
- const response = await fetch(
- '/changes/?O=1000081&S=0&n=25&q=attention%3Aself'
- );
- const payload = await readResponsePayload(response);
- const changes = payload.parsed as unknown as ParsedChangeInfo[] | undefined;
- // TODO(milutin): Filter changes you are already notified about.
- return changes ?? [];
- }
-}
-
/** Singleton instance being referenced in `onmessage` function above. */
-const serviceWorker = new ServiceWorker();
+const serviceWorker = new ServiceWorker(ctx);
