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

Change/remove a branch of an open issue #9080

Merged
merged 29 commits into from
Sep 8, 2020

Conversation

vedranMv
Copy link
Contributor

@vedranMv vedranMv commented Nov 19, 2019

Fix #3620 This PL addressed inability to change a branch of an open issue

b84622d Follows the pattern of changing a title of an issue (in the backend) and coupled with frontend changes allows changing the branch of an open issue. However, no comments are currently posted to the issue when a branch is changed. I'm still not sure about the best way to implement that but it looks like it might require changes in DB (add old_ref and new_ref columns to 'comment' table).

Perhaps it would also make sense to only merge this part for now, and then follow up with a new PL for comments :)

@vedranMv
Copy link
Contributor Author

668d3c0 Merged master into branch (I messed up commit message)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 20, 2019
@vedranMv vedranMv marked this pull request as ready for review November 20, 2019 16:49
@vedranMv vedranMv changed the title WIP: Change branch of an open issue (#3620) Change branch of an open issue (#3620) Nov 20, 2019
@codecov-io
Copy link

codecov-io commented Nov 20, 2019

Codecov Report

Merging #9080 into master will decrease coverage by 0.02%.
The diff coverage is 4.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9080      +/-   ##
==========================================
- Coverage   42.25%   42.23%   -0.03%     
==========================================
  Files         610      610              
  Lines       80372    80419      +47     
==========================================
+ Hits        33965    33966       +1     
- Misses      42227    42272      +45     
- Partials     4180     4181       +1
Impacted Files Coverage Δ
models/issue.go 54.33% <0%> (-0.48%) ⬇️
modules/notification/indexer/indexer.go 53.52% <0%> (-1.56%) ⬇️
modules/notification/base/null.go 63.63% <0%> (-1.99%) ⬇️
modules/notification/notification.go 74.1% <0%> (-2.2%) ⬇️
services/issue/issue.go 20.93% <0%> (-2.76%) ⬇️
routers/routes/routes.go 87.11% <100%> (+0.01%) ⬆️
routers/repo/issue.go 37.52% <5.26%> (-0.48%) ⬇️
services/pull/merge.go 51.36% <0%> (-0.69%) ⬇️
models/repo.go 49.66% <0%> (-0.14%) ⬇️
services/pull/patch.go 66.03% <0%> (+1.25%) ⬆️
... and 1 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 20d637a...d7cd33e. Read the comment docs.

@lunny lunny added the type/enhancement An improvement of existing functionality label Nov 21, 2019
@lunny
Copy link
Member

lunny commented Nov 21, 2019

@vedranMv Could you give any screenshot?

@vedranMv
Copy link
Contributor Author

Sure
branch-select

@vedranMv vedranMv changed the title Change branch of an open issue (#3620) Change branch of an open issue Nov 21, 2019
models/issue.go Outdated Show resolved Hide resolved

ref := ctx.QueryTrim("ref")
if len(ref) == 0 {
ctx.Error(204)
Copy link
Member

Choose a reason for hiding this comment

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

How can the user remove the target branch?

Copy link
Contributor Author

@vedranMv vedranMv Nov 21, 2019

Choose a reason for hiding this comment

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

So far it can't, however, that's the same problem as when you try to create an issue - once selected, the branch can't be removed short of refreshing the page. I'll look into it

And thanks for all the feedback, it's pretty useful :)

templates/repo/issue/branch_selector_field.tmpl Outdated Show resolved Hide resolved
@vedranMv
Copy link
Contributor Author

20f10af addresses all but one concern, deleting a branch.
I can't think of a simple solution as it involves completely new front-end element :)

@guillep2k
Copy link
Member

20f10af addresses all but one concern, deleting a branch.
I can't think of a simple solution as it involves completely new front-end element :)

Can a ficticious blank (" ") branch name added at the end of the list as a placeholder?

@vedranMv
Copy link
Contributor Author

That's why I like reviews :D I have a tendency to overcomplicate sometimes. I'll fix it later today

@vedranMv
Copy link
Contributor Author

How about 8b68d5d ? There's now a button which appears only when issue has a ref assigned to it? :)
s1
s2

The drawback of this button is that page now has to be refreshed when selecting a branch otherwise the branch list doesn't get updated and you either can't see the 'clear ref' button after the branch has been selected, or it stays there after you've cleared a branch.

Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

This works like a treat!

You may want to update the title of this PR though, to mention the possibility of branch removal. 😉

@GiteaBot 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 22, 2019
@lafriks lafriks modified the milestones: 1.12.0, 1.13.0 May 16, 2020
@vedranMv

This comment has been minimized.

@vedranMv

This comment has been minimized.

@vedranMv
Copy link
Contributor Author

Compiles and works as intended. What am I missing here for build to pass? :)

@techknowlogick
Copy link
Member

@vedranMv make fmt will allow the file to pass lint

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #9080 into master will decrease coverage by 0.05%.
The diff coverage is 22.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9080      +/-   ##
==========================================
- Coverage   43.30%   43.24%   -0.06%     
==========================================
  Files         647      648       +1     
  Lines       71800    71922     +122     
==========================================
+ Hits        31090    31102      +12     
- Misses      35675    35779     +104     
- Partials     5035     5041       +6     
Impacted Files Coverage Δ
cmd/migrate_storage.go 0.00% <0.00%> (ø)
models/issue.go 56.46% <0.00%> (-0.41%) ⬇️
modules/notification/base/null.go 71.42% <0.00%> (-2.11%) ⬇️
modules/notification/indexer/indexer.go 48.61% <0.00%> (-1.39%) ⬇️
modules/notification/notification.go 81.25% <0.00%> (-2.24%) ⬇️
modules/repofiles/update.go 40.30% <0.00%> (-0.31%) ⬇️
modules/repofiles/upload.go 0.00% <0.00%> (ø)
routers/repo/lfs.go 0.00% <0.00%> (ø)
services/issue/issue.go 32.85% <0.00%> (-3.66%) ⬇️
modules/storage/minio.go 48.43% <5.00%> (-20.46%) ⬇️
... and 24 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 0ed8d26...def127d. Read the comment docs.

@stale
Copy link

stale bot commented Jul 26, 2020

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 Jul 26, 2020
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Jul 26, 2020
@stale stale bot removed the issue/stale label Jul 26, 2020
@DefinitelyADev
Copy link
Contributor

Hi, will this get merged in 1.13.0?

That's would be really helpful instead of using ref #issue in the commits.

services/issue/issue.go Outdated Show resolved Hide resolved
Co-authored-by: zeripath <art27@cantab.net>
@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 Sep 4, 2020
@zeripath zeripath requested a review from lunny September 4, 2020 17:00
@techknowlogick techknowlogick merged commit e204398 into go-gitea:master Sep 8, 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/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modifying branch in issues