Add a way to mark Conversation (code comment) resolved #11037

a1012112796 · 2020-04-10T14:11:57Z

mark Conversation is a way to mark a Conversation is stale or be solved. when it's marked as stale, will be hided like stale. all Pull Request writer , Offical Reviewers and poster can add or remove Conversation resolved mark.

view:

mark Conversation is a way to mark a Conversation is stale or be solved. when it's marked as stale, will be hided like stale. all Pull Request writer , Offical Reviewers and poster can add or remove Conversation resolved mark. Signed-off-by: a1012112796 <[email protected]>

lafriks · 2020-04-10T14:45:05Z

Field should probably be named not Assignee but something like ResolvedBy

models/review.go

6543 · 2020-04-11T00:02:16Z

pleace resolve conflicts and dont use Asignee

* Add ResolveDoer * fix ui Co-Authored-By: Lauris BH <[email protected]> Co-Authored-By: 6543 <[email protected]>

models/issue_comment.go

codecov-io · 2020-04-12T13:54:06Z

Codecov Report

Merging #11037 into master will decrease coverage by 0.06%.
The diff coverage is 6.77%.

@@            Coverage Diff             @@
##           master   #11037      +/-   ##
==========================================
- Coverage   43.53%   43.46%   -0.07%     
==========================================
  Files         599      600       +1     
  Lines       84825    84943     +118     
==========================================
- Hits        36927    36920       -7     
- Misses      43351    43468     +117     
- Partials     4547     4555       +8

Impacted Files	Coverage Δ
models/migrations/migrations.go	`3.50% <ø> (ø)`
models/migrations/v138.go	`0.00% <0.00%> (ø)`
routers/repo/issue.go	`35.56% <0.00%> (-0.19%)`	⬇️
routers/repo/pull_review.go	`0.00% <0.00%> (ø)`
models/review.go	`29.64% <5.00%> (-2.57%)`	⬇️
routers/repo/pull.go	`34.24% <20.00%> (-0.08%)`	⬇️
models/issue_comment.go	`46.11% <25.00%> (-0.58%)`	⬇️
routers/routes/routes.go	`86.59% <100.00%> (+0.01%)`	⬆️
services/pull/check.go	`50.00% <0.00%> (-5.49%)`	⬇️
services/pull/patch.go	`62.93% <0.00%> (-2.80%)`	⬇️
... and 11 more

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 38d5f88...1ae80d1. Read the comment docs.

routers/repo/pull_review.go

Add permission check in UpdateResolveConversation

6543 · 2020-04-12T15:09:35Z

Thanks you can resolve my comments - I'll look into this pr later :)

models/issue_comment.go

models/review.go

routers/repo/pull_review.go

guillep2k · 2020-04-13T00:30:08Z

A migration needs to be added for creating the "ResolveDoerID" column. See the following files for examples:

models/migrations/migrations.go
models/migrations/v135.go

guillep2k · 2020-04-13T03:59:34Z

@6543 probably your test setup has data from other PRs that are not included here. This PR doesn't change any repo units.

6543 · 2020-04-13T09:57:33Z

O kanban hit me, had no time to look at trace yesterday - I'll try on new test enviroment

6543

I'm OK with this - only @guillep2k imprufements are missing
so I wont block.

* change return error for permisson check * add default message for deleted user * get issue message from comment * add migration for ``ResolveDoerID`` column another change: * block mark pending review as resolved because it's not necessary Co-authored-by: guillep2k <[email protected]>

lunny · 2020-04-14T11:58:08Z

Could we change the color of the new button ?

lunny · 2020-04-14T11:59:14Z

models/issue_comment.go

 	for _, comment := range comments {
+		if err := comment.LoadResolveDoer(); err != nil {


When the user deleted, but the comment should be still loaded.

yes, But It willn't be error if user is deleted now, it will load an default user to replace it, but the name to show will be not right , that's the thing what I think now. maybe shold save user name instead of user id, then it willn't be a problem , do you think so ? Thanks

@lunny If understand what you mean, that's taken care of in LoadResolveDoer:

gitea/models/issue_comment.go

Lines 430 to 437 in a2fa3ee

c.ResolveDoer, err = getUserByID(x, c.ResolveDoerID)

if err != nil {

if IsErrUserNotExist(err) {

c.ResolveDoerID = -1

c.ResolveDoer = NewGhostUser()

err = nil

}

}

lunny · 2020-04-14T12:01:50Z

models/review.go

+			return false, err
+		}
+
+		permResult = perm.CanAccess(AccessModeWrite, UnitTypePullRequests)


We should check if this PR's base branch is protected. If it's protected, we should check if it's official review, otherwise we should check if it's the writer of the pull request unit.

Hello, I think I also have done what you say, because IsOfficialReviewer will check them by step. then I'd like to do more easy check more earlly to lower average time cost , so I first check whether doer is poster of PR, if not ,then check if doer have write permission of pull request, at last check if doer is offical reviewer by IsOfficialReviewer. do ypu think so? Thanks.

gitea/models/review.go

Lines 185 to 205 in cc07b9c

// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals)

func IsOfficialReviewer(issue *Issue, reviewer *User) (bool, error) {

return isOfficialReviewer(x, issue, reviewer)

}

func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {

pr, err := getPullRequestByIssueID(e, issue.ID)

if err != nil {

return false, err

}

if err = pr.loadProtectedBranch(e); err != nil {

return false, err

}

if pr.ProtectedBranch == nil {

return false, nil

}

return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)

}

6543

just some docs issue

models/migrations/migrations.go

a1012112796 · 2020-04-14T15:09:52Z

Could we change the color of the new button ?

which color do you think is more better to use in here? Thanks

6543 · 2020-04-14T15:26:47Z

@a1012112796 can you resolve merge conflicts :)

guillep2k · 2020-04-15T04:19:38Z

which color do you think is more better to use in here? Thanks

Something less outstanding, perhaps half color saturation? (#89DD9B)

Or gray/whiteish, black text, similar to GH?

* fix conflix * fix code comments Co-Authored-By: 6543 <[email protected]>

guillep2k

Looks good to me. Just a couple of nits.

models/issue_comment.go

models/review.go

* remove unusefull code Co-authored-by: guillep2k <[email protected]>

guillep2k · 2020-04-17T05:04:40Z

@lunny this requires your review.

guillep2k · 2020-04-18T04:56:19Z

ping lg-tm

* Add a way to mark Conversation (code comment) resolved mark Conversation is a way to mark a Conversation is stale or be solved. when it's marked as stale, will be hided like stale. all Pull Request writer , Offical Reviewers and poster can add or remove Conversation resolved mark. Signed-off-by: a1012112796 <[email protected]> * fix lint * Apply suggestions from code review * Add ResolveDoer * fix ui Co-Authored-By: Lauris BH <[email protected]> Co-Authored-By: 6543 <[email protected]> * change IsResolved to an function Add permission check in UpdateResolveConversation * Apply suggestions from code review * change return error for permisson check * add default message for deleted user * get issue message from comment * add migration for ``ResolveDoerID`` column another change: * block mark pending review as resolved because it's not necessary Co-authored-by: guillep2k <[email protected]> * change button color * resolve button size * fix code style * remove unusefull code Co-authored-by: guillep2k <[email protected]> Co-authored-by: Lauris BH <[email protected]> Co-authored-by: 6543 <[email protected]> Co-authored-by: guillep2k <[email protected]>

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 10, 2020

fix lint

f962c5e

a1012112796 force-pushed the resolve-cov branch from 7a151dc to f962c5e Compare April 10, 2020 15:08

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Apr 10, 2020

6543 reviewed Apr 11, 2020

View reviewed changes

models/review.go Outdated Show resolved Hide resolved

a1012112796 and others added 3 commits April 12, 2020 09:54

Merge branch 'master' into resolve-cov

aa3bae7

Apply suggestions from code review

b99c2ac

* Add ResolveDoer * fix ui Co-Authored-By: Lauris BH <[email protected]> Co-Authored-By: 6543 <[email protected]>

Merge branch 'master' into resolve-cov

b27c8ad

6543 reviewed Apr 12, 2020

View reviewed changes

models/issue_comment.go Outdated Show resolved Hide resolved

6543 requested changes Apr 12, 2020

View reviewed changes

routers/repo/pull_review.go Show resolved Hide resolved

change IsResolved to an function

fdd4b5e

Add permission check in UpdateResolveConversation

guillep2k reviewed Apr 13, 2020

View reviewed changes

guillep2k added this to the 1.12.0 milestone Apr 13, 2020

This comment has been minimized.

Sign in to view

6543 approved these changes Apr 13, 2020

View reviewed changes

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 Apr 13, 2020

a1012112796 and others added 2 commits April 14, 2020 15:58

Merge branch 'master' into resolve-cov

a2fa3ee

lunny requested changes Apr 14, 2020

View reviewed changes

6543 reviewed Apr 14, 2020

View reviewed changes

models/migrations/migrations.go Outdated Show resolved Hide resolved

models/migrations/migrations.go Outdated Show resolved Hide resolved

a1012112796 and others added 4 commits April 15, 2020 16:30

Merge branch 'master' into resolve-cov

87672ff

* fix conflix * fix code comments Co-Authored-By: 6543 <[email protected]>

change button color

4907d7f

Merge branch 'master' into resolve-cov

bbbd8b5

resolve button size

982bd6f

guillep2k approved these changes Apr 16, 2020

View reviewed changes

models/issue_comment.go Outdated Show resolved Hide resolved

models/review.go Outdated Show resolved Hide resolved

models/review.go Outdated Show resolved Hide resolved

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 Apr 16, 2020

a1012112796 and others added 2 commits April 16, 2020 15:27

fix code style

3fbc628

* remove unusefull code Co-authored-by: guillep2k <[email protected]>

Merge branch 'master' into resolve-cov

655e2b6

Merge branch 'master' into resolve-cov

6ecd910

lunny approved these changes Apr 17, 2020

View reviewed changes

Merge branch 'master' into resolve-cov

4776c11

guillep2k and others added 2 commits April 18, 2020 01:56

Merge branch 'master' into resolve-cov

b7b4c0a

Merge branch 'master' into resolve-cov

1ae80d1

guillep2k merged commit 1b86f17 into go-gitea:master Apr 18, 2020

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to mark Conversation (code comment) resolved #11037

Add a way to mark Conversation (code comment) resolved #11037

a1012112796 commented Apr 10, 2020 •

edited

Loading

lafriks commented Apr 10, 2020

6543 commented Apr 11, 2020

codecov-io commented Apr 12, 2020 •

edited

Loading

6543 commented Apr 12, 2020

guillep2k commented Apr 13, 2020

This comment has been minimized.

guillep2k commented Apr 13, 2020

6543 commented Apr 13, 2020 •

edited

Loading

6543 left a comment

lunny commented Apr 14, 2020

lunny Apr 14, 2020

a1012112796 Apr 14, 2020 •

edited

Loading

guillep2k Apr 15, 2020

lunny Apr 14, 2020

a1012112796 Apr 14, 2020

6543 left a comment

a1012112796 commented Apr 14, 2020

6543 commented Apr 14, 2020

guillep2k commented Apr 15, 2020

guillep2k left a comment

guillep2k commented Apr 17, 2020

guillep2k commented Apr 18, 2020

		for _, comment := range comments {
		if err := comment.LoadResolveDoer(); err != nil {

	c.ResolveDoer, err = getUserByID(x, c.ResolveDoerID)
	if err != nil {
	if IsErrUserNotExist(err) {
	c.ResolveDoerID = -1
	c.ResolveDoer = NewGhostUser()
	err = nil
	}
	}


	// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals)
	func IsOfficialReviewer(issue Issue, reviewer User) (bool, error) {
	return isOfficialReviewer(x, issue, reviewer)
	}

	func isOfficialReviewer(e Engine, issue Issue, reviewer User) (bool, error) {
	pr, err := getPullRequestByIssueID(e, issue.ID)
	if err != nil {
	return false, err
	}
	if err = pr.loadProtectedBranch(e); err != nil {
	return false, err
	}
	if pr.ProtectedBranch == nil {
	return false, nil
	}

	return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
	}

Add a way to mark Conversation (code comment) resolved #11037

Add a way to mark Conversation (code comment) resolved #11037

Conversation

a1012112796 commented Apr 10, 2020 • edited Loading

lafriks commented Apr 10, 2020

6543 commented Apr 11, 2020

codecov-io commented Apr 12, 2020 • edited Loading

Codecov Report

6543 commented Apr 12, 2020

guillep2k commented Apr 13, 2020

This comment has been minimized.

guillep2k commented Apr 13, 2020

6543 commented Apr 13, 2020 • edited Loading

6543 left a comment

Choose a reason for hiding this comment

lunny commented Apr 14, 2020

lunny Apr 14, 2020

Choose a reason for hiding this comment

a1012112796 Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

guillep2k Apr 15, 2020

Choose a reason for hiding this comment

lunny Apr 14, 2020

Choose a reason for hiding this comment

a1012112796 Apr 14, 2020

Choose a reason for hiding this comment

6543 left a comment

Choose a reason for hiding this comment

a1012112796 commented Apr 14, 2020

6543 commented Apr 14, 2020

guillep2k commented Apr 15, 2020

guillep2k left a comment

Choose a reason for hiding this comment

guillep2k commented Apr 17, 2020

guillep2k commented Apr 18, 2020

a1012112796 commented Apr 10, 2020 •

edited

Loading

codecov-io commented Apr 12, 2020 •

edited

Loading

6543 commented Apr 13, 2020 •

edited

Loading

a1012112796 Apr 14, 2020 •

edited

Loading