This feature will add the option of displaying a select quote for the day on the OPAC homepage. It will include the addition of a QOD editor in the tools section of the staff interface which will allow the addition, editing, and deletion of quotes. A single system preference will enable/disable the display of the QOD on the OPAC homepage. A new granular permission will also be added to conrol user access to the QOD editor tool. Possible extentions to this would be code to allow alternate quote selection algorithms to be added to vary how the currently displayed quote is selected from the table of quotes.
WIP may be followed here: https://github.com/cnighswonger/koha-wip/tree/enhancement/bug_7977 A patch series will also be attached to this bug.
Created attachment 9310 [details] CSV of US Presidential Quotes for Testing
Created attachment 9321 [details] [review] Upgrading DataTables library to 1.9.1 Previous versions of the DataTables library did not support listeners for custom DataTabels events. These listeners are necessary to the "add quote" feature of the QOD editor.
Created attachment 9323 [details] [review] Updating-datatables.css-to-reflect-changes-from-v1.8
Created attachment 9324 [details] [review] Adding-jEditable-minified-version
Created attachment 9325 [details] [review] Adding-DateTime-Format-MySQL-as-a-core-dependency
Created attachment 9326 [details] [review] Quote-of-the-day-QOTD-Feature
Created attachment 9327 [details] [review] QOTD-uploader
Created attachment 9328 [details] [review] Misc-UI-improvements-to-both-the-QOTD-editor-and-upl
Created attachment 9338 [details] [review] Misc UI improvements to both the QOTD editor and uploader
Created attachment 9343 [details] [review] Bug 7977 follow-up: Fix errors that came up in testing * Add note explaining that one has to hit Enter to save changes * Add a span for the separator on the OPAC * Change abbreviation for Quote-of-the-Day to QOTD
Created attachment 9373 [details] [review] Adding sanity checks to verify file type and size
Created attachment 9376 [details] [review] Beginning work on Quote-of-the-day (QOTD) Feature for OPAC This feature will add the option of displaying a select quote for the day on the OPAC homepage. It will include the addition of a QOTD editor in the tools section of the staff interface which will allow the addition, editing, and deletion of quotes. A single system preference will enable/disable the display of the QOTD on the OPAC homepage. A new granular permission will also be added to conrol user access to the QOTD editor tool. Possible extentions to this would be code to allow alternate quote selection algorithms to be added to vary how the currently displayed quote is selected from the table of quotes. This patch is a squash of the following work: Adding quotes table to kohastructure.sql and updatedatabase.pl Note: This patch is intended for a MySQL based installation only. That means that it includes backticks because that is what is required at this point in history to install Koha correctly on a MySQL based installation. Feel free to port this over to the Pg stuff in the data/Pg directory. Adding system preference to control display of QOTD on OPAC main page Adding sample quote data These quotes are taken from various US presidents. I'm not sure these are applicable to the rest of the world, so I'm leaving it to translators and others to add sample quote data for other languages. Adding edit_quotes user permissions Squash with other db related patches in this series Adding quotes editor pages to tools Adding QOTD editor link to tools homepage Integration of DataTables into QOTD Editor This includes only the table generation; jEditable to be added next to allow quote adding and editing directly in the table. Impleminting jEditable into the quote editor Adding delete and add option to editor Fixing OPAC display so the QOTD div does not show if no quote is returned Also removing useless code from tools/quotes.pl Adding spans around QOTD foo in opac-main.tt Also fixing quote selection logic to accomodate the possibility of a single quote in the db a litte better. Changing timestamp column header to a more user-friendly 'Last Displayed' Fixing bug in quote selection logic This bug caused a situation where when the table had only a single quote with an id greater than 1 in it, no quote would be selected. Fixing up sorting in the quote editor table. (Thanks to jcamins for pointing out that this did not work.) Overriding the default empty table message supplied by datatables-strings.inc NOTE: Someone needs to verify that this local string is translatable.
Created attachment 9377 [details] [review] Beginning work on QOTD uploader This series will add a DataTable's based upload/editor with which to upload csv files containing quotes to be used by the QOTD feature. The file should be formatted thusly: "source","text-of-quote" "source","text-of-quote" ... Note: This work serves as a good example of potential improvements in all other "editor" and file upload areas of Koha. This patch is a squash of the following work: Adding code to parse CSV file contents and push it into a DataTable Adding in jEditable to enable table editing Adding ajax to post data back to the server to be saved Fixing edit and adding delete functionality Adding some missing css as well as server feedback on save Fixing a bug which limited the number of quotes which could be uploaded Also fixing a minor bug with fnCSVToArray and doing some style cleanup. Adding sanity checks to verify file type and size
Created attachment 9378 [details] [review] Misc UI improvements to both the QOTD editor and uploader including a number suggested by jcamins Also removing a debug warn
To test this enhancement: 1. Apply all 7 patches in the order they are attached. 2. Download the csv file. 4. Allow Koha to update the database. 5. Navigate to "Tools>Quote Editor." 6. You should see an empty table with a message stating "No quotes available. Please use the "Add Quote" button to add a quote." 7. Click the "Import Quotes" button in the Quote Editor toolbar. 8. You should now see a toolbar with a "Choose File" button. Click this button. 9. Browse to the directory containing the CSV file you previously downloaded. 10. Select the file. 11. You should now see a table populated with the quotes contained in the CSV file. 12. Browse this table and edit fields and/or delete quotes as you like. 13. When finished, click the "Save Quotes" button. 14. There are over 1500 quotes in this file, so you will probably notice lots of HDD activity while the back-end script inserts the quotes into the database. You may do other things while this is going on including leaving this page if you like. 15. If you opt to remain on the page, once the quotes have been saved, a notification will be displayed showing that 1524 quotes have been saved. You may click on the notification to close it. 16. You should now be directed back to the upload page and ready to upload more quotes if you like. 17. Click on the "Quote Editor" option in the menu bar to the right to return to the quote editor. 18. The editor should now be populated with the quotes you just uploaded. 19. You may edit the source or text columns as you wish or delete quotes. The table should update real-time. 20. You may also elect to add a single quote by clicking the "Add Quote" button. 21. Doing this will add a row at the end of the list of quotes. 22. You may now add a source and text. 23. Clicking any of the buttons in the "Action" column will result in that action being performed. 24. If you are satisfied that this monstrosity works, please sign off on this patch series. Please avoid squashing it any further in order to keep the upgrade/addition of various jQuery libraries separated from the work on this feature.
I have some issues with the staff client pages: 1. The pages have no headings. Both the main page and the upload page should show the name of the page. 2. The templates don't obey capitalization rules addressed by Bug 2780 ("Import Quotes" should be "Import quotes"). 3. The buttons in the toolbar have a custom style applied to them. Instead of making them different from other toolbar buttons they should be generated using the same code other toolbars use. 4. The comment in the toolbar ("Click Source or Text field...") belongs somewhere else. We don't have a well-established pattern for text like this, but the "Authorised values" page offers a possible example (/cgi-bin/koha/admin/authorised_values.pl). 5. I think it would be nice to have a compact table pager at the top in addition to the full pager at the bottom. 6. The "actions" columns should not be sortable. 7. datatables.css adds "border-radius: 5px;" to all inputs. This kind of broad style change shouldn't be happening in a module-specific CSS file. It could be qualified so that it applies only to datatables-related inputs. IMO it should be removed altogether because it doesn't look consistently good across browsers. 8. The "import quotes" page lacks any kind of instructions. Perhaps it should at least specify what format it expects? 9. The upload form on the import is in the toolbar div. It belongs in the main body of the page, preferably in a <fieldset> with a descriptive <legend> 10. The "save quotes" button doesn't belong in the toolbar. Generally sava buttons are at the bottom of tables, although some pages have them at the top. In any case putting it in the toolbar makes its purpose unclear. It wasn't clear to me that I had to click "save quotes" to complete the process. 11. The breadcrumbs on the import page should give the user a path back to the main quotes page: "Home > Tools > Quote editor > Quote uploader."
Created attachment 9392 [details] [review] Major JS cleanup and misc improvements to the quote editor Adds missing page heading... thanks oleonard. Adds beginning of click-for-help elements. Refactors delete functionality to allow selecting of quotes to delete, enabling multi-delete. Refactors saving added quote functionality so that striking <Enter> saves the new quote. Refactors canceling aded quote functionality so that striking <Esc> cancles the new quote.
Created attachment 9405 [details] [review] Quote-of-the-day (QOTD) Feature for OPAC This feature will add the option of displaying a select quote for the day on the OPAC homepage. It will include the addition of a QOTD editor in the tools section of the staff interface which will allow the addition, editing, and deletion of quotes. A single system preference will enable/disable the display of the QOTD on the OPAC homepage. A new granular permission will also be added to conrol user access to the QOTD editor tool. Possible extentions to this would be code to allow alternate quote selection algorithms to be added to vary how the currently displayed quote is selected from the table of quotes. This patch is a squash of the following work: --Adding quotes table to kohastructure.sql and updatedatabase.pl Note: This patch is intended for a MySQL based installation only. That means that it includes backticks because that is what is required at this point in history to install Koha correctly on a MySQL based installation. Feel free to port this over to the Pg stuff in the data/Pg directory. --Adding system preference to control display of QOTD on OPAC main page --Adding sample quote data These quotes are taken from various US presidents. I'm not sure these are applicable to the rest of the world, so I'm leaving it to translators and others to add sample quote data for other languages. --Adding edit_quotes user permissions --Squash with other db related patches in this series --Adding quotes editor pages to tools --Adding QOTD editor link to tools homepage --Integration of DataTables into QOTD Editor --Impleminting jEditable into the quote editor --Adding delete and add option to editor --Fixing OPAC display so the QOTD div does not show if no quote is returned --Also removing useless code from tools/quotes.pl --Adding spans around QOTD foo in opac-main.tt --Also fixing quote selection logic to accomodate the possibility of a single quote in the db a litte better. --Changing timestamp column header to a more user-friendly 'Last Displayed' --Fixing bug in quote selection logic. This bug caused a situation where when the table had only a single quote with an id greater than 1 in it, no quote would be selected. --Fixing up sorting in the quote editor table. --jcamins --Adding span element to quote separator --jcamins --Overriding the default empty table message supplied by datatables-strings.inc --Adds missing page heading --Adds beginning of click-for-help elements. --Refactors delete functionality to allow selecting of quotes to delete, enabling multi-delete. --Refactors saving added quote functionality so that striking <Enter> saves the new quote. --Refactors canceling aded quote functionality so that striking <Esc> cancels the new quote. --Removing debug console.log statements --Implementng the YUI button widget/toolbar used on other pages. --Adds a 5px radius to all YUI buttons to bring them into conformity with the general trend toward rounded corners --Fixes capitalization in quote editor --Implements improvements suggested by jcamins and oleonard --Adds DataTables Plugin dataTables.fnReloadAjax.js
Created attachment 9406 [details] [review] QOTD uploader to enable uploading csv files containing quotes This series will add a DataTable's based upload/editor with which to upload csv files containing quotes to be used by the QOTD feature. The file should be formatted thusly: "source","text-of-quote" "source","text-of-quote" ... Note: This work serves as a good example of potential improvements in all other "editor" and file upload areas of Koha. This patch is a squash of the following work: --Adding code to parse CSV file contents and push it into a DataTable --Adding in jEditable to enable table editing --Adding ajax to post data back to the server to be saved --Fixing edit and adding delete functionality --Adding some missing css as well as server feedback on save --Fixing a bug which limited the number of quotes which could be uploaded --Also fixing a minor bug with fnCSVToArray and doing some style cleanup. --Adding sanity checks to verify file type and size --Implements YUI button widget/toolbar --Improved handling of hiding uploader UI --Adds row selectability --Adds multi-delete capability --Adds YUI button/toolbar widget --Fixing capitalization in quote uploader --Implements improvments suggested by jcamins and oleonard
Created attachment 9407 [details] [review] Quote-of-the-day (QOTD) Feature for OPAC This feature will add the option of displaying a select quote for the day on the OPAC homepage. It will include the addition of a QOTD editor in the tools section of the staff interface which will allow the addition, editing, and deletion of quotes. A single system preference will enable/disable the display of the QOTD on the OPAC homepage. A new granular permission will also be added to conrol user access to the QOTD editor tool. Possible extentions to this would be code to allow alternate quote selection algorithms to be added to vary how the currently displayed quote is selected from the table of quotes. This patch is a squash of the following work: --Adding quotes table to kohastructure.sql and updatedatabase.pl Note: This patch is intended for a MySQL based installation only. That means that it includes backticks because that is what is required at this point in history to install Koha correctly on a MySQL based installation. Feel free to port this over to the Pg stuff in the data/Pg directory. --Adding system preference to control display of QOTD on OPAC main page --Adding sample quote data These quotes are taken from various US presidents. I'm not sure these are applicable to the rest of the world, so I'm leaving it to translators and others to add sample quote data for other languages. --Adding edit_quotes user permissions --Squash with other db related patches in this series --Adding quotes editor pages to tools --Adding QOTD editor link to tools homepage --Integration of DataTables into QOTD Editor --Impleminting jEditable into the quote editor --Adding delete and add option to editor --Fixing OPAC display so the QOTD div does not show if no quote is returned --Also removing useless code from tools/quotes.pl --Adding spans around QOTD foo in opac-main.tt --Also fixing quote selection logic to accomodate the possibility of a single quote in the db a litte better. --Changing timestamp column header to a more user-friendly 'Last Displayed' --Fixing bug in quote selection logic. This bug caused a situation where when the table had only a single quote with an id greater than 1 in it, no quote would be selected. --Fixing up sorting in the quote editor table. --jcamins --Adding span element to quote separator --jcamins --Overriding the default empty table message supplied by datatables-strings.inc --Adds missing page heading --Adds beginning of click-for-help elements. --Refactors delete functionality to allow selecting of quotes to delete, enabling multi-delete. --Refactors saving added quote functionality so that striking <Enter> saves the new quote. --Refactors canceling aded quote functionality so that striking <Esc> cancels the new quote. --Removing debug console.log statements --Implementng the YUI button widget/toolbar used on other pages. --Adds a 5px radius to all YUI buttons to bring them into conformity with the general trend toward rounded corners --Fixes capitalization in quote editor --Implements improvements suggested by jcamins and oleonard --Adds DataTables Plugin dataTables.fnReloadAjax.js
Created attachment 9408 [details] [review] QOTD uploader to enable uploading csv files containing quotes This series will add a DataTable's based upload/editor with which to upload csv files containing quotes to be used by the QOTD feature. The file should be formatted thusly: "source","text-of-quote" "source","text-of-quote" ... Note: This work serves as a good example of potential improvements in all other "editor" and file upload areas of Koha. This patch is a squash of the following work: --Adding code to parse CSV file contents and push it into a DataTable --Adding in jEditable to enable table editing --Adding ajax to post data back to the server to be saved --Fixing edit and adding delete functionality --Adding some missing css as well as server feedback on save --Fixing a bug which limited the number of quotes which could be uploaded --Also fixing a minor bug with fnCSVToArray and doing some style cleanup. --Adding sanity checks to verify file type and size --Implements YUI button widget/toolbar --Improved handling of hiding uploader UI --Adds row selectability --Adds multi-delete capability --Adds YUI button/toolbar widget --Fixing capitalization in quote uploader --Implements improvments suggested by jcamins and oleonard
These two patches reflect several bug-fixes, improvements, js refactoring, and address points 1,2,3,6 & 11 of Owen's concerns. More work will be forthcoming to address most of Owen's remaining points as well as to ensure translatability and add more click-helps as well as Koha's standard help. Marking this as "In Discussion" because that best represents the status of this bug at this time.
Created attachment 9409 [details] [review] QOTD uploader to enable uploading csv files containing quotes This series will add a DataTable's based upload/editor with which to upload csv files containing quotes to be used by the QOTD feature. The file should be formatted thusly: "source","text-of-quote" "source","text-of-quote" ... Note: This work serves as a good example of potential improvements in all other "editor" and file upload areas of Koha. This patch is a squash of the following work: --Adding code to parse CSV file contents and push it into a DataTable --Adding in jEditable to enable table editing --Adding ajax to post data back to the server to be saved --Fixing edit and adding delete functionality --Adding some missing css as well as server feedback on save --Fixing a bug which limited the number of quotes which could be uploaded --Also fixing a minor bug with fnCSVToArray and doing some style cleanup. --Adding sanity checks to verify file type and size --Implements YUI button widget/toolbar --Improved handling of hiding uploader UI --Adds row selectability --Adds multi-delete capability --Adds YUI button/toolbar widget --Fixing capitalization in quote uploader --Implements improvments suggested by jcamins and oleonard
Created attachment 9433 [details] [review] Further UI work on the QOTD uploader This patch addresses another of oleonard's points as well as some general cleanup: --Adds instructions to each stage of the uploader --Consolidates quote tool css into quotes.css
Created attachment 9434 [details] [review] Further UI work on the QOTD editor This patch implements more of oleonard's suggestions as well as other improvments: --Adds instructions for using the QOTD editor --Moves inline css to quotes.css, etc.
The last four patches attached here incorporate the majority of oleonard's recommendations as well as various improvements including the marking of JS strings for translatability. NOTE FOR TESTERS: These last four patches are new, so you will have to roll off any previous versions of them prior to applying the new ones.
Created attachment 9435 [details] [review] Fixing captialization on the tools main page
Created attachment 9441 [details] [review] Adding Daily Quotes Tests to Koha.t test suite; also fixing sort order on quote selection SQL
Created attachment 9442 [details] [review] Fixing quote sample data timestamps
Created attachment 9443 [details] [review] Adding Daily Quotes Tests to Koha.t test suite; also fixing sort order on quote selection SQL
Created attachment 9447 [details] [review] Adding Koha standard help files for QOTD feature
Created attachment 9473 [details] [review] Fixing sub refs; C4::Koha does not export all functions by default
Created attachment 9474 [details] [review] Bug 7977: Upgrading DataTables library to 1.9.1 Previous versions of the DataTables library did not support listeners for custom DataTabels events. These listeners are necessary to the "add quote" feature of the QOD editor. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9475 [details] [review] Bug 7977: Updating datatables.css to reflect changes from v1.8.1 to v1.9.1 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9476 [details] [review] Fixing C4::Koha import
Created attachment 9477 [details] [review] Bug 7977: Adding jEditable minified version jEditable is necessary to create DataTables with editable fields Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9478 [details] [review] Bug 7977: Adding DateTime::Format::MySQL as a core dependency Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9479 [details] [review] Bug 7977: Quote-of-the-day (QOTD) Feature for OPAC This feature will add the option of displaying a select quote for the day on the OPAC homepage. It will include the addition of a QOTD editor in the tools section of the staff interface which will allow the addition, editing, and deletion of quotes. A single system preference will enable/disable the display of the QOTD on the OPAC homepage. A new granular permission will also be added to conrol user access to the QOTD editor tool. Possible extentions to this would be code to allow alternate quote selection algorithms to be added to vary how the currently displayed quote is selected from the table of quotes. This patch is a squash of the following work: --Adding quotes table to kohastructure.sql and updatedatabase.pl Note: This patch is intended for a MySQL based installation only. That means that it includes backticks because that is what is required at this point in history to install Koha correctly on a MySQL based installation. Feel free to port this over to the Pg stuff in the data/Pg directory. --Adding system preference to control display of QOTD on OPAC main page --Adding sample quote data These quotes are taken from various US presidents. I'm not sure these are applicable to the rest of the world, so I'm leaving it to translators and others to add sample quote data for other languages. --Adding edit_quotes user permissions --Squash with other db related patches in this series --Adding quotes editor pages to tools --Adding QOTD editor link to tools homepage --Integration of DataTables into QOTD Editor --Impleminting jEditable into the quote editor --Adding delete and add option to editor --Fixing OPAC display so the QOTD div does not show if no quote is returned --Also removing useless code from tools/quotes.pl --Adding spans around QOTD foo in opac-main.tt --Also fixing quote selection logic to accomodate the possibility of a single quote in the db a litte better. --Changing timestamp column header to a more user-friendly 'Last Displayed' --Fixing bug in quote selection logic. This bug caused a situation where when the table had only a single quote with an id greater than 1 in it, no quote would be selected. --Fixing up sorting in the quote editor table. --jcamins --Adding span element to quote separator --jcamins --Overriding the default empty table message supplied by datatables-strings.inc --Adds missing page heading --Adds beginning of click-for-help elements. --Refactors delete functionality to allow selecting of quotes to delete, enabling multi-delete. --Refactors saving added quote functionality so that striking <Enter> saves the new quote. --Refactors canceling aded quote functionality so that striking <Esc> cancels the new quote. --Removing debug console.log statements --Implementng the YUI button widget/toolbar used on other pages. --Adds a 5px radius to all YUI buttons to bring them into conformity with the general trend toward rounded corners --Fixes capitalization in quote editor --Implements improvements suggested by jcamins and oleonard --Adds DataTables Plugin dataTables.fnReloadAjax.js Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9480 [details] [review] Bug 7977: QOTD uploader to enable uploading csv files containing quotes This series will add a DataTable's based upload/editor with which to upload csv files containing quotes to be used by the QOTD feature. The file should be formatted thusly: "source","text-of-quote" "source","text-of-quote" ... Note: This work serves as a good example of potential improvements in all other "editor" and file upload areas of Koha. This patch is a squash of the following work: --Adding code to parse CSV file contents and push it into a DataTable --Adding in jEditable to enable table editing --Adding ajax to post data back to the server to be saved --Fixing edit and adding delete functionality --Adding some missing css as well as server feedback on save --Fixing a bug which limited the number of quotes which could be uploaded --Also fixing a minor bug with fnCSVToArray and doing some style cleanup. --Adding sanity checks to verify file type and size --Implements YUI button widget/toolbar --Improved handling of hiding uploader UI --Adds row selectability --Adds multi-delete capability --Adds YUI button/toolbar widget --Fixing capitalization in quote uploader --Implements improvments suggested by jcamins and oleonard Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9481 [details] [review] Bug 7977: Further UI work on the QOTD uploader This patch addresses another of oleonard's points as well as some general cleanup: --Adds instructions to each stage of the uploader --Consolidates quote tool css into quotes.css Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9482 [details] [review] Bug 7977: Further UI work on the QOTD editor This patch implements more of oleonard's suggestions as well as other improvments: --Adds instructions for using the QOTD editor --Moves inline css to quotes.css, etc. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9483 [details] [review] Bug 7977: Fixing capitalization on the tools main page Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9484 [details] [review] Bug 7977: Fixing quote sample data timestamps Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9485 [details] [review] Bug 7977: Adding Daily Quotes Tests to Koha.t test suite; also fixing sort order on quote selection SQL Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9486 [details] [review] Bug 7977: Adding Koha standard help files for QOTD feature Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
QA Comment for Bug 7977: Upgrading DataTables library to 1.9.1 Bug 7977: Updating datatables.css to reflect changes from v1.8.1 to v1.9.1 Bug 7977: Adding jEditable minified version Bug 7977: Adding DateTime::Format::MySQL as a core dependency Marking as Passed QA
QA Comment for Bug 7977: Quote-of-the-day (QOTD) Feature for OPAC - backquotes exist in sql files (updatedatabase and kohastructure) - Debug javascript message is commited (console.log) - + var sEmptyTable = _('No quotes available. Please use the \"Add Quote\" button to add a quote.'); Why do you escaped double quote ? - html comments are commited (+<!-- <th>Actions</th>-->) - you use new font family (+ font-family: "Georgia","Palatino","Times New Roman",sans-serif;). Currently, these fonts are not used in Koha. Isn't it better to use already used fonts ? - + my $like = " id LIKE \"%$filter%\" OR source LIKE \"%$filter%\" OR text LIKE \"%$filter%\" OR timestamp LIKE \"%$filter%\""; Why don't you use bind parameters to protect against SQL injection attacks ?
QA comment: Following patches look good. Just... the link in your help page does not exist (http://manual.koha-community.org/3.8/en/QOTDuploader.html). I don't know if it is normal or not :-/
(In reply to comment #48) > QA Comment for Bug 7977: Quote-of-the-day (QOTD) Feature for OPAC > > - backquotes exist in sql files (updatedatabase and kohastructure) This is a non-issue since both of these files are DB specific and targeted for MySQL. Backticks are not optional for MySQL since certain keywords used as field/table names could cause problems. Its my opinion that those concerned with porting to other DBs should construct files specific to those DB's until such a time as we move to a DB agnostic interface. I think I mentioned this in the commit message. > - Debug javascript message is commited (console.log) > - + var sEmptyTable = _('No quotes available. Please use the \"Add > Quote\" button to add a quote.'); > Why do you escaped double quote ? > - html comments are commited (+<!-- > <th>Actions</th>-->) These are leftovers from development. I'll submit a followup patch to remove them. > - you use new font family (+ font-family: "Georgia","Palatino","Times New > Roman",sans-serif;). Currently, these fonts are not used in Koha. Isn't it > better to use already used fonts ? This was unintentional, where is it located? > - + my $like = " id LIKE \"%$filter%\" OR source LIKE \"%$filter%\" > OR text LIKE \"%$filter%\" OR timestamp LIKE \"%$filter%\""; > Why don't you use bind parameters to protect against SQL injection attacks ? Good catch. I'll include this in the followup patch. Thanks for the QA and the feedback!
(In reply to comment #49) > QA comment: > Following patches look good. > > Just... the link in your help page does not exist > (http://manual.koha-community.org/3.8/en/QOTDuploader.html). I don't know if > it is normal or not :-/ This is because no links exist atm. I mentioned this to Nicole on #koha the other day. These are really just "placeholders" which can be either used by her or a followup patch submitted once she has the info in the manual. Perhaps we should have a procedure for this, though, so I'll suggest that to her.
You should probably remove the link for now and I'll put in the right one once I write it up in the manual. Nicole
Created attachment 9503 [details] [review] Followup patch addressing items pointed out by Jonathan Druart
(In reply to comment #53) > Created attachment 9503 [details] [review] > Followup patch addressing items pointed out by Jonathan Druart This patch addresses the above issues with the exception of the font issue. Grep cannot seem to find where this font is referenced.
Created attachment 9504 [details] [review] Followup patch addressing items pointed out by Jonathan Druart
Created attachment 9505 [details] [review] Bug 7977: Followup patch addressing items pointed out by Jonathan Druart Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
(In reply to comment #55) > Created attachment 9504 [details] [review] > Followup patch addressing items pointed out by Jonathan Druart Hi Chris, Thanks for your followup. However, I think the following syntax is not usable: $iTotalRecords = $dbh->selectrow_array("SELECT count(*) FROM quotes WHERE id LIKE %?% OR source LIKE %?% OR text LIKE %?% OR timestamp LIKE %?%;",undef,($filter, $filter, $filter, $filter)); You can write this like: $iTotalRecords = $dbh->selectrow_array("SELECT count(*) FROM quotes WHERE id LIKE ? OR source LIKE ? OR text LIKE ? OR timestamp LIKE ?;",undef,("%$filter%")x4);
Created attachment 9510 [details] [review] Removing unused code from quotes_ajax.pl
(In reply to comment #57) > (In reply to comment #55) > > Created attachment 9504 [details] [review] > > Followup patch addressing items pointed out by Jonathan Druart > > Hi Chris, > Thanks for your followup. > > However, I think the following syntax is not usable: > > $iTotalRecords = $dbh->selectrow_array("SELECT count(*) FROM quotes > WHERE id LIKE %?% OR source LIKE %?% OR text LIKE %?% OR timestamp LIKE > %?%;",undef,($filter, $filter, $filter, $filter)); > > You can write this like: > > $iTotalRecords = $dbh->selectrow_array("SELECT count(*) FROM quotes > WHERE id LIKE ? OR source LIKE ? OR text LIKE ? OR timestamp LIKE > ?;",undef,("%$filter%")x4); This is actually unused due to the removal of server-side processing, so I've attached another followup removing it from quotes_ajax.pl Thanks!
Created attachment 9517 [details] [review] Fixing a bug which results in quote fields being incorrectly offset when saving uploaded quotes
Created attachment 9521 [details] [review] Bug 7977: Improving upload result message display
Created attachment 9529 [details] [review] Bug 7977: Removing unused code from quotes_ajax.pl Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9530 [details] [review] Bug 7977: Fixing a bug which results in quote fields being incorrectly offset when saving uploaded quotes Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 9531 [details] [review] Bug 7977: Improving upload result message display Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
All follow-ups signed off.
QA comments: I think these patchs can be passed QA. Given the length and the complexity of this development I prefer to need additional QA.
As this bug appears to have passed QA, please set the status accordingly so this can be pushed on to master. It will be difficult to keep rebased if too much time elapses.
Adding the other members of the QA team to the CC list and requesting a second QA opinion.
QA comments: 1) Introduces a new system dependency, DateTime::Format::MySQL, which only seems to be used to format datetimes into ISO for querying the quotes table. We already have Koha::DateUtils, which can process a datetime in ISO, or many other formats, so I'm not seeing the need to add a dependency on DateTime::Format::MySQL. Does this module do something more that I'm missing? 2) Also, the varchar(45) on the 'source' for the quote seems a bit short... I think having this as a 'text' field would provide more flexibility. 3) Minor typo in tools/quotes-upload.tt: "...you will be promoted to..." should be "...you will be prompted to..." 4) Link to documentation is hardcoded to 3.8 manual page... should it be this way? Won't that require maintenance every release, to keep pointing at the right version? 5) Hardcoded styling in tools/quotes.tt (the <p> under <h1>QOTD Editor</h1>) Marking as Failed QA primarily for 1), but 3) and 5) need t be addressed, as well. 2) and 4) are up for discussion. I think this would be a prime candidate for a Plugin as per Bug 7804... it seems like a pretty stand-alone thing, if it's basic requirements are met. The only complication I see is getting permissions handled properly, but that's just a matter of a small database update script, which would need to run anyway to install the table.
(In reply to comment #69) > QA comments: > I think this would be a prime candidate for a Plugin as per Bug 7804... it > seems like a pretty stand-alone thing, if it's basic requirements are met. > The only complication I see is getting permissions handled properly, but > that's just a matter of a small database update script, which would need to > run anyway to install the table. I just commented on bug 7804. I am profoundly concerned about the security implications of the current implementation there. I think it would be better to integrate this into the Koha codebase as is, while work on bug 7804 is underway. I documented what I feel would be necessary for a plugin host to be safe, and I don't think the fixes are trivial.
(In reply to comment #69) > QA comments: > > 1) Introduces a new system dependency, DateTime::Format::MySQL, which only > seems to be used to format datetimes into ISO for querying the quotes table. > We already have Koha::DateUtils, which can process a datetime in ISO, or > many other formats, so I'm not seeing the need to add a dependency on > DateTime::Format::MySQL. Does this module do something more that I'm > missing? Looking at the perldoc for Koha::DateUtils it appears that we are transitioning to DateTime and that the K::DU module is merely meant to ease that transition by hiding "the complexity of switch to DateTime." I would suggest that using DateTime::Format::MySQL is the better approach as we work toward phasing out Koha::DateUtils and moving completely to DateTime.
(In reply to comment #69) > QA comments: > > 2) Also, the varchar(45) on the 'source' for the quote seems a bit short... > I think having this as a 'text' field would provide more flexibility. Agreed. I'll do this with a followup patch.
(In reply to comment #69) > QA comments: > > > 3) Minor typo in tools/quotes-upload.tt: "...you will be promoted to..." > should be "...you will be prompted to..." > > > 5) Hardcoded styling in tools/quotes.tt (the <p> under <h1>QOTD Editor</h1>) > I'll fix these in the followup as well.
(In reply to comment #69) > QA comments: > 4) Link to documentation is hardcoded to 3.8 manual page... should it be > this way? Won't that require maintenance every release, to keep pointing at > the right version? > This was copied from existing references, so I'm assuming this is the normal method, but I'll defer to Nicole on that and make any changes she recommends.
(In reply to comment #69) > QA comments: > I think this would be a prime candidate for a Plugin as per Bug 7804... it > seems like a pretty stand-alone thing, if it's basic requirements are met. > The only complication I see is getting permissions handled properly, but > that's just a matter of a small database update script, which would need to > run anyway to install the table. I've pretty much burned up the hours allotted for development on this feature, so I will not be able to port it over to a plugin. That does not, however, prevent someone else from doing so if that is desirable. Or for funding the additional hours needed to do so.
Created attachment 9682 [details] [review] Making source a text field rather than a varchar to avoid potential length limitations
Created attachment 9683 [details] [review] Fixing a typo in the quotes-upload help file and hardcoded css
These patches address #'s 2,3,5 of Ian's concerns. I'd like to have a better handle on our long term use of Koha::DateUtils before removing DateTime::Format::MySQL.
(In reply to comment #78) > These patches address #'s 2,3,5 of Ian's concerns. > > I'd like to have a better handle on our long term use of Koha::DateUtils > before removing DateTime::Format::MySQL. It was my understanding that Koha::DateUtils was a transitory package, perhaps we could check with Colin Campbell.
Ah, I must have missed that about Koha::DateUtils. No need to recreate the wheel if we can include another stable, Debian-packaged library. If we're going to phase it out, then using DateTime::Format::__something__ makes sense, though __something__ == MySQL could be coding us into a corner when Postgres support roles along.
(In reply to comment #80) > Ah, I must have missed that about Koha::DateUtils. No need to recreate the > wheel if we can include another stable, Debian-packaged library. If we're > going to phase it out, then using DateTime::Format::__something__ makes > sense, though __something__ == MySQL could be coding us into a corner when > Postgres support roles along. It should be trivial to add code to check kohaconf.xml for our DB flavor and load the appropriate module. This will be a broad issue I suspect.
Created attachment 9711 [details] [review] Bug 7977: Upgrading DataTables library to 1.9.1 Previous versions of the DataTables library did not support listeners for custom DataTabels events. These listeners are necessary to the "add quote" feature of the QOD editor. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9712 [details] [review] Bug 7977: Upgrading DataTables library to 1.9.1 Previous versions of the DataTables library did not support listeners for custom DataTabels events. These listeners are necessary to the "add quote" feature of the QOD editor. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9713 [details] [review] Bug 7977: Updating datatables.css to reflect changes from v1.8.1 to v1.9.1 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9714 [details] [review] Bug 7977: Adding jEditable minified version jEditable is necessary to create DataTables with editable fields Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9715 [details] [review] Bug 7977: Adding DateTime::Format::MySQL as a core dependency Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9716 [details] [review] Bug 7977: Quote-of-the-day (QOTD) Feature for OPAC This feature will add the option of displaying a select quote for the day on the OPAC homepage. It will include the addition of a QOTD editor in the tools section of the staff interface which will allow the addition, editing, and deletion of quotes. A single system preference will enable/disable the display of the QOTD on the OPAC homepage. A new granular permission will also be added to conrol user access to the QOTD editor tool. Possible extentions to this would be code to allow alternate quote selection algorithms to be added to vary how the currently displayed quote is selected from the table of quotes. This patch is a squash of the following work: --Adding quotes table to kohastructure.sql and updatedatabase.pl Note: This patch is intended for a MySQL based installation only. That means that it includes backticks because that is what is required at this point in history to install Koha correctly on a MySQL based installation. Feel free to port this over to the Pg stuff in the data/Pg directory. --Adding system preference to control display of QOTD on OPAC main page --Adding sample quote data These quotes are taken from various US presidents. I'm not sure these are applicable to the rest of the world, so I'm leaving it to translators and others to add sample quote data for other languages. --Adding edit_quotes user permissions --Squash with other db related patches in this series --Adding quotes editor pages to tools --Adding QOTD editor link to tools homepage --Integration of DataTables into QOTD Editor --Impleminting jEditable into the quote editor --Adding delete and add option to editor --Fixing OPAC display so the QOTD div does not show if no quote is returned --Also removing useless code from tools/quotes.pl --Adding spans around QOTD foo in opac-main.tt --Also fixing quote selection logic to accomodate the possibility of a single quote in the db a litte better. --Changing timestamp column header to a more user-friendly 'Last Displayed' --Fixing bug in quote selection logic. This bug caused a situation where when the table had only a single quote with an id greater than 1 in it, no quote would be selected. --Fixing up sorting in the quote editor table. --jcamins --Adding span element to quote separator --jcamins --Overriding the default empty table message supplied by datatables-strings.inc --Adds missing page heading --Adds beginning of click-for-help elements. --Refactors delete functionality to allow selecting of quotes to delete, enabling multi-delete. --Refactors saving added quote functionality so that striking <Enter> saves the new quote. --Refactors canceling aded quote functionality so that striking <Esc> cancels the new quote. --Removing debug console.log statements --Implementng the YUI button widget/toolbar used on other pages. --Adds a 5px radius to all YUI buttons to bring them into conformity with the general trend toward rounded corners --Fixes capitalization in quote editor --Implements improvements suggested by jcamins and oleonard --Adds DataTables Plugin dataTables.fnReloadAjax.js Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9717 [details] [review] Bug 7977: QOTD uploader to enable uploading csv files containing quotes This series will add a DataTable's based upload/editor with which to upload csv files containing quotes to be used by the QOTD feature. The file should be formatted thusly: "source","text-of-quote" "source","text-of-quote" ... Note: This work serves as a good example of potential improvements in all other "editor" and file upload areas of Koha. This patch is a squash of the following work: --Adding code to parse CSV file contents and push it into a DataTable --Adding in jEditable to enable table editing --Adding ajax to post data back to the server to be saved --Fixing edit and adding delete functionality --Adding some missing css as well as server feedback on save --Fixing a bug which limited the number of quotes which could be uploaded --Also fixing a minor bug with fnCSVToArray and doing some style cleanup. --Adding sanity checks to verify file type and size --Implements YUI button widget/toolbar --Improved handling of hiding uploader UI --Adds row selectability --Adds multi-delete capability --Adds YUI button/toolbar widget --Fixing capitalization in quote uploader --Implements improvments suggested by jcamins and oleonard Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9718 [details] [review] Bug 7977: Further UI work on the QOTD uploader This patch addresses another of oleonard's points as well as some general cleanup: --Adds instructions to each stage of the uploader --Consolidates quote tool css into quotes.css Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9719 [details] [review] Bug 7977: Further UI work on the QOTD editor This patch implements more of oleonard's suggestions as well as other improvments: --Adds instructions for using the QOTD editor --Moves inline css to quotes.css, etc. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9720 [details] [review] Bug 7977: Fixing captialization on the tools main page Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9721 [details] [review] Bug 7977: Fixing quote sample data timestamps Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9722 [details] [review] Bug 7977: Fixing quote sample data timestamps Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9723 [details] [review] Bug 7977: Fixing quote sample data timestamps Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9724 [details] [review] Bug 7977: Adding Daily Quotes Tests to Koha.t test suite; also fixing sort order on quote selection SQL Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9725 [details] [review] Bug 7977: Adding Koha standard help files for QOTD feature Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9726 [details] [review] Bug 7977: Followup patch addressing items pointed out by Jonathan Druart Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9727 [details] [review] Bug 7977: Removing unused code from quotes_ajax.pl Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9728 [details] [review] Bug 7977: Fixing a bug which results in quote fields being incorrectly offset when saving uploaded quotes Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9729 [details] [review] Bug 7977: Improving upload result message display Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9730 [details] [review] Making source a text field rather than a varchar to avoid potential length limitations http://bugs.koha-community.org/show_bug.cgi?id=7977 Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9731 [details] [review] Fixing a typo in the quotes-upload help file and hardcoded css http://bugs.koha-community.org/show_bug.cgi?id=7977 Signed-off-by: Mason James <mtj@kohaaloha.com>
Created attachment 9732 [details] [review] add edit_quotes to es-ES/mandatory/userpermissions.sql Signed-off-by: Mason James <mtj@kohaaloha.com>
(In reply to comment #66) > QA comments: > > I think these patchs can be passed QA. > > Given the length and the complexity of this development I prefer to need > additional QA. hi Jonathon i have tested the feature, and looked over the code i am happy to PASS-QA on this feature the new code passes OK with both 'prove xt' and 'perlcritic' tests great work Chris :)
(In reply to comment #104) > (In reply to comment #66) > > QA comments: > > > > I think these patchs can be passed QA. > > > > Given the length and the complexity of this development I prefer to need > > additional QA. > > > hi Jonathon ^^^^^ oups, sorry about the spelling Jonathan! passing QA on this applied cleanly to 804b073a8274d9f719ef746d74198ade735dc620
2nd QA comment: * this patch does not respect UC on 1st word only that is applied everywhere in Koha now. I've added a follow-up * passes xt/permissions.t (as there is a new permission) * adds a styling to manage the display of the QOTD, good point ! Other than this, works fine, patch pushed ! About the Adding-DateTime-Format-MySQL-as-a-core-dependency => could there be another interesting use of this package ?
(In reply to comment #106) > 2nd QA comment: > * this patch does not respect UC on 1st word only that is applied > everywhere in Koha now. I've added a follow-up Thanks for fixing the spots I missed that in during my followup for it. > About the Adding-DateTime-Format-MySQL-as-a-core-dependency => could there > be another interesting use of this package ? I'm sure that anywhere we need DateTime formatting in MySQL format this will be useful. It takes Koha::DateUtils one step closer to being phased out. :-)
(In reply to comment #107) > (In reply to comment #106) > > 2nd QA comment: > > * this patch does not respect UC on 1st word only that is applied > > everywhere in Koha now. I've added a follow-up > > Thanks for fixing the spots I missed that in during my followup for it. > > > About the Adding-DateTime-Format-MySQL-as-a-core-dependency => could there > > be another interesting use of this package ? > > I'm sure that anywhere we need DateTime formatting in MySQL format this will > be useful. It takes Koha::DateUtils one step closer to being phased out. :-) Can you drop a mail to koha-devel to explain what this dependency introduces, and how to use it ? (i'll also add it to my monthly RM newsletter)
This feature works great so I am closing it.