Fix redirect url of legacy commits route #2825
Conversation
|
@Morlinest Can you give an example of what this fixes? |
tboerger
added
the
lgtm/need 2
|
@ethantkoenig |
Morlinest
force-pushed
the
fix/redirect-legacy-url
branch
from
6eaabbc to
119c696
Compare
Codecov Report
@@ Coverage Diff @@
## master #2825 +/- ##
=======================================
Coverage 26.84% 26.84%
=======================================
Files 89 89
Lines 17608 17608
=======================================
Hits 4727 4727
Misses 12195 12195
Partials 686 686Continue to review full report at Codecov.
|
modules/context/repo.go
Outdated
| return path.Join(urlPath[:idx], ctx.Repo.BranchNameSubURL()) | ||
| urlPath := strings.TrimSuffix(ctx.Req.URL.String(), ctx.Params("*")) | ||
| urlPath = strings.TrimPrefix(urlPath, "/") | ||
| return setting.AppURL + path.Join(urlPath, ctx.Repo.BranchNameSubURL()) |
There was a problem hiding this comment.
nit: could we use URLJoin instead of manually assembling the URL (it may make sense to also move URLJoin to modules/util)
There was a problem hiding this comment.
@ethantkoenig @lafriks Do you want me to do this change in this PR? It's a lot of lines changed, would it be in the scope of this PR?
There was a problem hiding this comment.
I think moving into path.Join would be enough for this PR
There was a problem hiding this comment.
@lafriks You can't move setting.AppURL to path.Join. Or you mean markup.URLJoin?
There was a problem hiding this comment.
I mean changing from return setting.AppURL + path.Join(urlPath, ctx... to return path.Join(setting.AppURL, urlPath, ctx....
There was a problem hiding this comment.
It's imposible because path.Join strips http://example.com to http:/example.com
There was a problem hiding this comment.
You can use that suburl variable then so than?
There was a problem hiding this comment.
@lafriks I can use setting.AppSubURL.
|
I think we have to add tests for subURL at first. |
|
@lafriks @ethantkoenig Changed and simplified. @lunny I think it is not needed now for this PR. |
|
LGTM |
tboerger
added
lgtm/need 1
|
LGTM |
tboerger
added
lgtm/done
|
@ethantkoenig @lafriks Shouldn't I remove |
|
If it is used in one place only than yes, no need for this function |
Morlinest
force-pushed
the
fix/redirect-legacy-url
branch
from
1266825 to
7c6cda8
Compare
|
@lafriks PR updated. |
As title.