A bug has crept into the /api/v1/illrequests API endpoint causing a GET on this route to take an unreasonably long time to respond, even with a few queries. This never used to be the case so either we're fetching something that we didn't before and that's slowing things down, or there's an actual bug. For example, a query on my dev server with 6 requests takes 2.76 seconds to respond. This is clearly an unreasonably long time and will almost certainly increase as more requests are added. We need to investigate the source of the slowdown and address it.
Hi, db design at least partly to blame. At least a quick fix would be to create an index on illrequestattributes CREATE INDEX `illrequestattributes_type_value` ON illrequestattributes(type,value(32));
Hi Benjamin I'm not aware of anywhere where we query that table on type or value, it's only ever the illrequest_id we query via the join. So I'm not sure of the value of adding these indexes. I might be missing something though, so please put me straight if so!
Created attachment 76687 [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.
Created attachment 76688 [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
Created attachment 76705 [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.
Created attachment 77100 [details] Script for creating large numbers of ILL requests, for performance testing
Since this is a performance enhancement, it's very hard to test without a substantial number of ILL requests in the database. I have therefore attached the script I created to populate large number of requests (add_ill.pl). To test: - Before applying patches, add a large number of requests, maybe 5000, using attached add_ill.pl - Make a request to: http://your-koha/api/v1/illrequests?embed=metadata,patron,capabilities,library - Time how long the response takes - Apply patches - Make another request to the same URL - Time the response => TEST: The response time should be substantially reduced
If <branch> is now mandatory inside <interlibrary_loans> in koha-conf.xml it should probably be added to these template files? - etc/koha-conf.xml - debian/templates/koha-conf-site.xml.in It looks like the increase in speed is really significant, so I'll try to sign off as soon as I can.
Excellent point, thanks Magnus. I'll create a patch to add these
Some observations from a DB with 359 illrequests and 34,485 illrequestattributes. Measurements of the standard API call to /api/v1/illrequests?embed=metadata,patron,capabilities,library: Without the patch: Size of data: 1MB Time: ~12 seconds With the patch: Size of data: 1.8MB Time: ~8 seconds The decrease in time is much appreciated! I'm not sure where the increase in weight comes from. Could it be avoided and save us even more time?
Interesting, I'd not noticed the weight increase. I'll investigate!
Also, with the patch applied, when I try to view /cgi-bin/koha/ill/ill-requests.pl I get two JS popup warnings: DataTables warning: table id=ill-requests - Requested unknown parameter 'metadata_Author' for row 0, column 0. For more information about this error, please see http://datatables.net/tn/4 DataTables warning: table id=ill-requests - Requested unknown parameter 'metadata_Author' for row 358, column 0. For more information about this error, please see http://datatables.net/tn/4 Author and Title is empty in the table. Not sure if this is a data problem, but it did not happen before I applied the patches.
It's not related to this bug, it's due to an item not having an author, I've seen it before. I'll create a bug if I've not already created one. Thanks for mentioning it!
Created attachment 79379 [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
Hi Magnus. I can't account for the weight increase, I've not been able to replicate it. You've not also got bug 21079 applied have you? That runs a database update that creates additional illrequestmetadata values for request from the FreeForm backend. That's the only thing I can thing that might have increased the response size.
No bug 21079 applied, no. At least I think I found the difference in the JSON delivered, thanks to the "Response preview" in the Chromium developer tools. Without the patches applied, each request described in the JSON data has a data element "metadata" which contains just the properties that are set up by "sub metadata" in Base.pm. With the patches applied, this same data element contains *all* the request attributes, but not the data from "sub metadata" (this results in Title and Author not being populated in the table, as well as the errors/warnings I mentioned earlier). I have not looked into what is causing this difference.
Oooh, good investigating, I completely failed to find any difference in responses. I clearly didn't have sufficiently well populated requests! What you've found sounds promising, I'll do some digging and see what I can find. Thanks!
I've dug into this some more. The additional weight appears to be due to exactly what you observed. This is due to us now returning all metadata associated with a request rather than just a subset defined by the relevant backend. For example, prior to these patches, the metadata was populated via (for example, with the BLDSS backend): https://github.com/PTFS-Europe/koha-ill-bldss/blob/master/Base.pm#L244-L293 With this bug, it's populated by: https://github.com/PTFS-Europe/koha/blob/bug_20996/Koha/REST/V1/Illrequests.pm#L126-L136 which gets *all* metadata regardless of the backend. Whether this is a good idea or not is up for debate I guess. Any calls on a request object's 'metadata' method will still use the backend's metadata method to return the curated metadata from the backend. Dunno what the best thing to do is really.. Anyway, putting the response weight aside, this all now appears to be working as designed, so I'm going to change the status back to "Needs Signoff"
Not quite that simple, I'm afraid... When I am on the master branch everything looks fine. Title and Author are displayed for the requests that have them. When I apply the patches from this bug title and author are not displayed, and this warning pops up, both on initial rendering of the "View ILL requests" page and whenever I use the Next and Previous links in the datatable to see more requests. DataTables warning: table id=ill-requests - Requested unknown parameter 'metadata_Author' for row 367, column 0. For more information about this error, please see http://datatables.net/tn/4 Could this be because we are only getting the full requestattributes, and not the data from "sub metadata"? I am testing with my in-progress backend for Libris, so I guess this might be an issue with that backend. Especially if things work as they should for you.
No, it's not backend dependent. I looked into it yesterday, prior to these patches "title" and "author" metadata were returned by the API as "metadata_Title" & "metadata_Author", which is what the JS expects, hence no error. With the patches, they're returned as "metadata_title" & "metadata_author". I didn't get to the bottom of why, but now I write it out, I think I might know why. I suspect the fix is going to be to change what the JS expects, since the API is just returning illrequestattribute names as they appear in the database, which is ultimately correct. I'm going to fix this now, I'm not going to rest until it's done damn it! :)
OK, I know what it is. In the FreeForm backend's metadata method (and, I presume, your backend's metadata method), there is this line: https://github.com/PTFS-Europe/koha-ill-freeform/blob/master/Base.pm#L124 Which, as you can see, title cases the name of the property (Author etc), which breaks the JS when we use the alternative method of getting our metadata, which doesn't title case it. I've no idea why the properties are title cased, but it seems to have been the case since day 1. I'm going to fix it by changing what the JS expects from "metadata_Author" to "metadata_author", same for title, and leave it at that. This whole issue is made irrelevant by bug 21063 which changes how the datatable JS works anyway and has no expectation of column names from the API. I'll attach a patch momentarily.
Created attachment 79576 [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
Created attachment 79583 [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> https://bugs.koha-community.org/show_bug.cgi?id=20797
Created attachment 79584 [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> https://bugs.koha-community.org/show_bug.cgi?id=20797
Created attachment 79585 [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> https://bugs.koha-community.org/show_bug.cgi?id=20797
Created attachment 79586 [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> https://bugs.koha-community.org/show_bug.cgi?id=20797
Created attachment 79587 [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. https://bugs.koha-community.org/show_bug.cgi?id=20797
Created attachment 79588 [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@libriotech.no>
Created attachment 79589 [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@libriotech.no>
Created attachment 79590 [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@libriotech.no>
Created attachment 79591 [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@libriotech.no>
Created attachment 79592 [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. Signed-off-by: Magnus Enger <magnus@libriotech.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.
Created attachment 81715 [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@libriotech.no>
Created attachment 81716 [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@libriotech.no>
Created attachment 81717 [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@libriotech.no>
Created attachment 81718 [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@libriotech.no>
Created attachment 81719 [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. Signed-off-by: Magnus Enger <magnus@libriotech.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.
Created attachment 81720 [details] [review] Bug 20996: (QA follow-up) Fix pod
Rebased on master and add small follow-up
Andrew is going to add comments to embed as it was done in bug 18591
Comment on attachment 81715 [details] [review] Bug 20996: Remove prefix use of borrower category One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment on attachment 81716 [details] [review] Bug 20996: Fix unit tests One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment on attachment 81717 [details] [review] Bug 20996: Further enhance performance One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment on attachment 81718 [details] [review] Bug 20996: (follow-up) Add <branch> to conf tpl One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment on attachment 81719 [details] [review] Bug 20996: (follow-up) Fix column name expectation One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment on attachment 81720 [details] [review] Bug 20996: (QA follow-up) Fix pod One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Created attachment 81747 [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@libriotech.no>
Created attachment 81748 [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@libriotech.no>
Created attachment 81749 [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@libriotech.no>
Created attachment 81750 [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@libriotech.no>
Created attachment 81751 [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. Signed-off-by: Magnus Enger <magnus@libriotech.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.
Created attachment 81752 [details] [review] Bug 20996: (follow-up) Fix merge problems Merge conflicts resolved
Created attachment 81753 [details] [review] Bug 20996: (QA follow-up) Fix pod
Rebased, fixed merge problems (including the "comments" embed in the API response) and added Josef's POD fix. Ready for QA again
Created attachment 81755 [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@libriotech.no> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81756 [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@libriotech.no> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81757 [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@libriotech.no> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81758 [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@libriotech.no> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81759 [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. Signed-off-by: Magnus Enger <magnus@libriotech.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. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81760 [details] [review] Bug 20996: (follow-up) Fix merge problems Merge conflicts resolved Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81761 [details] [review] Bug 20996: (QA follow-up) Fix pod Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Awesome work all! Pushed to master for 18.11
t/db_dependent/Illrequest/Config.t is failing. New code added to Koha/Illrequest/Config.pm but no tests?
Created attachment 81908 [details] [review] Bug 20966: Add column configuration to table of orders in a basket This patch adds column configuration to the table of orders in a basket. It replaces the "Show all details" checkbox which previously was used to show the hidden "tax included" columns. Now those columns are hidden by default in the columns configuration file. To test, apply the patch and view a basket with multiple orders. Test all DataTables functionality (paging, sorting, filtering) including showing and hiding columns. Test with both an open and a closed basket. Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81909 [details] [review] Bug 20966: (follow-up) Add template variable filter; Fix table export This patch makes to corrections: 1. The "$raw" filter has been added to the column configuration 2. The columns configuration now specifies which columns are exported in Copy, Excel, etc. operations. To test, apply the patch and test the various table export buttons on a both a closed and open basket. Confirm that the 'Modify' and 'Cancel' columns are not exported. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81928 [details] [review] Bug 20996: (follow-up) Fix test - Fixes the t/db_dependent/Illrequest/Config.t - Adds tests for Koha::Illrequest::Config->has_branch 0) Do not apply the path 1) Run test t/db_dependent/Illrequest/Config.t 2) It fails - it is because the Koha/Illrequest/Config.pm prefixes handling changed 3) Apply the patch 4) Run test again, it should pass now 5) Read through the test code and confirm it makes sense
(In reply to Jonathan Druart from comment #63) > t/db_dependent/Illrequest/Config.t is failing. > > New code added to Koha/Illrequest/Config.pm but no tests? I added a follow-up patch with test changes
Created attachment 81967 [details] [review] Bug 20996: (follow-up) Fix test - Fixes the t/db_dependent/Illrequest/Config.t - Adds tests for Koha::Illrequest::Config->has_branch 0) Do not apply the path 1) Run test t/db_dependent/Illrequest/Config.t 2) It fails - it is because the Koha/Illrequest/Config.pm prefixes handling changed 3) Apply the patch 4) Run test again, it should pass now 5) Read through the test code and confirm it makes sense Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Awesome work all! Pushed to master for 18.11(In reply to Tomás Cohen Arazi from comment #68) > Created attachment 81967 [details] [review] [review] > Bug 20996: (follow-up) Fix test > > - Fixes the t/db_dependent/Illrequest/Config.t > - Adds tests for Koha::Illrequest::Config->has_branch > > 0) Do not apply the path > 1) Run test t/db_dependent/Illrequest/Config.t > 2) It fails - it is because the Koha/Illrequest/Config.pm prefixes > handling changed > 3) Apply the patch > 4) Run test again, it should pass now > 5) Read through the test code and confirm it makes sense > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Followup pushed to master for 18.11
Created attachment 81994 [details] [review] Bug 20996: (RM follow-up) Fix the api tests
Added a follow-up for the RM that should fix the last remaining failing test.
Created attachment 81996 [details] [review] Bug 20996: (RM follow-up) Fix the api tests https://bugs.koha-community.org/show_bug.cgi?id=21766
I get a warning with the last patch: t/db_dependent/api/v1/illrequests.t .. Un-mocked method 'status_graph()' called at /home/vagrant/kohaclone/Koha/Illrequest.pm line 439.
Created attachment 81997 [details] [review] Bug 20996: (RM follow-up) Fix the api tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 81998 [details] [review] Bug 20996: Remove warning 'Un-mocked method' t/db_dependent/api/v1/illrequests.t .. Un-mocked method 'status_graph()' called at /home/vagrant/kohaclone/Koha/Illrequest.pm line 439. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Last two patches pushed to master for 18.11
Odd.. I didn't get that warning at all when I tested the patch.
Enhancement, will not be backported to 18.05.x series.