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

Don't show new pr button when page is not compare pull #26431

Merged

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Aug 10, 2023

Before:
image
After:
image
(TestOrg:test is a tag not branch)

Problem:
In the template, we will not add compare pull class when PageIsComparePull is false.

<div role="main" aria-label="{{.Title}}" class="page-content repository diff {{if .PageIsComparePull}}compare pull{{end}}">

But in the js, we are using .repository.compare.pull to find the button:
// Pull request
const $repoComparePull = $('.repository.compare.pull');
if ($repoComparePull.length > 0) {
// show pull request form
$repoComparePull.find('button.show-form').on('click', function (e) {
e.preventDefault();
hideElem($(this).parent());
const $form = $repoComparePull.find('.pullrequest-form');
showElem($form);
});
}

So, if PageIsComparePull is false, the New Pull Request button will be there, but has no response when we click it.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 10, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 10, 2023
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Unrelated to this PR but I would ideally prefer if we eventually remove the need to click this button and just open directly into the PR form.

@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 Aug 10, 2023
@a1012112796
Copy link
Member

That's not a bug, empty pull request is a feature, which learned from gitlab, you can use this featue to create a coding plan with a new branch before add the code. first added in #12543.

@a1012112796
Copy link
Member

a1012112796 commented Aug 11, 2023

example:
 test

test version: 1.21.0+dev-437-gb9baed2c7, will check the master branch latest status later.

--- update :

looks ok also (version: Version: 1.21.0+dev-511-g88479e0df):

image

@wxiaoguang
Copy link
Contributor

Not a bug but a feature, need to write some comments for it.

@silverwind
Copy link
Member

If empty PR is a feature, this change makes no sense. I'm sorry but I'll close it then.

@silverwind silverwind closed this Aug 12, 2023
@wxiaoguang
Copy link
Contributor

If empty PR is a feature, this change makes no sense. I'm sorry but I'll close it then.

why not keep it open and write some comments?

@silverwind
Copy link
Member

You mean make this PR comment-only?

@yp05327
Copy link
Contributor Author

yp05327 commented Aug 15, 2023

No, this is a bug, I know we can create empty pull request, but
when the base or the compare is a tag not branch, we should not display the new PR button
as I have mentioned in the discription

(TestOrg:test is a tag not branch)

So the root reason is that we forgot to check whether this is a tag selected in this compare page, not the problem of empty pull request.

You can confirm the difference in GitHub:
https://github.com/yp05327/test2/compare/main...testtag?expand=1
https://github.com/yp05327/test2/compare/main...testtagpr?expand=1

ps: I noticed this problem because I created a Release with 0 commits, something like this:
image
Then if you click 0 commits you will get this page:
Animation

@a1012112796 a1012112796 reopened this Aug 15, 2023
Copy link
Member

@a1012112796 a1012112796 left a comment

Choose a reason for hiding this comment

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

Okay, I see it.

image

@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 Aug 15, 2023
@a1012112796
Copy link
Member

a1012112796 commented Aug 15, 2023

but looks it's not necessary to show this messsage also. because one of these is not a 'branch' ...

image

ref:

image

or maybe add a new help tips like "can't create pull request because the head or base ref is not a branch. "

@a1012112796
Copy link
Member

a1012112796 commented Aug 15, 2023

so looks this option is better:

diff --git a/templates/repo/diff/compare.tmpl b/templates/repo/diff/compare.tmpl
index 3bb7c2e81..a63d4801d 100644
--- a/templates/repo/diff/compare.tmpl
+++ b/templates/repo/diff/compare.tmpl
@@ -176,7 +176,7 @@
 		</div>
 	</div>
 
-	{{if .IsNothingToCompare}}
+	{{if and .PageIsComparePull .IsNothingToCompare}}
 		{{if and $.IsSigned $.AllowEmptyPr (not .Repository.IsArchived)}}
 			<div class="ui segment">{{.locale.Tr "repo.pulls.nothing_to_compare_and_allow_empty_pr"}}</div>
 			<div class="ui info message show-form-container {{if .Flash}}gt-hidden{{end}}">
@@ -229,7 +229,7 @@
 			{{end}}
 			{{$showDiffBox = true}}
 		{{end}}
-	{{else}}
+	{{else if not .IsNothingToCompare}}
 		{{$showDiffBox = true}}
 	{{end}}
 	</div>

``

@silverwind silverwind self-requested a review August 15, 2023 12:33
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Aug 15, 2023
@silverwind
Copy link
Member

Retracting review for now.

@yp05327
Copy link
Contributor Author

yp05327 commented Aug 18, 2023

so looks this option is better:

Done.

@lunny lunny added this to the 1.21.0 milestone Aug 24, 2023
@lunny lunny added the type/enhancement An improvement of existing functionality label Aug 24, 2023
@lunny
Copy link
Member

lunny commented Aug 24, 2023

But we still need the comment that the two branches are equal.

@yp05327
Copy link
Contributor Author

yp05327 commented Aug 28, 2023

.PageIsComparePull is not just used in the case of tag selected.
We should use if and .HeadIsBranch .BaseIsBranch to check whether tag is selected.

@silverwind
Copy link
Member

silverwind commented Aug 28, 2023

Can we remove the "New PR" button altogether and just show the form directly if applicable? I'm willing to collaborate on this here. It would be nice to have for #26743 because it's rather unintuitive when you F5 on the new PR page and it does not immediatly show the restored content.

@lunny lunny modified the milestones: 1.21.0, 1.22.0 Sep 21, 2023
@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 Jan 20, 2024
@6543 6543 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 20, 2024
@lunny lunny enabled auto-merge (squash) January 21, 2024 14:13
@lunny lunny merged commit b693611 into go-gitea:main Jan 21, 2024
25 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 21, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 22, 2024
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Prevent anonymous container access if `RequireSignInView` is enabled (go-gitea#28877)
  Don't show new pr button when page is not compare pull (go-gitea#26431)
  Avoid duplicate JS error messages on UI (go-gitea#28873)
  Fix branch list bug which displayed default branch twice (go-gitea#28878)
  Revert adding htmx until we finaly decide to add it (go-gitea#28879)
  Don't do a full page load when clicking the follow button (go-gitea#28872)
  Don't do a full page load when clicking the subscribe button (go-gitea#28871)
  Fix incorrect PostgreSQL connection string for Unix sockets (go-gitea#28865)
  Run `npm audit fix` (go-gitea#28866)
  Fix migrate storage bug (go-gitea#28830)
  Set the `isPermaLink` attribute to `false` in the `guid` sub-element (go-gitea#28860)
  In administration documentation about environment variables, point to those for the Go runtime instead of Go compiler (go-gitea#28859)
  Move doctor package from modules to services (go-gitea#28856)
  Add support for sha256 repositories (go-gitea#23894)
  Fix incorrect action duration time when rerun the job before executed once (go-gitea#28364)
  Fix some RPM registry flaws (go-gitea#28782)
  tests: missing refs/ in bare repositories (go-gitea#28844)
  Fix archive creating LFS hooks and breaking pull requests (go-gitea#28848)
henrygoodman pushed a commit to henrygoodman/gitea that referenced this pull request Jan 31, 2024
@yp05327 yp05327 deleted the not-show-new-pr-button-when-page-is-not-compare-pull branch January 31, 2024 23:50
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2024
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. modifies/translation size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants