Created attachment 12762 [details] [review] Bug 7683: Cataloguing statistics wizard improvements - Add 3 options for cells value - Titles count "COUNT (biblionumber)" - Items count "COUNT (itemnumber)" (current behavior) - Deleted items count "COUNT (deleteditems.itemnumber)" - Add to selectable rows, colums and filters: - items.notforloan - items.materials - items.statisticvalue For each of these fields, retrieve the label in the default cataloging framework. If no MARC field is mapped to the field in question, it does not show this line in the wizard. If the MARC field is associated with a list of authorized values, it displays a dropdown list for the filter, otherwise it displays a text field. - Add to selectable rows, columns and filters: - items.dateaccessioned (creation date) - deleteditems.timestamp (deletion date) (Only if the third option (eg count deleted items) is selected) When I apply this patch, all the text on catalogue_stats.pl is shrunk/squished. Is this intended? Created attachment 12785 [details]
Before/After Screenshot
Hmm no it is not intended. The patch doesn't seem to contain a change that can cause that... I will take a look. Nothing change for me... are you sure the patch is the cause of this? The only thing I did was load master in one tab, and master + patch in another. I think if we can get one other person to verify, then we can assume it's a problem on my end and not with the patch. I didn't see anything in the code that would cause this, which made it even more peculiar. (In reply to comment #6) > I think if we can get one other person to verify, then we can > assume it's a problem on my end and not with the patch. I don't see the problem when I apply the patch. Created attachment 12940 [details] [review] Bug 7683: Cataloguing statistics wizard improvements - Add 3 options for cells value - Titles count "COUNT (biblionumber)" - Items count "COUNT (itemnumber)" (current behavior) - Deleted items count "COUNT (deleteditems.itemnumber)" - Add to selectable rows, colums and filters: - items.notforloan - items.materials - items.statisticvalue For each of these fields, retrieve the label in the default cataloging framework. If no MARC field is mapped to the field in question, it does not show this line in the wizard. If the MARC field is associated with a list of authorized values, it displays a dropdown list for the filter, otherwise it displays a text field. - Add to selectable rows, columns and filters: - items.dateaccessioned (creation date) - deleteditems.timestamp (deletion date) (Only if the third option (eg count deleted items) is selected) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Since the font anomaly appears to be specific to me, I will sign-off. Is this patch for 3.8 only? If not, please update the version listed. This is an enhancement, so this is for master. Version updated accordingly. QA comment :
* passes koha-qa.pl, except for a POD error I've fixed in a follow-up (the pod error was here before the patch)
* affect only catalogue_stats.pl, no side effect risk
* add a new sub in C4/Biblio.pm, with no unit test. This patch has been submitted a lot earlier than the "provide unit test" rule, so I think it's OK
* Testing =
- CGI mode = works before and after the patch (including new features)
- Plack = works before the patch
- after the patch, I get a nasty:
exited nonzero: 1 at /home/paul/koha.dev/koha-community/reports/catalogue_stats.pl line 629
failing QA for this reason. Once you've fixed plack compatibility, please switch back to "signed off", i'll QA asap
Created attachment 14399 [details] [review] Bug 7683 fix POD doc Created attachment 14466 [details] [review] Bug 7683: Fix plack compatibility Created attachment 16327 [details] [review] Bug 7683: Cataloguing statistics wizard improvements - Add 3 options for cells value - Titles count "COUNT (biblionumber)" - Items count "COUNT (itemnumber)" (current behavior) - Deleted items count "COUNT (deleteditems.itemnumber)" - Add to selectable rows, colums and filters: - items.notforloan - items.materials - items.statisticvalue For each of these fields, retrieve the label in the default cataloging framework. If no MARC field is mapped to the field in question, it does not show this line in the wizard. If the MARC field is associated with a list of authorized values, it displays a dropdown list for the filter, otherwise it displays a text field. - Add to selectable rows, columns and filters: - items.dateaccessioned (creation date) - deleteditems.timestamp (deletion date) (Only if the third option (eg count deleted items) is selected) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 16328 [details] [review] Bug 7683 fix POD doc Created attachment 16329 [details] [review] Bug 7683: Fix plack compatibility Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> QA comment: I'm sure Jared will fail QA because of GetMarcSubfieldStructureFromKohaField not having a unit test, so, please add one, and mark "passed QA" once you've added it. Created attachment 16367 [details] [review] Bug 7683: Add unit tests for GetMarcSubfieldStructureFromKohaField Unit tests added $ prove t/Biblio_GetMarcSubfieldStructureFromKohaField.t t/Biblio_GetMarcSubfieldStructureFromKohaField.t .. ok All tests successful. Files=1, Tests=24, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.22 cusr 0.00 csys = 0.24 CPU) Result: PASS Switching to Passed QA The unit tests for GetMarcSubfieldStructureFromKohaField should be added to t/db_dependent/Biblio.t rather than added as a separate file (and, since it requires the database, it should go in t/db_dependent/). However, that's not why I am failing this. After applying this patch, I get a stray "=1" in the "Filtered on" section of the results if I do not filter on anything: http://screencast.com/t/QQAyeuHRb Created attachment 16775 [details] [review] Bug 7683 [Follow-up] Avoid printing useless '=1' This was caused by 'like/not like' select which had no description in catalogue_stats.pl *** Bug 10608 has been marked as a duplicate of this bug. *** Created attachment 20241 [details] [review] Bug 7683: Cataloguing statistics wizard improvements - Add 3 options for cells value - Titles count "COUNT (biblionumber)" - Items count "COUNT (itemnumber)" (current behavior) - Deleted items count "COUNT (deleteditems.itemnumber)" - Add to selectable rows, colums and filters: - items.notforloan - items.materials - items.statisticvalue For each of these fields, retrieve the label in the default cataloging framework. If no MARC field is mapped to the field in question, it does not show this line in the wizard. If the MARC field is associated with a list of authorized values, it displays a dropdown list for the filter, otherwise it displays a text field. - Add to selectable rows, columns and filters: - items.dateaccessioned (creation date) - deleteditems.timestamp (deletion date) (Only if the third option (eg count deleted items) is selected) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 20242 [details] [review] Bug 7683 fix POD doc Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 20243 [details] [review] Bug 7683: Fix plack compatibility Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 20244 [details] [review] Bug 7683: Add unit tests for GetMarcSubfieldStructureFromKohaField Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 20245 [details] [review] Bug 7683 [Follow-up] Avoid printing useless '=1' This was caused by 'like/not like' select which had no description in catalogue_stats.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org> Doing a code review of the squashed patches I noticed some things: 1) There are multiple references to items.statisticvalue, but I think we don't have this column in our items table. Is there a dependency to another development? 2) The term 'Total' appears a few times in catalogue_stats.pl but is not translatable (problem was there before). Also those: + ( $i == 0 ) ? "Dewey Classification From" + : ( $i == 1 ) ? "Dewey Classification To" + : ( $i == 2 ) ? "Lccn Classification From" + : ( $i == 3 ) ? "Lccn Classification To" + : ( $i == 4 ) ? "Item CallNumber From" + : ( $i == 5 ) ? "Item CallNumber To" + : ( $i == 6 ) ? "Item type" + : ( $i == 7 ) ? "Publisher" + : ( $i == 8 ) ? "Publication year From" + : ( $i == 9 ) ? "Publication year To" + : ( $i == 10 ) ? "Library" + : ( $i == 11 ) ? "Shelving Location" + : ( $i == 12 ) ? "Collection Code" + : ( $i == 13 ) ? "Status" 3) Same file: DHTMLcalendar_dateformat => C4::Dates->DHTMLcalendar(), I think we are no longer using this construct. 4) Looking at the code I think the plugin for authorised values could be useful here. 5) Could you please move the tests as suggested by Jared on comment 20? Failed QA for 1) (needs clarification) and 5). Created attachment 21357 [details] [review] Bug 7683 [Follow-up] Move unit tests in t/db_dependent/Biblio.t Created attachment 21358 [details] [review] Bug 7683: Remove use of items.statisticvalue and DHTMLcalendar() Fixed 1, 3 and 5. Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff I tested with itemtypes and branches, with no filters Here are my results: Filtered on: =1 items.itype / items.homebranch BU bubis TOTAL CONC 0 DPC 0 DPN 0 TOTAL 0 0 0 I don't understand the "=1" And I have much more than "0" items of each category. So, failing QA for these 2 points. Mathieu Created attachment 23683 [details] [review] Bug 7683: Finish the removal of items.statisticvalue Some pieces of code was still referring to items.statisticvalue. Also fix indexes of @filters array Created attachment 23684 [details] [review] Bug 7683: Cataloguing statistics wizard improvements Rebased on master Created attachment 23685 [details] [review] Bug 7683 fix POD doc Rebased on master Created attachment 23686 [details] [review] Bug 7683: Fix plack compatibility Rebased on master Created attachment 23687 [details] [review] Bug 7683: Add unit tests for GetMarcSubfieldStructureFromKohaField Rebased on master Created attachment 23688 [details] [review] Bug 7683 [Follow-up] Avoid printing useless '=1' Rebased on master Created attachment 23689 [details] [review] Bug 7683 [Follow-up] Move unit tests in t/db_dependent/Biblio.t Rebased on master Created attachment 23690 [details] [review] Bug 7683: Remove use of items.statisticvalue and DHTMLcalendar() Rebased on master Created attachment 23691 [details] [review] Bug 7683: Finish the removal of items.statisticvalue Rebased on master I tested on a sandbox (langres) by comparing the results with sql reports. The acquired date filter doesn't work. It seems like the end of the limit is not used. --> SQL report SELECT homebranch, itemtype, COUNT(itemnumber) FROM items JOIN biblioitems USING(biblionumber) WHERE dateaccessioned BETWEEN "2013-01-01" AND "2013-12-31" GROUP BY homebranch, itemtype --> résultat MAURES LIVR 1 MAURES REV 147 --> using statistics wizards Row = itemtype Column = home library Acquired Date Filter From 01/01/2013 To 31/12/2013 --> résultats itemtype / items.homebranch BDP MAURES TOTAL CD 380 380 LAUD 6 6 LIVR 20 3783 3803 REV 862 862 SPECIAL 2 2 VHS 3 3 TOTAL 20 5036 5056 I tried the filter with "From 01/01/2012 To 31/12/2012", "From 01/01/2013 To 31/12/2013", "From 01/01/2014 To 31/01/2014". Each time the total is growing as if only the start of the limit is used. For that reason, I change status in "Failed QA". Created attachment 25072 [details] [review] Bug 7683: Remove dead code about dewey and lccn filters $hasdewey and $haslccn were hardcoded to 0 Created attachment 25073 [details] [review] Bug 7683: Fix a shift in date filter indexes Date filter indexes are 13, 14, 15, 16, not 14, 15, 16, 17 The last patch should fix issue of comment 50 Patch tested with a sandbox, by Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25091 [details] [review] Bug 11579: Show ordernumber in basket.pl tables Test plan: Verify the ordernumber is correctly displayed on the basket page (acqui/basket.pl?basketno=X) Signed-off-by: silvere <silvere.hanguehard@enc.sorbonne.fr> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed a tiny typo in the HTML: </th> > </td>. Passes all tests and QA script. Tested that the ordernumber is now displayed and table sorting works for the smaller and more detailed table. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25092 [details] [review] Bug 11439: (follow-up) adjust for change in list returned by OCLC XISBN The set of related ISBNs for the first Harry Potter book returned by OCLC's XISBN service has changed recently, so this patch adjusts the test to match. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25093 [details] [review] Bug 9823: Refactor return from GetReservesFromBiblionumber The return from GetReservesFromBiblionumber contains an unnecessary extra variable. In scalar context an array returns its element count. Maintaining a separate count can lead to unforeseen bugs and imposes ugly constructions on the subroutine's users. Remove the useless count variable from the return This patch also changes the parameters: now the routine takes a hashref. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Placed biblio holds, future holds and item holds. Works as expected. Tested Holds.t and Reserves.t. Pass. Tested /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=999 with two holds on one item. Fine. C4/SIP/ILS/Item.pm: Looked for "whatever" and "arrayref" and could not find them anymore. Looks good. Handled a few unneeded calls in QA follow-up. Left only one point to-do for serials/routing-preview.pl. See Bugzilla. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25094 [details] [review] Bug 9823: QA follow-up for GetReservesFromBiblionumber calls The template params holds and holdcount are not used in the ISBD template. Removed the associated code from catalogue/ISBDdetail.pl. Same applies for catalogue/MARCdetail.pl and labeledMARCdetail.pl. Same applies also for catalogue/imageviewer.pl. Same applies also for catalogue/moredetail.pl. In catalogue detail.tt only the number of holds is used. Removed the code that passed the holds array to the template. For consistency opac-detail should also incorporate future holds into its holds count; added the all_dates parameter for that reason. The Reserves module is no longer needed in moredetail. Removed it. Checked the other scripts also. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25095 [details] [review] Bug 9823: (follow-up) improve POD for C4::Reserves::GetReservesFromBiblionumber Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25096 [details] [review] Bug 7679: circulation statistics wizard improvements - Add patron branch to the list of possible rows, columns, and filters - Add extended patron attributes to the list of possible rows, columns, and filters - Allow to group period by hour - Allow to translate filter names Signed-off-by: Brendan <brendan@bywatersolutions.com> Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25097 [details] [review] Bug 7679: Followup: circulation statistics wizard improvements Signed-off-by: Brendan <brendan@bywatersolutions.com> Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25098 [details] [review] Bug 7679: Fix criteria display Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25099 [details] [review] Bug 7679: Group different case values in the same row/column Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Created attachment 25100 [details] [review] Bug 7679: Fix several bugs in circulation statistics wizard - Display values in NULL columns/rows - Display columns/rows that contain values only in NULL rows/columns - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> (In reply to Julian Maurice from comment #53) > The last patch should fix issue of comment 50 The acquired date filter is ok with this patch. I find the same results using SQL request and statistics wizards. I think by accident some patches got on this bug report, that don't belong here - tidying up a bit. ... now it looks a lot less scary already. The QA script complains about a few small things: FAIL reports/issues_stats.pl FAIL pod *** ERROR: =over on line 45 without closing =back at line EOF in file reports/issues_stats.pl FAIL forbidden patterns forbidden pattern: tab char (line 628) OK valid OK critic FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/issues_stats.tt FAIL forbidden patterns forbidden pattern: tab char (line 75) OK tt_valid OK valid_template Please provide a follow up for the POD and include the tab fixes. Also - are there patches missing? I can't find the new tests in t/db_dependent/Biblio.t the comments talk about. and some of the other changes mentioned. Julian, could you please check the patches are all there and complete for testing? Please also point me the correct test plan I should be using, thx. (In reply to Katrin Fischer from comment #66) > I think by accident some patches got on this bug report, that don't belong > here - tidying up a bit. In fact, all patches from sandboxes@biblibre.com were sent by error on this bug (they are about issues_stats.pl, but this bug is about catalogue_stats.pl) I will resubmit patches Created attachment 26678 [details] [review] Bug 7683: Cataloguing statistics wizard improvements - Add 3 options for cells value - Titles count "COUNT (biblionumber)" - Items count "COUNT (itemnumber)" (current behavior) - Deleted items count "COUNT (deleteditems.itemnumber)" - Add to selectable rows, colums and filters: - items.notforloan - items.materials - items.statisticvalue For each of these fields, retrieve the label in the default cataloging framework. If no MARC field is mapped to the field in question, it does not show this line in the wizard. If the MARC field is associated with a list of authorized values, it displays a dropdown list for the filter, otherwise it displays a text field. - Add to selectable rows, columns and filters: - items.dateaccessioned (creation date) - deleteditems.timestamp (deletion date) (Only if the third option (eg count deleted items) is selected) - Add unit tests for GetMarcSubfieldStructureFromKohaField Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 26679 [details] [review] Bug 7683 [Follow-up] Move unit tests in t/db_dependent/Biblio.t Created attachment 26680 [details] [review] Bug 7683: Remove use of items.statisticvalue and DHTMLcalendar() Also fix indexes of @filters array Created attachment 26681 [details] [review] Bug 7683: Remove dead code about dewey and lccn filters $hasdewey and $haslccn were hardcoded to 0 Created attachment 26682 [details] [review] Bug 7683: Fix a shift in date filter indexes Date filter indexes are 13, 14, 15, 16, not 14, 15, 16, 17 I squashed some patches (the first is a squash of all signed-off patches) but everything is here :) The sandbox you've requested is not ready. Some problems occurred applying patches from bug 7683: <h1>Something went wrong !</h1>Applying: Bug 7683: Cataloguing statistics wizard improvements Auto packing the repository for optimum performance. You may also run git gc manually. See git help gc for more information. Applying: Bug 7683 [Follow-up] Move unit tests in t/db_dependent/Biblio.t Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Removing t/Biblio_GetMarcSubfieldStructureFromKohaField.t Auto-merging t/db_dependent/Biblio.t CONFLICT (content): Merge conflict in t/db_dependent/Biblio.t Failed to merge in the changes. Patch failed at 0001 Bug 7683 [Follow-up] Move unit tests in t/db_dependent/Biblio.t When you have resolved this problem run git bz apply --continue. If you would prefer to skip this patch, instead run git bz apply --skip. To restore the original branch and stop patching run git bz apply --abort. Bug 7683 - statistic wizard: cataloging 26678 - Bug 7683: Cataloguing statistics wizard improvements 26679 - Bug 7683 [Follow-up] Move unit tests in t/db_dependent/Biblio.t 26680 - Bug 7683: Remove use of items.statisticvalue and DHTMLcalendar() 26681 - Bug 7683: Remove dead code about dewey and lccn filters 26682 - Bug 7683: Fix a shift in date filter indexes Apply? [(y)es, (n)o, (i)nteractive] Patch left in tmpBug-7683-Follow-up-Move-unit-tests-in-tdbdependent-kzhmvJ.patch . Created attachment 28466 [details] [review] Bug 7683: Cataloguing statistics wizard improvements Rebased on master Created attachment 28467 [details] [review] Bug 7683 [Follow-up] Move unit tests in t/db_dependent/Biblio.t Rebased on master Created attachment 28468 [details] [review] Bug 7683: Remove use of items.statisticvalue and DHTMLcalendar() Rebased on master Created attachment 28469 [details] [review] Bug 7683: Remove dead code about dewey and lccn filters Rebased on master Created attachment 28470 [details] [review] Bug 7683: Fix a shift in date filter indexes Rebased on master Created attachment 36489 [details] [review] Bug 7683: Cataloguing statistics wizard improvements Rebased on master Created attachment 36490 [details] [review] Bug 7683 [Follow-up] Move unit tests in t/db_dependent/Biblio.t Rebased on master Created attachment 36491 [details] [review] Bug 7683: Remove use of items.statisticvalue and DHTMLcalendar() Rebased on master Created attachment 36492 [details] [review] Bug 7683: Remove dead code about dewey and lccn filters Rebased on master Created attachment 36493 [details] [review] Bug 7683: Fix a shift in date filter indexes Rebased on master Created attachment 39129 [details] [review] [SIGNED-OFF] Bug 7683: Cataloguing statistics wizard improvements - Add 3 options for cells value - Titles count "COUNT (biblionumber)" - Items count "COUNT (itemnumber)" (current behavior) - Deleted items count "COUNT (deleteditems.itemnumber)" - Add to selectable rows, colums and filters: - items.notforloan - items.materials - items.statisticvalue For each of these fields, retrieve the label in the default cataloging framework. If no MARC field is mapped to the field in question, it does not show this line in the wizard. If the MARC field is associated with a list of authorized values, it displays a dropdown list for the filter, otherwise it displays a text field. - Add to selectable rows, columns and filters: - items.dateaccessioned (creation date) - deleteditems.timestamp (deletion date) (Only if the third option (eg count deleted items) is selected) - Add unit tests for GetMarcSubfieldStructureFromKohaField Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39130 [details] [review] [SIGNED-OFF] Bug 7683: [Follow-up] Move unit tests in t/db_dependent/Biblio.t Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39131 [details] [review] [SIGNED-OFF] Bug 7683: Remove use of items.statisticvalue and DHTMLcalendar() Also fix indexes of @filters array Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39132 [details] [review] [SIGNED-OFF] Bug 7683: Remove dead code about dewey and lccn filters $hasdewey and $haslccn were hardcoded to 0 Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 39133 [details] [review] [SIGNED-OFF] Bug 7683: Fix a shift in date filter indexes Date filter indexes are 13, 14, 15, 16, not 14, 15, 16, 17 Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works, no koha-qa errors (some message for the deleted file) Note: 1/ Date range should not permit to fill invalid range (from > to) 2/ Other date ranges don't have the datetime plugin (not introduced by this patch). 3/ Labels consistent "From:" vs "From" 4/ Mime-type for CSV is "text/csv", not "application/vnd.sun.xml.calc" (existed before this patch) 5/ Don't use C4::Dates, prefer Koha::DateUtils 6/ Use placeholders when you build the sql query (+ $strcalc .= " AND biblioitems.publishercode LIKE \"" . @$filters[3] . "\"";) Failing QA for 3, 4, 5 and 6. Created attachment 41821 [details] [review] Bug 7683: Catalog statistics wizard: QA fixes 1/ Do not allow invalid date ranges (from > to) (datepicker only) 2/ Relabel "From:" to "From" for consistency 3/ Fix MIME type for CSV 4/ Use Koha::DateUtils instead of C4::Dates 5/ Use placeholders in SQL query (In reply to Jonathan Druart from comment #91) > 2/ Other date ranges don't have the datetime plugin (not introduced by this > patch). The only other date range should be filled with year only, so there is no need for datepicker. Created attachment 41829 [details] [review] Bug 7683: Catalog statistics wizard: QA fixes 1/ Do not allow invalid date ranges (from > to) (datepicker only) 2/ Relabel "From:" to "From" for consistency 3/ Fix MIME type for CSV 4/ Use Koha::DateUtils instead of C4::Dates 5/ Use placeholders in SQL query 1) Hm, something strange, I see <td>Removed date</td> in the code, but it doesn't show up on the form for me? I was looking for it, as I wanted to suggest a change: Date deleted (item). 2) Similarly, I would use "Date acquired (item)" for "Acquired date". Because someone might question if this is about the record or the item and it's what the field in the item is labelled. 3) Can you please update and post the initial commit message? For example it still lists a items.statisticvalue that doesn't exist. So a bit hard to see what actually happens now as this has been going back and forth a bit. Maybe we should also upate the actual commit message? X) the ... characters in the second line seems to belong to a lccn related feature and has no label at all :( But this is an old problem, so out of scope here. Can you take a look please? Created attachment 44651 [details] [review] Bug 7683: Cataloguing statistics wizard improvements Squash of 3 first patches, rebased on master Created attachment 44652 [details] [review] Bug 7683: Remove dead code about dewey and lccn filters Rebased on master Created attachment 44653 [details] [review] Bug 7683: Fix a shift in date filter indexes Rebased on master Created attachment 44654 [details] [review] Bug 7683: Catalog statistics wizard: QA fixes Rebased on master Created attachment 44655 [details] [review] Bug 7683: Relabel "acquired date" and "removed date" (In reply to Katrin Fischer from comment #95) > 1) Hm, something strange, I see > <td>Removed date</td> in the code, but it doesn't show up on the form for > me? > I was looking for it, as I wanted to suggest a change: Date deleted (item). It shows up only when you select "Count deleted items" for "Cell value" "Removed date" changed by the last patch > > 2) Similarly, I would use "Date acquired (item)" for "Acquired date". > Because someone might question if this is about the record or the item and > it's what the field in the item is labelled. "Acquired date" changed by the last patch > > 3) Can you please update and post the initial commit message? For example it > still lists a items.statisticvalue that doesn't exist. So a bit hard to see > what actually happens now as this has been going back and forth a bit. Maybe > we should also upate the actual commit message? I squashed the 3 first commits and removed reference to items.statisticvalue in the initial commit message. Is that ok ? > > X) the ... characters in the second line seems to belong to a lccn related > feature and has no label at all :( But this is an old problem, so out of > scope here. I don't see what you are talking about, lccn related feature should be gone thanks to patch "Remove dead code about dewey and lccn filters" Where are the "..." characters ? I meant the second line in the report - a pull down labelled characters in hte second column. I have no idea what it does - it's referred to as cotedigits(?) in the HTML. Working on this again now... Created attachment 45238 [details] [review] [PASSED QA] Bug 7683: Cataloguing statistics wizard improvements - Add 3 options for cells value - Titles count "COUNT (biblionumber)" - Items count "COUNT (itemnumber)" (current behavior) - Deleted items count "COUNT (deleteditems.itemnumber)" - Add to selectable rows, colums and filters: - items.notforloan - items.materials For each of these fields, retrieve the label in the default cataloging framework. If no MARC field is mapped to the field in question, it does not show this line in the wizard. If the MARC field is associated with a list of authorized values, it displays a dropdown list for the filter, otherwise it displays a text field. - Add to selectable rows, columns and filters: - items.dateaccessioned (creation date) - deleteditems.timestamp (deletion date) (Only if the third option (eg count deleted items) is selected) - Add unit tests for GetMarcSubfieldStructureFromKohaField Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 45239 [details] [review] [PASSED QA] Bug 7683: Remove dead code about dewey and lccn filters $hasdewey and $haslccn were hardcoded to 0 Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 45240 [details] [review] [PASSED QA] Bug 7683: Fix a shift in date filter indexes Date filter indexes are 13, 14, 15, 16, not 14, 15, 16, 17 Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works, no koha-qa errors (some message for the deleted file) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 45241 [details] [review] [PASSED QA] Bug 7683: Catalog statistics wizard: QA fixes 1/ Do not allow invalid date ranges (from > to) (datepicker only) 2/ Relabel "From:" to "From" for consistency 3/ Fix MIME type for CSV 4/ Use Koha::DateUtils instead of C4::Dates 5/ Use placeholders in SQL query Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 45242 [details] [review] [PASSED QA] Bug 7683: Relabel "acquired date" and "removed date" Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to Master - Should be in the May 2016 release. Thanks! |
In the cataloguing wizard, add the following lines that can be used as row/column/filter: * status * item creation date (range with dynamic calendar) * item suppresion date (range with dynamic calendar) * origin : items.materials - authorized value list "origin" 995$s * Domaine : 995$t items.statisticvalue "domain" av list Adds options for distinct biblionumber count on title and items. Add 3 options (radio buttons) - Titles count "COUNT (biblionumber)" - Counting of copies "COUNT (ItemNumber)" (current behavior) - Counting of copies deleted "COUNT (deleteditems.itemnumber)" - Add to selected row or column + filter combo: - items.notforloan - items.materials - items.statisticvalue For each of these fields, retrieve the label and the code value allowed in the grid cataloging default. If no MARC field is mapped to the field in question, it does not show this line in the wizard. If the MARC field is associated with a legal value, it displays a dropdown list for the filter, if a text field. - Add to selected row or column + filter on a date range - Items.dateaccessioned (create Date) - Deleteditems.timestamp (date deleted) (Only if the third option (eg count. Del.) Is selected)