Skip to content
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

CSV Export #823

Merged
merged 54 commits into from Mar 15, 2016

Conversation

Projects
None yet
2 participants
@chacha
Copy link
Contributor

commented Feb 29, 2016

Closes #813

Adds a CSV export download link. Allows user to download any view of Stream Records. Uses the currently selected filters.

Also adds stream_records_per_page filter.

@chacha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 29, 2016

@lukecarbis The only thing I'm not quite happy with is the page item count seems to be pushed down. I've played around with it a bit, but haven't gotten very far. Suggestions?

@@ -687,6 +690,94 @@ public function render_list_table() {
<?php
}
public function maybe_render_csv_page() {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Feb 29, 2016

Contributor

I would suggest moving the maybe_render_csv_page, render_csv_disable_paginate, and render_csv_expand_columns into their own Export class, then instantiating it as a property of Admin.

This comment has been minimized.

Copy link
@chacha

chacha Feb 29, 2016

Author Contributor

Done. I agree that this spot is more suitable for it.

public function maybe_render_csv_page() {
$page = wp_stream_filter_input( INPUT_GET, 'page' );
$output_csv = wp_stream_filter_input( INPUT_GET, 'output_csv' );
if ( '1' !== $output_csv || 'wp_stream' !== $page ) {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Feb 29, 2016

Contributor

Rather than using the parameter output_csv with a value of 1, can we make this more extendable by using: output with a value of csv? Might mean some refactoring.

This comment has been minimized.

Copy link
@chacha

chacha Feb 29, 2016

Author Contributor

Done. Great suggestion.

'connector' => __( 'Connector', 'stream' ),
'context' => $columns['context'],
'action' => $columns['action'],
'blog_id' => __( 'Blog ID', 'stream' ),

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Feb 29, 2016

Contributor

Maybe only add this if is_multisite() && is_plugin_active_for_network( $this->plugin->locations['plugin'] ).

This comment has been minimized.

Copy link
@chacha

chacha Feb 29, 2016

Author Contributor

Good catch. I made a mental note to check when this is added regularly, but should have written it down.

}
header( 'Content-type: text/csv' );
header( 'Content-Disposition: attachment; filename="output.csv"' );

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Feb 29, 2016

Contributor

Let's brand it: stream.csv

chacha added some commits Feb 29, 2016

Change download file name
Branded as stream.csv
@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Feb 29, 2016

Loving the progress on this. What I'd really like to see if Exporting modularised. I'm imagining an Export class, and an Export_CSV class.

The Export class would register all the export types (filterable), and include them in the Export options at the bottom of the list table.

The Export_CSV class would look after the pagination filter, columns, and outputting the CSV file.

Basically, what I'm getting at is that it should be very easy to create an Export_JSON class without touching or duplicating any existing code.

Make sense?

@chacha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 29, 2016

Definitely. And we might as well add that while we’re at it.

I’m thinking about having Export build a data array that adds the base data (and could be filtered) that will then be passed to the specific exporter. That way anyone could add extra information without having to rely on individual exporter implementations.

On Feb 29, 2016, at 2:20 PM, Luke Carbis notifications@github.com wrote:

Loving the progress on this. What I'd really like to see if Exporting modularised. I'm imagining an Export class, and an Export_CSV class.

The Export class would register all the export types (filterable), and include them in the Export options at the bottom of the list table.

The Export_CSV class would look after the pagination filter, columns, and outputting the CSV file.

Basically, what I'm getting at is that it should be very easy to create an Export_JSON class without touching or duplicating any existing code.

Make sense?


Reply to this email directly or view it on GitHub #823 (comment).

@chacha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 29, 2016

Something about the copy messed up all of the tabs/indents causing Travis to fail. I'm going to have to go through and individually fix it. Will do that a little later today.

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Feb 29, 2016

Great thoughts. Let's do it.

chacha added some commits Feb 29, 2016

Modularize Exporters
Move CSV functionality to separate class
Allow exporters to be registered
List exporters at bottom of table
}
public function render_download() {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 1, 2016

Contributor

Drop blank line at top of method.

class Export_JSON {
public function output_file ( $data ) {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 1, 2016

Contributor

Exporters should also have a $name or $title property (which is output on the admin screen). There should be an abstract Exporter class which each exporter extends (similar to our Connector class). The Export class should check if each registered exporter is a type of Exporter.

$export_links[] = sprintf(
'<a href="%s">%s</a>',
esc_html( $download ),
esc_html( strtoupper ( $export_type ) )

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 1, 2016

Contributor

See comment above. This will change to something like esc_html( $this->admin->export->exporters[ $export_type ]->name ).

@@ -25,6 +25,11 @@ class Admin {
public $live_update;
/**
* @var Export
*/
public $export;

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 1, 2016

Contributor

@chacha Question: Should Export belong to Admin? Or should it belong to Plugin? Maybe List_Table? Thoughts?

This comment has been minimized.

Copy link
@chacha

chacha Mar 1, 2016

Author Contributor

Right now the exporter specifically takes Stream Records from a List Table and Exports. So I would probably place it with List_Table.

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 1, 2016

Contributor

Is it possible to decouple Exports from List_Table?

This comment has been minimized.

Copy link
@chacha

chacha Mar 1, 2016

Author Contributor

It is possible. One option would be an adapter class between the two, or moving several of the methods we just migrated away from Admin back into admin.

On Feb 29, 2016, at 5:26 PM, Luke Carbis notifications@github.com wrote:

In classes/class-admin.php #823 (comment):

@@ -25,6 +25,11 @@ class Admin {
public $live_update;

/**
  • * @var Export
  • */
  • public $export;
    Is it possible to decouple Exports from List_Table?


Reply to this email directly or view it on GitHub https://github.com/xwp/stream/pull/823/files#r54509913.

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 1, 2016

Contributor

Since Exports relies on List_Table for filters, it doesn't really make sense. Let's keep it in Admin.

chacha added some commits Mar 8, 2016

Reintroduce PHPCS ignore line
For text-only output
Fix use of wp_json_encode
For WP < 4.0
@chacha

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2016

@lukecarbis Everything we've talked about is finished. I think we're ready to merge. Let me know if something is missing.

header( 'Content-Disposition: attachment; filename="stream.json"' );
}
if ( function_exists( 'wp_json_encode' ) ) {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 9, 2016

Contributor

😍

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2016

@chacha Could you please review my commits above, and give me your thoughts? a900133...49c1642

@chacha

This comment has been minimized.

Copy link
Contributor

commented on de3ce44 Mar 9, 2016

@lukecarbis What other actions do you foresee going in here? Given this pattern I would expect us to add checkboxes to the records and be able to select a subset of records to preform actions on.

@chacha

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

@lukecarbis Reviewed all commits. I am all for bringing this inline with how connectors are registered. Consistency is important.

👍

@chacha

This comment has been minimized.

Copy link
Contributor

commented on tests/tests/test-class-export.php in 48b8d6c Mar 9, 2016

I think the exporters array needs to be cleaned up after this test. It triggering an error that is making other tests fail.

This comment has been minimized.

Copy link
Contributor

replied Mar 14, 2016

I'm not getting any other errors. If you still are, could you do the cleanup?

This comment has been minimized.

Copy link
Contributor

replied Mar 15, 2016

@lukecarbis Fixed in e016085. Forgot to make it a separate commit though.

*/
public function is_valid_exporter( $exporter ) {
if ( ! is_a( $exporter, 'WP_Stream\Exporter' ) ) {
trigger_error(

This comment has been minimized.

Copy link
@chacha

chacha Mar 9, 2016

Author Contributor

We should probably trigger this error in register_exporters(). You should be able to call is_valid_exporter() on anything and simply get true or false. Otherwise it might be disjarring, akin to raising an exception with you call is_array( 'string' ).

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 9, 2016

Contributor

👍

'user_id' => '',
'context' => '',
'action' => '',
'ip' => '',

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 14, 2016

Contributor

Spacing needs to be fixed here.

Luke Carbis

lukecarbis pushed a commit that referenced this pull request Mar 15, 2016

Luke Carbis

@lukecarbis lukecarbis merged commit 4468d6d into develop Mar 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@lukecarbis lukecarbis deleted the csv-export branch Mar 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.