WordPress.org

GlotPress

Opened 4 years ago

Closed 18 months ago

Last modified 10 months ago

#14 closed enhancement (fixed)

Allow a variable number of items per page

Reported by: yoavf Owned by:
Milestone: 1.0 Priority: normal
Version: Component: interface
Keywords: 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 3 years ago.
glotpress-profile.2.patch (5.2 KB) - added by joostdevalk 3 years ago.
Version 2, using user_meta
glotpress-profile-2.patch (9.6 KB) - added by joostdevalk 3 years ago.
version 3, adding in sort options
glotpress-profile-3.patch (10.1 KB) - added by joostdevalk 3 years ago.
Patch #3 :)
glotpress-profile-3.2.patch (10.2 KB) - added by joostdevalk 3 years ago.
Patch #4
glotpress-profile-4.patch (10.3 KB) - added by joostdevalk 2 years ago.
Updated patch with prefixed meta's.

Download all attachments as: .zip

Change History (25)

comment:1 nbachiyski4 years ago

  • Description modified (diff)

comment:2 nbachiyski4 years ago

  • Milestone set to Future Release
  • Priority changed from major to normal

comment:3 joostdevalk3 years ago

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.

comment:4 joostdevalk3 years ago

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 3 years ago by joostdevalk (previous) (diff)

comment:5 vanillalounge3 years ago

  • Keywords has-patch needs-review added

comment:6 nbachiyski3 years ago

See GP_User::get_meta() and its friends.

joostdevalk3 years ago

Version 2, using user_meta

comment:7 joostdevalk3 years ago

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.

comment:8 joostdevalk3 years ago

  • Cc joostdevalk added

joostdevalk3 years ago

version 3, adding in sort options

comment:9 joostdevalk3 years ago

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

joostdevalk3 years ago

Patch #3 :)

comment:10 joostdevalk3 years ago

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

joostdevalk3 years ago

Patch #4

comment:11 joostdevalk3 years ago

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

comment:12 nacin2 years ago

  • 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).

joostdevalk2 years ago

Updated patch with prefixed meta's.

comment:13 nbachiyski2 years ago

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.

comment:14 nbachiyski2 years ago

  • Cc nbachiyski added

comment:15 juanramondiaz2 years ago

  • Cc juanramon.diaz@… added

comment:16 nbachiyski18 months ago

  • 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.

comment:17 vanillalounge18 months ago

  • 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

comment:18 nacin18 months ago

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

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

comment:19 markoheijnen10 months ago

  • Keywords has-patch needs-review dev-feedback removed
  • Milestone changed from Future Release to 1.0
Note: See TracTickets for help on using tickets.