-
Notifications
You must be signed in to change notification settings - Fork 197
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
Remove limit from fetching data for data export #7647
Conversation
@@ -328,7 +328,7 @@ public function generate_report( $report ) { | |||
$this->search = $search; | |||
|
|||
$args = array( | |||
'number' => '', | |||
'number' => -1, |
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.
@donnapep
Here, I removed the limit completely to allow export all existing data (even if there are 100000 rows). This is how I get “Export all rows”.
On the other hand, we can read “Export all rows” as “Export all visible rows”. In this case, we need to apply the same limit we use on the page.
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 noticed that in another place we already removed the limit: https://github.com/Automattic/sensei/blob/trunk/includes/reports/overview/list-table/class-sensei-reports-overview-list-table-abstract.php#L179
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
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.
LGTM!
I'm not concerned that the server will be running out of memory because of too many lessons. It seems unlikely and should be considered as an edge case.
Resolves #7629
When we export student progress by a course, when the course has many lessons (more than 10), we miss part of the progress in exported data.
That happens because of the default limit (normally, 10) for the query that fetches data.
Proposed Changes
Testing Instructions
Pre-Merge Checklist