The response object returned from the /api/vi/illrequest GET route isn't consistent with response objects supplied by other routes, such as the patrons route. We have an RFC for this route here: https://wiki.koha-community.org/wiki/Illrequests_endpoint_RFC Josef has made some initial recommendations on what the properties should be named to make them consistent with other routes. We could do with expanding this RFC to make it complete, including a spec on how object embedding should be done since, to date, this is the only route that does this.
Further suggestions from Josef to bring this endpoint into line. We should follow the convention illustrated here: https://github.com/Koha-Community/Koha/blob/master/Koha/REST/V1/Library.pm#L46 This automagically looks at the parameters passed in the request and, if they match parameters described in the OpenAPI spec and, if they match, uses their values as filters. The _to_api & _to_model methods will allow us to carry out the property name consistency described in the bug description
*** Bug 30277 has been marked as a duplicate of this bug. ***
Created attachment 150715 [details] [review] Bug 22440: Add Koha::Illrequests->filter_by_visible This patch introduces a method for filtering out requests that match the statuses specified on the *ILLHiddenRequestStatuses* system preference. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Illrequests.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150716 [details] [review] Bug 22440: Add missing foreign key Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150717 [details] [review] Bug 22440: Add standard accessors for later usage Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150718 [details] [review] Bug 22440: Add GET /ill_requests Co-authored-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150719 [details] [review] Bug 22440: New ILL requests table Use kohaTable requesting the REST api instead of the old KohaTable Co-authored-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150720 [details] [review] Bug 22440: ILL API changes * Add GET /illbackends/{id}/statuses * Change API route to ill/requests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150721 [details] [review] Bug 22440: Tests Updated API tests Co-authored-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150722 [details] [review] Bug 22440: Move backend statuses api endpoint Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150723 [details] [review] Bug 22440: Schema Update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This has had significant testing in house at PTFS-E and was worked on collaboratively by Tomas and Pedro (with a little interruption from myself a few times) We all agreed to go PQA given the eyes's it's had on :)
*** Bug 22714 has been marked as a duplicate of this bug. ***
Pushed to master for 23.05. Nice work everyone, thanks!
Created attachment 150788 [details] [review] Bug 22440: (follow-up) Nothing is forever This patch picks (yet) another class for testing purposes. The class needs to not implement to_api() and to_api_mapping().
Created attachment 150789 [details] [review] Bug 22440: (follow-up) Fix existing tests As we are introducing a FK for illrequests.biblio_id, TestBuilder is generating a linked biblio, and so tests expecting undef are failing. This fixes it by explicitly setting biblio_id => undef on creating the request. The patch also cleans the area a bit, removing a redundant and fragile test as well.
Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release
Missing dependencies for 22.05.x, no backport.
Hm, I am not so happy about the FK change hidden/sneaked in with this bug. Especially since it's now without documentation/warning to users and can create data loss. If you don't store all information also in ILL request attributes it will be hard to determine what has been ordered for making acq decisions and creating reports. Also: there already was as specific bug for this change, stuck in discussion for these reasons: bug 21983
From https://tree.taiga.io/project/koha-ils/task/117 And IRC: Joubu> tcohen: see https://tree.taiga.io/project/koha-ils/task/117 Joubu> tcohen: installer/data/mysql/db_revs/221200026.pl Joubu> it must be *unless* fk exists! Joubu> there is also a typo in the description endopoint
Fixed in 23.05 and 23.11. Please backport the fix for 22.11.
(In reply to Katrin Fischer from comment #19) > Hm, I am not so happy about the FK change hidden/sneaked in with this bug. > Especially since it's now without documentation/warning to users and can > create data loss. > > If you don't store all information also in ILL request attributes it will be > hard to determine what has been ordered for making acq decisions and > creating reports. > > Also: there already was as specific bug for this change, stuck in discussion > for these reasons: bug 21983 I'd have loved to at least get a comment/reaction. The fix mentioned seems to be to enforce the change I marked as critical.
(In reply to Katrin Fischer from comment #22) > (In reply to Katrin Fischer from comment #19) > > Hm, I am not so happy about the FK change hidden/sneaked in with this bug. > > Especially since it's now without documentation/warning to users and can > > create data loss. > > > > If you don't store all information also in ILL request attributes it will be > > hard to determine what has been ordered for making acq decisions and > > creating reports. > > > > Also: there already was as specific bug for this change, stuck in discussion > > for these reasons: bug 21983 > > I'd have loved to at least get a comment/reaction. The fix mentioned seems > to be to enforce the change I marked as critical. Also, if you really want to do this, because it needs to be done: what's with the old ill_requests? It caused bug 33873.
(In reply to Katrin Fischer from comment #23) > (In reply to Katrin Fischer from comment #22) > > (In reply to Katrin Fischer from comment #19) > > > Hm, I am not so happy about the FK change hidden/sneaked in with this bug. > > > Especially since it's now without documentation/warning to users and can > > > create data loss. > > > > > > If you don't store all information also in ILL request attributes it will be > > > hard to determine what has been ordered for making acq decisions and > > > creating reports. > > > > > > Also: there already was as specific bug for this change, stuck in discussion > > > for these reasons: bug 21983 > > > > I'd have loved to at least get a comment/reaction. The fix mentioned seems > > to be to enforce the change I marked as critical. > > Also, if you really want to do this, because it needs to be done: what's > with the old ill_requests? It caused bug 33873. Should we do the same we did for orderlines?
(In reply to Tomás Cohen Arazi from comment #24) > (In reply to Katrin Fischer from comment #23) > > (In reply to Katrin Fischer from comment #22) > > > (In reply to Katrin Fischer from comment #19) > > > > Hm, I am not so happy about the FK change hidden/sneaked in with this bug. > > > > Especially since it's now without documentation/warning to users and can > > > > create data loss. > > > > > > > > If you don't store all information also in ILL request attributes it will be > > > > hard to determine what has been ordered for making acq decisions and > > > > creating reports. > > > > > > > > Also: there already was as specific bug for this change, stuck in discussion > > > > for these reasons: bug 21983 > > > > > > I'd have loved to at least get a comment/reaction. The fix mentioned seems > > > to be to enforce the change I marked as critical. > > > > Also, if you really want to do this, because it needs to be done: what's > > with the old ill_requests? It caused bug 33873. > > Should we do the same we did for orderlines? A database table has the option to use COALESCE, so that might be an option. We could also then shift the existing data. I am not sure what the technical requirement for the FK was, so can't tell if removing it again might be a valid option.
This is not used, right? + if ( col.datatype == 'date' ) { + let rfc3339 = $date_to_rfc3339(value); + if ( rfc3339 != 'Invalid Date' ) { + built_value = rfc3339; + } + }
*** Bug 34246 has been marked as a duplicate of this bug. ***
*** Bug 23557 has been marked as a duplicate of this bug. ***