Improvements to content history (#17746)

* Improvements to content history

* initialize content history when making an edit to an old item created before the introduction of content history
* show edit history for code comments on pull request files tab

* Fix a flaw in keepLimitedContentHistory
Fix a flaw in keepLimitedContentHistory, the first and the last should never be deleted

* Remove obsolete eager initialization of content history

diff --git a/models/issue.go b/models/issue.go
index 6de7c3204b214e53880880406bba44e70c8b6742..7aab8b93995b16a715f07374b86240b58ac7c563 100644 (file)
--- a/models/issue.go
+++ b/models/issue.go
@@ -824,14 +824,25 @@ func (issue *Issue) UpdateAttachments(uuids []string) (err error) {
 
 // ChangeContent changes issue content, as the given user.
 func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
-       issue.Content = content
-
        ctx, committer, err := db.TxContext()
        if err != nil {
                return err
        }
        defer committer.Close()
 
+       hasContentHistory, err := issues.HasIssueContentHistory(ctx, issue.ID, 0)
+       if err != nil {
+               return fmt.Errorf("HasIssueContentHistory: %v", err)
+       }
+       if !hasContentHistory {
+               if err = issues.SaveIssueContentHistory(db.GetEngine(ctx), issue.PosterID, issue.ID, 0,
+                       issue.CreatedUnix, issue.Content, true); err != nil {
+                       return fmt.Errorf("SaveIssueContentHistory: %v", err)
+               }
+       }
+
+       issue.Content = content
+
        if err = updateIssueCols(db.GetEngine(ctx), issue, "content"); err != nil {
                return fmt.Errorf("UpdateIssueCols: %v", err)
        }
@@ -1012,11 +1023,6 @@ func newIssue(ctx context.Context, doer *User, opts NewIssueOptions) (err error)
                return err
        }
 
-       if err = issues.SaveIssueContentHistory(e, doer.ID, opts.Issue.ID, 0,
-               timeutil.TimeStampNow(), opts.Issue.Content, true); err != nil {
-               return err
-       }
-
        return opts.Issue.addCrossReferences(ctx, doer, false)
 }
 

diff --git a/models/issues/content_history.go b/models/issues/content_history.go
index 04c30fde9236d4edad5283ed4c6f4ae96ae67971..b423fb8fcfebd9943fd336c358a2147ab91cf023 100644 (file)
--- a/models/issues/content_history.go
+++ b/models/issues/content_history.go
@@ -75,7 +75,7 @@ func keepLimitedContentHistory(e db.Engine, issueID, commentID int64, limit int)
                log.Error("can not query content history for deletion, err=%v", err)
                return
        }
-       if len(res) <= 1 {
+       if len(res) <= 2 {
                return
        }
 
@@ -83,8 +83,8 @@ func keepLimitedContentHistory(e db.Engine, issueID, commentID int64, limit int)
        for outDatedCount > 0 {
                var indexToDelete int
                minEditedInterval := -1
-               // find a history revision with minimal edited interval to delete
-               for i := 1; i < len(res); i++ {
+               // find a history revision with minimal edited interval to delete, the first and the last should never be deleted
+               for i := 1; i < len(res)-1; i++ {
                        editedInterval := int(res[i].EditedUnix - res[i-1].EditedUnix)
                        if minEditedInterval == -1 || editedInterval < minEditedInterval {
                                minEditedInterval = editedInterval
@@ -167,7 +167,20 @@ func FetchIssueContentHistoryList(dbCtx context.Context, issueID int64, commentI
        return res, nil
 }
 
-//SoftDeleteIssueContentHistory soft delete
+// HasIssueContentHistory check if a ContentHistory entry exists
+func HasIssueContentHistory(dbCtx context.Context, issueID int64, 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
+}
+
+// SoftDeleteIssueContentHistory soft delete
 func SoftDeleteIssueContentHistory(dbCtx context.Context, historyID int64) error {
        if _, err := db.GetEngine(dbCtx).ID(historyID).Cols("is_deleted", "content_text").Update(&ContentHistory{
                IsDeleted:   true,

diff --git a/models/issues/content_history_test.go b/models/issues/content_history_test.go
index f040a7dc57f117969c7f9240cc729802991b5da6..cc9a7c51074cb13208004c69d3235c9b16eb39d8 100644 (file)
--- a/models/issues/content_history_test.go
+++ b/models/issues/content_history_test.go
@@ -53,6 +53,11 @@ func TestContentHistory(t *testing.T) {
        list2, _ := FetchIssueContentHistoryList(dbCtx, 10, 100)
        assert.Len(t, list2, 5)
 
+       hasHistory1, _ := HasIssueContentHistory(dbCtx, 10, 0)
+       assert.True(t, hasHistory1)
+       hasHistory2, _ := HasIssueContentHistory(dbCtx, 10, 1)
+       assert.False(t, hasHistory2)
+
        h6, h6Prev, _ := GetIssueContentHistoryAndPrev(dbCtx, 6)
        assert.EqualValues(t, 6, h6.ID)
        assert.EqualValues(t, 5, h6Prev.ID)
@@ -63,13 +68,13 @@ func TestContentHistory(t *testing.T) {
        assert.EqualValues(t, 6, h6.ID)
        assert.EqualValues(t, 4, h6Prev.ID)
 
-       // only keep 3 history revisions for comment_id=100
+       // only keep 3 history revisions for comment_id=100, the first and the last should never be deleted
        keepLimitedContentHistory(dbEngine, 10, 100, 3)
        list1, _ = FetchIssueContentHistoryList(dbCtx, 10, 0)
        assert.Len(t, list1, 3)
        list2, _ = FetchIssueContentHistoryList(dbCtx, 10, 100)
        assert.Len(t, list2, 3)
-       assert.EqualValues(t, 7, list2[0].HistoryID)
-       assert.EqualValues(t, 6, list2[1].HistoryID)
+       assert.EqualValues(t, 8, list2[0].HistoryID)
+       assert.EqualValues(t, 7, list2[1].HistoryID)
        assert.EqualValues(t, 4, list2[2].HistoryID)
 }

diff --git a/services/comments/comments.go b/services/comments/comments.go
index d1e5ea4d88a73b5a9df68b111e2fc85e5eec70f9..2477f2d2b0e499ec31f6bf97563a54cd0004379e 100644 (file)
--- a/services/comments/comments.go
+++ b/services/comments/comments.go
@@ -25,10 +25,6 @@ func CreateIssueComment(doer *models.User, repo *models.Repository, issue *model
        if err != nil {
                return nil, err
        }
-       err = issues.SaveIssueContentHistory(db.GetEngine(db.DefaultContext), doer.ID, issue.ID, comment.ID, timeutil.TimeStampNow(), comment.Content, true)
-       if err != nil {
-               return nil, err
-       }
 
        mentions, err := issue.FindAndUpdateIssueMentions(db.DefaultContext, doer, comment.Content)
        if err != nil {
@@ -42,11 +38,26 @@ func CreateIssueComment(doer *models.User, repo *models.Repository, issue *model
 
 // UpdateComment updates information of comment.
 func UpdateComment(c *models.Comment, doer *models.User, oldContent string) error {
+       var needsContentHistory = c.Content != oldContent &&
+               (c.Type == models.CommentTypeComment || c.Type == models.CommentTypeReview || c.Type == models.CommentTypeCode)
+       if needsContentHistory {
+               hasContentHistory, err := issues.HasIssueContentHistory(db.DefaultContext, c.IssueID, c.ID)
+               if err != nil {
+                       return err
+               }
+               if !hasContentHistory {
+                       if err = issues.SaveIssueContentHistory(db.GetEngine(db.DefaultContext), c.PosterID, c.IssueID, c.ID,
+                               c.CreatedUnix, oldContent, true); err != nil {
+                               return err
+                       }
+               }
+       }
+
        if err := models.UpdateComment(c, doer); err != nil {
                return err
        }
 
-       if c.Type == models.CommentTypeComment && c.Content != oldContent {
+       if needsContentHistory {
                err := issues.SaveIssueContentHistory(db.GetEngine(db.DefaultContext), doer.ID, c.IssueID, c.ID, timeutil.TimeStampNow(), c.Content, false)
                if err != nil {
                        return err

diff --git a/templates/repo/pulls/files.tmpl b/templates/repo/pulls/files.tmpl
index 7866697629e3e366b9cf5c6eb0d07161b0c25435..ede487d31b3659c3cbea5c9dd3c33072fd9df542 100644 (file)
--- a/templates/repo/pulls/files.tmpl
+++ b/templates/repo/pulls/files.tmpl
@@ -1,4 +1,8 @@
 {{template "base/head" .}}
+
+<input type="hidden" id="repolink" value="{{$.RepoRelPath}}">
+<input type="hidden" id="issueIndex" value="{{.Issue.Index}}"/>
+
 <div class="page-content repository view issue pull files diff">
        {{template "repo/header" .}}
        <div class="ui container {{if .IsSplitStyle}}fluid padded{{end}}">

diff --git a/web_src/js/features/repo-issue-content.js b/web_src/js/features/repo-issue-content.js
index a2fc6c3cbee69a975f3f59f62091e171e2e28e8c..602523f89d3740e92ee9bf38392114428175ea69 100644 (file)
--- a/web_src/js/features/repo-issue-content.js
+++ b/web_src/js/features/repo-issue-content.js
@@ -106,8 +106,11 @@ function showContentHistoryMenu(issueBaseUrl, $item, commentId) {
 
 export function initRepoIssueContentHistory() {
   const issueIndex = $('#issueIndex').val();
-  const $itemIssue = $('.timeline-item.comment.first');
-  if (!issueIndex || !$itemIssue.length) return;
+  if (!issueIndex) return;
+
+  const $itemIssue = $('.repository.issue .timeline-item.comment.first'); // issue(PR) main content
+  const $comments = $('.repository.issue .comment-list .comment'); // includes: issue(PR) comments, code rerview comments
+  if (!$itemIssue.length && !$comments.length) return;
 
   const repoLink = $('#repolink').val();
   const issueBaseUrl = `${appSubUrl}/${repoLink}/issues/${issueIndex}`;
@@ -123,7 +126,7 @@ export function initRepoIssueContentHistory() {
     i18nTextDeleteFromHistoryConfirm = resp.i18n.textDeleteFromHistoryConfirm;
     i18nTextOptions = resp.i18n.textOptions;
 
-    if (resp.editedHistoryCountMap[0]) {
+    if (resp.editedHistoryCountMap[0] && $itemIssue.length) {
       showContentHistoryMenu(issueBaseUrl, $itemIssue, '0');
     }
     for (const [commentId, _editedCount] of Object.entries(resp.editedHistoryCountMap)) {