We will add needed tables & help to be able to build a guided report for subscriptions & serials
I'm wondering if a little more detail might be good here. For example: 1. What sort of tables need to be added? 2. What is the proposed structure of these tables? 3. What modules/scripts need to be touched upon or added?
Experience has shown that the main obstacle to setting up comprehendable reports on serials is the fact that the serial schema is not that well thought out and the interface to it is not very clear. There is a definite requirement for some refactoring here.
Created attachment 11601 [details] [review] Bug 5338: Serials Report Blocked by Bug 7688
Created attachment 22908 [details] [review] Bug 5338: Serials Report
Test plan: Build some guided reports for subscriptions and serials.
(In reply to Jonathan Druart from comment #5) > Test plan: > Build some guided reports for subscriptions and serials. Hi Could you make a more detailed plan please? There is already a guided report for serials (serials_stats.pl). So, I would like to know exactly the changes made by this patch. Mathieu
(In reply to mathieu saby from comment #6) > Could you make a more detailed plan please? No, there are many ways to test serial reports, so I don't think I should let a detailed test plan. If you want to test it, try to generate reports you would like to have for your library. > There is already a guided report for serials (serials_stats.pl). So, I would > like to know exactly the changes made by this patch. The existing one only generates one table with hard coded column.
Mmm I realized I was confusing guided reports and statistic wizard!
Test made In Guided Reports, I selected the new 'Serial' value in modules list then I selected all the fields of the following tables: serial, serialitems, subscription, subscriptionhistory, subscriptionroutinglist I did not choose sorting, grouping or counting option The resulting report is: SELECT serial.serialid,serial.biblionumber,serial.subscriptionid,serial.serialseq,serial.status,serial.planneddate,serial.notes,serial.publisheddate,serial.claimdate,serial.routingnotes, serialitems.itemnumber,serialitems.serialid, subscription.biblionumber,subscription.subscriptionid,subscription.librarian,subscription.startdate,subscription.aqbooksellerid,subscription.cost,subscription.aqbudgetid,subscription.weeklength,subscription.monthlength,subscription.numberlength,subscription.periodicity,subscription.countissuesperunit,subscription.notes,subscription.status,subscription.lastvalue1,subscription.lastvalue2,subscription.innerloop1,subscription.innerloop2,subscription.innerloop3,subscription.lastvalue3,subscription.firstacquidate,subscription.manualhistory,subscription.irregularity,subscription.skip_serialseq,subscription.letter,subscription.numberpattern,subscription.locale,subscription.distributedto,subscription.internalnotes,subscription.callnumber,subscription.location,subscription.branchcode,subscription.lastbranch,subscription.serialsadditems,subscription.staffdisplaycount,subscription.opacdisplaycount,subscription.graceperiod,subscription.enddate,subscription.closed,subscription.reneweddate,subscriptionhistory.biblionumber, subscriptionhistory.subscriptionid,subscriptionhistory.histstartdate,subscriptionhistory.histenddate,subscriptionhistory.missinglist,subscriptionhistory.recievedlist,subscriptionhistory.opacnote,subscriptionhistory.librariannote, subscriptionroutinglist.routingid,subscriptionroutinglist.borrowernumber,subscriptionroutinglist.ranking,subscriptionroutinglist.subscriptionid FROM biblio LEFT JOIN biblioitems on (biblio.biblionumber=biblioitems.biblionumber) LEFT JOIN serial on (biblioitems.biblionumber=serial.biblionumber) LEFT JOIN serialitems on (serial.serialid=serialitems.serialid) LEFT JOIN subscription on (serial.subscriptionid=subscription.subscriptionid) LEFT JOIN subscriptionhistory on (serial.subscriptionid=subscriptionhistory.subscriptionid) LEFT JOIN subscriptionroutinglist on (serial.subscriptionid=subscriptionroutinglist.subscriptionid) LEFT JOIN aqbooksellers on (subscription.aqbooksellerid=aqbooksellers.id) Koha accepted to run it and get 1311 results I only have 35 serials in my db. In fact the reports gets 1 line for each of my records :( So I fail QA.
Created attachment 23299 [details] [review] Bug 5338: Serials Report
Mathieu, the table order was wrong. The results look better now.
(In reply to Jonathan Druart from comment #11) > Mathieu, the table order was wrong. The results look better now. Thank you. You see, it was not so trivial to test ;-)
perl -c C4/Reports/Guided.pm C4/Reports/Guided.pm syntax OK apply patch perl -c C4/Reports/Guided.pm when is experimental at C4/Reports/Guided.pm line 341. when is experimental at C4/Reports/Guided.pm line 350. when is experimental at C4/Reports/Guided.pm line 357. when is experimental at C4/Reports/Guided.pm line 1032. C4/Reports/Guided.pm syntax OK Given we are trying to remove given/when from our code (as well as smartmatch) I am failing this patch because it introduces new instances of them
Created attachment 29271 [details] [review] Bug 5338: Serials Report
I've applied the patch against master 3.19.00.005 Applying: Bug 5338: Serials Report Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/columns.def Auto-merging C4/Reports/Guided.pm CONFLICT (content): Merge conflict in C4/Reports/Guided.pm Failed to merge in the changes. Patch failed at 0001 Bug 5338: Serials Report I pass the patch to "Patch doesn't apply" status.
Created attachment 35148 [details] [review] Bug 5338: Serials Report
Created attachment 35149 [details] [review] Bug 5338: Serials Report
Thanks, Jonathan. I've applied the patch against master 3.19.00.005 Then, on Home › Reports › Guided reports wizard › , I've tried to create a "New report" for serials: a "New guided report". 1) Before saving the new report, in the "Save your custom report" form, "Serials" was not in the "Report group" drop down list. So I could not select the "Serials" Report group [I had to create it as a new report-group]. I think that this can be an error (a lack) because the "Serials" option was available in the modules' drop down list on the "1 of 6" step. And because all the other 5 modules names (+ "none") were listed in the "Report group" drop down list. So I pass the patch to "Failed QA" status. NB1. I've selected a periodicity. In the report I saw the right periodicity, but there was the code (id 7), not the description ('1/month'). Was it right? NB2. I've chosen to create a "public" report, but the new report was not created as "public" (ERROR), as I've seen afterwards in the "Public" column of the "Saved reports" table: Home › Reports › Guided reports wizard › Saved reports On the contrary (RIGHT), I could change the report to public on editing it. [I can't recognize if this error is related to this bug or not.]
Created attachment 35179 [details] [review] Bug 5338: Serials Report
Created attachment 35180 [details] [review] Bug 5338: Add "Serials" to the report group list
(In reply to Paola Rossi from comment #18) > Thanks, Jonathan. > 1) Before saving the new report, in the "Save your custom report" form, > "Serials" was not in the "Report group" drop down list. > So I could not select the "Serials" Report group [I had to create it as > a new report-group]. The last patch adds a new "Serials" group to the list. > NB1. I've selected a periodicity. In the report I saw the right periodicity, > but there was the code (id 7), not the description ('1/month'). Was it right? I don't plan to do that on this bug report. > NB2. I've chosen to create a "public" report, but the new report was not > created as "public" (ERROR), as I've seen afterwards in the "Public" column > of > the "Saved reports" table: > Home › Reports › Guided reports wizard › Saved reports > > On the contrary (RIGHT), I could change the report to public on editing it. > > [I can't recognize if this error is related to this bug or not.] It's not, I reproduce the problem on master, I will try to submit a patch on another bug report.
(In reply to Jonathan Druart from comment #21) > It's not, I reproduce the problem on master, I will try to submit a patch on > another bug report. See bug 13578 for a patch.
(In reply to Jonathan Druart from comment #21) > (In reply to Paola Rossi from comment #18) > > Thanks, Jonathan. > > > 1) Before saving the new report, in the "Save your custom report" form, > > "Serials" was not in the "Report group" drop down list. > > So I could not select the "Serials" Report group [I had to create it as > > a new report-group]. > > The last patch adds a new "Serials" group to the list. > I've applied the patch against master 3.19.00.006 Thanks, Jonathan: now the "Report group" list is OK! Now on the 2-nd step: Step 2 of 6: Pick a report type 3 values were listed [Tabular,Summary,Matrix], but only the first one was available. So I pass the patch to "Failed QA" status. NB. About the Mathieu's problem in Comment 9, now the report got 1 line for each serial's record in the DB (RIGHT imo).
(In reply to Paola Rossi from comment #23) > Now on the 2-nd step: > Step 2 of 6: Pick a report type > > 3 values were listed [Tabular,Summary,Matrix], but only the first one was > available. Hello Paola, Yes, it's expected. It's the same for all reports you will create. If you look at the code, the other options are disabled: 460 <select id="types" name="types"> 461 <option value="1">Tabular</option> 462 <option value="2" disabled="disabled">Summary</option> 463 <option value="3" disabled="disabled">Matrix</option> 464 </select>
Created attachment 35300 [details] [review] [SIGNED OFF] Bug 5338: Serials Report Thanks, Jonathan! So I pass the patch to "Signed Off" status.
Created attachment 35301 [details] [review] [SIGNED OFF] Bug 5338: Add "Serials" to the report group list
Starting to work on this today, I'd like to suggest a change: - Remove the database update and don't force the new report group into existing installatoins. My reasoning is that when they have serials reports, they will already have created the REPORT_GROUPS to match their needs and it might be annoying or even create a duplicate warning if they have used SER as well. And adding a report group is optional on the saving step. - Add the new REPORT_GROUP values to the sample data for new installations instead. Otherwise this works pretty well and I am almost ready to pass QA :) Setting to "FQA" to gt Jonathan's attention.
Created attachment 36457 [details] [review] Bug 5338: Add "Serials" to the report group list Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 36458 [details] [review] Bug 5338: Use insert ignore to avoid a warning if 'SER' already exists
Created attachment 36459 [details] [review] Bug 5338: Add the "SER" report group for all languages.
(In reply to Katrin Fischer from comment #27) > - Remove the database update and don't force the new report group into > existing installatoins. My reasoning is that when they have serials reports, > they will already have created the REPORT_GROUPS to match their needs and it > might be annoying or even create a duplicate warning if they have used SER > as well. And adding a report group is optional on the saving step. Compromise found using insert ignore :)
Created attachment 36621 [details] [review] [PASSED QA] Bug 5338: Serials Report Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 36622 [details] [review] [PASSED QA] Bug 5338: Add "Serials" to the report group list Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 36623 [details] [review] [PASSED QA] Bug 5338: Use insert ignore to avoid a warning if 'SER' already exists Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 36624 [details] [review] [PASSED QA] Bug 5338: Add the "SER" report group for all languages. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Patches pushed to master. Thanks Jonathan!
Created attachment 39302 [details] [review] Bug 5338: (QA followup) update the tests for the new reports This patch fixes the unit tests related to guided reports. It is needed now that there are more options available. It also fixes a wrong 'savedsql' key (should be 'sql') that caused the tests to print warnings. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>