Skip to content

Commit 6bdc556

Browse files
lunnylunny
authored
Fix some webhooks bugs (#3981)
* fix some webhooks bugs * update vendor Signed-off-by: Bo-Yi Wu <[email protected]> * fix test * fix clearlabels * fix pullrequest webhook bug fix #3492 * update release webhook description * remove unused code * fix push webhook in pull request * small changes
1 parent dc0ef38 commit 6bdc556

16 files changed

+277
-45
lines changed

‎models/issue.go

+93-8
Original file line numberDiff line numberDiff line change
@@ -370,11 +370,19 @@ func (issue *Issue) HasLabel(labelID int64) bool {
370370

371371
func (issue *Issue) sendLabelUpdatedWebhook(doer *User) {
372372
var err error
373+
374+
if err = issue.loadRepo(x); err != nil {
375+
log.Error(4, "loadRepo: %v", err)
376+
return
377+
}
378+
379+
if err = issue.loadPoster(x); err != nil {
380+
log.Error(4, "loadPoster: %v", err)
381+
return
382+
}
383+
384+
mode, _ := AccessLevel(issue.Poster.ID, issue.Repo)
373385
if issue.IsPull {
374-
if err = issue.loadRepo(x); err != nil {
375-
log.Error(4, "loadRepo: %v", err)
376-
return
377-
}
378386
if err = issue.loadPullRequest(x); err != nil {
379387
log.Error(4, "loadPullRequest: %v", err)
380388
return
@@ -390,6 +398,14 @@ func (issue *Issue) sendLabelUpdatedWebhook(doer *User) {
390398
Repository: issue.Repo.APIFormat(AccessModeNone),
391399
Sender: doer.APIFormat(),
392400
})
401+
} else {
402+
err = PrepareWebhooks(issue.Repo, HookEventIssues, &api.IssuePayload{
403+
Action: api.HookIssueLabelUpdated,
404+
Index: issue.Index,
405+
Issue: issue.APIFormat(),
406+
Repository: issue.Repo.APIFormat(mode),
407+
Sender: doer.APIFormat(),
408+
})
393409
}
394410
if err != nil {
395411
log.Error(4, "PrepareWebhooks [is_pull: %v]: %v", issue.IsPull, err)
@@ -505,6 +521,11 @@ func (issue *Issue) ClearLabels(doer *User) (err error) {
505521
return fmt.Errorf("Commit: %v", err)
506522
}
507523

524+
if err = issue.loadPoster(x); err != nil {
525+
return fmt.Errorf("loadPoster: %v", err)
526+
}
527+
528+
mode, _ := AccessLevel(issue.Poster.ID, issue.Repo)
508529
if issue.IsPull {
509530
err = issue.PullRequest.LoadIssue()
510531
if err != nil {
@@ -515,9 +536,17 @@ func (issue *Issue) ClearLabels(doer *User) (err error) {
515536
Action: api.HookIssueLabelCleared,
516537
Index: issue.Index,
517538
PullRequest: issue.PullRequest.APIFormat(),
518-
Repository: issue.Repo.APIFormat(AccessModeNone),
539+
Repository: issue.Repo.APIFormat(mode),
519540
Sender: doer.APIFormat(),
520541
})
542+
} else {
543+
err = PrepareWebhooks(issue.Repo, HookEventIssues, &api.IssuePayload{
544+
Action: api.HookIssueLabelCleared,
545+
Index: issue.Index,
546+
Issue: issue.APIFormat(),
547+
Repository: issue.Repo.APIFormat(mode),
548+
Sender: doer.APIFormat(),
549+
})
521550
}
522551
if err != nil {
523552
log.Error(4, "PrepareWebhooks [is_pull: %v]: %v", issue.IsPull, err)
@@ -675,13 +704,14 @@ func (issue *Issue) ChangeStatus(doer *User, repo *Repository, isClosed bool) (e
675704
return fmt.Errorf("Commit: %v", err)
676705
}
677706

707+
mode, _ := AccessLevel(issue.Poster.ID, issue.Repo)
678708
if issue.IsPull {
679709
// Merge pull request calls issue.changeStatus so we need to handle separately.
680710
issue.PullRequest.Issue = issue
681711
apiPullRequest := &api.PullRequestPayload{
682712
Index: issue.Index,
683713
PullRequest: issue.PullRequest.APIFormat(),
684-
Repository: repo.APIFormat(AccessModeNone),
714+
Repository: repo.APIFormat(mode),
685715
Sender: doer.APIFormat(),
686716
}
687717
if isClosed {
@@ -690,6 +720,19 @@ func (issue *Issue) ChangeStatus(doer *User, repo *Repository, isClosed bool) (e
690720
apiPullRequest.Action = api.HookIssueReOpened
691721
}
692722
err = PrepareWebhooks(repo, HookEventPullRequest, apiPullRequest)
723+
} else {
724+
apiIssue := &api.IssuePayload{
725+
Index: issue.Index,
726+
Issue: issue.APIFormat(),
727+
Repository: repo.APIFormat(mode),
728+
Sender: doer.APIFormat(),
729+
}
730+
if isClosed {
731+
apiIssue.Action = api.HookIssueClosed
732+
} else {
733+
apiIssue.Action = api.HookIssueReOpened
734+
}
735+
err = PrepareWebhooks(repo, HookEventIssues, apiIssue)
693736
}
694737
if err != nil {
695738
log.Error(4, "PrepareWebhooks [is_pull: %v, is_closed: %v]: %v", issue.IsPull, isClosed, err)
@@ -723,6 +766,7 @@ func (issue *Issue) ChangeTitle(doer *User, title string) (err error) {
723766
return err
724767
}
725768

769+
mode, _ := AccessLevel(issue.Poster.ID, issue.Repo)
726770
if issue.IsPull {
727771
issue.PullRequest.Issue = issue
728772
err = PrepareWebhooks(issue.Repo, HookEventPullRequest, &api.PullRequestPayload{
@@ -734,10 +778,24 @@ func (issue *Issue) ChangeTitle(doer *User, title string) (err error) {
734778
},
735779
},
736780
PullRequest: issue.PullRequest.APIFormat(),
737-
Repository: issue.Repo.APIFormat(AccessModeNone),
781+
Repository: issue.Repo.APIFormat(mode),
738782
Sender: doer.APIFormat(),
739783
})
784+
} else {
785+
err = PrepareWebhooks(issue.Repo, HookEventIssues, &api.IssuePayload{
786+
Action: api.HookIssueEdited,
787+
Index: issue.Index,
788+
Changes: &api.ChangesPayload{
789+
Title: &api.ChangesFromPayload{
790+
From: oldTitle,
791+
},
792+
},
793+
Issue: issue.APIFormat(),
794+
Repository: issue.Repo.APIFormat(mode),
795+
Sender: issue.Poster.APIFormat(),
796+
})
740797
}
798+
741799
if err != nil {
742800
log.Error(4, "PrepareWebhooks [is_pull: %v]: %v", issue.IsPull, err)
743801
} else {
@@ -774,6 +832,7 @@ func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
774832
return fmt.Errorf("UpdateIssueCols: %v", err)
775833
}
776834

835+
mode, _ := AccessLevel(issue.Poster.ID, issue.Repo)
777836
if issue.IsPull {
778837
issue.PullRequest.Issue = issue
779838
err = PrepareWebhooks(issue.Repo, HookEventPullRequest, &api.PullRequestPayload{
@@ -785,9 +844,22 @@ func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
785844
},
786845
},
787846
PullRequest: issue.PullRequest.APIFormat(),
788-
Repository: issue.Repo.APIFormat(AccessModeNone),
847+
Repository: issue.Repo.APIFormat(mode),
789848
Sender: doer.APIFormat(),
790849
})
850+
} else {
851+
err = PrepareWebhooks(issue.Repo, HookEventIssues, &api.IssuePayload{
852+
Action: api.HookIssueEdited,
853+
Index: issue.Index,
854+
Changes: &api.ChangesPayload{
855+
Body: &api.ChangesFromPayload{
856+
From: oldContent,
857+
},
858+
},
859+
Issue: issue.APIFormat(),
860+
Repository: issue.Repo.APIFormat(mode),
861+
Sender: doer.APIFormat(),
862+
})
791863
}
792864
if err != nil {
793865
log.Error(4, "PrepareWebhooks [is_pull: %v]: %v", issue.IsPull, err)
@@ -980,6 +1052,19 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in
9801052
log.Error(4, "MailParticipants: %v", err)
9811053
}
9821054

1055+
mode, _ := AccessLevel(issue.Poster.ID, issue.Repo)
1056+
if err = PrepareWebhooks(repo, HookEventIssues, &api.IssuePayload{
1057+
Action: api.HookIssueOpened,
1058+
Index: issue.Index,
1059+
Issue: issue.APIFormat(),
1060+
Repository: repo.APIFormat(mode),
1061+
Sender: issue.Poster.APIFormat(),
1062+
}); err != nil {
1063+
log.Error(4, "PrepareWebhooks: %v", err)
1064+
} else {
1065+
go HookQueue.Add(issue.RepoID)
1066+
}
1067+
9831068
return nil
9841069
}
9851070

‎models/issue_assignees.go

+23-3
Original file line numberDiff line numberDiff line change
@@ -159,20 +159,40 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in
159159
return fmt.Errorf("createAssigneeComment: %v", err)
160160
}
161161

162+
mode, _ := accessLevel(sess, doer.ID, issue.Repo)
162163
if issue.IsPull {
163-
issue.PullRequest = &PullRequest{Issue: issue}
164+
if err = issue.loadPullRequest(sess); err != nil {
165+
return fmt.Errorf("loadPullRequest: %v", err)
166+
}
167+
issue.PullRequest.Issue = issue
164168
apiPullRequest := &api.PullRequestPayload{
165169
Index: issue.Index,
166170
PullRequest: issue.PullRequest.APIFormat(),
167-
Repository: issue.Repo.APIFormat(AccessModeNone),
171+
Repository: issue.Repo.APIFormat(mode),
168172
Sender: doer.APIFormat(),
169173
}
170174
if removed {
171175
apiPullRequest.Action = api.HookIssueUnassigned
172176
} else {
173177
apiPullRequest.Action = api.HookIssueAssigned
174178
}
175-
if err := PrepareWebhooks(issue.Repo, HookEventPullRequest, apiPullRequest); err != nil {
179+
if err := prepareWebhooks(sess, issue.Repo, HookEventPullRequest, apiPullRequest); err != nil {
180+
log.Error(4, "PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err)
181+
return nil
182+
}
183+
} else {
184+
apiIssue := &api.IssuePayload{
185+
Index: issue.Index,
186+
Issue: issue.APIFormat(),
187+
Repository: issue.Repo.APIFormat(mode),
188+
Sender: doer.APIFormat(),
189+
}
190+
if removed {
191+
apiIssue.Action = api.HookIssueUnassigned
192+
} else {
193+
apiIssue.Action = api.HookIssueAssigned
194+
}
195+
if err := prepareWebhooks(sess, issue.Repo, HookEventIssues, apiIssue); err != nil {
176196
log.Error(4, "PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err)
177197
return nil
178198
}

‎models/issue_comment.go

+6
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,8 @@ func CreateIssueComment(doer *User, repo *Repository, issue *Issue, content stri
612612
Sender: doer.APIFormat(),
613613
}); err != nil {
614614
log.Error(2, "PrepareWebhooks [comment_id: %d]: %v", comment.ID, err)
615+
} else {
616+
go HookQueue.Add(repo.ID)
615617
}
616618
return comment, nil
617619
}
@@ -754,6 +756,8 @@ func UpdateComment(doer *User, c *Comment, oldContent string) error {
754756
Sender: doer.APIFormat(),
755757
}); err != nil {
756758
log.Error(2, "PrepareWebhooks [comment_id: %d]: %v", c.ID, err)
759+
} else {
760+
go HookQueue.Add(c.Issue.Repo.ID)
757761
}
758762

759763
return nil
@@ -805,6 +809,8 @@ func DeleteComment(doer *User, comment *Comment) error {
805809
Sender: doer.APIFormat(),
806810
}); err != nil {
807811
log.Error(2, "PrepareWebhooks [comment_id: %d]: %v", comment.ID, err)
812+
} else {
813+
go HookQueue.Add(comment.Issue.Repo.ID)
808814
}
809815

810816
return nil

‎models/issue_milestone.go

+2
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,8 @@ func ChangeMilestoneAssign(issue *Issue, doer *User, oldMilestoneID int64) (err
402402
}
403403
if err != nil {
404404
log.Error(2, "PrepareWebhooks [is_pull: %v]: %v", issue.IsPull, err)
405+
} else {
406+
go HookQueue.Add(issue.RepoID)
405407
}
406408
return nil
407409
}

‎models/issue_user.go

+7-5
Original file line numberDiff line numberDiff line change
@@ -54,28 +54,30 @@ func newIssueUsers(e Engine, repo *Repository, issue *Issue) error {
5454
func updateIssueAssignee(e *xorm.Session, issue *Issue, assigneeID int64) (removed bool, err error) {
5555

5656
// Check if the user exists
57-
_, err = GetUserByID(assigneeID)
57+
assignee, err := GetUserByID(assigneeID)
5858
if err != nil {
5959
return false, err
6060
}
6161

6262
// Check if the submitted user is already assigne, if yes delete him otherwise add him
63-
var toBeDeleted bool
64-
for _, assignee := range issue.Assignees {
65-
if assignee.ID == assigneeID {
66-
toBeDeleted = true
63+
var i int
64+
for i = 0; i < len(issue.Assignees); i++ {
65+
if issue.Assignees[i].ID == assigneeID {
6766
break
6867
}
6968
}
7069

7170
assigneeIn := IssueAssignees{AssigneeID: assigneeID, IssueID: issue.ID}
7271

72+
toBeDeleted := i < len(issue.Assignees)
7373
if toBeDeleted {
74+
issue.Assignees = append(issue.Assignees[:i], issue.Assignees[i:]...)
7475
_, err = e.Delete(assigneeIn)
7576
if err != nil {
7677
return toBeDeleted, err
7778
}
7879
} else {
80+
issue.Assignees = append(issue.Assignees, assignee)
7981
_, err = e.Insert(assigneeIn)
8082
if err != nil {
8183
return toBeDeleted, err

‎models/pull.go

+13-6
Original file line numberDiff line numberDiff line change
@@ -457,15 +457,18 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle
457457
log.Error(4, "LoadAttributes: %v", err)
458458
return nil
459459
}
460+
461+
mode, _ := AccessLevel(doer.ID, pr.Issue.Repo)
460462
if err = PrepareWebhooks(pr.Issue.Repo, HookEventPullRequest, &api.PullRequestPayload{
461463
Action: api.HookIssueClosed,
462464
Index: pr.Index,
463465
PullRequest: pr.APIFormat(),
464-
Repository: pr.Issue.Repo.APIFormat(AccessModeNone),
466+
Repository: pr.Issue.Repo.APIFormat(mode),
465467
Sender: doer.APIFormat(),
466468
}); err != nil {
467469
log.Error(4, "PrepareWebhooks: %v", err)
468-
return nil
470+
} else {
471+
go HookQueue.Add(pr.Issue.Repo.ID)
469472
}
470473

471474
l, err := baseGitRepo.CommitsBetweenIDs(pr.MergedCommitID, pr.MergeBase)
@@ -492,12 +495,14 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle
492495
After: mergeCommit.ID.String(),
493496
CompareURL: setting.AppURL + pr.BaseRepo.ComposeCompareURL(pr.MergeBase, pr.MergedCommitID),
494497
Commits: ListToPushCommits(l).ToAPIPayloadCommits(pr.BaseRepo.HTMLURL()),
495-
Repo: pr.BaseRepo.APIFormat(AccessModeNone),
498+
Repo: pr.BaseRepo.APIFormat(mode),
496499
Pusher: pr.HeadRepo.MustOwner().APIFormat(),
497500
Sender: doer.APIFormat(),
498501
}
499502
if err = PrepareWebhooks(pr.BaseRepo, HookEventPush, p); err != nil {
500-
return fmt.Errorf("PrepareWebhooks: %v", err)
503+
log.Error(4, "PrepareWebhooks: %v", err)
504+
} else {
505+
go HookQueue.Add(pr.BaseRepo.ID)
501506
}
502507
return nil
503508
}
@@ -782,16 +787,18 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str
782787

783788
pr.Issue = pull
784789
pull.PullRequest = pr
790+
mode, _ := AccessLevel(pull.Poster.ID, repo)
785791
if err = PrepareWebhooks(repo, HookEventPullRequest, &api.PullRequestPayload{
786792
Action: api.HookIssueOpened,
787793
Index: pull.Index,
788794
PullRequest: pr.APIFormat(),
789-
Repository: repo.APIFormat(AccessModeNone),
795+
Repository: repo.APIFormat(mode),
790796
Sender: pull.Poster.APIFormat(),
791797
}); err != nil {
792798
log.Error(4, "PrepareWebhooks: %v", err)
799+
} else {
800+
go HookQueue.Add(repo.ID)
793801
}
794-
go HookQueue.Add(repo.ID)
795802

796803
return nil
797804
}

0 commit comments

Comments
 (0)