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

Forum Thread paging #2473

Merged
merged 25 commits into from Jul 27, 2018

Conversation

Projects
None yet
3 participants
@scopeInfinity
Copy link
Member

scopeInfinity commented Jul 18, 2018

Closes: #2376

Tasks

  • Refactor DB queries method.
  • Load thread list page in chunks(loads more when scroll hits bottom)
  • ThreadStatus stored in the cookie
  • List of the possible thread for merging is loaded (on clicking "Merge Thread")

@scopeInfinity scopeInfinity changed the title [WIP] Forum Thread paging Forum Thread paging Jul 22, 2018

@scopeInfinity scopeInfinity requested a review from andrewaikens87 Jul 22, 2018

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented Jul 23, 2018

Status from an instructor perspective on a local system.

Now, for around 10000 threads

  • view_thread page load time: 6.8KB ~300ms
  • For loading next 10 threads info(left thread list) : 1.0KB ~300ms
  • Loading threads by clicking "Merge Thread"(~10k threads) : 50KB 200ms(Request) + 300ms(JS) ~500ms

Before this PR, while working on 3000 threads.

  • view_thread page load time: ~30 secs
@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented Jul 24, 2018

Currently loading 10000 threads to test, I am going to have to let the tool run overnight. I will update this tomorrow

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented Jul 25, 2018

I am getting a couple of undefined variables
bad

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented Jul 25, 2018

  • If a user selects a thread that is on a thread page that is greater than the first and refreshes, the thread list doesn't preserve the current thread's active style (the cyan blue color) when trying scrolling down to it again

  • Could we have a ui loading circle or something similar displayed to the user to show that more threads are being loaded when scrolling to the bottom?

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented Jul 26, 2018

It seems like if I select a filter I lose the ability to scroll in the thread list and then when I try to select one of the threads that exists in the list I get alerted that there was an issue loading the thread list (same thing occurs for filtering on resolved/unresolved or thread category).

@@ -594,58 +569,61 @@ private function getSortedThreads($categories_ids, $max_thread, $show_deleted =
}
public function getThreads(){
$pageNumber = !empty($_GET["page_number"]) && is_numeric($_GET["page_number"]) ? (int)$_GET["page_number"] : -1;

This comment has been minimized.

@andrewaikens87

andrewaikens87 Jul 26, 2018

Member

We should be validating the lower bound of this page number, i.e. what if the page number is input as -15 (this can be done by simply changing the next_page html attribute). This will throw an exception since the offset in the query can't be negative.

scopeInfinity added some commits Jul 26, 2018

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented Jul 26, 2018

Thankyou @andrewaikens87
Fixed those bugs.

scopeInfinity and others added some commits Jul 26, 2018

@@ -569,7 +569,7 @@ private function getSortedThreads($categories_ids, $max_thread, $show_deleted, $
}
public function getThreads(){
$pageNumber = !empty($_GET["page_number"]) && is_numeric($_GET["page_number"]) ? (int)$_GET["page_number"] : -1;
$pageNumber = !empty($_GET["page_number"]) && is_numeric($_GET["page_number"]) && $_GET["page_number"] >= 1 ? (int)$_GET["page_number"] : -1;

This comment has been minimized.

@andrewaikens87

andrewaikens87 Jul 27, 2018

Member

Please test, if you input anything that is below 1 it will make the page number -1 (since ternary exp will be false), resulting in an exception when trying to run the offset query. A possible place to put this check might be where we try to load the threads.

scopeInfinity and others added some commits Jul 27, 2018

@andrewaikens87
Copy link
Member

andrewaikens87 left a comment

Good to merge

@bmcutler bmcutler merged commit 42cbc93 into master Jul 27, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@bmcutler bmcutler deleted the forum_thread_paging branch Jul 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment