It would be nice to have an API for creating a purchase suggestion. It would be used for instance in VuFind. Proposed implementation: To create: POST /suggestions/{borrowernumber} Incoming data: { title (required) author copyrightdate isbn publishercode collectiontitle place itemtype patronreason note negcap } Outcoming data: 200 OK / 403 Forbidden / 409 Conflict (when already exists the proposal) { error status } To list: GET /suggestions/{borrowernumber} Outcoming data: list of suggestions To delete: DELETE /suggestions/{borrowernumber}/{suggestion_id} Outcoming data: 204 No Content (successful deletion) / 403 Forbidden / 404 Not Found
Hi, just read through this and have some notes/questions: - collectiontitle: I think it's not clear in the interface what this is to be used for, maybe a question we should talk about first and then maybe choose a better name? - copyrightdate: MARC21 uses copyrightdate in the database, UNIMARC publicationyear. Both fields appear in biblioitems/biblio and in suggestions. Something to tidy up/take into account here? - negcap - This is to keep the robots off. My impression is that this would always be empty? It's possible to make anonymous suggestions - will this be taken into account? For DELETE: DELETE /suggestions/{borrowernumber}/{suggestion_id} Why include the borrowernumber? Would the suggestion_id not be sufficient?
(In reply to Katrin Fischer from comment #1) > Hi, just read through this and have some notes/questions: > > - collectiontitle: I think it's not clear in the interface what this is to > be used for, maybe a question we should talk about first and then maybe > choose a better name? I don't actually know exactly what does it have to mean, but you can see it as "Collection title" on the OPAC site for creating a suggestion: http://koha-opac/cgi-bin/koha/opac-suggestions.pl?op=add > - copyrightdate: MARC21 uses copyrightdate in the database, UNIMARC > publicationyear. Both fields appear in biblioitems/biblio and in > suggestions. Something to tidy up/take into account here? In OPAC the copyrightdate has maximum 4 characters, so it is the MARC21. The question is, whether we should allow specifying also the publicationyear? > - negcap - This is to keep the robots off. My impression is that this would > always be empty? That's right, we leave it empty, thus we won't include it to the incoming data for POST - the controller will take care of that. > It's possible to make anonymous suggestions - will this be taken into > account? Oh, that's the first time I hear about this functionality. Could you provide me with an example how to submit an anonymous suggestion? > For DELETE: > DELETE /suggestions/{borrowernumber}/{suggestion_id} > Why include the borrowernumber? Would the suggestion_id not be sufficient? It's because of the privileges check. Imagine some curious user in VuFind, who changes his suggestion_id in the form and tries to delete suggestion of someone else - this should prevent it and return 403 Forbidden.
> > - collectiontitle: I think it's not clear in the interface what this is to > > be used for, maybe a question we should talk about first and then maybe > > choose a better name? > > I don't actually know exactly what does it have to mean, but you can see it > as "Collection title" on the OPAC site for creating a suggestion: > http://koha-opac/cgi-bin/koha/opac-suggestions.pl?op=add Yes, I know - it just seems to cause some confusion. I was wondering if it should be better named 'series' or similar. As we are trying to use good terminology in the API from the beginning it would be nice to take a look if that would make more sense. > > - copyrightdate: MARC21 uses copyrightdate in the database, UNIMARC > > publicationyear. Both fields appear in biblioitems/biblio and in > > suggestions. Something to tidy up/take into account here? > > In OPAC the copyrightdate has maximum 4 characters, so it is the MARC21. The > question is, whether we should allow specifying also the publicationyear? What I tried to explain (badly) is that there is something a little odd in Koha. The same information goes into different fields depending on MARC flavour - so it would be good to take a look at the code to verify if both suggestions.copyrightdate and suggestions.publicationsyear are used maybe. Also kind of a terminology question - but it might not play a big role here. > > It's possible to make anonymous suggestions - will this be taken into > > account? > > Oh, that's the first time I hear about this functionality. Could you provide > me with an example how to submit an anonymous suggestion? Take a look at the system preferences related to suggestions: suggestion - on/off switch for suggestions in the OPAC AllowPurchaseSuggestionBranchChoice - on/off for ability to select the branch OPACViewOthersSuggestions - on/off for seeing ALL suggestions in the system (without creators name) AnonSuggestions - ability to make suggestions without logging in > > > For DELETE: > > DELETE /suggestions/{borrowernumber}/{suggestion_id} > > Why include the borrowernumber? Would the suggestion_id not be sufficient? > > It's because of the privileges check. Imagine some curious user in VuFind, > who changes his suggestion_id in the form and tries to delete suggestion of > someone else - this should prevent it and return 403 Forbidden. I think if changing the URL is enough - we are doing something wrong :) Will the permission check be so that guessing a borrowernumber will also not allow to delete another user's suggestions?
(In reply to Katrin Fischer from comment #3) I'm sorry for such a delay in my response - there was a lot of job to do. From today's point of view and existence of the How-To write REST API https://wiki.koha-community.org/wiki/Rest_Api_HowTo we shouldn't take care of the data it flows in or out at the API, but create Koha Object if it doesn't exists yet and use it to pass the data. So, if you suggest to rename collectiontitle to series, just create a new bug for it to compose it into the DB directly - the API will also use it implicitly. The issue between copyrightdate & publicationyear should be handled from within the Koha Object -> I'll submit a new dependent bug for this.
Created attachment 58098 [details] [review] Implemented suggestions REST API
(In reply to Jiri Kozlovsky from comment #5) > Created attachment 58098 [details] [review] [review] > Implemented suggestions REST API Basic test plan 1.part: 1. Login using account without any permissions and copy Cookie's CGISESSID=TOKEN and copy it's borrowernumber 2. Using your favorite HttpRequester send POST to /api/v1/suggestions with the cookie in Headers: { "title": "Suggested title", "author": "hawking", "suggestedby": "BORROWER_NUMBER" } 3. Obtain the suggestionid from the response and send GET to /api/v1/suggestions/YOUR_SUGG_ID 4. Check that returned data matches data sent at point 2. 5. Send PUT request to /api/v1/suggestions/YOUR_SUGG_ID: { "title": "Updated suggested title" } 6. Repeat steps 3 + 4. 7. Login using another account again without any permissions and copy Cookie's CGISESSID=TOKEN 8. Using your favorite HttpRequester send POST to /api/v1/suggestions with the cookie in Headers: { "title": "Suggested title", "author": "hawking", "suggestedby": "YOUR_BORROWER_NUMBER", "STATUS": "ACCEPTED" } 9. Check that error was returned. 10. Create valid suggestion (repeat 8. step, but remove the STATUS) and remember the suggestionid. 11. Send PUT request to /api/v1/suggestions/YOUR_SUGG_ID: { "STATUS": "ACCEPTED" } 12. Check that error was returned. 13. Send GET request to suggestionid from the first user. (Check that you don't have enough permissions) 14. Send GET request to suggestionid of current user. (should succeed) 15. Send DELETE request to suggestion of current user. (should return 200 OK only .. without any body) 16. Repeat 14. point - should fail. 17. Send GET request to /api/v1/suggestions/?suggestedby=BORROWER_NUMBER 18. Check that there were returned all active suggestions for the user. 19. Send DELETE request to suggestion of the first user (not enough permissions - should fail) Basic test plan 2.part: 1. Login using user with permissions of "acquisition" and copy your CGISESSID from the Cookie. 2. Using your favorite HttpRequester send POST to /api/v1/suggestions with the cookie in Headers: { "title": "Suggested title from librarian", "author": "hawking", "STATUS": "ACCEPTED", "suggestedby": "BORROWER_NUMBER" } 3. Check that new suggestion was created successfully and note the suggestionid. 4. Disable "AnonSuggestions" syspref and repeat 1. & 2. step without "suggestedby". (should fail because anonymous suggestions are disabled) 5. Enable "AnonSuggestions" syspref and repeat 1. & 2. step without "suggestedby". (success) 6. Send PUT request to /api/v1/suggestions/YOUR_SUGG_ID: { "STATUS": "REJECTED" } 7. Check that the suggestion was updated successfully. This should provide you basic understanding of how does "add", "update", "list" and "delete" work. Now you can test the following: 1. While adding / updating suggestion with patronreason or branchcode or STATUS or itemtype specified, check that the API refuses invalid values, while accepts valid ones. 2. While adding / updating suggestion with suggestedby or managedby specified, as a user with acquisition permissions, check that the API refuses ids which are invalid, while accepts valid ones (invalid are nonexisting borrowernumbers). 3. Setup "MaxOpenSuggestions" syspref to custom value and check that it applies to all borrowers excluding anonymous suggestions. 4. Setup "OPACSuggestionMandatoryFields" syspref to custom values and check that those required fields are required only while creating suggestions, not when updating, because you may want to update only a title for example. 5. Setup "AllowPurchaseSuggestionBranchChoice" syspref to enabled and check that regular users (without acquisition rights) can create and update suggestions with branchcode specified. Also check that it is forbidden for them to do so when the syspref is disabled. 6. Check that regular users can create and update suggestions only with the following fields specified: ('suggestedby', 'title', 'author', 'copyrightdate', 'isbn', 'publishercode', 'collectiontitle', 'place', 'itemtype', 'patronreason', 'note') ... branchcode is added when syspref AllowPurchaseSuggestionBranchChoice is enabled. ... That's it ! You got it all covered! Btw, thanks for signing this off in advance !
Created attachment 58100 [details] [review] Implemented suggestions REST API Added HTTP Codes for remaining errors
Created attachment 58101 [details] [review] Implemented suggestions REST API Added HTTP Codes for remaining errors (previous attachment didn't change anything [I forgot to add changes to the commit]) Test plan unchanged.
Started testing... For first koha-qa complains: FAIL Koha/REST/V1/Suggestions.pm FAIL valid Smartmatch is experimental
prove t/db_dependent/api/v1/swagger/definitions.t says: # Failed test 'Columns is nullable in DB, not in swagger file for suggestion: publicationyear, suggesteddate'
Created attachment 58993 [details] [review] QA followup test plan unchanged
On step 5 of test plan part 1 i got: { "error": "Anonymous suggestions are disabled" } step 11: i don't think this is the error it should return: { "errors": [ { "path": "/body/title", "message": "Missing property." } ] } And I found one problem: when I'am adding suggestion through API, I can't see them in the user interface, unless I add the branchcode - it should be somehow set as mandatory I think
Also, unit test are needed I think
> And I found one problem: when I'am adding suggestion through API, I can't > see them in the user interface, unless I add the branchcode - it should be > somehow set as mandatory I think Well, you have to change your filters (click on "Acquisition information" -> Library -> Any) If you would like to set branch_code as mandatory, you should set up "OPACSuggestionMandatoryFields" accordingly. Note that there is always possible to enter a suggestion without it's branchcode unless it is forced by the settings. You can do it in the OPAC & the Intranet also by setting Library -> Any.
(In reply to Jiri Kozlovsky from comment #14) > > And I found one problem: when I'am adding suggestion through API, I can't > > see them in the user interface, unless I add the branchcode - it should be > > somehow set as mandatory I think > > Well, you have to change your filters (click on "Acquisition information" -> > Library -> Any) > > If you would like to set branch_code as mandatory, you should set up > "OPACSuggestionMandatoryFields" accordingly. > > Note that there is always possible to enter a suggestion without it's > branchcode unless it is forced by the settings. You can do it in the OPAC & > the Intranet also by setting Library -> Any. But yeah, I've just noticed, that regular users doesn't have the possibility to specify the branchcode, it'll automatically belong to the library they're registered in. Only those with access to intranet can create suggestions belonging to Any library.
(In reply to Jiri Kozlovsky from comment #15) > (In reply to Jiri Kozlovsky from comment #14) > > > And I found one problem: when I'am adding suggestion through API, I can't > > > see them in the user interface, unless I add the branchcode - it should be > > > somehow set as mandatory I think > > > > Well, you have to change your filters (click on "Acquisition information" -> > > Library -> Any) > > > > If you would like to set branch_code as mandatory, you should set up > > "OPACSuggestionMandatoryFields" accordingly. > > > > Note that there is always possible to enter a suggestion without it's > > branchcode unless it is forced by the settings. You can do it in the OPAC & > > the Intranet also by setting Library -> Any. > > But yeah, I've just noticed, that regular users doesn't have the possibility > to specify the branchcode, it'll automatically belong to the library they're > registered in. Only those with access to intranet can create suggestions > belonging to Any library. Well, it's a bit more complicated .. There can be set "AllowPurchaseSuggestionBranchChoice" preference, which allows user to pick a library. But there's no choice for "Any", so the behavior I'm going to implement is to choose user's default library if the preference is Off. If it is On, then it'll let user pick a library, except Any, choosing his own library if not specified.
Created attachment 60133 [details] [review] Bug 17314 - QA followup 2nd Fixed: Smartmatch Updating suggestion when AnonSuggestions disabled Not requiring title when updating Branch choosing with respect to AllowPurchaseSuggestionBranchChoice Test plan unchanged
I think that when the API makes new suggestion, it should inject "STATUS":"ASKED" as default, if STATUS is not passed by client... Also, when i make branch mandatory and turn on allowPurchaseSuggestionBranchChoice preference, I get the "Properties not allowed: branch" error message when providing branch code.
Just been playing with this (as I've been asked to look at getting our LORLS reading list system purchase predictor to generate purchasing suggestions directly into Koha rather than just emailing the suggestions to library staff). Its good stuff. I'd concur about the "STATUS": "ASKED" by default, and I also needed to put a "branchcode" of "TEST" in (on kohadevbox). Without these, the suggestions made via the API didn't show up in the "Home › Acquisitions › Suggestions management" interface, even if I searched for status of "Any" and even though they were in the tables. Good to see that it can be used to insert quantity and price too. Is development of this patch still ongoing? Also PUT appears to actually be a PATCH operation as it doesn't require the whole object data to be specified - should the HTTP method be changed to reflect that?
I noticed that this doesn't apply cleanly to the latest master HEAD on my kohadevbox. I could help rebase if that would be useful to nudge this bug along? Unless of course Jiri is working on an updated version?
(In reply to Jon Knight from comment #20) > I noticed that this doesn't apply cleanly to the latest master HEAD on my > kohadevbox. I could help rebase if that would be useful to nudge this bug > along? Unless of course Jiri is working on an updated version? Well, I've been working on rebasing on top of master HEAD for a little time already and I'm near completion. Right now I'm actually solving a really strange bug when GET on an item (/suggestions/{id}) works perfectly fine, but GET on a list of items (/suggestions) fails on "Method not implemented" although I've basically copy & pasted the /patrons implementation. As soon as I'll solve this strange behaviour, I'll post my work here ;) But thank you for your effort.(In reply to Jon Knight from comment #19) > Is development of this patch still ongoing? Also PUT appears to actually be > a PATCH operation as it doesn't require the whole object data to be > specified - should the HTTP method be changed to reflect that? I'm going to fix this too in my next patch.
Created attachment 90171 [details] [review] Implemented suggestions REST API
Created attachment 90172 [details] [review] Bug 17314 : Migration from Swagger2 to OpenApi
I do not know if there is a precedence, but here, adding a new suggestion from the REST API and from the interface will not called the same method/subroutine IMO we should not do that. Before implementing this, we should move/update C4 code and controllers to use Koha::Suggestion.
Created attachment 90176 [details] [review] Bug 17314 : QA Followup Test plan unchanged
(In reply to Jonathan Druart from comment #24) > I do not know if there is a precedence, but here, adding a new suggestion > from the REST API and from the interface will not called the same > method/subroutine > > IMO we should not do that. > Before implementing this, we should move/update C4 code and controllers to > use Koha::Suggestion. Hi Jonathan, Thanks for the remark, I've just rewrote part of this patch but it's quite old and I might definitely have missed something. I'll check your comment right now and provide a followup :) Thanks for your fast QA answer!
Hhmmm... I'd say the following : The API is not a huge mass of users. Also, C4 code is also going to move toward Koha::Suggestion one day or another. It's not like we'd be developing new features in C4 I'd say it's rather a move forward. I've been looking into the dependencies to C4 but I'm still feeling a bit newbie for that task. Is there a bug already? Couldn't find one (and it's getting late here I'll go to bed instead of creating a new one). Plus, I'd really like to get this patch integrated to have portal software start using it (not the best reason I admit :D)
I have a few remarks: - The method you are adding to Koha::Patron (->suggestions) needs tests, and it should be actually relying on a relationship, and thus using _new_from_dbic so it is prefetchable. This has been discussed recently so you couldn't know beforehand. - There's now a clear distinction between endpoints that target unprivileged users (OPAC) and administrative ones (e.g. staff interface). If this bug only targets the 'privileged' one, maybe make it explicit and make sure the permissions are correct. - The attribute names seem to come straight from the DB structure. This should be fixed. Look at the RFCs page [1] and please follow naming conventions discussed and voted there. For example, borrowernumber is not used on the API, use patron_id instead. - POD is missing on the controller - Take a look at Koha::REST::V1::Cities and how it uses ->objects_search. It will automagically add pagination to the list method, which is desierd on busy sites (i.e. make the list manageable). - objects_search requires you to pass the _to_api and _to_model methods you will need to write to comply with the currently voted RFCs regarding terminology. - If you want the end user (session) to create purchase suggestions, then you need an endpoint in /public. And if you need to list them on the patron's page, maybe worth generating /public/patron/{patron_id}/purchase_suggestions (this is very arguable, just a thought). [1] https://wiki.koha-community.org/wiki/REST_api_RFCs
Added RFC : https://wiki.koha-community.org/wiki/Suggestions_endpoint_RFC Work still in progress to make the patch conform with RFC.
Created attachment 91545 [details] [review] Bug 17314 : Added _to_model and _to_api methods
Hi Arthur, nice to see progress in this bug. I'm ready to test :-)
Created attachment 91546 [details] [review] Bug 17314 : Added _to_model and _to_api methods and changed attirbutes naming
Hi Michal, great to see your interest! feel free to test and let me know :)
Hi all, since this got written we made some design decisions that affect this patches. They are easy to address, though :-D - Request validation is done using the spec, by Mojolicious::Plugin::OpenAPI - We decided not to mix 'public' endpoints (that don't require privileged access) from the ones restricted to privileged access users. - The above decision implies that endpoints that have 'allow_owner' or 'allow_guarantor' permissions, need to be separated, and moved to /public. In this case, I'd say: GET /public/patrons/:patron_id/suggestions <- list of suggestions POST /public/patrons/:patron_id/suggestions { object with required attributes } <- add This change should simplify the logic inside the controller (i.e. a separate controller method for the public one) I haven't checked if we allow cancelling a request and in which cases it is/should be allowed. In that case a DELETE verb could be allowed in the /public namespace. - Using C4::Context::user_env is forbidden here. It relies on Cookie auth, which is not obvious to assume is the case (we have several auth mechanisms for the API). So you have two options: (a) make the parameter mandatory (why not? and just return 400 bad request) or (b) use the stashed Koha::Patron object (i.e. the one guessed from the authentication mechanism, accessible through $c->stash('koha.user') (yes, it is the Koha::Patron object for the patron that was granted the access). Keep in mind this use case: do we allow anonymous purchase suggestions? Only authenticated users get the Koha::Patron stashed of course :-D - This should be better handled by the separation of concerns (public vs privileged), and relaying on the spec for validation: + unless (any { /^$param$/ } @allowed_fields) { + # Ouch ! User trying to edit field he has no rights to edit!
Created attachment 93761 [details] [review] Bug 17314: Routes to handle purchase suggestions
Created attachment 93762 [details] [review] Bug 17314: Migration from Swagger2 to OpenApi This patch migrates the original implementation so it uses the new OpenAPI plugin.
Created attachment 93763 [details] [review] Bug 17314: QA Followup Test plan unchanged
Created attachment 93764 [details] [review] Bug 17314 : Added _to_model and _to_api methods and changed attirbutes naming
Rebased it on top of current master.
Created attachment 93765 [details] [review] Bug 17314: (QA follow-up) Adjust spec This patch makes the exposed attribute names match what is defined in the internal mapping in the code, and also the query parameters. It also: - Makes 'status' an enum, as we know the list of possible values/codes... - Removes allow-owner and allow-guarantor, as said before, endpoints in /public namespace are allowed to use those. I didn't go and write those endpoints becuase I wasn't sure about the restrictions that apply in the OPAC and might differ from the ones in the staff interface. - Some numeric values are marked as such - Dates are strings in OpenAPI, but there's a way to specify the format inside the string: "format": "date". Use it
Hi Tomas, Since you've been doing most of the rework on this patch i thought maybe it would be best to have you as assignee. I've just tried to test it and maybe signoff (also a good reason for not being assignee myself) but failed to apply on current master. Kr, Arthur <pre> arthur@carl9000:~/repos/koha$ git bz apply 17314 Bug 17314 - Routes to create, list and delete a purchase suggestion 93761 - Bug 17314: Routes to handle purchase suggestions 93762 - Bug 17314: Migration from Swagger2 to OpenApi 93763 - Bug 17314: QA Followup 93764 - Bug 17314 : Added _to_model and _to_api methods and changed attirbutes naming 93765 - Bug 17314: (QA follow-up) Adjust spec Apply? [(y)es, (n)o, (i)nteractive] y Application de Bug 17314: Routes to handle purchase suggestions Utilisation de l'information de l'index pour reconstruire un arbre de base... M Koha/Patron.pm M api/v1/swagger/definitions.json M api/v1/swagger/parameters.json M api/v1/swagger/paths.json Retour à un patch de la base et fusion à 3 points... Fusion automatique de api/v1/swagger/paths.json CONFLIT (contenu) : Conflit de fusion dans api/v1/swagger/paths.json Fusion automatique de api/v1/swagger/parameters.json Fusion automatique de api/v1/swagger/definitions.json CONFLIT (contenu) : Conflit de fusion dans api/v1/swagger/definitions.json Fusion automatique de Koha/Patron.pm CONFLIT (contenu) : Conflit de fusion dans Koha/Patron.pm error: Échec d'intégration des modifications. le patch a échoué à 0001 Bug 17314: Routes to handle purchase suggestions astuce: Utilisez 'git am --show-current-patch' pour visualiser le patch en échec When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-17314-Routes-to-handle-purchase-suggestions-OvK0GF.patch arthur@carl9000:~/repos/koha$ </pre>
Created attachment 108380 [details] [review] Bug 17314: Routes to handle purchase suggestions
fixed conflicts but i get errors, also qa-tools are not happy. Http response from api: <pre> { "errors": [ { "message": "Properties not allowed: suggestionid, rejecteddate, date, acceptedby, place, patronreason, manageddate, price, copyrightdate, suggesteddate, branchcode, collectiontitle, rejectedby, accepteddate, STATUS, total, itemtype, managedby, lastmodificationdate, biblionumber, publishercode, suggestedby, publicationyear, budgetid, lastmodificationby, volumedesc, archived.", "path": "/" } ], "status": 500 } </pre> errors from qa tools: <pre> testing 5 commit(s) (applied to 6d7ff0d '04 Bug 16371: (follow-up) DB changes') Processing files before patches |========================>| 2 / 2 (100.00%) Processing files after patches |========================>| 2 / 2 (100.00%) FAIL Koha/Patron.pm FAIL pod *** ERROR: Apparent command =cut not preceded by blank line in file Koha/Patron.pm FAIL Koha/REST/V1/Suggestions.pm FAIL forbidden patterns forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16) forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 178) forbidden pattern: tab char (line 181) forbidden pattern: tab char (line 182) forbidden pattern: tab char (line 190) forbidden pattern: tab char (line 224) forbidden pattern: tab char (line 231) forbidden pattern: tab char (line 238) FAIL pod *** ERROR: Spurious =cut command in file Koha/REST/V1/Suggestions.pm *** WARNING: =head3 without preceding higher level in file Koha/REST/V1/Suggestions.pm *** WARNING: =head2 without preceding higher level in file Koha/REST/V1/Suggestions.pm FAIL pod coverage POD is missing for add POD is missing for delete POD is missing for get POD is missing for list POD is missing for update Processing additional checks * Commit title does not start with 'Bug XXXXX: ' - 0fd078368e * Commit title does not contain 'follow-up' correctly spelt - 2195ec1b0a * Commit title does not contain '(QA follow-up)' correctly spelt - 2195ec1b0a </pre>
Created attachment 109972 [details] [review] Bug 17314: Routes to handle purchase suggestions
Created attachment 109973 [details] [review] Bug 17314: Migration from Swagger2 to OpenApi This patch migrates the original implementation so it uses the new OpenAPI plugin.
Created attachment 109974 [details] [review] Bug 17314: (QA follow-up) Test plan unchanged
Created attachment 109975 [details] [review] Bug 17314: Added _to_model and _to_api methods and changed attirbutes naming
Created attachment 109976 [details] [review] Bug 17314: (QA follow-up) Adjust spec This patch makes the exposed attribute names match what is defined in the internal mapping in the code, and also the query parameters. It also: - Makes 'status' an enum, as we know the list of possible values/codes... - Removes allow-owner and allow-guarantor, as said before, endpoints in /public namespace are allowed to use those. I didn't go and write those endpoints becuase I wasn't sure about the restrictions that apply in the OPAC and might differ from the ones in the staff interface. - Some numeric values are marked as such - Dates are strings in OpenAPI, but there's a way to specify the format inside the string: "format": "date". Use it
Created attachment 109977 [details] [review] Bug 17314: (QA follow-up)
I've just got everything to work fine on current master. Did a bit of rework and a bit of reword to comply with QA rules. QA scripts green.
up? This one need QA :)
Doesn't apply. It would help to have the test plan in one of the commits. And confirm that it's the same since 2016.
Created attachment 114113 [details] [review] Bug 17314: Routes to handle purchase suggestions
Created attachment 114114 [details] [review] Bug 17314: Migration from Swagger2 to OpenApi This patch migrates the original implementation so it uses the new OpenAPI plugin.
Created attachment 114115 [details] [review] Bug 17314: (QA follow-up) Test plan unchanged
Created attachment 114116 [details] [review] Bug 17314: Added _to_model and _to_api methods and changed attirbutes naming
Created attachment 114117 [details] [review] Bug 17314: (QA follow-up) Adjust spec This patch makes the exposed attribute names match what is defined in the internal mapping in the code, and also the query parameters. It also: - Makes 'status' an enum, as we know the list of possible values/codes... - Removes allow-owner and allow-guarantor, as said before, endpoints in /public namespace are allowed to use those. I didn't go and write those endpoints becuase I wasn't sure about the restrictions that apply in the OPAC and might differ from the ones in the staff interface. - Some numeric values are marked as such - Dates are strings in OpenAPI, but there's a way to specify the format inside the string: "format": "date". Use it
Created attachment 114118 [details] [review] Bug 17314: (QA follow-up)
does patch still apply? anyone willing to test?
The patches still apply. I attempted to test using the first test plan, however APIs, REST clients, etc are not something I'm familiar with and is a bit too complicated for me... So will leave for someone else to test. Testing notes (as far as I got, using koha-testing-docker): - Enable the REST API - system preference RESTPublicAPI - Apply patch - flush_memcached and restart_all - Check that you can access the API documentation: http://127.0.0.1:8080/api/v1/.html#op-get-suggestions - Install RESTer add on for Firefox (not sure of any other sthat can be used)
This is a hard move, but in my effort on making this more in line with how we do it, I found myself removing things and rewriting most of it. And it didn't contain tests. So I decided we should start over with a new, single patch.
Created attachment 120111 [details] [review] Bug 17314: Add routes for purchase suggestions This patch adds routes for handling purchase suggestions. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120133 [details] [review] Bug 17314: Make TestBuilder set good defaults for Suggestions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120134 [details] [review] Bug 17314: Unit tests
Created attachment 120135 [details] [review] Bug 17314: OpenAPI spec Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120136 [details] [review] Bug 17314: Implement /suggestions routes This patch introduces routes to handle purchase suggestions, from the staff POV. Tests are added as well. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/suggestions.t => SUCCESS: Tests pass! And they are meaningful! 3. Play with your favourite REST tool (Postman?) 4. Sign off :-D
Hi Thomas, Tests are failing! I also tried in "real" with rester, although I get error 500, status property in the reply is 201 and the suggestion is created in Koha : <pre> { "errors": [ { "message": "/anyOf Expected string/null - got scalar.", "path": "/date_created" } ], "status": 201 } </pre> Tests : <pre> root@kohadevbox:koha(bz17314)$ prove t/db_dependent/api/v1/suggestions.t t/db_dependent/api/v1/suggestions.t .. 2/5 # Failed test 'SWAGGER3.2.1' # at t/db_dependent/api/v1/suggestions.t line 211. # got: '500' # expected: '201' # Looks like you planned 15 tests but ran 8. # Looks like you failed 1 test of 8 run. t/db_dependent/api/v1/suggestions.t .. 3/5 # Failed test 'add() tests' # at t/db_dependent/api/v1/suggestions.t line 243. Can't call method "to_api" on an undefined value at t/db_dependent/api/v1/suggestions.t line 219. # Looks like your test exited with 255 just after 3. t/db_dependent/api/v1/suggestions.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 3/5 subtests Test Summary Report ------------------- t/db_dependent/api/v1/suggestions.t (Wstat: 65280 Tests: 3 Failed: 1) Failed test: 3 Non-zero exit status: 255 Parse errors: Bad plan. You planned 5 tests but ran 3. Files=1, Tests=3, 7 wallclock secs ( 0.02 usr 0.00 sys + 6.05 cusr 0.61 csys = 6.68 CPU) Result: FAIL </pre>
Created attachment 125097 [details] [review] Bug 17314: Make TestBuilder set good defaults for Suggestions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125098 [details] [review] Bug 17314: Unit tests
Created attachment 125099 [details] [review] Bug 17314: OpenAPI spec Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125100 [details] [review] Bug 17314: Implement /suggestions routes This patch introduces routes to handle purchase suggestions, from the staff POV. Tests are added as well. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/suggestions.t => SUCCESS: Tests pass! And they are meaningful! 3. Play with your favourite REST tool (Postman?) 4. Sign off :-D
Created attachment 125101 [details] [review] Bug 17314: Make TestBuilder set good defaults for Suggestions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125102 [details] [review] Bug 17314: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125103 [details] [review] Bug 17314: OpenAPI spec Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125104 [details] [review] Bug 17314: Implement /suggestions routes This patch introduces routes to handle purchase suggestions, from the staff POV. Tests are added as well. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/suggestions.t => SUCCESS: Tests pass! And they are meaningful! 3. Play with your favourite REST tool (Postman?) 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
* Agree with field names * Tested manually and all works as expected * Tests pass * QA script passes Going straight for PQA.
+ # FIXME: This should be handled in Koha::Suggestion->store + $body->{'status'} = 'ASKED' + unless defined $body->{'status'}; Please open a separate bug report for that.
suggestions.suggestedby - Koha is setting it to the logged in user. Shouldn't we do the same here?
Pushed to master for 21.11, thanks to everybody involved!
Great \o/ thanks everyone! Hope this one can go to stable and old-stable as well :)
> Hope this one can go to stable and old-stable as well :) Do you confirm that it's not changing any behavior somewhere else? Like it's just adding routes, right?
(In reply to Victor Grousset/tuxayo from comment #81) > > Hope this one can go to stable and old-stable as well :) > > Do you confirm that it's not changing any behavior somewhere else? Like it's > just adding routes, right? Yes, it just adds routes to make the Suggestions accessible from the API.
"Just add routes" is not as trivial as it sounds. We could introduce permission issues.
Pushed to 21.05.x for 21.05.05
(In reply to Jonathan Druart from comment #83) > "Just add routes" is not as trivial as it sounds. We could introduce > permission issues. Indeed! I just checked and the tests check unauthorized accesses cases. Is that enough to not worry about issues in case the way permissions work between master and 21.05 and 20.11 has changed?
I just checked by git logging into C4/Auth.pm, and reading commits messages, I don't see significant changes nor breaking changes about how permissions works since 20.11.
Ah patches does not apply in 20.11.x because of Bug 28189 Can you try to provide rebased patches ?
Created attachment 127533 [details] [review] Bug 17314: Make TestBuilder set good defaults for Suggestions [20.11.x] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 127534 [details] [review] Bug 17314: Unit tests [20.11.x] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 127535 [details] [review] Bug 17314: OpenAPI spec [20.11.x] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 127536 [details] [review] Bug 17314: Implement /suggestions routes [20.11.x] This patch introduces routes to handle purchase suggestions, from the staff POV. Tests are added as well. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/suggestions.t => SUCCESS: Tests pass! And they are meaningful! 3. Play with your favourite REST tool (Postman?) 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I added a missing dependency, bug 26636. Patches for both this bug and its dependency have been adapted/rebased to 20.11.x. Bug 26636 itself is pretty self-contained and I confider it harmless. The worst thing that could happen is that tests can fail (they don't) as the helper it adds is only used on this bug (for 20.11.x). And this bug is pretty self-contained too.