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

[RFC] Update issues via pull requests #3695

Closed
wants to merge 32 commits into from

Conversation

krrutkow
Copy link
Contributor

@krrutkow krrutkow commented Mar 19, 2018

Various improvements on how issues are referenced, opened, or closed by pull requests. Steps to accomplish:

  • factor out reference, open, and close detection code
  • enable commits in a PR creation or update on the tracked branch to reference issues
  • enable PR and issue title or message to reference issues
  • enable PR and issue comments to reference issues
  • enable a PR's commits to open/close issues when PR is merged (Auto-closing of issues via PR doesn't work #2735)
  • enable a PR's title or message to open/close issues when PR is merged (Auto-closing of issues via PR doesn't work #2735)
  • add testing of updates to issues via PR
  • enable a manually merged PR to open/close issues
  • enable code comments to reference issues
  • enable review comment to reference issues
  • add migration to update content of existing references and retroactively create issue references

@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch from d4a0573 to 3ecdbcd Compare March 19, 2018 20:55
@codecov-io
Copy link

codecov-io commented Mar 19, 2018

Codecov Report

Merging #3695 into master will decrease coverage by 0.41%.
The diff coverage is 21.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3695      +/-   ##
==========================================
- Coverage   37.58%   37.17%   -0.42%     
==========================================
  Files         318      306      -12     
  Lines       46928    45653    -1275     
==========================================
- Hits        17638    16971     -667     
+ Misses      26778    26210     -568     
+ Partials     2512     2472      -40
Impacted Files Coverage Δ
models/migrations/migrations.go 2.61% <ø> (ø) ⬆️
routers/api/v1/repo/pull.go 13.38% <ø> (-3.75%) ⬇️
routers/repo/pull.go 34.11% <ø> (+0.38%) ⬆️
models/issue.go 49.25% <0%> (+0.85%) ⬆️
models/migrations/v73.go 0% <0%> (ø) ⬆️
models/issue_comment.go 48.13% <32.65%> (-0.24%) ⬇️
models/pull.go 50.15% <32.65%> (-1.27%) ⬇️
models/action.go 56.81% <45.73%> (-1.95%) ⬇️
routers/repo/issue.go 32.34% <62.5%> (-4.94%) ⬇️
models/update.go 26.2% <7.84%> (-5.45%) ⬇️
... and 120 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 9681c83...ac30d94. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 19, 2018
@lunny lunny added the type/enhancement An improvement of existing functionality label Mar 21, 2018
@lunny lunny added this to the 1.x.x milestone Mar 21, 2018
@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch 2 times, most recently from 4b2d3d9 to 13844b7 Compare March 27, 2018 17:00
@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch from 98d97af to d612d1a Compare April 2, 2018 14:50
@krrutkow krrutkow changed the title [WIP] Update issues via pull requests [RFC] Update issues via pull requests Apr 2, 2018
models/action.go Outdated Show resolved Hide resolved
models/action.go Show resolved Hide resolved
models/issue_comment.go Outdated Show resolved Hide resolved
@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch 2 times, most recently from b49e7b9 to d2e2f9c Compare April 11, 2018 00:31
@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch from adcd721 to 6cba105 Compare April 20, 2018 00:42
@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch from 6cba105 to 57b73c8 Compare May 14, 2018 23:03
@yasuokav
Copy link
Contributor

Maybe we should update the issues when a PR got manually merged?

models/action.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented May 23, 2018

please resolve conflicts

@krrutkow
Copy link
Contributor Author

@yasuokav I'm not sure the PR can be automatically closed if the commits are present since the PR's tracked branch could still be committed to. Commits that are pushed directly to the repo are processed with the pre-existing function CommitRepoAction, so a manual merge commit message can mention the PR to explicitly close it.

@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch from 57b73c8 to 18b6758 Compare May 23, 2018 16:06
@yasuokav
Copy link
Contributor

@krrutkow Thanks for your answer, you may misunderstand what I mean.
If I manually merge the PR branch into master and push to remote, this PR will be automatically marked as merged.

gitea/models/pull.go

Lines 553 to 585 in b908ac9

func (pr *PullRequest) manuallyMerged() bool {
commit, err := pr.getMergeCommit()
if err != nil {
log.Error(4, "PullRequest[%d].getMergeCommit: %v", pr.ID, err)
return false
}
if commit != nil {
pr.MergedCommitID = commit.ID.String()
pr.MergedUnix = util.TimeStamp(commit.Author.When.Unix())
pr.Status = PullRequestStatusManuallyMerged
merger, _ := GetUserByEmail(commit.Author.Email)
// When the commit author is unknown set the BaseRepo owner as merger
if merger == nil {
if pr.BaseRepo.Owner == nil {
if err = pr.BaseRepo.getOwner(x); err != nil {
log.Error(4, "BaseRepo.getOwner[%d]: %v", pr.ID, err)
return false
}
}
merger = pr.BaseRepo.Owner
}
pr.Merger = merger
pr.MergerID = merger.ID
if err = pr.setMerged(); err != nil {
log.Error(4, "PullRequest[%d].setMerged : %v", pr.ID, err)
return false
}
log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String())
return true
}
return false
But MergePullRequestAction does not trigger

@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch from 18b6758 to e9fc2f9 Compare May 25, 2018 13:02
@InExtremaRes
Copy link

What is the status of this? Is ready for use or merge into master?

@lunny
Copy link
Member

lunny commented Jun 21, 2018

@krrutkow It's ready for review? @InExtremaRes need mantianers' review.

@krrutkow krrutkow force-pushed the add-autoclose-issues-via-pr branch from e9fc2f9 to aef893e Compare June 21, 2018 01:09
@krrutkow
Copy link
Contributor Author

I just rebased it to master, its ready for review.

@lafriks lafriks modified the milestones: 1.x.x, 1.6.0 Jun 21, 2018
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.

Nice work! Thank you for your dedication!

models/action.go Outdated Show resolved Hide resolved
models/action.go Outdated Show resolved Hide resolved
models/action.go Outdated Show resolved Hide resolved
models/issue_comment.go Outdated Show resolved Hide resolved
models/issue_comment.go Outdated Show resolved Hide resolved
models/issue_comment.go Outdated Show resolved Hide resolved
@jonasfranz
Copy link
Member

Fixes #4302 and #3134

@lafriks
Copy link
Member

lafriks commented Mar 26, 2019

@MichiiL if you want you can also rebase/fix it and open new PR if original author has no time to fix this

@stale
Copy link

stale bot commented May 25, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label May 25, 2019
@lafriks lafriks added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label May 25, 2019
@stale stale bot removed the issue/stale label May 25, 2019
@techknowlogick techknowlogick modified the milestones: 1.9.0, 1.10.0 Jun 4, 2019
@buhtz
Copy link

buhtz commented Jun 5, 2019

What is the current status?

<span class="text grey"><a href="{{.RefURL}}"><span class="title has-emoji">{{.RefIssue.Title | Escape}}</span> (#{{.RefIssue.Index}})</a></span>
</div>
</div>
{{else if and (eq .Type 24) .RefExists}}
Copy link
Member

Choose a reason for hiding this comment

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

I know this file previously used hardcoded ints to do an equality check against type, but could you change it to do an equality check where 24 could easily be determined? (same applies for other checks too)

@techknowlogick
Copy link
Member

@noMICROSOFTbuhtz the status can be read above. There is feedback that needs to be addressed and conflicts need to be resolved, however due to the change freeze for 1.9.0 being in place this has been moved to the 1.10.0 milestone.

@joelft
Copy link

joelft commented Jun 27, 2019

I've tried to rebase this, but I don't know enough about the code base to do it successfully without breaking things. I'll continue to try, but if anyone else can jump on this it would be awesome. Seems like maybe @krrutkow doesn't have time anymore. 😢

@lunny
Copy link
Member

lunny commented Aug 14, 2019

conflicted

@buhtz
Copy link

buhtz commented Aug 14, 2019

Btw: How can it be that a PR is open for more then one year?

Conflictes? This is not an argument for such a hugh time span.

It is more a clue that the PR is to big and complex - 32 commits solidify this argument. Split it into easer handable commits and PRs.

I would recommand to the core developers to find a standardized way to handle, accept and reject PRs depending on defined parameter (e.g. number of lines, commit count, PR age, ...).

@lafriks
Copy link
Member

lafriks commented Aug 14, 2019

@Codeberg-AsGithubAlternative-buhtz Someone has to take over to resolve conflicts (either by maintainers or someone else by opening new PR), we can't really review while there are conflicts that could result in broken code if not resolved correctly

@techknowlogick techknowlogick modified the milestones: 1.10.0, 1.11.0 Sep 3, 2019
@guillep2k
Copy link
Member

guillep2k commented Sep 10, 2019

Mmm... I'm working on something that should greatly simplify this code; but you were first. Want me to stop over there?
Link #8137

@6543
Copy link
Member

6543 commented Nov 2, 2019

@krrutkow @guillep2k whats the state of this PR ?

lot of code has changed in this area so :(

@guillep2k
Copy link
Member

@krrutkow @guillep2k whats the state of this PR ?

lot of code has changed in this area so :(

#8137 is already merged, and I'm working on closing/reopening by keyword.

@6543
Copy link
Member

6543 commented Nov 2, 2019

so we can considder this as closed?

@krrutkow
Copy link
Contributor Author

krrutkow commented Nov 3, 2019

This PR is free to be closed. It applies to (and was working for) ~v1.6, but it would be nearly a complete rewrite to get working with the current codebase. The feature checklist is valuable though and should be expressed in other issues/PRs to ensure the features of this PR are not lost to the project.

@lunny lunny modified the milestones: 1.11.0, 1.12.0 Dec 13, 2019
@lafriks lafriks modified the milestones: 1.12.0, 1.13.0 May 16, 2020
@techknowlogick techknowlogick removed this from the 1.13.0 milestone Sep 7, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.