Bug 5338 - Add serials module to guided reports
Summary: Add serials module to guided reports
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Deadline: 2011-05-01
Assignee: Jonathan Druart
QA Contact: Bugs List
URL: http://wiki.koha-community.org/wiki/G...
Whiteboard:
Keywords:
Depends on: 7688
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-02 02:38 UTC by Paul Poulain
Modified: 2015-12-03 22:11 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 5338: Serials Report (10.37 KB, patch)
2012-08-14 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Serials Report (9.67 KB, patch)
2013-11-13 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Serials Report (9.67 KB, patch)
2013-12-04 14:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Serials Report (5.99 KB, patch)
2014-06-26 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Serials Report (6.34 KB, patch)
2015-01-13 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Serials Report (6.30 KB, patch)
2015-01-13 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Serials Report (6.31 KB, patch)
2015-01-14 09:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Add "Serials" to the report group list (1.00 KB, patch)
2015-01-14 09:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 5338: Serials Report (6.53 KB, patch)
2015-01-15 11:13 UTC, Paola Rossi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 5338: Add "Serials" to the report group list (1.10 KB, patch)
2015-01-15 11:14 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 5338: Add "Serials" to the report group list (1.05 KB, patch)
2015-03-04 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Use insert ignore to avoid a warning if 'SER' already exists (1.14 KB, patch)
2015-03-04 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5338: Add the "SER" report group for all languages. (6.28 KB, patch)
2015-03-04 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 5338: Serials Report (6.43 KB, patch)
2015-03-05 16:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5338: Add "Serials" to the report group list (1.12 KB, patch)
2015-03-05 16:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5338: Use insert ignore to avoid a warning if 'SER' already exists (1.20 KB, patch)
2015-03-05 16:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5338: Add the "SER" report group for all languages. (6.35 KB, patch)
2015-03-05 16:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5338: (QA followup) update the tests for the new reports (2.28 KB, patch)
2015-05-19 16:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2010-11-02 02:38:33 UTC
We will add needed tables & help to be able to build a guided report for subscriptions & serials
Comment 1 Chris Nighswonger 2010-11-10 20:21:10 UTC
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?
Comment 2 Colin Campbell 2010-11-11 15:31:42 UTC
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.
Comment 3 Jonathan Druart 2012-08-14 10:36:12 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-11-13 14:38:09 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2013-11-13 14:39:43 UTC
Test plan:
Build some guided reports for subscriptions and serials.
Comment 6 mathieu saby 2013-11-17 13:58:20 UTC
(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
Comment 7 Jonathan Druart 2013-11-18 10:55:49 UTC
(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.
Comment 8 mathieu saby 2013-11-18 12:41:15 UTC
Mmm I realized I was confusing guided reports and statistic wizard!
Comment 9 mathieu saby 2013-11-29 18:21:35 UTC
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.
Comment 10 Jonathan Druart 2013-12-04 14:59:09 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-12-04 14:59:59 UTC
Mathieu, the table order was wrong. The results look better now.
Comment 12 mathieu saby 2013-12-04 15:44:58 UTC
(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 ;-)
Comment 13 Chris Cormack 2014-06-26 04:28:40 UTC
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
Comment 14 Jonathan Druart 2014-06-26 07:37:49 UTC Comment hidden (obsolete)
Comment 15 Paola Rossi 2015-01-13 10:29:16 UTC
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.
Comment 16 Jonathan Druart 2015-01-13 10:58:22 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-01-13 11:00:03 UTC Comment hidden (obsolete)
Comment 18 Paola Rossi 2015-01-13 14:34:53 UTC
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.]
Comment 19 Jonathan Druart 2015-01-14 09:59:11 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-01-14 09:59:16 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2015-01-14 10:00:58 UTC
(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.
Comment 22 Jonathan Druart 2015-01-14 11:12:36 UTC
(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.
Comment 23 Paola Rossi 2015-01-14 14:44:14 UTC
(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).
Comment 24 Jonathan Druart 2015-01-15 10:58:52 UTC
(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>
Comment 25 Paola Rossi 2015-01-15 11:13:44 UTC Comment hidden (obsolete)
Comment 26 Paola Rossi 2015-01-15 11:14:30 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2015-03-03 16:20:05 UTC
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.
Comment 28 Jonathan Druart 2015-03-04 11:28:25 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2015-03-04 11:28:40 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-03-04 11:28:48 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2015-03-04 11:29:28 UTC
(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 :)
Comment 32 Katrin Fischer 2015-03-05 16:26:54 UTC
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>
Comment 33 Katrin Fischer 2015-03-05 16:26:59 UTC
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>
Comment 34 Katrin Fischer 2015-03-05 16:27:03 UTC
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>
Comment 35 Katrin Fischer 2015-03-05 16:27:07 UTC
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>
Comment 36 Tomás Cohen Arazi 2015-05-19 14:50:51 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 37 Tomás Cohen Arazi 2015-05-19 16:56:01 UTC
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>