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 the incorrect route path in the user edit page. #27007

Merged
merged 13 commits into from
Sep 18, 2023

Conversation

CaiCandong
Copy link
Member

@CaiCandong CaiCandong commented Sep 11, 2023

Regression of #26713

After #26713 , the base path of user edit has been changed to /admin/users/{userid}/edit

Before

demo.mp4

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 11, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 11, 2023
@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 Sep 11, 2023
@CaiCandong CaiCandong marked this pull request as draft September 11, 2023 08:37
routers/web/web.go Outdated Show resolved Hide resolved
@CaiCandong CaiCandong marked this pull request as ready for review September 11, 2023 09:29
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

No, do not do this ctx.Data["Link"] = "/admin/users/" + ctx.Params(":userid") , it is wrong.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 11, 2023
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Sep 11, 2023
@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 Sep 12, 2023
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

It is still not right.

You only have BaseUrl in EditUser, but not in EditUserPost ....

ps: I really dislike playing such tricks

@denyskon
Copy link
Member

Honestly, I don't really understand what this PR is trying to do....

@CaiCandong
Copy link
Member Author

The Link used to splice the route address started out as /admin/users/{userId}, but now it becomes /admin/users/{userId}/edit. This results in a 404 for requests for user deletion, avatar upload, etc. to the same route.

@puni9869
Copy link
Member

It is still not right.

You only have BaseUrl in EditUser, but not in EditUserPost ....

ps: I really dislike playing such tricks

I agree with @wxiaoguang BaseUrl should not be override.

@CaiCandong
Copy link
Member Author

CaiCandong commented Sep 16, 2023

I mean ctx["PathVaribleName"] = fmt.Printf("<pathurl>/%s", variableName) in the go code.

In the template {{.PathVaribleName}}/slug

image

<form class="ui form" action="{{.BaseUrl}}/edit" method="post">
ctx.Data["BaseUrl"] = "/admin/users/" + ctx.Params(":userid")

Maybe I've done what you said?

@puni9869
Copy link
Member

Apologies for the syntax typo. ctx.Data["PathV...
ctx.Data["PathVaribleName"] = fmt.Printf("<pathurl>/%s", variableName)

@wxiaoguang
Copy link
Contributor

It is still not right.

You only have BaseUrl in EditUser, but not in EditUserPost ....

Still not fixed.

Actually, the relative URL could also help. If the browser's current URL is /foo/bar, then ./xxx means /foo/xxx.

@CaiCandong
Copy link
Member Author

Actually, the relative URL could also help. If the browser's current URL is /foo/bar, then ./xxx means /foo/xxx.

Thank you so much for your patient advice, I'll try this method.

@CaiCandong CaiCandong force-pushed the bugfix/Fix-the-incorrect-route-path branch from 249f0a2 to de718a9 Compare September 18, 2023 05:09
@github-actions github-actions bot added the topic/ui Change the appearance of the Gitea UI label Sep 18, 2023
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Keep in mind, Gitea has a very unfriendly backend approach: if you put a variable in Xxx handler (UserEdit) , then you might also need to put it in XxxPost handler (UserEditPost). Otherwise, if an user-side error occurs when the end user submits the form, the re-rendered page will go wrong.

Or, just like this PR does, get rid of such variables.

@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 18, 2023
templates/admin/user/edit.tmpl Show resolved Hide resolved
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 18, 2023
@CaiCandong
Copy link
Member Author

Keep in mind, Gitea has a very unfriendly backend approach: if you put a variable in Xxx handler (UserEdit) , then you might also need to put it in XxxPost handler (UserEditPost). Otherwise, if an user-side error occurs when the end user submits the form, the re-rendered page will go wrong.

Thank you so much for your patience and guidance

@puni9869
Copy link
Member

are we good with PR.
if Y 🚀

@delvh delvh merged commit 323135b into go-gitea:main Sep 18, 2023
26 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 18, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 18, 2023
@lunny lunny modified the milestones: 1.22.0, 1.21.0 Sep 18, 2023
@CaiCandong CaiCandong added type/bug and removed topic/ui Change the appearance of the Gitea UI labels Sep 18, 2023
silverwind added a commit to silverwind/gitea that referenced this pull request Sep 18, 2023
* origin/main:
  Fix the incorrect route path in the user edit page. (go-gitea#27007)
  Refactor lfs requests (go-gitea#26783)
  Display archived labels specially when listing labels (go-gitea#26820)
  Remove a `gt-float-right` and some unnecessary helpers (go-gitea#27110)
  [skip ci] Updated licenses and gitignores
  Fix token endpoints ignore specified account (go-gitea#27080)
  Make SSPI auth mockable (go-gitea#27036)
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 20, 2023
* giteaofficial/main:
  Improve actions docs related to `pull_request` event (go-gitea#27126)
  Remove outdated paragraphs when comparing Gitea Actions to GitHub Actions (go-gitea#27119)
  Fix: treat tab "overview" as "repositories" in user profiles without readme (go-gitea#27124)
  Fix incorrect test code for error handling (go-gitea#27139)
  Increase auth provider icon size on login page (go-gitea#27122)
  fix pagination for followers and following (go-gitea#27127)
  services/wiki: Close() after error handling (go-gitea#27129)
  Use fetch helpers instead of fetch (go-gitea#27026)
  Change green buttons to primary color (go-gitea#27099)
  Fix wrong xorm get usage on migration (go-gitea#27111)
  Fix the incorrect route path in the user edit page. (go-gitea#27007)
  Refactor lfs requests (go-gitea#26783)
  Display archived labels specially when listing labels (go-gitea#26820)
  Remove a `gt-float-right` and some unnecessary helpers (go-gitea#27110)
  [skip ci] Updated licenses and gitignores
  Fix token endpoints ignore specified account (go-gitea#27080)
  Make SSPI auth mockable (go-gitea#27036)
@CaiCandong CaiCandong deleted the bugfix/Fix-the-incorrect-route-path branch October 3, 2023 03:21
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 17, 2023
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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants