From 331fa449561ae8e1280c9ca939962d48dc51a427 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 28 Feb 2024 19:23:14 +0100 Subject: [PATCH] [BUG] Ensure `HasIssueContentHistory` takes into account `comment_id` - The content history table contains the content history of issues and comments. For issues they are saved with an comment id of zero. - If you want to check if the issue has an content history, it should take into account that SQL has `comment_id = 0`, as it otherwise could return incorrect results when for example the issue already has an comment that has an content history. - Fix the code of `HasIssueContentHistory` to take this into account, it relied on XORM to generate the SQL from the non-default values of the struct, this wouldn't generate the `comment_id = 0` SQL as `0` is the default value of an integer. - Remove an unncessary log (it's not the responsibility of `models` code to do logging). - Adds unit test. - Resolves #2513 --- models/issues/content_history.go | 10 +--------- models/issues/content_history_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/models/issues/content_history.go b/models/issues/content_history.go index 8b00adda9..cd3e217b2 100644 --- a/models/issues/content_history.go +++ b/models/issues/content_history.go @@ -172,15 +172,7 @@ func FetchIssueContentHistoryList(dbCtx context.Context, issueID, commentID int6 // HasIssueContentHistory check if a ContentHistory entry exists func HasIssueContentHistory(dbCtx context.Context, issueID, commentID int64) (bool, error) { - exists, err := db.GetEngine(dbCtx).Cols("id").Exist(&ContentHistory{ - IssueID: issueID, - CommentID: commentID, - }) - if err != nil { - log.Error("can not fetch issue content history. err=%v", err) - return false, err - } - return exists, err + return db.GetEngine(dbCtx).Where("issue_id = ? AND comment_id = ?", issueID, commentID).Exist(new(ContentHistory)) } // SoftDeleteIssueContentHistory soft delete diff --git a/models/issues/content_history_test.go b/models/issues/content_history_test.go index 0ea1d0f7b..89d77a1df 100644 --- a/models/issues/content_history_test.go +++ b/models/issues/content_history_test.go @@ -78,3 +78,16 @@ func TestContentHistory(t *testing.T) { assert.EqualValues(t, 7, list2[1].HistoryID) assert.EqualValues(t, 4, list2[2].HistoryID) } + +func TestHasIssueContentHistory(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // Ensures that comment_id is into taken account even if it's zero. + _ = issues_model.SaveIssueContentHistory(db.DefaultContext, 1, 11, 100, timeutil.TimeStampNow(), "c-a", true) + _ = issues_model.SaveIssueContentHistory(db.DefaultContext, 1, 11, 100, timeutil.TimeStampNow().Add(5), "c-b", false) + + hasHistory1, _ := issues_model.HasIssueContentHistory(db.DefaultContext, 11, 0) + assert.False(t, hasHistory1) + hasHistory2, _ := issues_model.HasIssueContentHistory(db.DefaultContext, 11, 100) + assert.True(t, hasHistory2) +}