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

Fix dependency link rendering in PR sidebar #16754

Merged
merged 3 commits into from
Aug 21, 2021

Conversation

justusbunsi
Copy link
Member

Signed-off-by: Steven Kriegler 61625851+justusbunsi@users.noreply.github.com

Signed-off-by: Steven Kriegler <61625851+justusbunsi@users.noreply.github.com>
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Aug 20, 2021
@techknowlogick techknowlogick added this to the 1.16.0 milestone Aug 20, 2021
@techknowlogick techknowlogick added modifies/translation skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug and removed modifies/translation labels Aug 20, 2021
@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 20, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #16754 (4742891) into main (3ecc4a1) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16754      +/-   ##
==========================================
- Coverage   45.37%   45.36%   -0.02%     
==========================================
  Files         760      760              
  Lines       85485    85485              
==========================================
- Hits        38789    38780       -9     
- Misses      40413    40421       +8     
- Partials     6283     6284       +1     
Impacted Files Coverage Δ
models/repo_mirror.go 58.69% <0.00%> (-10.87%) ⬇️
services/mirror/mirror.go 9.61% <0.00%> (-7.70%) ⬇️
models/gpg_key_common.go 59.67% <0.00%> (-4.84%) ⬇️
modules/cron/tasks_basic.go 85.43% <0.00%> (-2.92%) ⬇️
modules/process/manager.go 72.83% <0.00%> (-2.47%) ⬇️
modules/git/blame.go 65.71% <0.00%> (-1.43%) ⬇️
services/pull/pull.go 41.78% <0.00%> (-0.41%) ⬇️
models/unit.go 43.83% <0.00%> (+2.73%) ⬆️
modules/queue/workerpool.go 57.25% <0.00%> (+3.43%) ⬆️

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 3ecc4a1...4742891. Read the comment docs.

@zeripath zeripath merged commit e9747de into go-gitea:main Aug 21, 2021
@lunny
Copy link
Member

lunny commented Aug 21, 2021

It's not a bug, it will automatically redirect to the right URL.

@justusbunsi justusbunsi deleted the dependency-link-rendering branch August 21, 2021 06:37
@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

It's not a bug, it will automatically redirect to the right URL.

indeed, the autoredirect is a very nice feature. not sure what exactly this PR was fixing.
possibly all the change will do (again) is pose issues with redirecting to external pulls.
now, truthfully, I am yet to test this change but I recall having issues with similar stuff before, in particular with mirrored repos where PRs were disabled and issues were pointed to the remote repo's issue index.

@justusbunsi
Copy link
Member Author

The redirect feature is a great one. That's true. This PR faces an issue with external issue trackers. If configured, the redirect feature seems to not always correctly separate issues from pulls. In this particular case it identifies a link pointing to a pull request as an issue reference and therefore redirects to the external tracker.

I agree that the fix might not be the best. Maybe I should've identified the issue inside the redirect feature instead of this change. 🤔

@zeripath
Copy link
Contributor

zeripath commented Aug 21, 2021

Sorry @wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf I forgot about your case.

Unfortunately I think this needs to be reverted. What were you trying to fix @justusbunsi?

--
Edit: @wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf has confirmed this doesn't break them.

@justusbunsi
Copy link
Member Author

@zeripath

This PR faces an issue with external issue trackers. If configured, the redirect feature seems to not always correctly separate issues from pulls. In this particular case it identifies a link pointing to a pull request as an issue reference and therefore redirects to the external tracker.

Given a repository with "External Issue Tracker" configured and having two PRs where PR 1 is added as dependency for PR 2. If you open the discussion page of PR 2 and have a look at the right sidebar, you can click on the dependency (which is PR 1). Without this fix, I get redirected to the external tracker instead of navigated to the PR 1 page in Gitea.

image

It's ok to revert, if this breaks others environments. I guess, we need to identify the actual root cause of my described behavior and fix it, since it's a generic problem for links when an external issue tracker is configured.

@zeripath
Copy link
Contributor

I guess what we need is @wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf to try this on their configuration to double check that this PR has broken things...

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

thank you both for considering me, yay.

alright, I have checked and this does not break my use case, I suppose that is because my mirrors in fact have the PR pages disabled.
sorry if I caused too much of a stir.

on closer look, I still think a better suited place for a more complete solution might have been the issue router redirection logic.
happy this fix solved your issue, though, @justusbunsi

@zeripath
Copy link
Contributor

Cool ... we can all stand-down!

@zeripath
Copy link
Contributor

Thanks @justusbunsi for the fix.

@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants