Moodle
  1. Moodle
  2. MDL-14184 General Improvements to Quiz Reports
  3. MDL-14187

Improve tablelib - improve api and add functionality to download table contents in a variety of formats - XLS, ODS and CSV

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.1
    • Fix Version/s: 1.9.2
    • Component/s: Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      35053

      Description

      Add functionality to tablelib to optionally allow a table's contents to be downloaded as XLS, ODS or CSV.

      Functionality will allow for slightly different contents in html and downloaded files eg. there will be no links or action icons in the downloaded files.

      1. newtableliboverviewpatch.txt
        42 kB
        Jamie Pratt
      2. tablelibkeyedrow.txt
        1 kB
        Jamie Pratt
      3. tablelibpatch.txt
        39 kB
        Jamie Pratt
      4. test.php
        0.9 kB
        Jamie Pratt

        Issue Links

          Activity

          Hide
          Pierre Pichet added a comment -

          some of the improvments have been done already on the item analysis report i.e. adding a HTML output and adding columns so that the data can be more useful for other software analysis

          Show
          Pierre Pichet added a comment - some of the improvments have been done already on the item analysis report i.e. adding a HTML output and adding columns so that the data can be more useful for other software analysis
          Hide
          Jamie Pratt added a comment -

          I attach a patch to tablelib :

          + /**
          + * Add a row of data to the table. This function takes an array with
          + * column names as keys.
          + * It ignores any elements with keys that are not defined as columns. It
          + * puts in empty strings into the row when there is no element in the passed
          + * array corresponding to a column in the table. It puts the row elements in
          + * the proper order.
          + * @param $rowwithkeys array
          + *
          + */

          The basic idea is to have a way to add a row to a table without having to worry about getting the elements in the right order.

          All tables have an array $this->columns defined with unique names for each column in the table, this method takes an array with these column names as keys.

          This is the first part of my proposed additions to tablelib to make it a little easier to use and reduce code duplication.

          Comments please, do people agree that this can be added to HEAD and 1.9 branch? It is an addition to the API and won't affect any existing code.

          Show
          Jamie Pratt added a comment - I attach a patch to tablelib : + /** + * Add a row of data to the table. This function takes an array with + * column names as keys. + * It ignores any elements with keys that are not defined as columns. It + * puts in empty strings into the row when there is no element in the passed + * array corresponding to a column in the table. It puts the row elements in + * the proper order. + * @param $rowwithkeys array + * + */ The basic idea is to have a way to add a row to a table without having to worry about getting the elements in the right order. All tables have an array $this->columns defined with unique names for each column in the table, this method takes an array with these column names as keys. This is the first part of my proposed additions to tablelib to make it a little easier to use and reduce code duplication. Comments please, do people agree that this can be added to HEAD and 1.9 branch? It is an addition to the API and won't affect any existing code.
          Hide
          Jamie Pratt added a comment -

          My second proposal to improve tablelib is to have a third way to add data to a table.

          Most tables are constructed from an array of data returned by a sql query.

          I propose that we have a method on flexible_table, get_data();

          get_data can be called after the columns array is set.

          It will call a virtual private method on itself to construct the sql and fetch any data needed from the db or elsewhere.

          Then it will iterate through the data and :

          • For each column in the array get_data will call a method on the object passed to it 'get_data_{$columnname}' with parameters :
            • $data - a row from the returned sql and
            • $download - a parameter indicating whether we are building data for download or for display.

          The parameters are only for convenience. The get_data_{$columnname} can also pull data from properties on $this.

          If no method get_data_{$columnname} is found then the column content is assumed to be $data->$columname. So for a simple table with just text you don't need to define get_data_{$columnname} methods at all. Or you can define them one by one after checking that the correct data is being fetched from the db.

          Then people who want to use this new api for building table data need to create a child class of flexible_table overriding the virtual function to fetch data and optionally add get_data_{$columnname} methods.

          Show
          Jamie Pratt added a comment - My second proposal to improve tablelib is to have a third way to add data to a table. Most tables are constructed from an array of data returned by a sql query. I propose that we have a method on flexible_table, get_data(); get_data can be called after the columns array is set. It will call a virtual private method on itself to construct the sql and fetch any data needed from the db or elsewhere. Then it will iterate through the data and : For each column in the array get_data will call a method on the object passed to it 'get_data_{$columnname}' with parameters : $data - a row from the returned sql and $download - a parameter indicating whether we are building data for download or for display. The parameters are only for convenience. The get_data_{$columnname} can also pull data from properties on $this. If no method get_data_{$columnname} is found then the column content is assumed to be $data->$columname. So for a simple table with just text you don't need to define get_data_{$columnname} methods at all. Or you can define them one by one after checking that the correct data is being fetched from the db. Then people who want to use this new api for building table data need to create a child class of flexible_table overriding the virtual function to fetch data and optionally add get_data_{$columnname} methods.
          Hide
          Jamie Pratt added a comment -

          My third proposal is to move the code for producing downloads into tablelib.

          • we would filter download page parameter at the top of the page on which the table is printed and pass it to flexible_table to activate the download mode of tablelib.
          • headers and adding data to rows in the correct way is handled by flexible_table internals.
          • see above get_data_{$columnname} is passed a parameter $download which is set to 0 for no download or contains a string 'CSV', 'ODS' etc.
          • we would add methods to print download buttons. Buttons would be included for CSV, ODS, Excel and, if the table is paged, optionally to output full table as one XHTML file.
          Show
          Jamie Pratt added a comment - My third proposal is to move the code for producing downloads into tablelib. we would filter download page parameter at the top of the page on which the table is printed and pass it to flexible_table to activate the download mode of tablelib. headers and adding data to rows in the correct way is handled by flexible_table internals. see above get_data_{$columnname} is passed a parameter $download which is set to 0 for no download or contains a string 'CSV', 'ODS' etc. we would add methods to print download buttons. Buttons would be included for CSV, ODS, Excel and, if the table is paged, optionally to output full table as one XHTML file.
          Hide
          Pierre Pichet added a comment -

          These tablelib functions can be usefull for general uses and if correclty designed (as you always do ) will be a + .

          The different tables from the reports are a good way to test and code all the necessary features.

          Take care that flexible_table remains sufficiently flexible...

          I worry about "flexible_table internals." in

          "we would filter download page parameter at the top of the page on which the table is printed and pass it to flexible_table to activate the download mode of tablelib.

          • headers and adding data to rows in the correct way is handled by flexible_table internals. "

          I have to struggle with the internals of moodle_forms to obtain specific displays so ...

          Show
          Pierre Pichet added a comment - These tablelib functions can be usefull for general uses and if correclty designed (as you always do ) will be a + . The different tables from the reports are a good way to test and code all the necessary features. Take care that flexible_table remains sufficiently flexible... I worry about "flexible_table internals." in "we would filter download page parameter at the top of the page on which the table is printed and pass it to flexible_table to activate the download mode of tablelib. headers and adding data to rows in the correct way is handled by flexible_table internals. " I have to struggle with the internals of moodle_forms to obtain specific displays so ...
          Hide
          Tim Hunt added a comment -

          I just added some of the usual suspects from Moodle HQ, so they can give their opinion on this.

          I have long thought it silly that there is almost-duplicate code in the quiz reports for producing the HTML table and the various download formats. So I certainly support doing something.

          On a quick read through, Jamie's proposals sound plausible. But I want to think more before I comment.

          Show
          Tim Hunt added a comment - I just added some of the usual suspects from Moodle HQ, so they can give their opinion on this. I have long thought it silly that there is almost-duplicate code in the quiz reports for producing the HTML table and the various download formats. So I certainly support doing something. On a quick read through, Jamie's proposals sound plausible. But I want to think more before I comment.
          Hide
          Jamie Pratt added a comment -

          I started a docs page that documents how the new additions to the api might work, rather presumptuously here http://docs.moodle.org/en/Development:lib/tablelib.php

          Show
          Jamie Pratt added a comment - I started a docs page that documents how the new additions to the api might work, rather presumptuously here http://docs.moodle.org/en/Development:lib/tablelib.php
          Hide
          Jamie Pratt added a comment - - edited

          Here is my suggested patch to add to the tablelib api. It won't affect existing code using the tablelib api as it is. The patch is for the latest Moodle 1.9 stable branch.

          Show
          Jamie Pratt added a comment - - edited Here is my suggested patch to add to the tablelib api. It won't affect existing code using the tablelib api as it is. The patch is for the latest Moodle 1.9 stable branch.
          Hide
          Jamie Pratt added a comment - - edited

          test.php is a complete script with a basic usage example as explained in the docs page whose link is above.

          Show
          Jamie Pratt added a comment - - edited test.php is a complete script with a basic usage example as explained in the docs page whose link is above.
          Hide
          Jamie Pratt added a comment -

          overviewreportusingnewtablelib.txt is a patch to apply to 1.9 stable branch that shows the new api in use in the quiz overview report.

          Show
          Jamie Pratt added a comment - overviewreportusingnewtablelib.txt is a patch to apply to 1.9 stable branch that shows the new api in use in the quiz overview report.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Jamie,

          I've been looking docs and patch. And here there are some comments:

          • Architectural: I think it's better to make all the "download" stuff to be supported within flexitable itself, so any flexitable should be able to:
          • flexitable->downloadable(array(TABLE_D_ODS, TABLE_D_XLS, TABLE_D_XML, TABLE_D_TXT) (setting the "is_downloadeable" attribute). Defaults to null or empty array.
          • optionaly: flexitable->show_download_buttons_at(array(TABLE_P_TOP, TABLE_P_BOTTOM), to specify where the buttons should be showed, default to TABLE_P_TOP IMO.
          • flexitable->print_html() should show dynamically the buttons (hopefully one div, no table) respecting previous settings.
          • flexitable->is_downloading(optional_param('download', '', PARAM_ALPHA)) can be used to set the downloading status, and also to request current status if called without param.
          • flexitable->download() should perform all the download, calling download_XXX() methods, one for each supported format.

          And then, with download support in flexitable... I'd implement your ultra-cool simple_table, perhaps calling it sql_table or database_table, only with the bits necessary to work against one query.

          I know it's one radical change from your proposed patch, but I think we should separate downloads (core function) and "suppliers of information" (array of data, sql...).

          Just my opinion. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jamie, I've been looking docs and patch. And here there are some comments: Architectural: I think it's better to make all the "download" stuff to be supported within flexitable itself, so any flexitable should be able to: flexitable->downloadable(array(TABLE_D_ODS, TABLE_D_XLS, TABLE_D_XML, TABLE_D_TXT) (setting the "is_downloadeable" attribute). Defaults to null or empty array. optionaly: flexitable->show_download_buttons_at(array(TABLE_P_TOP, TABLE_P_BOTTOM), to specify where the buttons should be showed, default to TABLE_P_TOP IMO. flexitable->print_html() should show dynamically the buttons (hopefully one div, no table) respecting previous settings. flexitable->is_downloading(optional_param('download', '', PARAM_ALPHA)) can be used to set the downloading status, and also to request current status if called without param. flexitable->download() should perform all the download, calling download_XXX() methods, one for each supported format. And then, with download support in flexitable... I'd implement your ultra-cool simple_table, perhaps calling it sql_table or database_table, only with the bits necessary to work against one query. I know it's one radical change from your proposed patch, but I think we should separate downloads (core function) and "suppliers of information" (array of data, sql...). Just my opinion. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Of course, I missed that, you ideas (and the implementation) looks quite interesting! Thanks!
          (in fact that's the cause I proposed the alternative way in my previous message, because both downloads and sql are two ultra-cool extensions for the tablelib stuff). Re-thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Of course, I missed that, you ideas (and the implementation) looks quite interesting! Thanks! (in fact that's the cause I proposed the alternative way in my previous message, because both downloads and sql are two ultra-cool extensions for the tablelib stuff). Re-thanks! Ciao
          Hide
          Jamie Pratt added a comment - - edited

          Thanks for your comments Eloy. I like your ideas. Am willing to refactor the code I've written when we have some agreement of what the api should be like.

          Refactoring tablelib.php so much of it's functionality like paging, sorting by column etc. work with data sources other than one sql query will be quite a lot of work but I agree that putting the download code into the flexible_table class makes sense. Then at some later date someone else might agree to refactor tablelib so that it could work well with other data sources.

          Show
          Jamie Pratt added a comment - - edited Thanks for your comments Eloy. I like your ideas. Am willing to refactor the code I've written when we have some agreement of what the api should be like. Refactoring tablelib.php so much of it's functionality like paging, sorting by column etc. work with data sources other than one sql query will be quite a lot of work but I agree that putting the download code into the flexible_table class makes sense. Then at some later date someone else might agree to refactor tablelib so that it could work well with other data sources.
          Hide
          Jamie Pratt added a comment -

          Thought more about Eloy's comments and think that :

          • it would be inefficient to implement a method "download()" which would be an alternative to print_html. The problem is that this would entail always keeping a copy of all data added to the table. There would necessarily be a method add_data as there is now. The data added to the table would have to be stored in an array. If download is called after adding all the data to the table then another for loop would have to write these values into the data structure the export plug in would need.
          • we have already told the table we are downloading by calling is_downloading(). Why do we have to call the download method to tell the table again that we are downloading. Instead add_data should do the correct thing with the data depending on what format we set the is_downloading flag to.

          So I think that we should probably do a download as follows :

          //start_output is always called. If this is not a download, it does nothing.
          $table->start_output($filename, $sheettitle);
          //add data to table/export here with a for loop
          for ($data as $datarow)

          { //process data here //then : $table->add_data_keyed($datatoaddtotable); }

          $table->finish_output();

          In the case of the export formats to spreadsheet or to CSV we will probably output the data as we go along, instead of keeping a copy in memory. The old table class currently keeps a copy of the data in memory and then constructs the html from the internal array when you call print_html. We could call print_html from the finish_output function.

          I would propose we have a new plugin folder called lib/tableexport/

          {tableexportformats}

          / these would contain folders with files of a sub class of table_export_format that would have methods add_data, start_output and finish_output

          Instead of is_downloading() calls to check whether to output html or non-html when formatting rows of data we would call $table->wants_html() there would be a wants_html() method on the tableexport plug in classes which would be set to return false on the default parent class.

          Show
          Jamie Pratt added a comment - Thought more about Eloy's comments and think that : it would be inefficient to implement a method "download()" which would be an alternative to print_html. The problem is that this would entail always keeping a copy of all data added to the table. There would necessarily be a method add_data as there is now. The data added to the table would have to be stored in an array. If download is called after adding all the data to the table then another for loop would have to write these values into the data structure the export plug in would need. we have already told the table we are downloading by calling is_downloading(). Why do we have to call the download method to tell the table again that we are downloading. Instead add_data should do the correct thing with the data depending on what format we set the is_downloading flag to. So I think that we should probably do a download as follows : //start_output is always called. If this is not a download, it does nothing. $table->start_output($filename, $sheettitle); //add data to table/export here with a for loop for ($data as $datarow) { //process data here //then : $table->add_data_keyed($datatoaddtotable); } $table->finish_output(); In the case of the export formats to spreadsheet or to CSV we will probably output the data as we go along, instead of keeping a copy in memory. The old table class currently keeps a copy of the data in memory and then constructs the html from the internal array when you call print_html. We could call print_html from the finish_output function. I would propose we have a new plugin folder called lib/tableexport/ {tableexportformats} / these would contain folders with files of a sub class of table_export_format that would have methods add_data, start_output and finish_output Instead of is_downloading() calls to check whether to output html or non-html when formatting rows of data we would call $table->wants_html() there would be a wants_html() method on the tableexport plug in classes which would be set to return false on the default parent class.
          Hide
          Jamie Pratt added a comment -

          Uploaded new test.php which works with new tablelib.php patch which I'm about to upload.

          Show
          Jamie Pratt added a comment - Uploaded new test.php which works with new tablelib.php patch which I'm about to upload.
          Hide
          Jamie Pratt added a comment -

          My new patch for tablelib takes into account Eloy's comments above. Seperated out functionality download export types into seperate classes.

          Data is now added straight to either the spreadsheet or output to the buffer rather than keeping a redundant copy of data in an array inside the table object as this might consume significant amounts of extra memory for large downloadable tables.

          The existing flexible_table API is still supported.

          Show
          Jamie Pratt added a comment - My new patch for tablelib takes into account Eloy's comments above. Seperated out functionality download export types into seperate classes. Data is now added straight to either the spreadsheet or output to the buffer rather than keeping a redundant copy of data in an array inside the table object as this might consume significant amounts of extra memory for large downloadable tables. The existing flexible_table API is still supported.
          Hide
          Jamie Pratt added a comment -

          This is a patch for the overview report that uses the new tablelib.

          Show
          Jamie Pratt added a comment - This is a patch for the overview report that uses the new tablelib.
          Hide
          Martin Dougiamas added a comment -

          Looks pretty cool to me and fairly easy to understand. My +1 for HEAD iff Eloy agrees.

          Show
          Martin Dougiamas added a comment - Looks pretty cool to me and fairly easy to understand. My +1 for HEAD iff Eloy agrees.
          Hide
          Tim Hunt added a comment -

          Looks good to me too.

          Show
          Tim Hunt added a comment - Looks good to me too.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 gogogo! Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - +1 gogogo! Thanks!
          Hide
          Jamie Pratt added a comment -

          additions to tablelib api are in HEAD.

          Show
          Jamie Pratt added a comment - additions to tablelib api are in HEAD.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: