Skip to content

Improvements to content history #17746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 22, 2021
Merged

Improvements to content history #17746

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e574f78
Improvements to content history
jpraet Nov 21, 2021
8ab05a0
Apply code suggestion of wxiaoguang
jpraet Nov 21, 2021
0dc7e5c
Fix a flaw in keepLimitedContentHistory
wxiaoguang Nov 21, 2021
b25731e
Fix keepLimitedContentHistory unit test
wxiaoguang Nov 21, 2021
547cee9
Remove obsolete eager initialization of content history
jpraet Nov 21, 2021
1899189
Merge branch 'main' into #17676-init-content-history
lunny Nov 22, 2021
4a8e899
Merge branch 'main' into #17676-init-content-history
wxiaoguang Nov 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions models/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
15 changes: 14 additions & 1 deletion models/issues/content_history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return the error but add a special log here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it just follows my first PR, a personal preference for error handling.

Usually I would like to report important errors as early as possible.

Otherwise:

  1. The error may be ignored be callers
  2. The caller prints the error with a unrelated stacktrace (especially in Go, the errors don't have stacktrace)

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,
Expand Down
5 changes: 5 additions & 0 deletions models/issues/content_history_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
17 changes: 16 additions & 1 deletion services/comments/comments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have content history for "pending" code comments on non-submitted review? GitHub doesn't do that either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought about it. For a simple idea, if the logic is not complex then we can have it, but if we need to pay much effect to follow the GitHub behavior, well, it depends 😂. Both are fine to me.

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
Expand Down
4 changes: 4 additions & 0 deletions templates/repo/pulls/files.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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}}">
Expand Down
5 changes: 3 additions & 2 deletions web_src/js/features/repo-issue-content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ function showContentHistoryMenu(issueBaseUrl, $item, commentId) {
export function initRepoIssueContentHistory() {
const issueIndex = $('#issueIndex').val();
const $itemIssue = $('.timeline-item.comment.first');
if (!issueIndex || !$itemIssue.length) return;
const $comments = $('.comment');
if (!issueIndex || !$comments.length) return;

const repoLink = $('#repolink').val();
const issueBaseUrl = `${appSubUrl}/${repoLink}/issues/${issueIndex}`;
Expand All @@ -123,7 +124,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)) {
Expand Down