Skip to content
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.

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

Implement "conversation lock" for issue comments #5073

Merged
merged 60 commits into from Feb 18, 2019

Conversation

adelowo
Copy link
Member

@adelowo adelowo commented Oct 11, 2018

The goal of this PR is to add a lock conversation feature
present in Github.

Fixes #5063 .

  • Lock Conversation
  • Unlock Conversation
  • Update issues templates to show lock events
  • Disable entire comment box when an issue is locked if the user is not an instance admin or repo admin/writer
  • Allow a reason to be specified when locking an issue

screenshot 2018-10-13 at 00 36 06

Youtube video of what it looks like

Conversation lock

@codecov-io
Copy link

codecov-io commented Oct 11, 2018

Codecov Report

Merging #5073 into master will decrease coverage by 0.06%.
The diff coverage is 14.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5073      +/-   ##
==========================================
- Coverage   38.95%   38.88%   -0.07%     
==========================================
  Files         346      349       +3     
  Lines       49635    49754     +119     
==========================================
+ Hits        19334    19347      +13     
- Misses      27512    27612     +100     
- Partials     2789     2795       +6
Impacted Files Coverage Δ
models/migrations/migrations.go 1.52% <ø> (ø) ⬆️
models/issue.go 47.44% <ø> (ø) ⬆️
modules/setting/setting.go 47.68% <ø> (ø) ⬆️
models/issue_comment.go 46.33% <ø> (ø) ⬆️
models/migrations/v80.go 0% <0%> (ø)
routers/repo/issue_lock.go 0% <0%> (ø)
routers/api/v1/repo/issue_comment.go 56.09% <0%> (-0.7%) ⬇️
models/issue_lock.go 0% <0%> (ø)
routers/repo/issue.go 36.51% <33.33%> (-0.06%) ⬇️
routers/routes/routes.go 82.72% <37.5%> (-0.57%) ⬇️
... and 5 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 64ce159...48bff7d. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 11, 2018
@adelowo adelowo changed the title Implement conversation locking Implement conversation locking feature present in Github Oct 11, 2018
@adelowo
Copy link
Member Author

adelowo commented Oct 11, 2018

Any idea why drone is failing with package github.com/golang/lint/golint: code in directory /go/src/github.com/golang/lint/golint expects import "golang.org/x/lint/golint" ? make lint doesn't fail locally.. No idea why

Started failing at 9d66bc3


Edit

Looks like all builds are failing with this cryptic error, #5009 and #5051 just failed

@lunny lunny added this to the 1.7.0 milestone Oct 12, 2018
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Oct 12, 2018
@adelowo adelowo changed the title Implement conversation locking feature present in Github Implement "conversation lock" feature present in Github Oct 12, 2018
custom/conf/app.ini.sample Outdated Show resolved Hide resolved
templates/repo/issue/comment_tab.tmpl Outdated Show resolved Hide resolved
templates/repo/issue/view_content/comments.tmpl Outdated Show resolved Hide resolved
models/issue_lock.go Outdated Show resolved Hide resolved
Copy link
Member

@jonasfranz jonasfranz left a comment

Choose a reason for hiding this comment

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

Please add a middleware to router.go in order to prevent creating comments via POST requests or API requests.


// HasValidReason checks to make sure that the reason submitted in
// the form matches any of the values in the config
func (i IssueLockForm) HasValidReason() bool {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add this to Validate.

Copy link
Member Author

@adelowo adelowo Oct 17, 2018

Choose a reason for hiding this comment

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

I think it is better if explicit enough ( I have a couple merged PRs that follow this route though ) .. And that regular Validate is just meant for validating the struct bindings..

I wouldn't want to mess with bindings.Error

Copy link
Member Author

Choose a reason for hiding this comment

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

Thoughts @JonasFranzDEV ?

Copy link

Choose a reason for hiding this comment

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

Nudge

Copy link
Member

Choose a reason for hiding this comment

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

options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
@adelowo
Copy link
Member Author

adelowo commented Oct 15, 2018

prevent creating comments

You can still create comments but you must have write access,
I attached the repoWriter middleware to the route already..
See https://github.com/go-gitea/gitea/pull/5073/files#diff-7aa71b75b5fddf1332a485052ffbca9aR543

and https://github.com/go-gitea/gitea/pull/5073/files#diff-ece22e0fe138da6e0fe0f25213406313R1033


Would fix for api requests

@techknowlogick techknowlogick modified the milestones: 1.7.0, 1.8.0 Dec 19, 2018
models/issue_lock.go Outdated Show resolved Hide resolved
@@ -61,6 +61,23 @@ var (
}
)

// MustAllowUserComment checks to make sure if an issue is locked.
// If locked and user has permissions to write to the repository,
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making it an option whether or not a repo writer can still comment on a locked issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds nice.

Will this potentially be a global config or an option set at the point of locking the issue ?

Copy link
Member

Choose a reason for hiding this comment

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

I'd make a global option (in the config file) and then a setting in the repo to overwrite the global default.

Copy link
Member

Choose a reason for hiding this comment

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

So, what about this? I think it'd be ok if we move the configuration to another pr.

Copy link
Member

Choose a reason for hiding this comment

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

@kolaente this PR is already long history, let's move such option to other PR

Copy link
Member

Choose a reason for hiding this comment

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

@lafriks sounds good!

@adelowo adelowo force-pushed the lock_conversation branch 2 times, most recently from 0070a94 to 4c3a0b7 Compare January 16, 2019 20:12
custom/conf/app.ini.sample Show resolved Hide resolved
custom/conf/app.ini.sample Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Jan 17, 2019

CI still failed.

@adelowo
Copy link
Member Author

adelowo commented Jan 17, 2019

CI has been fixed @lunny

@adelowo adelowo force-pushed the lock_conversation branch 2 times, most recently from 73a5bca to 428d8df Compare January 19, 2019 14:32
@adelowo
Copy link
Member Author

adelowo commented Jan 19, 2019

Can anyone help me look at the icon thing ? So it doesn't align because other icons have this octicon-primitive-iconName but what I am using doesn't ( octicion-lock and octicon-key) ..

I have added an hack here though :)
428d8df 3

@lafriks
Copy link
Member

lafriks commented Feb 12, 2019

@lunny @jonasfranz please review

@GiteaBot 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 Feb 12, 2019
@lunny
Copy link
Member

lunny commented Feb 17, 2019

@jonasfranz Please review this PR.

@lafriks lafriks dismissed jonasfranz’s stale review February 17, 2019 14:21

Dismissing your review for not ponging back ;)

@lafriks
Copy link
Member

lafriks commented Feb 18, 2019

@kolaente please review

Copy link
Member

@kolaente kolaente left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far, just one tiny nit.

@@ -61,6 +61,23 @@ var (
}
)

// MustAllowUserComment checks to make sure if an issue is locked.
// If locked and user has permissions to write to the repository,
Copy link
Member

Choose a reason for hiding this comment

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

So, what about this? I think it'd be ok if we move the configuration to another pr.

@lafriks lafriks changed the title Implement "conversation lock" feature present in Github Implement "conversation lock" feature Feb 18, 2019
@lafriks lafriks changed the title Implement "conversation lock" feature Implement "conversation lock" for issue comments Feb 18, 2019
@lafriks lafriks merged commit 44114b3 into go-gitea:master Feb 18, 2019
@adelowo adelowo deleted the lock_conversation branch February 19, 2019 14:28
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "lock conversation" for issues
10 participants