Skip to content

Commit a3efd04

Browse files
jpraetjpraet
authored
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
1 parent 49b2cb9 commit a3efd04

File tree

6 files changed

+64
-22
lines changed

6 files changed

+64
-22
lines changed

‎models/issue.go

+13-7
Original file line numberDiff line numberDiff line change
@@ -824,14 +824,25 @@ func (issue *Issue) UpdateAttachments(uuids []string) (err error) {
824824

825825
// ChangeContent changes issue content, as the given user.
826826
func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
827-
issue.Content = content
828-
829827
ctx, committer, err := db.TxContext()
830828
if err != nil {
831829
return err
832830
}
833831
defer committer.Close()
834832

833+
hasContentHistory, err := issues.HasIssueContentHistory(ctx, issue.ID, 0)
834+
if err != nil {
835+
return fmt.Errorf("HasIssueContentHistory: %v", err)
836+
}
837+
if !hasContentHistory {
838+
if err = issues.SaveIssueContentHistory(db.GetEngine(ctx), issue.PosterID, issue.ID, 0,
839+
issue.CreatedUnix, issue.Content, true); err != nil {
840+
return fmt.Errorf("SaveIssueContentHistory: %v", err)
841+
}
842+
}
843+
844+
issue.Content = content
845+
835846
if err = updateIssueCols(db.GetEngine(ctx), issue, "content"); err != nil {
836847
return fmt.Errorf("UpdateIssueCols: %v", err)
837848
}
@@ -1012,11 +1023,6 @@ func newIssue(ctx context.Context, doer *User, opts NewIssueOptions) (err error)
10121023
return err
10131024
}
10141025

1015-
if err = issues.SaveIssueContentHistory(e, doer.ID, opts.Issue.ID, 0,
1016-
timeutil.TimeStampNow(), opts.Issue.Content, true); err != nil {
1017-
return err
1018-
}
1019-
10201026
return opts.Issue.addCrossReferences(ctx, doer, false)
10211027
}
10221028

‎models/issues/content_history.go

+17-4
Original file line numberDiff line numberDiff line change
@@ -75,16 +75,16 @@ func keepLimitedContentHistory(e db.Engine, issueID, commentID int64, limit int)
7575
log.Error("can not query content history for deletion, err=%v", err)
7676
return
7777
}
78-
if len(res) <= 1 {
78+
if len(res) <= 2 {
7979
return
8080
}
8181

8282
outDatedCount := len(res) - limit
8383
for outDatedCount > 0 {
8484
var indexToDelete int
8585
minEditedInterval := -1
86-
// find a history revision with minimal edited interval to delete
87-
for i := 1; i < len(res); i++ {
86+
// find a history revision with minimal edited interval to delete, the first and the last should never be deleted
87+
for i := 1; i < len(res)-1; i++ {
8888
editedInterval := int(res[i].EditedUnix - res[i-1].EditedUnix)
8989
if minEditedInterval == -1 || editedInterval < minEditedInterval {
9090
minEditedInterval = editedInterval
@@ -167,7 +167,20 @@ func FetchIssueContentHistoryList(dbCtx context.Context, issueID int64, commentI
167167
return res, nil
168168
}
169169

170-
//SoftDeleteIssueContentHistory soft delete
170+
// HasIssueContentHistory check if a ContentHistory entry exists
171+
func HasIssueContentHistory(dbCtx context.Context, issueID int64, commentID int64) (bool, error) {
172+
exists, err := db.GetEngine(dbCtx).Cols("id").Exist(&ContentHistory{
173+
IssueID: issueID,
174+
CommentID: commentID,
175+
})
176+
if err != nil {
177+
log.Error("can not fetch issue content history. err=%v", err)
178+
return false, err
179+
}
180+
return exists, err
181+
}
182+
183+
// SoftDeleteIssueContentHistory soft delete
171184
func SoftDeleteIssueContentHistory(dbCtx context.Context, historyID int64) error {
172185
if _, err := db.GetEngine(dbCtx).ID(historyID).Cols("is_deleted", "content_text").Update(&ContentHistory{
173186
IsDeleted: true,

‎models/issues/content_history_test.go

+8-3
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,11 @@ func TestContentHistory(t *testing.T) {
5353
list2, _ := FetchIssueContentHistoryList(dbCtx, 10, 100)
5454
assert.Len(t, list2, 5)
5555

56+
hasHistory1, _ := HasIssueContentHistory(dbCtx, 10, 0)
57+
assert.True(t, hasHistory1)
58+
hasHistory2, _ := HasIssueContentHistory(dbCtx, 10, 1)
59+
assert.False(t, hasHistory2)
60+
5661
h6, h6Prev, _ := GetIssueContentHistoryAndPrev(dbCtx, 6)
5762
assert.EqualValues(t, 6, h6.ID)
5863
assert.EqualValues(t, 5, h6Prev.ID)
@@ -63,13 +68,13 @@ func TestContentHistory(t *testing.T) {
6368
assert.EqualValues(t, 6, h6.ID)
6469
assert.EqualValues(t, 4, h6Prev.ID)
6570

66-
// only keep 3 history revisions for comment_id=100
71+
// only keep 3 history revisions for comment_id=100, the first and the last should never be deleted
6772
keepLimitedContentHistory(dbEngine, 10, 100, 3)
6873
list1, _ = FetchIssueContentHistoryList(dbCtx, 10, 0)
6974
assert.Len(t, list1, 3)
7075
list2, _ = FetchIssueContentHistoryList(dbCtx, 10, 100)
7176
assert.Len(t, list2, 3)
72-
assert.EqualValues(t, 7, list2[0].HistoryID)
73-
assert.EqualValues(t, 6, list2[1].HistoryID)
77+
assert.EqualValues(t, 8, list2[0].HistoryID)
78+
assert.EqualValues(t, 7, list2[1].HistoryID)
7479
assert.EqualValues(t, 4, list2[2].HistoryID)
7580
}

‎services/comments/comments.go

+16-5
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,6 @@ func CreateIssueComment(doer *models.User, repo *models.Repository, issue *model
2525
if err != nil {
2626
return nil, err
2727
}
28-
err = issues.SaveIssueContentHistory(db.GetEngine(db.DefaultContext), doer.ID, issue.ID, comment.ID, timeutil.TimeStampNow(), comment.Content, true)
29-
if err != nil {
30-
return nil, err
31-
}
3228

3329
mentions, err := issue.FindAndUpdateIssueMentions(db.DefaultContext, doer, comment.Content)
3430
if err != nil {
@@ -42,11 +38,26 @@ func CreateIssueComment(doer *models.User, repo *models.Repository, issue *model
4238

4339
// UpdateComment updates information of comment.
4440
func UpdateComment(c *models.Comment, doer *models.User, oldContent string) error {
41+
var needsContentHistory = c.Content != oldContent &&
42+
(c.Type == models.CommentTypeComment || c.Type == models.CommentTypeReview || c.Type == models.CommentTypeCode)
43+
if needsContentHistory {
44+
hasContentHistory, err := issues.HasIssueContentHistory(db.DefaultContext, c.IssueID, c.ID)
45+
if err != nil {
46+
return err
47+
}
48+
if !hasContentHistory {
49+
if err = issues.SaveIssueContentHistory(db.GetEngine(db.DefaultContext), c.PosterID, c.IssueID, c.ID,
50+
c.CreatedUnix, oldContent, true); err != nil {
51+
return err
52+
}
53+
}
54+
}
55+
4556
if err := models.UpdateComment(c, doer); err != nil {
4657
return err
4758
}
4859

49-
if c.Type == models.CommentTypeComment && c.Content != oldContent {
60+
if needsContentHistory {
5061
err := issues.SaveIssueContentHistory(db.GetEngine(db.DefaultContext), doer.ID, c.IssueID, c.ID, timeutil.TimeStampNow(), c.Content, false)
5162
if err != nil {
5263
return err

‎templates/repo/pulls/files.tmpl

+4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
{{template "base/head" .}}
2+
3+
<input type="hidden" id="repolink" value="{{$.RepoRelPath}}">
4+
<input type="hidden" id="issueIndex" value="{{.Issue.Index}}"/>
5+
26
<div class="page-content repository view issue pull files diff">
37
{{template "repo/header" .}}
48
<div class="ui container {{if .IsSplitStyle}}fluid padded{{end}}">

‎web_src/js/features/repo-issue-content.js

+6-3
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,11 @@ function showContentHistoryMenu(issueBaseUrl, $item, commentId) {
106106

107107
export function initRepoIssueContentHistory() {
108108
const issueIndex = $('#issueIndex').val();
109-
const $itemIssue = $('.timeline-item.comment.first');
110-
if (!issueIndex || !$itemIssue.length) return;
109+
if (!issueIndex) return;
110+
111+
const $itemIssue = $('.repository.issue .timeline-item.comment.first'); // issue(PR) main content
112+
const $comments = $('.repository.issue .comment-list .comment'); // includes: issue(PR) comments, code rerview comments
113+
if (!$itemIssue.length && !$comments.length) return;
111114

112115
const repoLink = $('#repolink').val();
113116
const issueBaseUrl = `${appSubUrl}/${repoLink}/issues/${issueIndex}`;
@@ -123,7 +126,7 @@ export function initRepoIssueContentHistory() {
123126
i18nTextDeleteFromHistoryConfirm = resp.i18n.textDeleteFromHistoryConfirm;
124127
i18nTextOptions = resp.i18n.textOptions;
125128

126-
if (resp.editedHistoryCountMap[0]) {
129+
if (resp.editedHistoryCountMap[0] && $itemIssue.length) {
127130
showContentHistoryMenu(issueBaseUrl, $itemIssue, '0');
128131
}
129132
for (const [commentId, _editedCount] of Object.entries(resp.editedHistoryCountMap)) {

0 commit comments

Comments
 (0)