-
Notifications
You must be signed in to change notification settings - Fork 34
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
Issue/1508 pagination #1541
Issue/1508 pagination #1541
Conversation
… image size of user avatars in search results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work so far 👍, some initial feedback items as a first pass
@@ -14,7 +14,7 @@ static public function get($inst_id, $load_qset=false, $timestamp=false, $delete | |||
return count($instances) > 0 ? $instances[0] : false; | |||
} | |||
|
|||
static public function get_all(Array $inst_ids, $load_qset=false, $timestamp=false, bool $deleted=false): array | |||
static public function get_all(Array $inst_ids, $load_qset=false, $timestamp=false, bool $deleted=false, $offset=0, $limit=2147483647): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Y2K38 protection 💪
src/util/api.js
Outdated
*/ | ||
export const apiSearchWidgets = input => { | ||
export const apiSearchWidgets = (input, page_number) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of a nitpick, but I think it's important to distinguish widgets from instances when it comes to API-related methods. Since this is searching for instances, it should be named as such.
@@ -69,13 +69,20 @@ public function post_user($user_id) | |||
return \Service_User::update_user($user_id, $user); | |||
} | |||
|
|||
public function get_widget_search(string $input) | |||
public function get_widget_paginated_search(string $input, string $page_number) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this comment in the front-end's api
file too, but if we're looking for instances and not widgets, the method name should explicitly use the term instance.
import { iconUrl } from '../../util/icon-url' | ||
|
||
export default function useInstanceList() { | ||
export default function useInstanceList(userOnly = true, query = "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this, the more I want this hook to be split into two different instance list hooks, because while there are some similarities, there are some key differences:
- They're hitting different API endpoints
- They're both requesting lists of widgets, but for different purposes. This especially affects the
widgets
query cache: one is a list of instances the user owns, the other is a list of instances the user may not expressly have access to. IMO, thewidgets
query cache should remain the list of widgets we own (in the My Widgets page) and the new hook would use a different query key. - It makes references to the hooks elsewhere much cleaner as to what exactly the hook is being used for
Additional feedback: loading the entire list of users and instances on admin page load is a definite no-go. That's a very expensive request and there's no good reason for it. I'd much rather exclusively return the results of search queries instead. |
New changes: updated the function names to use "instances" instead of "widgets", created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I'm seeing is the user panel continues to fetch the entire table's contents on load, which I'm not a fan of. It looks like the infinite query is missing the enabled: query.length > 0
param present in the instance search query.
The limit in the instance manager's get_all
method is fine, generally, but I'm not sure when it would realistically (or should realistically) be used. Prior to the reach branch, superusers would have implicit ownership of every instance the database, and thus the API would try and respond with the entire table when visiting My Widgets (because the request wasn't paginated). That request actually caused PHP-FPM to run out of memory. I think I'm fine leaving it in, but it's very much a "you probably shouldn't ever request every instance in the database in one go" situation.
Other than that: LGTM 💪
That's a good point; functions querying the widget_instance table are either fetching a page or only the first instance in the results, so it's safe to leave the default limit at 80. Caught an issue with paginated results returning duplicate entries because of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three small requests to change the manner in which you're calculating $has_next_page
; other than that it's good!
$displayable_items = self::get_all($inst_ids, false, false, false, $offset, $items_per_page); | ||
|
||
// if the returned number of instances is greater than a page, there's more pages | ||
$has_next_page = sizeof($displayable_items) > ($items_per_page - 1) ? true : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting into the weeds a bit, but this method of determining whether there's another page will still return true if the total item count is divisible by items_per_page
.
A better option might be to set the query limit to $limit + 1
, and simply check whether $displayable_items
is greater than $items_per_page
. That shouldn't affect the offset, since the +1 isn't part of the calculation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I'm also wondering if the behavior you noticed with order by
was influenced by the +1 offset. I'd prefer to keep the order by
clause when performing the query, so it'd be worth double-checking after updating these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a single order_by
seems to have been causing the issue, since multiple instances can have the same created_at
value. I added an additional criteria to break up ties by ordering by ID, and reinstated the original pagination logic (adding +1 to the limit and then popping the last entry).
$displayable_items = self::get_widget_instance_search($input, $offset, $items_per_page); | ||
|
||
// if the returned number of instances is greater than a page, there's more pages | ||
$has_next_page = sizeof($displayable_items) > ($items_per_page - 1) ? true : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thought process as my comment on instance/manager
line 91.
fuel/app/classes/materia/api/v1.php
Outdated
// query DB for only a single page + 1 item | ||
$displayable_items = \Model_User::find_by_name_search($input, $offset, $items_per_page); | ||
|
||
$has_next_page = sizeof($displayable_items) > ($items_per_page - 1) ? true : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback here as my comment in instance/manager
line 91.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second order by
clause addition is a nice touch. Nicely done.
This looks good, I think it's ready to ship.
Reopens #1518
Fixes #1508
Currently, the user and instance search retrieve all entries in the database on page load. Depending on feedback, this may switch to fetching only after someone has typed something.Fixed.