Merge pull request 'fix: issue labels are not set after deleting one label' (#5843) from earl-warren/forgejo:wip-unlabeled into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5843
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>

models/issues/issue_label.go

@@ -111,9 +111,7 @@ func NewIssueLabel(ctx context.Context, issue *Issue, label *Label, doer *user_m
 		return err
 	}
 
-	issue.isLabelsLoaded = false
-	issue.Labels = nil
-	if err = issue.LoadLabels(ctx); err != nil {
+	if err = issue.ReloadLabels(ctx); err != nil {
 		return err
 	}
 
@@ -161,10 +159,7 @@ func NewIssueLabels(ctx context.Context, issue *Issue, labels []*Label, doer *us
 		return err
 	}
 
-	// reload all labels
-	issue.isLabelsLoaded = false
-	issue.Labels = nil
-	if err = issue.LoadLabels(ctx); err != nil {
+	if err = issue.ReloadLabels(ctx); err != nil {
 		return err
 	}
 
@@ -205,8 +200,7 @@ func DeleteIssueLabel(ctx context.Context, issue *Issue, label *Label, doer *use
 		return err
 	}
 
-	issue.Labels = nil
-	return issue.LoadLabels(ctx)
+	return issue.ReloadLabels(ctx)
 }
 
 // DeleteLabelsByRepoID  deletes labels of some repository
@@ -326,14 +320,23 @@ func FixIssueLabelWithOutsideLabels(ctx context.Context) (int64, error) {
 	return res.RowsAffected()
 }
 
-// LoadLabels loads labels
+// LoadLabels only if they are not already set
 func (issue *Issue) LoadLabels(ctx context.Context) (err error) {
-	if !issue.isLabelsLoaded && issue.Labels == nil && issue.ID != 0 {
+	if !issue.isLabelsLoaded && issue.Labels == nil {
+		if err := issue.ReloadLabels(ctx); err != nil {
+			return err
+		}
+		issue.isLabelsLoaded = true
+	}
+	return nil
+}
+
+func (issue *Issue) ReloadLabels(ctx context.Context) (err error) {
+	if issue.ID != 0 {
 		issue.Labels, err = GetLabelsByIssueID(ctx, issue.ID)
 		if err != nil {
 			return fmt.Errorf("getLabelsByIssueID [%d]: %w", issue.ID, err)
 		}
-		issue.isLabelsLoaded = true
 	}
 	return nil
 }
@@ -496,9 +499,7 @@ func ReplaceIssueLabels(ctx context.Context, issue *Issue, labels []*Label, doer
 		}
 	}
 
-	issue.isLabelsLoaded = false
-	issue.Labels = nil
-	if err = issue.LoadLabels(ctx); err != nil {
+	if err = issue.ReloadLabels(ctx); err != nil {
 		return err
 	}
 

models/issues/issue_label_test.go

@@ -15,6 +15,114 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestIssueNewIssueLabels(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+	label1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 1})
+	label2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 4})
+	doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	label3 := &issues_model.Label{RepoID: 1, Name: "label3", Color: "#123"}
+	require.NoError(t, issues_model.NewLabel(db.DefaultContext, label3))
+
+	// label1 is already set, do nothing
+	// label3 is new, add it
+	require.NoError(t, issues_model.NewIssueLabels(db.DefaultContext, issue, []*issues_model.Label{label1, label3}, doer))
+
+	assert.Len(t, issue.Labels, 3)
+	// check that the pre-existing label1 is still present
+	assert.Equal(t, label1.ID, issue.Labels[0].ID)
+	// check that new label3 was added
+	assert.Equal(t, label3.ID, issue.Labels[1].ID)
+	// check that pre-existing label2 was not removed
+	assert.Equal(t, label2.ID, issue.Labels[2].ID)
+}
+
+func TestIssueNewIssueLabel(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3})
+	doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	label := &issues_model.Label{RepoID: 1, Name: "label3", Color: "#123"}
+	require.NoError(t, issues_model.NewLabel(db.DefaultContext, label))
+
+	require.NoError(t, issues_model.NewIssueLabel(db.DefaultContext, issue, label, doer))
+
+	assert.Len(t, issue.Labels, 1)
+	assert.Equal(t, label.ID, issue.Labels[0].ID)
+}
+
+func TestIssueReplaceIssueLabels(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+	label1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 1})
+	label2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 4})
+	doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	label3 := &issues_model.Label{RepoID: 1, Name: "label3", Color: "#123"}
+	require.NoError(t, issues_model.NewLabel(db.DefaultContext, label3))
+
+	issue.LoadLabels(db.DefaultContext)
+	assert.Len(t, issue.Labels, 2)
+	assert.Equal(t, label1.ID, issue.Labels[0].ID)
+	assert.Equal(t, label2.ID, issue.Labels[1].ID)
+
+	// label1 is already set, do nothing
+	// label3 is new, add it
+	// label2 is not in the list but already set, remove it
+	require.NoError(t, issues_model.ReplaceIssueLabels(db.DefaultContext, issue, []*issues_model.Label{label1, label3}, doer))
+
+	assert.Len(t, issue.Labels, 2)
+	assert.Equal(t, label1.ID, issue.Labels[0].ID)
+	assert.Equal(t, label3.ID, issue.Labels[1].ID)
+}
+
+func TestIssueDeleteIssueLabel(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+	label1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 1})
+	label2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 4})
+	doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	issue.LoadLabels(db.DefaultContext)
+	assert.Len(t, issue.Labels, 2)
+	assert.Equal(t, label1.ID, issue.Labels[0].ID)
+	assert.Equal(t, label2.ID, issue.Labels[1].ID)
+
+	require.NoError(t, issues_model.DeleteIssueLabel(db.DefaultContext, issue, label2, doer))
+
+	assert.Len(t, issue.Labels, 1)
+	assert.Equal(t, label1.ID, issue.Labels[0].ID)
+}
+
+func TestIssueLoadLabels(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+	label1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 1})
+	label2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 4})
+
+	assert.Empty(t, issue.Labels)
+	issue.LoadLabels(db.DefaultContext)
+	assert.Len(t, issue.Labels, 2)
+	assert.Equal(t, label1.ID, issue.Labels[0].ID)
+	assert.Equal(t, label2.ID, issue.Labels[1].ID)
+
+	unittest.AssertSuccessfulDelete(t, &issues_model.IssueLabel{IssueID: issue.ID, LabelID: label2.ID})
+
+	// the database change is not noticed because the labels are cached
+	issue.LoadLabels(db.DefaultContext)
+	assert.Len(t, issue.Labels, 2)
+
+	issue.ReloadLabels(db.DefaultContext)
+	assert.Len(t, issue.Labels, 1)
+	assert.Equal(t, label1.ID, issue.Labels[0].ID)
+}
+
 func TestNewIssueLabelsScope(t *testing.T) {
 	require.NoError(t, unittest.PrepareTestDatabase())