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
tokarchuk/v1.17
Jimmy Praet 3 years ago committed by GitHub
parent 49b2cb998b
commit a3efd048a7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 20
      models/issue.go
  2. 21
      models/issues/content_history.go
  3. 11
      models/issues/content_history_test.go
  4. 21
      services/comments/comments.go
  5. 4
      templates/repo/pulls/files.tmpl
  6. 9
      web_src/js/features/repo-issue-content.js

@ -824,14 +824,25 @@ func (issue *Issue) UpdateAttachments(uuids []string) (err error) {
// ChangeContent changes issue content, as the given user. // ChangeContent changes issue content, as the given user.
func (issue *Issue) ChangeContent(doer *User, content string) (err error) { func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
issue.Content = content
ctx, committer, err := db.TxContext() ctx, committer, err := db.TxContext()
if err != nil { if err != nil {
return err return err
} }
defer committer.Close() 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 { if err = updateIssueCols(db.GetEngine(ctx), issue, "content"); err != nil {
return fmt.Errorf("UpdateIssueCols: %v", err) return fmt.Errorf("UpdateIssueCols: %v", err)
} }
@ -1012,11 +1023,6 @@ func newIssue(ctx context.Context, doer *User, opts NewIssueOptions) (err error)
return err 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) return opts.Issue.addCrossReferences(ctx, doer, false)
} }

@ -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) log.Error("can not query content history for deletion, err=%v", err)
return return
} }
if len(res) <= 1 { if len(res) <= 2 {
return return
} }
@ -83,8 +83,8 @@ func keepLimitedContentHistory(e db.Engine, issueID, commentID int64, limit int)
for outDatedCount > 0 { for outDatedCount > 0 {
var indexToDelete int var indexToDelete int
minEditedInterval := -1 minEditedInterval := -1
// find a history revision with minimal edited interval to delete // 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); i++ { for i := 1; i < len(res)-1; i++ {
editedInterval := int(res[i].EditedUnix - res[i-1].EditedUnix) editedInterval := int(res[i].EditedUnix - res[i-1].EditedUnix)
if minEditedInterval == -1 || editedInterval < minEditedInterval { if minEditedInterval == -1 || editedInterval < minEditedInterval {
minEditedInterval = editedInterval minEditedInterval = editedInterval
@ -167,7 +167,20 @@ func FetchIssueContentHistoryList(dbCtx context.Context, issueID int64, commentI
return res, nil 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 { func SoftDeleteIssueContentHistory(dbCtx context.Context, historyID int64) error {
if _, err := db.GetEngine(dbCtx).ID(historyID).Cols("is_deleted", "content_text").Update(&ContentHistory{ if _, err := db.GetEngine(dbCtx).ID(historyID).Cols("is_deleted", "content_text").Update(&ContentHistory{
IsDeleted: true, IsDeleted: true,

@ -53,6 +53,11 @@ func TestContentHistory(t *testing.T) {
list2, _ := FetchIssueContentHistoryList(dbCtx, 10, 100) list2, _ := FetchIssueContentHistoryList(dbCtx, 10, 100)
assert.Len(t, list2, 5) 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) h6, h6Prev, _ := GetIssueContentHistoryAndPrev(dbCtx, 6)
assert.EqualValues(t, 6, h6.ID) assert.EqualValues(t, 6, h6.ID)
assert.EqualValues(t, 5, h6Prev.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, 6, h6.ID)
assert.EqualValues(t, 4, h6Prev.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) keepLimitedContentHistory(dbEngine, 10, 100, 3)
list1, _ = FetchIssueContentHistoryList(dbCtx, 10, 0) list1, _ = FetchIssueContentHistoryList(dbCtx, 10, 0)
assert.Len(t, list1, 3) assert.Len(t, list1, 3)
list2, _ = FetchIssueContentHistoryList(dbCtx, 10, 100) list2, _ = FetchIssueContentHistoryList(dbCtx, 10, 100)
assert.Len(t, list2, 3) assert.Len(t, list2, 3)
assert.EqualValues(t, 7, list2[0].HistoryID) assert.EqualValues(t, 8, list2[0].HistoryID)
assert.EqualValues(t, 6, list2[1].HistoryID) assert.EqualValues(t, 7, list2[1].HistoryID)
assert.EqualValues(t, 4, list2[2].HistoryID) assert.EqualValues(t, 4, list2[2].HistoryID)
} }

@ -25,10 +25,6 @@ func CreateIssueComment(doer *models.User, repo *models.Repository, issue *model
if err != nil { if err != nil {
return nil, err 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) mentions, err := issue.FindAndUpdateIssueMentions(db.DefaultContext, doer, comment.Content)
if err != nil { if err != nil {
@ -42,11 +38,26 @@ func CreateIssueComment(doer *models.User, repo *models.Repository, issue *model
// UpdateComment updates information of comment. // UpdateComment updates information of comment.
func UpdateComment(c *models.Comment, doer *models.User, oldContent string) error { 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 { if err := models.UpdateComment(c, doer); err != nil {
return err 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) err := issues.SaveIssueContentHistory(db.GetEngine(db.DefaultContext), doer.ID, c.IssueID, c.ID, timeutil.TimeStampNow(), c.Content, false)
if err != nil { if err != nil {
return err return err

@ -1,4 +1,8 @@
{{template "base/head" .}} {{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"> <div class="page-content repository view issue pull files diff">
{{template "repo/header" .}} {{template "repo/header" .}}
<div class="ui container {{if .IsSplitStyle}}fluid padded{{end}}"> <div class="ui container {{if .IsSplitStyle}}fluid padded{{end}}">

@ -106,8 +106,11 @@ function showContentHistoryMenu(issueBaseUrl, $item, commentId) {
export function initRepoIssueContentHistory() { export function initRepoIssueContentHistory() {
const issueIndex = $('#issueIndex').val(); const issueIndex = $('#issueIndex').val();
const $itemIssue = $('.timeline-item.comment.first'); if (!issueIndex) return;
if (!issueIndex || !$itemIssue.length) 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 repoLink = $('#repolink').val();
const issueBaseUrl = `${appSubUrl}/${repoLink}/issues/${issueIndex}`; const issueBaseUrl = `${appSubUrl}/${repoLink}/issues/${issueIndex}`;
@ -123,7 +126,7 @@ export function initRepoIssueContentHistory() {
i18nTextDeleteFromHistoryConfirm = resp.i18n.textDeleteFromHistoryConfirm; i18nTextDeleteFromHistoryConfirm = resp.i18n.textDeleteFromHistoryConfirm;
i18nTextOptions = resp.i18n.textOptions; i18nTextOptions = resp.i18n.textOptions;
if (resp.editedHistoryCountMap[0]) { if (resp.editedHistoryCountMap[0] && $itemIssue.length) {
showContentHistoryMenu(issueBaseUrl, $itemIssue, '0'); showContentHistoryMenu(issueBaseUrl, $itemIssue, '0');
} }
for (const [commentId, _editedCount] of Object.entries(resp.editedHistoryCountMap)) { for (const [commentId, _editedCount] of Object.entries(resp.editedHistoryCountMap)) {

Loading…
Cancel
Save