[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

2 files changed, 14 insertions, 9 deletions

diff --git a/models/issues/content_history.go b/models/issues/content_history.go
index 8b00adda99..cd3e217b21 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 0ea1d0f7b2..89d77a1df3 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)
+}