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

      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.

        Gliffy Diagrams

        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: