From 6de52dc9951f00949150594cb04b318efc3d2560 Mon Sep 17 00:00:00 2001 From: Tomas Vik Date: Mon, 20 Jun 2022 10:51:31 +0000 Subject: [PATCH] Revert "Merge branch 'approvers-view' into 'main'" This reverts merge request !996 --- commands/mr/approvers/mr_approvers.go | 50 +++++++-- commands/mr/mrutils/mrutils.go | 50 +-------- commands/mr/mrutils/mrutils_test.go | 151 -------------------------- commands/mr/view/mr_view.go | 16 +-- 4 files changed, 49 insertions(+), 218 deletions(-) diff --git a/commands/mr/approvers/mr_approvers.go b/commands/mr/approvers/mr_approvers.go index 4ab7382cf..70f836413 100644 --- a/commands/mr/approvers/mr_approvers.go +++ b/commands/mr/approvers/mr_approvers.go @@ -4,27 +4,29 @@ import ( "fmt" "github.com/profclems/glab/api" - "github.com/profclems/glab/commands/cmdutils" "github.com/profclems/glab/commands/mr/mrutils" + "github.com/profclems/glab/pkg/tableprinter" + + "github.com/profclems/glab/commands/cmdutils" "github.com/spf13/cobra" + "github.com/xanzy/go-gitlab" ) func NewCmdApprovers(f *cmdutils.Factory) *cobra.Command { var mrApproversCmd = &cobra.Command{ Use: "approvers [ | ] [flags]", - Short: `List eligible approvers for merge requests in any state`, + Short: `List merge request eligible approvers`, Long: ``, Aliases: []string{}, - Args: cobra.MaximumNArgs(1), + Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + c := f.IO.Color() apiClient, err := f.HttpClient() if err != nil { return err } - // Obtain the MR from the positional arguments, but allow users to find approvers for - // merge requests in any valid state - mr, repo, err := mrutils.MRFromArgs(f, args, "any") + mr, repo, err := mrutils.MRFromArgs(f, args, "opened") if err != nil { return err } @@ -35,9 +37,43 @@ func NewCmdApprovers(f *cmdutils.Factory) *cobra.Command { if err != nil { return err } + if mrApprovals.ApprovalRulesOverwritten { + fmt.Fprintln(f.IO.StdOut, c.Yellow("Approval rules overwritten")) + } + for _, rule := range mrApprovals.Rules { + table := tableprinter.NewTablePrinter() + if rule.Approved { + fmt.Fprintln(f.IO.StdOut, c.Green(fmt.Sprintf("Rule %q sufficient approvals (%d/%d required):", rule.Name, len(rule.ApprovedBy), rule.ApprovalsRequired))) + } else { + fmt.Fprintln(f.IO.StdOut, c.Yellow(fmt.Sprintf("Rule %q insufficient approvals (%d/%d required):", rule.Name, len(rule.ApprovedBy), rule.ApprovalsRequired))) + } - mrutils.PrintMRApprovalState(f.IO, mrApprovals) + eligibleApprovers := rule.EligibleApprovers + approvedBy := map[string]*gitlab.BasicUser{} + for _, by := range rule.ApprovedBy { + approvedBy[by.Username] = by + } + + for _, eligibleApprover := range eligibleApprovers { + approved := "-" + source := "" + if _, exists := approvedBy[eligibleApprover.Username]; exists { + approved = "👍" + } + if rule.SourceRule != nil { + source = rule.SourceRule.RuleType + } + table.AddRow(eligibleApprover.Name, eligibleApprover.Username, approved, source) + delete(approvedBy, eligibleApprover.Username) + } + + for _, approver := range approvedBy { + approved := "👍" + table.AddRow(approver.Name, approver.Username, approved, "") + } + fmt.Fprintln(f.IO.StdOut, table) + } return nil }, } diff --git a/commands/mr/mrutils/mrutils.go b/commands/mr/mrutils/mrutils.go index c29a773b1..04ef79db4 100644 --- a/commands/mr/mrutils/mrutils.go +++ b/commands/mr/mrutils/mrutils.go @@ -117,12 +117,12 @@ func DisplayAllMRs(streams *iostreams.IOStreams, mrs []*gitlab.MergeRequest, pro return table.Render() } -// MRFromArgs is wrapper around MRFromArgsWithOpts without any custom options +//MRFromArgs is wrapper around MRFromArgsWithOpts without any custom options func MRFromArgs(f *cmdutils.Factory, args []string, state string) (*gitlab.MergeRequest, glrepo.Interface, error) { return MRFromArgsWithOpts(f, args, &gitlab.GetMergeRequestsOptions{}, state) } -// MRFromArgsWithOpts gets MR with custom request options passed down to it +//MRFromArgsWithOpts gets MR with custom request options passed down to it func MRFromArgsWithOpts( f *cmdutils.Factory, args []string, @@ -216,6 +216,7 @@ func MRsFromArgs(f *cmdutils.Factory, args []string, state string) ([]*gitlab.Me return nil, nil, err } return mrs, baseRepo, nil + } var getMRForBranch = func(apiClient *gitlab.Client, baseRepo glrepo.Interface, arg string, state string) (*gitlab.MergeRequest, error) { @@ -327,48 +328,3 @@ func RebaseMR(ios *iostreams.IOStreams, apiClient *gitlab.Client, repo glrepo.In fmt.Fprintln(ios.StdOut, ios.Color().GreenCheck(), "Rebase successful") return nil } - -// PrintMRApprovalState renders an output to summarize the approval state of a merge request -func PrintMRApprovalState(ios *iostreams.IOStreams, mrApprovals *gitlab.MergeRequestApprovalState) { - const approvedIcon = "👍" - - c := ios.Color() - - if mrApprovals.ApprovalRulesOverwritten { - fmt.Fprintln(ios.StdOut, c.Yellow("Approval rules overwritten")) - } - for _, rule := range mrApprovals.Rules { - table := tableprinter.NewTablePrinter() - if rule.Approved { - fmt.Fprintln(ios.StdOut, c.Green(fmt.Sprintf("Rule %q sufficient approvals (%d/%d required):", rule.Name, len(rule.ApprovedBy), rule.ApprovalsRequired))) - } else { - fmt.Fprintln(ios.StdOut, c.Yellow(fmt.Sprintf("Rule %q insufficient approvals (%d/%d required):", rule.Name, len(rule.ApprovedBy), rule.ApprovalsRequired))) - } - - eligibleApprovers := rule.EligibleApprovers - - approvedBy := map[string]*gitlab.BasicUser{} - for _, by := range rule.ApprovedBy { - approvedBy[by.Username] = by - } - - for _, eligibleApprover := range eligibleApprovers { - approved := "-" - source := "" - if _, exists := approvedBy[eligibleApprover.Username]; exists { - approved = approvedIcon - } - if rule.SourceRule != nil { - source = rule.SourceRule.RuleType - } - table.AddRow(eligibleApprover.Name, eligibleApprover.Username, approved, source) - delete(approvedBy, eligibleApprover.Username) - } - - for _, approver := range approvedBy { - approved := approvedIcon - table.AddRow(approver.Name, approver.Username, approved, "") - } - fmt.Fprintln(ios.StdOut, table) - } -} diff --git a/commands/mr/mrutils/mrutils_test.go b/commands/mr/mrutils/mrutils_test.go index cc33c3d43..6c2daa826 100644 --- a/commands/mr/mrutils/mrutils_test.go +++ b/commands/mr/mrutils/mrutils_test.go @@ -543,154 +543,3 @@ func Test_DisplayAllMRs(t *testing.T) { got := DisplayAllMRs(streams, mrs, "unused") assert.Equal(t, expected, got) } - -func Test_PrintMRApprovalState(t *testing.T) { - scenarios := []struct { - name string - approvalState *gitlab.MergeRequestApprovalState - expected string - }{ - { - name: "approved, min approvers met", - approvalState: &gitlab.MergeRequestApprovalState{ - Rules: []*gitlab.MergeRequestApprovalRule{ - { - ID: 1, - Name: "rule 1", - ApprovalsRequired: 2, - ApprovedBy: []*gitlab.BasicUser{ - { - ID: 1, - Username: "user1", - Name: "User One", - }, - { - ID: 2, - Username: "user2", - Name: "User Two", - }, - }, - Approved: true, - }, - }, - }, - expected: `Rule "rule 1" sufficient approvals (2/2 required): -User One user1 👍 -User Two user2 👍 - -`, - }, - { - name: "not-approved, min approvers not met", - approvalState: &gitlab.MergeRequestApprovalState{ - Rules: []*gitlab.MergeRequestApprovalRule{ - { - ID: 1, - Name: "rule 1", - ApprovalsRequired: 2, - ApprovedBy: []*gitlab.BasicUser{ - { - ID: 1, - Username: "user1", - Name: "User One", - }, - }, - Approved: false, - }, - }, - }, - expected: `Rule "rule 1" insufficient approvals (1/2 required): -User One user1 👍 - -`, - }, - { - name: "approved, eligible approvers", - approvalState: &gitlab.MergeRequestApprovalState{ - Rules: []*gitlab.MergeRequestApprovalRule{ - { - ID: 1, - Name: "rule 1", - ApprovalsRequired: 2, - EligibleApprovers: []*gitlab.BasicUser{ - { - ID: 1, - Username: "user1", - Name: "User One", - }, - { - ID: 2, - Username: "user2", - Name: "User Two", - }, - }, - ApprovedBy: []*gitlab.BasicUser{ - { - ID: 1, - Username: "user1", - Name: "User One", - }, - { - ID: 2, - Username: "user2", - Name: "User Two", - }, - }, - Approved: true, - }, - }, - }, - expected: `Rule "rule 1" sufficient approvals (2/2 required): -User One user1 👍 -User Two user2 👍 - -`, - }, - { - name: "not approved, missing eligible approver", - approvalState: &gitlab.MergeRequestApprovalState{ - Rules: []*gitlab.MergeRequestApprovalRule{ - { - ID: 1, - Name: "rule 1", - ApprovalsRequired: 2, - EligibleApprovers: []*gitlab.BasicUser{ - { - ID: 1, - Username: "user1", - Name: "User One", - }, - { - ID: 2, - Username: "user2", - Name: "User Two", - }, - }, - ApprovedBy: []*gitlab.BasicUser{ - { - ID: 1, - Username: "user1", - Name: "User One", - }, - }, - Approved: false, - }, - }, - }, - expected: `Rule "rule 1" insufficient approvals (1/2 required): -User One user1 👍 -User Two user2 - - -`, - }, - } - - for _, scenario := range scenarios { - t.Run(scenario.name, func(t *testing.T) { - streams, _, stdout, _ := iostreams.Test() - - PrintMRApprovalState(streams, scenario.approvalState) - assert.Equal(t, scenario.expected, stdout.String()) - }) - } -} diff --git a/commands/mr/view/mr_view.go b/commands/mr/view/mr_view.go index 0f68b7591..2b0ac54fe 100644 --- a/commands/mr/view/mr_view.go +++ b/commands/mr/view/mr_view.go @@ -51,15 +51,9 @@ func NewCmdView(f *cmdutils.Factory) *cobra.Command { if err != nil { return err } - - mrApprovals, err := api.GetMRApprovalState(apiClient, baseRepo.FullName(), mr.IID) - if err != nil { - return err - } - cfg, _ := f.Config() - if opts.OpenInBrowser { // open in browser if --web flag is specified + if opts.OpenInBrowser { //open in browser if --web flag is specified if f.IO.IsOutputTTY() { fmt.Fprintf(f.IO.StdErr, "Opening %s in your browser.\n", utils.DisplayURL(mr.WebURL)) } @@ -91,7 +85,7 @@ func NewCmdView(f *cmdutils.Factory) *cobra.Command { defer f.IO.StopPager() if f.IO.IsOutputTTY() { - return printTTYMRPreview(opts, mr, mrApprovals, notes) + return printTTYMRPreview(opts, mr, notes) } return printRawMRPreview(opts, mr) }, @@ -142,7 +136,7 @@ func mrState(c *iostreams.ColorPalette, mr *gitlab.MergeRequest) (mrState string return mrState } -func printTTYMRPreview(opts *ViewOpts, mr *gitlab.MergeRequest, mrApprovals *gitlab.MergeRequestApprovalState, notes []*gitlab.Note) error { +func printTTYMRPreview(opts *ViewOpts, mr *gitlab.MergeRequest, notes []*gitlab.Note) error { c := opts.IO.Color() out := opts.IO.StdOut mrTimeAgo := utils.TimeToPrettyTimeAgo(*mr.CreatedAt) @@ -198,10 +192,6 @@ func printTTYMRPreview(opts *ViewOpts, mr *gitlab.MergeRequest, mrApprovals *git fmt.Fprintf(out, "%s Requires pipeline to succeed before merging\n", c.WarnIcon()) } } - if mrApprovals != nil { - fmt.Fprintln(out, c.Bold("Approvals Status:")) - mrutils.PrintMRApprovalState(opts.IO, mrApprovals) - } fmt.Fprintf(out, "%s This merge request has %s changes\n", c.GreenCheck(), c.Yellow(mr.ChangesCount)) if mr.State == "merged" && mr.MergedBy != nil { fmt.Fprintf(out, "%s The changes were merged into %s by %s %s\n", c.GreenCheck(), mr.TargetBranch, mr.MergedBy.Name, utils.TimeToPrettyTimeAgo(*mr.MergedAt)) -- GitLab