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

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

This comment has been minimized.

Copy link

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 label Oct 11, 2018

@adelowo adelowo changed the title Implement conversation locking Implement conversation locking feature present in Github Oct 11, 2018

@adelowo

This comment has been minimized.

Copy link
Member Author

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 kind/feature label Oct 12, 2018

@adelowo adelowo force-pushed the adelowo:lock_conversation branch from 671749f to 3252b6f 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
@jonasfranz
Copy link
Member

left a comment

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 {

This comment has been minimized.

Copy link
@jonasfranz

jonasfranz Oct 15, 2018

Member

Maybe add this to Validate.

This comment has been minimized.

Copy link
@adelowo

adelowo Oct 17, 2018

Author Member

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

This comment has been minimized.

Copy link
@adelowo

adelowo Oct 18, 2018

Author Member

Thoughts @JonasFranzDEV ?

This comment has been minimized.

Copy link
@ghost

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

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,

This comment has been minimized.

Copy link
@kolaente

kolaente Jan 16, 2019

Member

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

This comment has been minimized.

Copy link
@adelowo

adelowo Jan 16, 2019

Author Member

Sounds nice.

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

This comment has been minimized.

Copy link
@kolaente

kolaente Jan 16, 2019

Member

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

This comment has been minimized.

Copy link
@kolaente

kolaente Feb 18, 2019

Member

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

This comment has been minimized.

Copy link
@lafriks

lafriks Feb 18, 2019

Member

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

This comment has been minimized.

Copy link
@kolaente

kolaente Feb 18, 2019

Member

@lafriks sounds good!

@adelowo adelowo force-pushed the adelowo:lock_conversation branch 2 times, most recently from 0070a94 to 4c3a0b7 Jan 16, 2019

custom/conf/app.ini.sample Show resolved Hide resolved
custom/conf/app.ini.sample Outdated Show resolved Hide resolved
@lunny

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

CI still failed.

@adelowo adelowo force-pushed the adelowo:lock_conversation branch from a775b61 to 28dffd7 Jan 17, 2019

@adelowo

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

CI has been fixed @lunny

@adelowo adelowo force-pushed the adelowo:lock_conversation branch 2 times, most recently from 73a5bca to 428d8df Jan 19, 2019

@adelowo

This comment has been minimized.

Copy link
Member Author

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

adelowo added some commits Oct 11, 2018

adelowo added some commits Feb 11, 2019

@adelowo adelowo force-pushed the adelowo:lock_conversation branch from eeae85b to 8315601 Feb 11, 2019

adelowo added some commits Feb 12, 2019

@lafriks

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@lunny @jonasfranz please review

@lunny

lunny approved these changes Feb 12, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Feb 12, 2019

adelowo added some commits Feb 15, 2019

@adelowo adelowo force-pushed the adelowo:lock_conversation branch from 5df50b8 to aefe3bc Feb 15, 2019

@lunny

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

@jonasfranz Please review this PR.

Dismissing your review for not ponging back ;)

@lafriks

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@kolaente please review

@kolaente
Copy link
Member

left a comment

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,

This comment has been minimized.

Copy link
@kolaente

kolaente Feb 18, 2019

Member

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

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@adelowo adelowo deleted the adelowo:lock_conversation branch Feb 19, 2019

Mikescher added a commit to Mikescher/gitea that referenced this pull request Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.