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.

Sign up for GitHub

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

Jump to bottom

Only use supported sort order for "explore/users" page #29430

Merged
merged 6 commits into from
Feb 27, 2024
Merged

Only use supported sort order for "explore/users" page #29430

merged 6 commits into from
Feb 27, 2024

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Feb 26, 2024 •
edited
Loading

Thanks to inferenceus : some sort orders on the "explore/users" page could list users by their lastlogintime/updatetime.

It leaks user's activity unintentionally. This PR makes that page only use "supported" sort orders.

Removing the "sort orders" could also be a good solution, while IMO at the moment keeping the "create time" and "name" orders is also fine, in case some users would like to find a target user in the search result, the "sort order" might help.

image

@wxiaoguang wxiaoguang added the type/enhancement An improvement of existing functionality label Feb 26, 2024
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Feb 26, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 26, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 26, 2024
@wxiaoguang wxiaoguang mentioned this pull request Feb 26, 2024
Closed
delvh
delvh reviewed Feb 26, 2024
View reviewed changes
tests/integration/explore_user_test.go Outdated Show resolved Hide resolved
tests/integration/explore_user_test.go Outdated Show resolved Hide resolved
routers/web/explore/user.go Show resolved Hide resolved
@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 Feb 26, 2024
@wxiaoguang wxiaoguang added the backport/v1.21 This PR should be backported to Gitea 1.21 label Feb 26, 2024
zokkis
zokkis reviewed Feb 26, 2024
View reviewed changes
routers/web/explore/user.go
ctx.Data["SortType"] = "recentupdate"
orderBy = "`user`.updated_unix DESC"
}

if opts.SupportedSortOrders != nil && !opts.SupportedSortOrders.Contains(sortOrder) {
ctx.NotFound("unsupported sort order", nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400
Should be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree to use it, but at the moment Gitea doesn't have the ability to render such a page .....

It needs other framework-level refacotring first.

@zokkis
Copy link
Contributor

zokkis commented Feb 26, 2024

And maybe extends the supportedSortOrders to all for admins?

delvh
delvh approved these changes Feb 26, 2024
View reviewed changes
Copy link
Member

@delvh delvh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I still think that

  1. unsupported search params should simply ignore the param instead of throwing a 404
  2. the test is prone to fail once we refactor the templates

However, these changes are not important enough to warrant not giving an LGTM.

@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 Feb 26, 2024
@silverwind
Copy link
Member

silverwind commented Feb 26, 2024 •
edited
Loading

unsupported search params should simply ignore the param instead of throwing a 404

Agree, I also design API's to just ignore unknown search params. We should do whatever is consistent with existing APIs.

One problem with param validation on APIs is that if you remove a previously known param, you have broken the API. This is also why things like JSON Schema by default allows additional properties to be present, they don't hurt.

@delvh
Copy link
Member

delvh commented Feb 26, 2024

Not only because of consistency, imagine a user that enters a URL manually and fatfingers for some reason.
At the moment, they'll receive a 404 instead of what they wanted in a different sorting.

@silverwind
Copy link
Member

silverwind commented Feb 26, 2024 •
edited
Loading

Yeah, additional params could in theory even be introduced by systems outside our control. I think some analytic engines like google like to append tracking params to URLs, assuming the destination will just ignore them.

@wxiaoguang
Copy link
Contributor Author

This change is not ideal enough. But I would like to keep it as-is to make it easier to backport.

I think we can propose new improvements later. Actually the "RenderUser" related code was much too over-abstracted and there are various problems in it. Sooner or later it would be refactored.

@wxiaoguang wxiaoguang added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 27, 2024
@wxiaoguang wxiaoguang added type/bug and removed type/enhancement An improvement of existing functionality labels Feb 27, 2024
@GiteaBot
Copy link
Contributor

@wxiaoguang please fix the merge conflicts. 🍵

@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 27, 2024
@wxiaoguang wxiaoguang mentioned this pull request Feb 27, 2024
Merged
@wxiaoguang
Merge branch 'main' into fix-explore-user
3277a6e 
# Conflicts:
#	routers/web/explore/org.go
#	routers/web/explore/user.go
@lunny lunny merged commit eedb8f4 into go-gitea:main Feb 27, 2024
26 checks passed
@GiteaBot
Copy link
Contributor

I was unable to create a backport for 1.21. @wxiaoguang, please send one manually. 🍵

go run ./contrib/backport 29430
...  // fix git conflicts if any
go run ./contrib/backport --continue

@GiteaBot GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Feb 27, 2024
@wxiaoguang wxiaoguang deleted the fix-explore-user branch February 27, 2024 09:14
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Feb 27, 2024
@wxiaoguang
Only use supported sort order for "explore/users" page (go-gitea#29430)
8deafa9 
Thanks to inferenceus : some sort orders on the "explore/users" page
could list users by their lastlogintime/updatetime.

It leaks user's activity unintentionally. This PR makes that page only
use "supported" sort orders.

Removing the "sort orders" could also be a good solution, while IMO at
the moment keeping the "create time" and "name" orders is also fine, in
case some users would like to find a target user in the search result,
the "sort order" might help.

![image](https://github.com/go-gitea/gitea/assets/2114189/ce5c39c1-1e86-484a-80c3-33cac6419af8)
# Conflicts:
#	routers/web/explore/org.go
#	routers/web/explore/user.go
@wxiaoguang wxiaoguang mentioned this pull request Feb 27, 2024
Merged
@wxiaoguang wxiaoguang added the backport/done All backports for this PR have been created label Feb 27, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 29, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
dd1ca6c 
* giteaofficial/main: (23 commits)
  Fix wrong test usage of `AppSubURL` (go-gitea#29459)
  Improve contrast on blame timestamp, fix double border (go-gitea#29482)
  Fix/Improve `processWindowErrorEvent` (go-gitea#29407)
  Apply compact padding to small buttons with svg icons (go-gitea#29471)
  Fix counter display number incorrectly displayed on the page (go-gitea#29448)
  Fix incorrect user location link on profile page (go-gitea#29474)
  Fix workflow trigger event bugs (go-gitea#29467)
  Fix URL calculation in clone input box (go-gitea#29470)
  Remove jQuery from the "find file" page (go-gitea#29456)
  Move generate from module to service (go-gitea#29465)
  The job should always run when `if` is `always()` (go-gitea#29464)
  Recolor dark theme to blue shade (go-gitea#29283)
  Let ctx.FormOptionalBool() return optional.Option[bool] (go-gitea#29461)
  Implement actions badge svgs (go-gitea#28102)
  Fix missed return (go-gitea#29450)
  Use tailwind instead of `gt-[wh]-` helper classes (go-gitea#29423)
  Lock issues and pulls faster (go-gitea#29436)
  Allow to change primary email before account activation (go-gitea#29412)
  Update docs about `DEFAULT_ACTIONS_URL` (go-gitea#29442)
  Only use supported sort order for "explore/users" page (go-gitea#29430)
  ...
lunny pushed a commit that referenced this pull request Mar 3, 2024
@wxiaoguang
Only use supported sort order for "explore/users" page (#29430) (#29443)
2b059f4 
Backport #29430

Thanks to inferenceus : some sort orders on the "explore/users" page
could list users by their lastlogintime/updatetime.

It leaks user's activity unintentionally. This PR makes that page only
use "supported" sort orders.

Removing the "sort orders" could also be a good solution, while IMO at
the moment keeping the "create time" and "name" orders is also fine, in
case some users would like to find a target user in the search result,
the "sort order" might help.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zokkis zokkis zokkis left review comments

@lunny lunny lunny approved these changes

@silverwind silverwind silverwind approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/manual No power to the bots! Create your backport yourself! backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wxiaoguang @zokkis @silverwind @delvh @GiteaBot @lunny