From db899c19d88866df984d27cac70b7d63a9fc5a6c Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 7 Nov 2024 10:38:01 +0100 Subject: [PATCH 1/2] fix: issue labels are not set after deleting one label Because issue.isLabelsLoaded = false is missing, LoadLabels is a noop and the issue.Labels is nil. --- models/issues/issue_label.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/issues/issue_label.go b/models/issues/issue_label.go index 2f0e2897a9..90e7f9c625 100644 --- a/models/issues/issue_label.go +++ b/models/issues/issue_label.go @@ -205,6 +205,7 @@ func DeleteIssueLabel(ctx context.Context, issue *Issue, label *Label, doer *use return err } + issue.isLabelsLoaded = false issue.Labels = nil return issue.LoadLabels(ctx) } From f06bdb0552e4925742c95006c9ce13bae734f036 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 7 Nov 2024 10:39:46 +0100 Subject: [PATCH 2/2] chore(refactor): split ReloadLabels out of LoadLabels in issue model Functions modifying the labels in the database (DeleteIssueLabel, NewIssueLabels, NewIssueLabel, ReplaceIssueLabels) need to force reload them. Instead of: issue.isLabelsLoaded = false issue.Labels = nil if err = issue.LoadLabels(ctx); err != nil { return err } They can now use: if err = issue.ReloadLabels(ctx); err != nil { return err } --- models/issues/issue_label.go | 32 ++++----- models/issues/issue_label_test.go | 108 ++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 16 deletions(-) diff --git a/models/issues/issue_label.go b/models/issues/issue_label.go index 90e7f9c625..04e1fa3d7d 100644 --- a/models/issues/issue_label.go +++ b/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,9 +200,7 @@ func DeleteIssueLabel(ctx context.Context, issue *Issue, label *Label, doer *use return err } - issue.isLabelsLoaded = false - issue.Labels = nil - return issue.LoadLabels(ctx) + return issue.ReloadLabels(ctx) } // DeleteLabelsByRepoID deletes labels of some repository @@ -327,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 } @@ -497,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 } diff --git a/models/issues/issue_label_test.go b/models/issues/issue_label_test.go index b6b39d683d..67f4874c8f 100644 --- a/models/issues/issue_label_test.go +++ b/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())