Skip to content

Fix 404 when click compare on non-forked repository #948

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

Closed
wants to merge 2 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 15, 2017

Fix #936

@lunny lunny added the type/bug label Feb 15, 2017
@lunny lunny added this to the 1.1.0 milestone Feb 15, 2017
@lunny lunny mentioned this pull request Feb 15, 2017
6 tasks
@Bwko
Copy link
Member

Bwko commented Feb 15, 2017

This fix will reintroduce #304
You could cherry pick this commit

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 15, 2017
@lunny lunny removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 15, 2017
@lunny
Copy link
Member Author

lunny commented Feb 15, 2017

OK. I will see that.

@lunny lunny force-pushed the lunny/compare_404 branch from 10e2e97 to 1dd2c4c Compare February 16, 2017 04:50
@lunny
Copy link
Member Author

lunny commented Feb 16, 2017

@Bwko done.

@Bwko
Copy link
Member

Bwko commented Feb 16, 2017

LGTM

@tboerger tboerger 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 Feb 16, 2017
@@ -10,7 +10,7 @@
<div class="ui secondary menu">
{{if .PullRequestCtx.Allowed}}
<div class="fitted item">
<a href="{{.BaseRepo.Link}}/compare/{{.BaseRepo.DefaultBranch}}...{{.Username}}:{{.BranchName}}">
<a href="{{.BaseRepo.Link}}/compare/{{.BaseRepo.DefaultBranch}}...{{.BranchName}}">
Copy link
Member

Choose a reason for hiding this comment

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

Removing {{.Username}} fixes two-branches-inside-the-same-repo comparisons, but break comparisons across forks.

Copy link
Member Author

Choose a reason for hiding this comment

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

But on compare UI, it's not ready for compare between forks. I think this need another PR to do that. For this one, we should let the button works at first.
Gitea's vs Github's
untitled

Copy link
Member

Choose a reason for hiding this comment

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

How about you just make sure that .Username expands to what you expect it to expand, and later work on allowing different names ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the later work should be similar with github. Add two dropdown menus and list the forked repos.

@appleboy
Copy link
Member

appleboy commented Mar 2, 2017

LGTM

@tboerger tboerger 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 Mar 2, 2017
@lunny
Copy link
Member Author

lunny commented Mar 3, 2017

I will close this one since it will break comparing across forks and I will send another one.

@lunny lunny closed this Mar 3, 2017
@lunny lunny deleted the lunny/compare_404 branch March 3, 2017 01:51
@lunny lunny removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Mar 3, 2017
@lunny lunny removed this from the 1.1.0 milestone Mar 3, 2017
@lunny
Copy link
Member Author

lunny commented Mar 3, 2017

Deprecated by #1104

@tboerger tboerger added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Mar 3, 2017
@lunny lunny removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Mar 3, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compare-Button leads to 404
7 participants