Improvements to content history #17746

jpraet · 2021-11-21T17:26:30Z

initialize content history when making an edit to an old item created before the introduction of content history (Comment history is loading slowly + has an offByOne error #17676)
also show edit history for code comments on files tab of pull request

* 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

jpraet · 2021-11-21T17:30:40Z

Should I remove these calls that eagerly initialize the content history when creating a new issue / pr / comment?
Now that the content history is initialized whenever an edit is made this is technically no longer needed.

gitea/models/issue.go

Lines 1015 to 1018 in d710af6

    
           if err = issues.SaveIssueContentHistory(e, doer.ID, opts.Issue.ID, 0, 
        
           	timeutil.TimeStampNow(), opts.Issue.Content, true); err != nil { 
        
           	return err 
        
           }

gitea/services/comments/comments.go

Lines 28 to 31 in c5c88f2

    
           err = issues.SaveIssueContentHistory(db.GetEngine(db.DefaultContext), doer.ID, issue.ID, comment.ID, timeutil.TimeStampNow(), comment.Content, true) 
        
           if err != nil { 
        
           	return nil, err 
        
           }

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

jpraet · 2021-11-21T17:54:12Z

services/comments/comments.go

 // 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)


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

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.

services/comments/comments.go

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

wxiaoguang · 2021-11-21T18:04:24Z

I also added a patch to fix a flaw (I was going to fix it, but since we have this PR, so I think it's good to have it now 😊)

wxiaoguang · 2021-11-21T18:09:55Z

Should I remove these calls that eagerly initialize the content history when creating a new issue / pr / comment? Now that the content history is initialized whenever an edit is made this is technically no longer needed.

Yep, I think we can remove them, then an issue without editing won't have history data, it would save some storage space.

wxiaoguang · 2021-11-21T18:25:05Z

Tested on my side and LGTM.

codecov-commenter · 2021-11-21T18:45:46Z

Codecov Report

❗ No coverage uploaded for pull request base (main@8511eec). Click here to learn what that means.
The diff coverage is 56.66%.

❗ Current head b25731e differs from pull request most recent head 547cee9. Consider uploading reports for the commit 547cee9 to get more accurate results

@@           Coverage Diff           @@
##             main   #17746   +/-   ##
=======================================
  Coverage        ?   45.54%           
=======================================
  Files           ?      807           
  Lines           ?    89885           
  Branches        ?        0           
=======================================
  Hits            ?    40936           
  Misses          ?    42400           
  Partials        ?     6549

Impacted Files	Coverage Δ
models/issue.go	`58.65% <25.00%> (ø)`
services/comments/comments.go	`62.79% <63.63%> (ø)`
models/issues/content_history.go	`68.99% <72.72%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8511eec...547cee9. Read the comment docs.

services/comments/comments.go

lunny · 2021-11-22T11:18:30Z

models/issues/content_history.go

+		CommentID: commentID,
+	})
+	if err != nil {
+		log.Error("can not fetch issue content history. err=%v", err)


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

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:

The error may be ignored be callers

The caller prints the error with a unrelated stacktrace (especially in Go, the errors don't have stacktrace)

* 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

Improvements to content history

e574f78

* 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

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 21, 2021

wxiaoguang reviewed Nov 21, 2021

View reviewed changes

web_src/js/features/repo-issue-content.js Outdated Show resolved Hide resolved

jpraet commented Nov 21, 2021

View reviewed changes

wxiaoguang reviewed Nov 21, 2021

View reviewed changes

services/comments/comments.go Show resolved Hide resolved

jpraet and others added 2 commits November 21, 2021 19:01

Apply code suggestion of wxiaoguang

8ab05a0

Fix a flaw in keepLimitedContentHistory

0dc7e5c

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

wxiaoguang and others added 2 commits November 22, 2021 02:13

Fix keepLimitedContentHistory unit test

b25731e

Remove obsolete eager initialization of content history

547cee9

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 21, 2021

wxiaoguang approved these changes Nov 21, 2021

View reviewed changes

wxiaoguang added this to the 1.16.0 milestone Nov 21, 2021

wxiaoguang added the type/enhancement An improvement of existing functionality label Nov 21, 2021

lunny reviewed Nov 22, 2021

View reviewed changes

services/comments/comments.go Show resolved Hide resolved

lunny added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Nov 22, 2021

lunny reviewed Nov 22, 2021

View reviewed changes

lunny approved these changes Nov 22, 2021

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 22, 2021

lunny and others added 2 commits November 22, 2021 19:20

Merge branch 'main' into go-gitea#17676-init-content-history

1899189

Merge branch 'main' into go-gitea#17676-init-content-history

4a8e899

wxiaoguang approved these changes Nov 22, 2021

View reviewed changes

wxiaoguang merged commit a3efd04 into go-gitea:main Nov 22, 2021

wxiaoguang mentioned this pull request Nov 22, 2021

Comment history is loading slowly + has an offByOne error #17676

Closed

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Improvements to content history #17746

Improvements to content history #17746

Uh oh!

jpraet commented Nov 21, 2021

Uh oh!

jpraet commented Nov 21, 2021

Uh oh!

Uh oh!

jpraet Nov 21, 2021

Uh oh!

wxiaoguang Nov 21, 2021

Uh oh!

Uh oh!

wxiaoguang commented Nov 21, 2021

Uh oh!

wxiaoguang commented Nov 21, 2021 •

edited

Loading

Uh oh!

wxiaoguang commented Nov 21, 2021

Uh oh!

codecov-commenter commented Nov 21, 2021

Uh oh!

Uh oh!

lunny Nov 22, 2021

Uh oh!

wxiaoguang Nov 22, 2021

Uh oh!

Uh oh!

Uh oh!

Improvements to content history #17746

Improvements to content history #17746

Uh oh!

Conversation

jpraet commented Nov 21, 2021

Uh oh!

jpraet commented Nov 21, 2021

Uh oh!

Uh oh!

jpraet Nov 21, 2021

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Nov 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wxiaoguang commented Nov 21, 2021

Uh oh!

wxiaoguang commented Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wxiaoguang commented Nov 21, 2021

Uh oh!

codecov-commenter commented Nov 21, 2021

Codecov Report

Uh oh!

Uh oh!

lunny Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wxiaoguang commented Nov 21, 2021 •

edited

Loading