Bug 8256 - Reports webservice should allow selection by name
Summary: Reports webservice should allow selection by name
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jared Camins-Esakov
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-15 17:59 UTC by Jared Camins-Esakov
Modified: 2013-12-05 20:01 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8256: Teach webservice to select reports by name (5.15 KB, patch)
2012-06-15 18:40 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8256: Teach webservice to select reports by name (6.83 KB, patch)
2012-06-15 22:01 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Thanks, Katrin, for testing! (6.89 KB, patch)
2012-06-16 11:45 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8256: Teach webservice to select reports by name (7.04 KB, patch)
2012-06-16 12:31 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-06-15 17:59:55 UTC
At the moment, reports cannot be selected from the reports webservice by name, only by report ID. It would be very useful to be able to select a report by name.
Comment 1 Jared Camins-Esakov 2012-06-15 18:40:27 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-06-15 18:43:16 UTC
Test plan:
1. Create a report (or choose an existing one), and mark it public
2. Run the report using the web service: [IntranetBaseURL]/cgi-bin/koha/svc/report?id=whatever
3. Confirm you get the expected results
4. Run the report by name using the web service: [IntranetBaseURL]/cgi-bin/koha/svc/report?name=[Report name] (keep spaces in the name)
5. Confirm you get the same results
6. Run the report using the public web service: [OPACBaseURL]/cgi-bin/koha/svc/report?id=whatever
7. Confirm you get the same results
8. Run the report by name using the public web service: [OPACBaseURL]/cgi-bin/koha/svc/report?name=[Report name] (keep spaces in the name)
9. Confirm you get the same results
10. Sign off
Comment 3 Katrin Fischer 2012-06-15 20:09:09 UTC
Hi Jared,

I can't get this to work :( The id works nicely for OPAC and for staff, but the name does not work for me. 

http://localhost:8080/cgi-bin/koha/svc/report?name=Borrowers

Can't call method "fetchall_arrayref" on an undefined value at /home/katrin/kohaclone/svc/report line 65.

or

http://localhost/cgi-bin/koha/svc/report?name=Borrowers

Sorry this report is not public

With id=2 both URLs work.
Comment 4 Jared Camins-Esakov 2012-06-15 22:01:49 UTC Comment hidden (obsolete)
Comment 5 Jared Camins-Esakov 2012-06-15 22:03:13 UTC
I'm not sure the changes I made will have fixed it, but could you please try again? At the very least, you shouldn't get that error about fetchall_arrayref not working.
Comment 6 Katrin Fischer 2012-06-16 09:28:34 UTC
Hi Jared,

the new feature works fine with the new patch, but I found another big problem. After applying your patch I can not edit existing reports. All fields are empty on the edit screen.

Another interesting find is a problem with authentication. I reached the URL for the report today, before I logged into the staff client. I got redirected to the login screen, which is fine. I logged in and got to see the report's results.
But the URL changed from http://localhost:8080/cgi-bin/koha/svc/report?id=2 to 
http://localhost:8080/cgi-bin/koha/svc/report. I opened a new tab and tried again to reach the page with the first URL and got told my session has timed out.

Not sure if this is a problem or not, but there is some difference between logging in from one of the webservice pages and logging in from the normal staff client URL.
Comment 7 Katrin Fischer 2012-06-16 09:36:08 UTC
[Sat Jun 16 11:22:15 2012] [error] [client 127.0.0.1] [Sat Jun 16 11:22:15 2012] report: "my" variable $report_id masks earlier declaration in same scope at /home/katrin/kohaclone/opac/svc/report line 36.
Comment 8 Jared Camins-Esakov 2012-06-16 11:45:45 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2012-06-16 12:31:03 UTC
Created attachment 10320 [details] [review]
[SIGNED-OFF] Bug 8256: Teach webservice to select reports by name

Adds the ability to pass a hash to C4::Reports::Guided::get_saved_report
which specifies a name or id to select the report.

Test plan:
1. Create a report (or choose an existing one), and mark it public
2. Run the report using the web service: [IntranetBaseURL]/cgi-bin/koha/svc/report?id=whatever
3. Confirm you get the expected results
4. Run the report by name using the web service: [IntranetBaseURL]/cgi-bin/koha/svc/report?name=[Report name] (keep spaces in the name)
5. Confirm you get the same results
6. Run the report using the public web service: [OPACBaseURL]/cgi-bin/koha/svc/report?id=whatever
7. Confirm you get the same results
8. Run the report by name using the public web service: [OPACBaseURL]/cgi-bin/koha/svc/report?name=[Report name] (keep spaces in the name)
9. Confirm you get the same results
10. Sign off

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
- Adding, editing and deleting reports works
- id parameter works
- new name parameter works
- public and non-public works
Comment 10 Marcel de Rooy 2012-07-09 07:37:25 UTC
QA Comment:
Works fine. Code looks good to me.

Some minor points, just to be strict for this time :-)
Routine get_saved_report:
Removing the my ($id)=@_ by the more obscure $#_ and $_[0] stuff is no improvement. (Although I understand your motivation..) 
You are adding several cuddled else's. Like: } else. Not a best practice.. You reduce one line, but also readability. Please use two lines.
Just wondering about the use of selector->{id}==0. Could you explain?

svc/report line 53-59:
First, thanks for commenting! What would be the cost of storing both in the cache BTW?
Note that PBP recommends breaking the line before the = operator, and even before reaching the 78 coulmns limit (line 57).
About the extended end-of-line comments: For readability, I would prefer here two comment lines starting under the if. (Don't know if that is a PBP or a personal opinion ;)

No reasons to block this patch however. Just working on my own PBP knowledge..
Passed QA
Comment 11 Jared Camins-Esakov 2012-12-03 13:25:42 UTC
This now works.