Merge branch 'main' into lunny/fix_38016

services/actions/commit_status.go

@@ -139,10 +139,24 @@ func getCommitStatusEventNameAndCommitID(run *actions_model.ActionRun) (event, c
 func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event, commitID string, run *actions_model.ActionRun, job *actions_model.ActionRunJob) error {
 	// TODO: store workflow name as a field in ActionRun to avoid parsing
 	runName := path.Base(run.WorkflowID)
+	// fall back to the file name when the workflow has no non-blank `name:`
 	if wfs, err := jobparser.Parse(job.WorkflowPayload); err == nil && len(wfs) > 0 {
-		runName = wfs[0].Name
+		if name := strings.TrimSpace(wfs[0].Name); name != "" {
+			runName = name
+		}
 	}
 	ctxName := strings.TrimSpace(fmt.Sprintf("%s / %s (%s)", runName, job.Name, event)) // git_model.NewCommitStatus also trims spaces
+	// Mix the workflow file path into the hash so two workflow files that
+	// share the same `name:` and job name produce distinct commit statuses
+	// even though they render identically — matching GitHub's behavior
+	// (issue #35699).
+	ctxHash := git_model.HashCommitStatusContext(ctxName + "\x00" + run.WorkflowID)
+	// Pre-fix rows were hashed from Context alone. If a pre-existing row with
+	// the legacy hash is still the "latest" for this SHA, reuse that hash so
+	// the new row supersedes it; otherwise the old pending status would stay
+	// stuck forever (it lives in its own dedupe group). Only relevant for
+	// in-flight workflows at upgrade time.
+	legacyHash := git_model.HashCommitStatusContext(ctxName)
 	state := toCommitStatus(job.Status)
 	targetURL := fmt.Sprintf("%s/jobs/%d", run.Link(), job.ID)
 	description := toCommitStatusDescription(job)
@@ -152,7 +166,13 @@ func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event,
 		return fmt.Errorf("GetLatestCommitStatus: %w", err)
 	}
 	for _, v := range statuses {
-		if v.Context == ctxName {
+		if v.ContextHash == legacyHash && v.Context == ctxName {
+			ctxHash = legacyHash
+			break
+		}
+	}
+	for _, v := range statuses {
+		if v.ContextHash == ctxHash {
 			if v.State == state && v.TargetURL == targetURL && v.Description == description {
 				return nil
 			}
@@ -166,6 +186,7 @@ func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event,
 		TargetURL:   targetURL,
 		Description: description,
 		Context:     ctxName,
+		ContextHash: ctxHash,
 		State:       state,
 		CreatorID:   creator.ID,
 	}

services/actions/commit_status_test.go

@@ -11,8 +11,10 @@ import (
 	git_model "gitea.dev/models/git"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unittest"
+	user_model "gitea.dev/models/user"
 	actions_module "gitea.dev/modules/actions"
 	"gitea.dev/modules/commitstatus"
+	"gitea.dev/modules/git"
 	"gitea.dev/modules/gitrepo"
 	"gitea.dev/modules/timeutil"
 
@@ -146,6 +148,158 @@ func TestGetCommitActionsStatusMap(t *testing.T) {
 	assert.Empty(t, nilInfo.IconStatus(statuses[0]))
 }
 
+// TestCreateCommitStatus_DistinctWorkflowFilesSameName covers issue #35699:
+// two workflow files with the same `name:` and same job name must produce
+// two distinct commit statuses, not be deduplicated into one.
+func TestCreateCommitStatus_DistinctWorkflowFilesSameName(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+	branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch})
+
+	payload := []byte(`
+name: test-run
+on: pull_request
+jobs:
+  my-test:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo hi
+`)
+
+	for _, spec := range []struct {
+		workflowID   string
+		runID, jobID int64
+	}{
+		{"workflow1.yaml", 99101, 99201},
+		{"workflow2.yaml", 99102, 99202},
+	} {
+		run := &actions_model.ActionRun{
+			ID: spec.runID, Index: spec.runID, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID,
+			WorkflowID: spec.workflowID, CommitSHA: branch.CommitID,
+		}
+		require.NoError(t, db.Insert(t.Context(), run))
+		job := &actions_model.ActionRunJob{
+			ID: spec.jobID, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID,
+			Name: "my-test", Status: actions_model.StatusWaiting,
+			WorkflowPayload: payload,
+		}
+		require.NoError(t, db.Insert(t.Context(), job))
+		require.NoError(t, createCommitStatus(t.Context(), repo, "pull_request", branch.CommitID, run, job))
+	}
+
+	statuses, err := git_model.GetLatestCommitStatus(t.Context(), repo.ID, branch.CommitID, db.ListOptionsAll)
+	require.NoError(t, err)
+
+	// Both workflow files should produce a row even though the display
+	// Context is identical — matching GitHub's behavior.
+	hashes := map[string]struct{}{}
+	targets := map[string]struct{}{}
+	for _, st := range statuses {
+		hashes[st.ContextHash] = struct{}{}
+		targets[st.TargetURL] = struct{}{}
+		assert.Equal(t, "test-run / my-test (pull_request)", st.Context)
+	}
+	assert.Len(t, hashes, 2, "expected distinct ContextHash per workflow file")
+	assert.Len(t, targets, 2, "expected distinct TargetURL per workflow file")
+}
+
+// TestCreateCommitStatus_LegacyHashRecovery covers the upgrade path: a pending
+// status created before the fix (hashed from Context alone) must still be
+// superseded by a follow-up event, instead of being orphaned in its own dedupe
+// group while a new row accumulates under the new hash.
+func TestCreateCommitStatus_LegacyHashRecovery(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+	branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch})
+
+	ctxName := "legacy.yaml / my-job (push)"
+	legacyHash := git_model.HashCommitStatusContext(ctxName)
+	sha, err := git.NewIDFromString(branch.CommitID)
+	require.NoError(t, err)
+	creator := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+	require.NoError(t, git_model.NewCommitStatus(t.Context(), git_model.NewCommitStatusOptions{
+		Repo:    repo,
+		Creator: creator,
+		SHA:     sha,
+		CommitStatus: &git_model.CommitStatus{
+			State:       commitstatus.CommitStatusPending,
+			Context:     ctxName,
+			ContextHash: legacyHash,
+			TargetURL:   "https://example.invalid/legacy",
+			Description: "Waiting to run",
+		},
+	}))
+
+	run := &actions_model.ActionRun{
+		ID: 99301, Index: 99301, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID,
+		WorkflowID: "legacy.yaml", CommitSHA: branch.CommitID,
+	}
+	require.NoError(t, db.Insert(t.Context(), run))
+	job := &actions_model.ActionRunJob{
+		ID: 99302, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID,
+		Name: "my-job", Status: actions_model.StatusSuccess,
+	}
+	require.NoError(t, db.Insert(t.Context(), job))
+	require.NoError(t, createCommitStatus(t.Context(), repo, "push", branch.CommitID, run, job))
+
+	latest, err := git_model.GetLatestCommitStatus(t.Context(), repo.ID, branch.CommitID, db.ListOptionsAll)
+	require.NoError(t, err)
+	// The new row must reuse the legacy hash so GetLatestCommitStatus returns
+	// only one entry for this Context — the success, not the orphaned pending.
+	matches := 0
+	for _, s := range latest {
+		if s.Context == ctxName {
+			matches++
+			assert.Equal(t, legacyHash, s.ContextHash)
+			assert.Equal(t, commitstatus.CommitStatusSuccess, s.State)
+		}
+	}
+	assert.Equal(t, 1, matches)
+}
+
+// TestCreateCommitStatus_UnnamedWorkflowUsesFileName: a workflow with no
+// non-blank `name:` uses the file name in the Context, not an empty
+// "/ job (event)" — covers both an omitted and a whitespace-only name.
+func TestCreateCommitStatus_UnnamedWorkflowUsesFileName(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+	branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch})
+
+	for _, tc := range []struct {
+		workflowID   string
+		runID, jobID int64
+		payload      string
+	}{
+		{"unnamed.yaml", 99401, 99411, "on: push\n"},
+		{"blank.yaml", 99402, 99412, "name: \"   \"\non: push\n"},
+	} {
+		run := &actions_model.ActionRun{
+			ID: tc.runID, Index: tc.runID, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID,
+			WorkflowID: tc.workflowID, CommitSHA: branch.CommitID,
+		}
+		require.NoError(t, db.Insert(t.Context(), run))
+		job := &actions_model.ActionRunJob{
+			ID: tc.jobID, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID,
+			Name: "my-test", Status: actions_model.StatusWaiting,
+			WorkflowPayload: []byte(tc.payload + `jobs:
+  my-test:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo hi
+`),
+		}
+		require.NoError(t, db.Insert(t.Context(), job))
+		require.NoError(t, createCommitStatus(t.Context(), repo, "push", branch.CommitID, run, job))
+
+		statuses := findCommitStatusesForContext(t, repo.ID, branch.CommitID, tc.workflowID+" / my-test (push)")
+		require.Len(t, statuses, 1)
+		assert.Equal(t, commitstatus.CommitStatusPending, statuses[0].State)
+	}
+}
+
 func findCommitStatusesForContext(t *testing.T, repoID int64, sha, context string) []*git_model.CommitStatus {
 	t.Helper()
 

services/context/repo.go

@@ -242,7 +242,7 @@ func (r *Repository) CanUseTimetracker(ctx context.Context, issue *issues_model.
 	// Checking for following:
 	// 1. Is timetracker enabled
 	// 2. Is the user a contributor, admin, poster or assignee and do the repository policies require this?
-	isAssigned, _ := issues_model.IsUserAssignedToIssue(ctx, issue, user)
+	isAssigned, _ := issues_model.IsUserAssignedToIssue(ctx, issue, user.ID)
 	return r.Repository.IsTimetrackerEnabled(ctx) && (!r.Repository.AllowOnlyContributorsToTrackTime(ctx) ||
 		r.Permission.CanWriteIssuesOrPulls(issue.IsPull) || issue.IsPoster(user.ID) || isAssigned)
 }

services/issue/assignee.go

@@ -6,6 +6,7 @@ package issue
 import (
 	"context"
 
+	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
 	access_model "gitea.dev/models/perm/access"
 	repo_model "gitea.dev/models/repo"
@@ -14,8 +15,7 @@ import (
 	notify_service "gitea.dev/services/notify"
 )
 
-// DeleteNotPassedAssignee deletes all assignees who aren't passed via the "assignees" array
-func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assignees []*user_model.User) (err error) {
+func toBeRemovedAssignees(issue *issues_model.Issue, assignees []*user_model.User) (toBeRemovedAssignees []*user_model.User) {
 	var found bool
 	oriAssignees := make([]*user_model.User, len(issue.Assignees))
 	_ = copy(oriAssignees, issue.Assignees)
@@ -31,28 +31,54 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe
 
 		if !found {
 			// This function also does comments and hooks, which is why we call it separately instead of directly removing the assignees here
-			if _, _, err := ToggleAssigneeWithNotify(ctx, issue, doer, assignee.ID); err != nil {
-				return err
-			}
+			toBeRemovedAssignees = append(toBeRemovedAssignees, assignee)
+		}
+	}
+	return toBeRemovedAssignees
+}
+
+// DeleteNotPassedAssignee deletes all assignees who aren't passed via the "assignees" array
+func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assignees []*user_model.User) (err error) {
+	toBeRemoved := toBeRemovedAssignees(issue, assignees)
+
+	for _, assignee := range toBeRemoved {
+		// This function also does comments and hooks, which is why we call it separately instead of directly removing the assignees here
+		removed, comment, err := ToggleAssignee(ctx, issue, doer, assignee)
+		if err != nil {
+			return err
+		}
+		if removed {
+			notify_service.IssueChangeAssignee(ctx, doer, issue, assignee, true, comment)
 		}
 	}
 
 	return nil
 }
 
-// ToggleAssigneeWithNoNotify changes a user between assigned and not assigned for this issue, and make issue comment for it.
-func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) {
-	removed, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
+// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it.
+func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer, assignee *user_model.User) (removed bool, comment *issues_model.Comment, err error) {
+	removed, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assignee.ID)
 	if err != nil {
 		return false, nil, err
 	}
 
+	issue.AssigneeID = assignee.ID
+	issue.Assignee = assignee
+
+	return removed, comment, nil
+}
+
+// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it.
+func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) {
 	assignee, err := user_model.GetUserByID(ctx, assigneeID)
 	if err != nil {
 		return false, nil, err
 	}
-	issue.AssigneeID = assigneeID
-	issue.Assignee = assignee
+
+	removed, comment, err = ToggleAssignee(ctx, issue, doer, assignee)
+	if err != nil {
+		return false, nil, err
+	}
 
 	notify_service.IssueChangeAssignee(ctx, doer, issue, assignee, removed, comment)
 
@@ -81,43 +107,85 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee
 			return err
 		}
 
-		if user_model.IsUserBlockedBy(ctx, doer, assignee.ID) {
-			return user_model.ErrBlockedUser
+		if err := validateAssignee(ctx, issue, doer, assignee); err != nil {
+			return err
 		}
 
 		allNewAssignees = append(allNewAssignees, assignee)
 	}
 
-	// Delete all old assignees not passed
-	if err = DeleteNotPassedAssignee(ctx, issue, doer, allNewAssignees); err != nil {
+	assigneeCommentMap := make(map[int64]*issues_model.Comment)
+	assigneeRemovedCommentMap := make(map[int64]*issues_model.Comment)
+	assigneeRemoved := make(map[int64]*user_model.User)
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
+		// Delete all old assignees not passed.
+		toBeRemoved := toBeRemovedAssignees(issue, allNewAssignees)
+
+		for _, assignee := range toBeRemoved {
+			// This function also does comments and hooks, which is why we call it separately instead of directly removing the assignees here
+			removed, comment, err := ToggleAssignee(ctx, issue, doer, assignee)
+			if err != nil {
+				return err
+			}
+			if removed {
+				assigneeRemoved[assignee.ID] = assignee
+				assigneeRemovedCommentMap[assignee.ID] = comment
+			}
+		}
+
+		// Add all new assignees.
+		// Update the assignee. The function will check if the user exists, is already
+		// assigned (which he shouldn't as we deleted all assignees before) and
+		// has access to the repo.
+		for _, assignee := range allNewAssignees {
+			// Extra method to prevent double adding (which would result in removing).
+			comment, err := AddAssigneeIfNotAssigned(ctx, issue, doer, assignee)
+			if err != nil {
+				return err
+			}
+			assigneeCommentMap[assignee.ID] = comment
+		}
+
+		return nil
+	}); err != nil {
 		return err
 	}
 
-	// Add all new assignees
-	// Update the assignee. The function will check if the user exists, is already
-	// assigned (which he shouldn't as we deleted all assignees before) and
-	// has access to the repo.
+	for _, assignee := range assigneeRemoved {
+		notify_service.IssueChangeAssignee(ctx, doer, issue, assignee, true, assigneeRemovedCommentMap[assignee.ID])
+	}
+
 	for _, assignee := range allNewAssignees {
-		// Extra method to prevent double adding (which would result in removing)
-		_, err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID, true)
-		if err != nil {
-			return err
+		comment := assigneeCommentMap[assignee.ID]
+		if comment != nil {
+			notify_service.IssueChangeAssignee(ctx, doer, issue, assignee, false, comment)
 		}
 	}
 
-	return err
+	return nil
+}
+
+func validateAssignee(ctx context.Context, issue *issues_model.Issue, doer, assignee *user_model.User) error {
+	if user_model.IsUserBlockedBy(ctx, doer, assignee.ID) {
+		return user_model.ErrBlockedUser
+	}
+
+	valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo)
+	if err != nil {
+		return err
+	}
+	if !valid {
+		return repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assignee.ID, RepoName: issue.Repo.Name}
+	}
+
+	return nil
 }
 
 // AddAssigneeIfNotAssigned adds an assignee only if he isn't already assigned to the issue.
 // Also checks for access of assigned user
-func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64, notify bool) (comment *issues_model.Comment, err error) {
-	assignee, err := user_model.GetUserByID(ctx, assigneeID)
-	if err != nil {
-		return nil, err
-	}
-
+func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer, assignee *user_model.User) (comment *issues_model.Comment, err error) {
 	// Check if the user is already assigned
-	isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee)
+	isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee.ID)
 	if err != nil {
 		return nil, err
 	}
@@ -126,18 +194,92 @@ func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, do
 		return nil, nil //nolint:nilnil // return nil because the user is already assigned
 	}
 
-	valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo, issue.IsPull)
-	if err != nil {
+	if err := validateAssignee(ctx, issue, doer, assignee); err != nil {
 		return nil, err
 	}
-	if !valid {
-		return nil, repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}
-	}
 
-	if notify {
-		_, comment, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID)
-		return comment, err
-	}
-	_, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
+	_, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assignee.ID)
 	return comment, err
 }
+
+// AddAssignees adds multiple assignees to an issue atomically.
+func AddAssignees(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeIDs []int64) error {
+	assigneeCommentMap := make(map[int64]*issues_model.Comment)
+	assignees := make(map[int64]*user_model.User)
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
+		for _, assigneeID := range assigneeIDs {
+			isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assigneeID)
+			if err != nil {
+				return err
+			}
+			if isAssigned {
+				continue
+			}
+
+			assignee, err := user_model.GetUserByID(ctx, assigneeID)
+			if err != nil {
+				return err
+			}
+			if err := validateAssignee(ctx, issue, doer, assignee); err != nil {
+				return err
+			}
+
+			comment, err := AddAssigneeIfNotAssigned(ctx, issue, doer, assignee)
+			if err != nil {
+				return err
+			}
+			assignees[assigneeID] = assignee
+			assigneeCommentMap[assigneeID] = comment
+		}
+
+		return nil
+	}); err != nil {
+		return err
+	}
+
+	if len(assignees) > 0 {
+		for assigneeID, assignee := range assignees {
+			notify_service.IssueChangeAssignee(ctx, doer, issue, assignee, false, assigneeCommentMap[assigneeID])
+		}
+	}
+	return nil
+}
+
+// RemoveAssignees removes multiple assignees from an issue atomically.
+func RemoveAssignees(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeIDs []int64) error {
+	assigneeCommentMap := make(map[int64]*issues_model.Comment)
+	assignees := make(map[int64]*user_model.User)
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
+		for _, assigneeID := range assigneeIDs {
+			isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assigneeID)
+			if err != nil {
+				return err
+			}
+			if !isAssigned {
+				continue
+			}
+			removed, comment, err := issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
+			if err != nil {
+				return err
+			}
+			if removed {
+				assignee, err := user_model.GetUserByID(ctx, assigneeID)
+				if err != nil {
+					return err
+				}
+				assignees[assigneeID] = assignee
+				assigneeCommentMap[assigneeID] = comment
+			}
+		}
+		return nil
+	}); err != nil {
+		return err
+	}
+
+	if len(assignees) > 0 {
+		for assigneeID, assignee := range assignees {
+			notify_service.IssueChangeAssignee(ctx, doer, issue, assignee, true, assigneeCommentMap[assigneeID])
+		}
+	}
+	return nil
+}

services/issue/assignee_test.go

@@ -6,6 +6,7 @@ package issue
 import (
 	"testing"
 
+	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
@@ -29,7 +30,7 @@ func TestDeleteNotPassedAssignee(t *testing.T) {
 	assert.NoError(t, err)
 
 	// Check if he got removed
-	isAssigned, err := issues_model.IsUserAssignedToIssue(t.Context(), issue, user1)
+	isAssigned, err := issues_model.IsUserAssignedToIssue(t.Context(), issue, user1.ID)
 	assert.NoError(t, err)
 	assert.True(t, isAssigned)
 
@@ -44,3 +45,55 @@ func TestDeleteNotPassedAssignee(t *testing.T) {
 	assert.Empty(t, issue.Assignees)
 	assert.Empty(t, issue.Assignee)
 }
+
+func TestAddAssigneeIfNotAssignedBlocked(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	issue, err := issues_model.GetIssueByID(t.Context(), 1)
+	assert.NoError(t, err)
+	assert.NoError(t, issue.LoadRepo(t.Context()))
+
+	doer, err := user_model.GetUserByID(t.Context(), 4)
+	assert.NoError(t, err)
+
+	assignee, err := user_model.GetUserByID(t.Context(), 2)
+	assert.NoError(t, err)
+
+	assert.NoError(t, db.Insert(t.Context(), &user_model.Blocking{
+		BlockerID: assignee.ID,
+		BlockeeID: doer.ID,
+	}))
+
+	_, err = AddAssigneeIfNotAssigned(t.Context(), issue, doer, assignee)
+	assert.ErrorIs(t, err, user_model.ErrBlockedUser)
+
+	isAssigned, err := issues_model.IsUserAssignedToIssue(t.Context(), issue, assignee.ID)
+	assert.NoError(t, err)
+	assert.False(t, isAssigned)
+}
+
+func TestAddAssigneesBlockedIsAtomic(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	issue, err := issues_model.GetIssueByID(t.Context(), 1)
+	assert.NoError(t, err)
+	assert.NoError(t, issue.LoadAttributes(t.Context()))
+
+	doer, err := user_model.GetUserByID(t.Context(), 2)
+	assert.NoError(t, err)
+
+	blockedAssignee, err := user_model.GetUserByID(t.Context(), 40)
+	assert.NoError(t, err)
+
+	assert.NoError(t, db.Insert(t.Context(), &user_model.Blocking{
+		BlockerID: blockedAssignee.ID,
+		BlockeeID: doer.ID,
+	}))
+
+	err = AddAssignees(t.Context(), issue, doer, []int64{doer.ID, blockedAssignee.ID})
+	assert.ErrorIs(t, err, user_model.ErrBlockedUser)
+
+	assigneeIDs, err := issues_model.GetAssigneeIDsByIssue(t.Context(), issue.ID)
+	assert.NoError(t, err)
+	assert.ElementsMatch(t, []int64{1}, assigneeIDs)
+}

services/issue/issue.go

@@ -32,14 +32,24 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo
 		return user_model.ErrBlockedUser
 	}
 
+	assigneeCommentMap := make(map[int64]*issues_model.Comment)
+	assignees := make(map[int64]*user_model.User)
 	if err := db.WithTx(ctx, func(ctx context.Context) error {
 		if err := issues_model.NewIssue(ctx, repo, issue, labelIDs, uuids); err != nil {
 			return err
 		}
 		for _, assigneeID := range assigneeIDs {
-			if _, err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, true); err != nil {
+			assignee, err := user_model.GetUserByID(ctx, assigneeID)
+			if err != nil {
+				log.Error("GetUserByID: %v", err)
+				continue
+			}
+			assignees[assigneeID] = assignee
+			comment, err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assignee)
+			if err != nil {
 				return err
 			}
+			assigneeCommentMap[assigneeID] = comment
 		}
 		if len(projectIDs) > 0 {
 			err := issues_model.IssueAssignOrRemoveProject(ctx, issue, issue.Poster, projectIDs)
@@ -65,6 +75,12 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo
 		notify_service.IssueChangeMilestone(ctx, issue.Poster, issue, 0)
 	}
 
+	if len(assigneeIDs) > 0 {
+		for _, assignee := range assignees {
+			notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assignee.ID])
+		}
+	}
+
 	return nil
 }
 

services/pull/pull.go

@@ -96,7 +96,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
 	}
 
 	assigneeCommentMap := make(map[int64]*issues_model.Comment)
-
+	assignees := make(map[int64]*user_model.User)
 	var reviewNotifiers []*issue_service.ReviewRequestNotifier
 	if err := db.WithTx(ctx, func(ctx context.Context) error {
 		if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil {
@@ -104,10 +104,16 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
 		}
 
 		for _, assigneeID := range assigneeIDs {
-			comment, err := issue_service.AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, false)
+			assignee, err := user_model.GetUserByID(ctx, assigneeID)
+			if err != nil {
+				log.Error("GetUserByID: %v", err)
+				continue
+			}
+			comment, err := issue_service.AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assignee)
 			if err != nil {
 				return err
 			}
+			assignees[assigneeID] = assignee
 			assigneeCommentMap[assigneeID] = comment
 		}
 
@@ -186,12 +192,8 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
 	if issue.Milestone != nil {
 		notify_service.IssueChangeMilestone(ctx, issue.Poster, issue, 0)
 	}
-	for _, assigneeID := range assigneeIDs {
-		assignee, err := user_model.GetUserByID(ctx, assigneeID)
-		if err != nil {
-			return ErrDependenciesLeft
-		}
-		notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID])
+	for _, assignee := range assignees {
+		notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assignee.ID])
 	}
 
 	return nil

services/repository/collaboration.go

@@ -100,7 +100,7 @@ func DeleteCollaboration(ctx context.Context, repo *repo_model.Repository, colla
 }
 
 func ReconsiderRepoIssuesAssignee(ctx context.Context, repo *repo_model.Repository, user *user_model.User) error {
-	if canAssigned, err := access_model.CanBeAssigned(ctx, user, repo, true); err != nil || canAssigned {
+	if canAssigned, err := access_model.CanBeAssigned(ctx, user, repo); err != nil || canAssigned {
 		return err
 	}