Opened 3 years ago

Closed 7 months ago

#14 closed enhancement (fixed)

Allow a variable number of items per page

Reported by: yoavf Owned by:
Priority: normal Milestone: Future Release
Component: interface Version:
Keywords: has-patch needs-review dev-feedback Cc: joostdevalk, nbachiyski, juanramon.diaz@…

Description (last modified by nbachiyski)

GlotPress should let the user decide how many items to display per page.

For example: 5-20-50-100 etc...

Attachments (6)

glotpress-profile.patch (2.9 KB) - added by joostdevalk 20 months ago.
glotpress-profile.2.patch (5.2 KB) - added by joostdevalk 20 months ago.
Version 2, using user_meta
glotpress-profile-2.patch (9.6 KB) - added by joostdevalk 20 months ago.
version 3, adding in sort options
glotpress-profile-3.patch (10.1 KB) - added by joostdevalk 20 months ago.
Patch #3 :)
glotpress-profile-3.2.patch (10.2 KB) - added by joostdevalk 20 months ago.
Patch #4
glotpress-profile-4.patch (10.3 KB) - added by joostdevalk 14 months ago.
Updated patch with prefixed meta's.

Download all attachments as: .zip

Change History (24)

  • Description modified (diff)
  • Milestone set to Future Release
  • Priority changed from major to normal

I came up with a patch for this today as the default 15 annoyed the heck out of me and a lot of people. Attached patch adds a /profile page where people can set how many items they want displayed on a page, currently stored in a cookie.

Biggest issue I have with the patch myself is that it's stored in a cookie. In WP core I'd use a user_meta, I didn't see an API for that in GP yet (even though there is a gp_usermeta table), but I think that'd be the way to go. If we had that we could also store a default sort.

Last edited 20 months ago by joostdevalk (previous) (diff)
  • Keywords has-patch needs-review added

See GP_User::get_meta() and its friends.

Version 2, using user_meta

Ok found that indeed, couple of issues that needed fixing to get this to work:

  • get_meta, set_meta and delete_meta all assumed $this->id was filled, when it wasn't in this case, so switched them over to first using the current method of GP_User.
  • gp_update_meta always returned false when $global wasn't set, which it isn't when called from GP_User, so I removed that check.
  • GP_Thing's sql_limit_for_paging method first correctly set its internal $per_page variable to then use $this->per_page for the query, fixed.
  • GP_Translation now checks whether there's user setting per_page and uses that value, otherwise it reverts to its default.
  • Cc joostdevalk added

version 3, adding in sort options

Just found a bug in pagination, tracking down, don't commit patch yet.

Patch #3 :)

Pagination issue found and fixed, actually fixed it by implementing the $per_page fix cleaner and earlier, before getting the translations.

This last patch adds a redirect for non-logged in users to the login page when they try to open the /profile page.

  • Keywords changed from has-patch, needs-review to has-patch needs-review

Figured I would review this. It looks good, but a few thoughts:

get_meta() et al. store raw usermeta keys into the DB. In WP.org's case, we share user tables across literally everything -- WP, bbPress, SVN, GP, etc. Even without our complex setup, since GP does not have an interface, then it is almost assuredly going to be sharing the user table with a WP install nearby. What we may need here is a get_option() method that keys things by the table prefix. This is exactly how get_user_option() works in WP.

Or, really, we do want it to be *global* but global to GlotPress. (We may have multiple installs of GlotPress in the future, for example, and those settings should span installs.) So perhaps the meta keys could start with 'glotpress_' or 'gp_'.

Also, since the default sort is stored in an array, gp_array_get() will probably enable some of those pieces to appear a bit cleaner (especially since it also takes a default).

Updated patch with prefixed meta's.

A couple of things on the patch:

  • It's not OK to always use the current user in GP_User:: get_meta(). What if I have an instance of GP_User and I want to get its meta? I will get the current user's one instead. Please use GP::$user->current->get_meta().
  • The default sort logic is duplicated, it's worth it to have it somewhere.
  • In a couple of places you're using double quotes, where single would be enough

And a general thing: please run the tests with your changes. For example your user meta changes prevented user meta to be set and get correctly and it showed in the tests.

Nacin, if you think the prefixing is needed, we should implement it deeper in GP_User::get/set_meta() – it could transparently add gp_ to the meta key and we won't have to prefix it by ourselves.

  • Cc nbachiyski added
  • Cc juanramon.diaz@… added
  • Resolution set to fixed
  • Status changed from new to closed

(In [710]) Add per-profile settings.

Adds /profile where you can set number of items per page, default sort
by, and default sort order.

Props joostdevalk, fixes #14.

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Both my local install (bleeding edge) and http://translate.wordpress.org/profile return a 500. Local log says:

PHP Fatal error: Class 'GP_Route_Profile' not found in /Applications/MAMP/htdocs/trunkgp.dev/gp-includes/router.php on line 119

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [729]) Include the profile route. fixes #14.

Note: See TracTickets for help on using tickets.