Skip to content

Commit 331fa44

Browse files
committed
[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 go-gitea#2513
1 parent d0e5af7 commit 331fa44

File tree

2 files changed

+14
-9
lines changed

2 files changed

+14
-9
lines changed

models/issues/content_history.go

+1-9
Original file line numberDiff line numberDiff line change
@@ -172,15 +172,7 @@ func FetchIssueContentHistoryList(dbCtx context.Context, issueID, commentID int6
172172

173173
// HasIssueContentHistory check if a ContentHistory entry exists
174174
func HasIssueContentHistory(dbCtx context.Context, issueID, commentID int64) (bool, error) {
175-
exists, err := db.GetEngine(dbCtx).Cols("id").Exist(&ContentHistory{
176-
IssueID: issueID,
177-
CommentID: commentID,
178-
})
179-
if err != nil {
180-
log.Error("can not fetch issue content history. err=%v", err)
181-
return false, err
182-
}
183-
return exists, err
175+
return db.GetEngine(dbCtx).Where("issue_id = ? AND comment_id = ?", issueID, commentID).Exist(new(ContentHistory))
184176
}
185177

186178
// SoftDeleteIssueContentHistory soft delete

models/issues/content_history_test.go

+13
Original file line numberDiff line numberDiff line change
@@ -78,3 +78,16 @@ func TestContentHistory(t *testing.T) {
7878
assert.EqualValues(t, 7, list2[1].HistoryID)
7979
assert.EqualValues(t, 4, list2[2].HistoryID)
8080
}
81+
82+
func TestHasIssueContentHistory(t *testing.T) {
83+
assert.NoError(t, unittest.PrepareTestDatabase())
84+
85+
// Ensures that comment_id is into taken account even if it's zero.
86+
_ = issues_model.SaveIssueContentHistory(db.DefaultContext, 1, 11, 100, timeutil.TimeStampNow(), "c-a", true)
87+
_ = issues_model.SaveIssueContentHistory(db.DefaultContext, 1, 11, 100, timeutil.TimeStampNow().Add(5), "c-b", false)
88+
89+
hasHistory1, _ := issues_model.HasIssueContentHistory(db.DefaultContext, 11, 0)
90+
assert.False(t, hasHistory1)
91+
hasHistory2, _ := issues_model.HasIssueContentHistory(db.DefaultContext, 11, 100)
92+
assert.True(t, hasHistory2)
93+
}

0 commit comments

Comments
 (0)