Currently, the ILL request detail view shows a plain biblionumber.. this should be a link through to the bib record view.
Created attachment 75454 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view
The "ILL requests" page (with the table of all the requests) also shows the biblionumber, could we not turn that into a link too?
Totally.. not sure how I missed it.. tunnel vision.
Created attachment 75485 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view
Hi Magus - Latest attachment addresses this. Thanks for the feedback :)
Created attachment 75486 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view Signed-off-by: Magnus Enger <magnus@libriotech.no> Tested thusly: - Make sure there are a couple of ILL requests with biblionumber - Apply the patch - Go to "List requests" page - Biblionumber is now a link, clicking on it leads to the detail view of the biblio - Go to "Manage request" - Biblionumber is a link here too, clicking on it leads to detail view
Please surround 'N/A' with tags (to help translators)
Created attachment 75656 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view
I've now added <span> tags around all "N/A" strings. Thanks for the feedback Jonathan.
If "Biblio ID" is not filled, the column contains "0" with a link to /catalogue/detail.pl?biblionumber=0 Also, Andrew, I do not know your workflow when you submit a new patch, but you removed the signed-off-by line and the commit message. You should prefer a follow-up patch (which will be squashed if needed by QAer or RM). It also eases the review.
Thanks for that Jonathan. I've been squashing fix commits into the original commit, then using git-bz to attach it. So, yes, I can see how that would wipe stuff out, apologies for that. I'll submit follow up patches from now on.
Hi Jonathan Further to your comment: If "Biblio ID" is not filled, the column contains "0" with a link to /catalogue/detail.pl?biblionumber=0 I've done some digging. It appears that, for some reason, the API response contains a biblio_id of 0, even though the value in the database is NULL. I've checked and there's definitely no casting of this value to an integer in my code so I'm wondering if it's happening at a lower level. The schema definition for the column defines it as an integer, but it's also nullable. I'm not sure what to do about this. I can obviously work around it in the template, but that would just be masking the problem. Any ideas?
Further to this, I think this is the problem: https://github.com/PTFS-Europe/koha/blob/master/Koha/Object.pm#L249 If it's a numeric column, the value is being cast into an integer regardless of whether it's populated or not. Do you agree this is the bug?
(In reply to Andrew Isherwood from comment #13) > Further to this, I think this is the problem: > > https://github.com/PTFS-Europe/koha/blob/master/Koha/Object.pm#L249 > > If it's a numeric column, the value is being cast into an integer regardless > of whether it's populated or not. > > Do you agree this is the bug? That sounds like a bug yes, can you report it and CC Tomas please?
Done: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20886
Created attachment 75870 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null
Can this be signed off and QA'ed in conjunction with bug 20886. With the fix for 20886 applied, this patch now works as expected.
Setting back to 'Needs Signoff' as it now depends on bug 20886
Bug 20886 is now both pushed to master and pushed to stable.
Created attachment 79495 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Created attachment 79496 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Created attachment 79497 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null Signed-off-by: Magnus Enger <magnus@enger.priv.no> Works as advertised. The link is only displayed if there is a biblionumber to link to.
Created attachment 79578 [details] [review] Bug 20996: Remove prefix use of borrower category This patch removes the potential use of borrower category as a ILL request ID prefix. It makes no sense. We provide the ability for a site to define a request prefix based on branch, there is no use case for using the borrower category. Add to this that the borrower for every request was being retrieved in order to get the category, it's a huge performance hit also. We also now require the <branch> block in the <interlibrary_loans> block and complain if it's not present. The request prefix should be defined in this block. This patch also improves the performance of the API request that returns all requests, optionally including additional data. It also deprecates the overloaded TO_JSON method and moves the request augmentation code into the API route's controller. It may be that we want to shift it out of there at some point, but it is fine where it is for now. Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Created attachment 79579 [details] [review] Bug 20996: Fix unit tests - Remove unit tests relating to TO_JSON - Add tests for new explicit embedding - Modify tests relating to now deprecated brw_cat Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Created attachment 79580 [details] [review] Bug 20996: Further enhance performance We were immediately unblessing all request objects after retrieval then, at a later stage, re-instantiating each one so we could call methods on that. This was a very bad idea. We now preserve the objects for as long as we can. Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Created attachment 79581 [details] [review] Bug 20996: (follow-up) Add <branch> to conf tpl Since we now require the <branch> block, we should add it to the config templates Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Created attachment 79582 [details] [review] Bug 20996: (follow-up) Fix column name expectation This bug changes how request metadata is obtained. Previously the appropriate backend's metadata function would return it. For at least the FreeForm backend, the metadata property names we title cased. The datatable JS was expecting this and breaks with the new metadata getting which is performed in the API controller, which just returns the property names as they appear in the DB. This patch changes the expectation of the JS to match what's coming back from the API Signed-off-by: Magnus Enger <magnus@enger.priv.no> Tested by repeatedly loading the API-call http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library in Chromium and watching the reported time to load the URL. There is a siginificant decrease in the time with the patches compared to the time it takes without the patches. I had some concerns along the way, but they have all been fixed.
Ooops, attached some patches to the wrong bug. Sorry!
Please add the filters: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt FAIL filters missing_filter at line 568 ( [% request.cost %]) missing_filter at line 456 ( [% request.cost %]) missing_filter at line 528 (
Created attachment 79946 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view
Created attachment 79947 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null
Rebased and fixed problem raised in comment #29 - thanks Josef!
Created attachment 80018 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 80019 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 80021 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 80022 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 80038 [details] [review] Bug 20797 - Add link to biblio detail view from ILL detail view Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 80040 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 80047 [details] [review] k
Created attachment 80048 [details] [review] Bug 20797: Add link to biblio detail view from ILL detail view Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 80049 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Retested as we rebased it on top of 20772 - trying to build a path through the jungle here :)
Created attachment 80055 [details] [review] Bug 20797: (QA follow-up) Fix ill requests table ... by adding a missing comma.
Created attachment 80071 [details] [review] Bug 20797: Add link to biblio detail view from ILL detail view Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 80072 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 80073 [details] [review] Bug 20797: (QA follow-up) Fix ill requests table ... by adding a missing comma.
Created attachment 80120 [details] [review] Bug 20797: Add link to biblio detail view from ILL detail view Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 80121 [details] [review] Bug 20797: (follow-up) Conditionally link We don't want to display the link to the biblio if the biblio_id is null Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 80122 [details] [review] Bug 20797: (QA follow-up) Fix ill requests table ... by adding a missing comma.
Pushed for 18.11. Thanks all!
Enhancement, will not be backported to 18.05.x series.