Implement pagination for the Users page #541
Conversation
googlebot
added
the
cla: yes
|
/lgtm Based on screenshot , couple of future improvements
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikehelmick, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -308,6 +308,7 @@ func realMain(ctx context.Context) error { | |||
|
|
|||
| userController := user.New(ctx, cacher, config, db, h) | |||
| userSub.Handle("", userController.HandleIndex()).Methods("GET") | |||
| userSub.Handle("", userController.HandleIndex()).Queries("offset", "{[0-9]*?}").Methods("GET") | |||
There was a problem hiding this comment.
I think this might be better as:
userSub.Handle("page/{offset:[0-9]+}", usersController.HandleIndex()).Methods("GET")And then pull the pagination from mux.Vars(). That will give us nicer urls like /users/page/2
| func (c *Controller) HandleIndex() http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| ctx := r.Context() | ||
| qvar := r.FormValue("offset") | ||
| offset, _ := strconv.Atoi(qvar) |
There was a problem hiding this comment.
Need to handle this error, probably an internal server error.
|
|
||
| realm := controller.RealmFromContext(ctx) | ||
| if realm == nil { | ||
| controller.MissingRealm(w, r, c.h) | ||
| return | ||
| } | ||
|
|
||
| users, err := realm.ListUsers(c.db) | ||
| count, err := realm.CountUsers(c.db) |
There was a problem hiding this comment.
Hmm, this is pretty expensive (count is a full table lock). I feel like we should have a perPage constant and then just pipe the current count into a SQL OFFSET
| // ListUsers returns the list of users on this realm. | ||
| func (r *Realm) ListUsers(db *Database) ([]*User, error) { | ||
| func (r *Realm) ListUsers(db *Database, offset, limit int) ([]*User, error) { | ||
| if limit > 100 { |
|
Oh looks like @mikehelmick was reviewing at the same time. I left some comments @whaught |
Fixes #538
Proposed Changes
Release Note