Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinguish official vs non-official reviews, add tool tips, and upgr… #31924

Merged
merged 11 commits into from
Sep 6, 2024
Merged
25 changes: 25 additions & 0 deletions models/issues/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,13 @@ func (r *Review) LoadAttributes(ctx context.Context) (err error) {
return err
}

// HTMLTypeColorName returns the color used in the ui indicating the review
func (r *Review) HTMLTypeColorName() string {
switch r.Type {
case ReviewTypeApprove:
if !r.Official {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I made the decision to prioritize the unofficial vs official indicator over stale. So an unofficial reviewer will not see a stale indication. To me, this made the most sense since seeing why a review doesn't count towards a protected branches approval limit felt more pertinent then seeing it was stale.

Let me know if you disagree with this logic.

return "grey"
}
if r.Stale {
return "yellow"
}
Expand All @@ -231,6 +235,27 @@ func (r *Review) HTMLTypeColorName() string {
return "grey"
}

// TooltipContent returns the locale string describing the review type
func (r *Review) TooltipContent() string {
switch r.Type {
case ReviewTypeApprove:
if r.Stale {
return "repo.issues.review.stale"
}
if !r.Official {
return "repo.issues.review.unofficial"
}
return "repo.issues.review.official"
case ReviewTypeComment:
return "repo.issues.review.comment"
case ReviewTypeReject:
return "repo.issues.review.rejected"
case ReviewTypeRequest:
return "repo.issues.review.requested"
}
return ""
}

// GetReviewByID returns the review by the given ID
func GetReviewByID(ctx context.Context, id int64) (*Review, error) {
review := new(Review)
Expand Down
11 changes: 9 additions & 2 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1752,6 +1752,12 @@ issues.review.hide_resolved = Hide resolved
issues.review.resolve_conversation = Resolve conversation
issues.review.un_resolve_conversation = Unresolve conversation
issues.review.resolved_by = marked this conversation as resolved
issues.review.comment = Comment
issues.review.official = Approved
issues.review.requested = Review pending
issues.review.rejected = Changes requested
issues.review.stale = Updated since approval
issues.review.unofficial = Uncounted approval
issues.assignee.error = Not all assignees was added due to an unexpected error.
issues.reference_issue.body = Body
issues.content_history.deleted = deleted
Expand Down Expand Up @@ -1825,7 +1831,8 @@ pulls.is_empty = "The changes on this branch are already on the target branch. T
pulls.required_status_check_failed = Some required checks were not successful.
pulls.required_status_check_missing = Some required checks are missing.
pulls.required_status_check_administrator = As an administrator, you may still merge this pull request.
pulls.blocked_by_approvals = "This pull request doesn't have enough approvals yet. %d of %d approvals granted."
pulls.blocked_by_approvals = "This pull request doesn't have enough required approvals yet. %d of %d official approvals granted."
pulls.blocked_by_approvals_whitelisted = "This pull request doesn't have enough required approvals yet. %d of %d approvals granted from users or teams on the allowlist."
pulls.blocked_by_rejection = "This pull request has changes requested by an official reviewer."
pulls.blocked_by_official_review_requests = "This pull request has official review requests."
pulls.blocked_by_outdated_branch = "This pull request is blocked because it's outdated."
Expand Down Expand Up @@ -2413,7 +2420,7 @@ settings.protect_status_check_matched = Matched
settings.protect_invalid_status_check_pattern = Invalid status check pattern: "%s".
settings.protect_no_valid_status_check_patterns = No valid status check patterns.
settings.protect_required_approvals = Required approvals:
settings.protect_required_approvals_desc = Allow only to merge pull request with enough positive reviews.
settings.protect_required_approvals_desc = Allow only to merge pull request with enough required approvals. Required approvals are either from users or teams who are on the allowlist or anyone with write access.
settings.protect_approvals_whitelist_enabled = Restrict approvals to allowlisted users or teams
settings.protect_approvals_whitelist_enabled_desc = Only reviews from allowlisted users or teams will count to the required approvals. Without approval allowlist, reviews from anyone with write access count to the required approvals.
settings.protect_approvals_whitelist_users = Allowlisted reviewers:
Expand Down
1 change: 1 addition & 0 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1940,6 +1940,7 @@ func ViewIssue(ctx *context.Context) {
ctx.Data["ChangedProtectedFiles"] = pull.ChangedProtectedFiles
ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0
ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles)
ctx.Data["RequireApprovalsWhitelist"] = pb.EnableApprovalsWhitelist
}
ctx.Data["WillSign"] = false
if ctx.Doer != nil {
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/issue/view_content/comments.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@
{{ctx.AvatarUtils.Avatar .Poster 40}}
</a>
{{end}}
<span class="badge{{if eq $reviewType 1}} tw-bg-green tw-text-white{{else if eq $reviewType 3}} tw-bg-red tw-text-white{{end}}">
<span class="badge tw-text-white{{if eq $reviewType 1}}{{if .Review.Official}} tw-bg-green {{else}} tw-bg-grey{{end}}{{else if eq $reviewType 3}} tw-bg-red{{end}}">
{{if .Review}}{{svg (printf "octicon-%s" .Review.Type.Icon)}}{{end}}
</span>
<span class="text grey muted-links">
Expand Down
6 changes: 5 additions & 1 deletion templates/repo/issue/view_content/pull.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,11 @@
{{if .IsBlockedByApprovals}}
<div class="item">
{{svg "octicon-x"}}
{{ctx.Locale.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .ProtectedBranch.RequiredApprovals}}
{{if .RequireApprovalsWhitelist}}
{{ctx.Locale.Tr "repo.pulls.blocked_by_approvals_whitelisted" .GrantedApprovals .ProtectedBranch.RequiredApprovals}}
{{else}}
{{ctx.Locale.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .ProtectedBranch.RequiredApprovals}}
{{end}}
</div>
{{else if .IsBlockedByRejection}}
<div class="item">
Expand Down
8 changes: 6 additions & 2 deletions templates/repo/issue/view_content/sidebar.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@
{{if and .CanChange (or .Checked (and (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged)))}}
<a href="#" class="ui muted icon re-request-review{{if .Checked}} checked{{end}}" data-tooltip-content="{{if .Checked}}{{ctx.Locale.Tr "repo.issues.remove_request_review"}}{{else}}{{ctx.Locale.Tr "repo.issues.re_request_review"}}{{end}}" data-issue-id="{{$.Issue.ID}}" data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">{{svg (Iif .Checked "octicon-trash" "octicon-sync")}}</a>
{{end}}
{{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}}
<span {{if .Review.TooltipContent}}data-tooltip-content="{{ctx.Locale.Tr .Review.TooltipContent}}"{{end}}>
{{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}}
</span>
</div>
</div>
{{end}}
Expand All @@ -107,7 +109,9 @@
</a>
</div>
<div class="tw-flex tw-items-center tw-gap-2">
{{svg (printf "octicon-%s" .Type.Icon) 16 (printf "text %s" (.HTMLTypeColorName))}}
<span {{if .Review.TooltipContent}}data-tooltip-content="{{ctx.Locale.Tr .Review.TooltipContent}}"{{end}}>
{{svg (printf "octicon-%s" .Type.Icon) 16 (printf "text %s" (.HTMLTypeColorName))}}
</span>
</div>
</div>
{{end}}
Expand Down