https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Jiri Kozlovsky <mail@jkozlovsky.cz> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16652 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16652 [Bug 16652] Omnibus: RestfulAPI supporting services for Vufind and for xnciptoolkit drivers -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Jiri Kozlovsky <mail@jkozlovsky.cz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion --- Comment #2 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Jiri Kozlovsky <mail@jkozlovsky.cz> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |mail@jkozlovsky.cz |ity.org | Status|In Discussion |ASSIGNED --- Comment #4 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- (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. 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #5 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- Created attachment 58098 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=58098&action=edit Implemented suggestions REST API -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #6 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- (In reply to Jiri Kozlovsky from comment #5)

Created attachment 58098 [details] [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 ! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Jiri Kozlovsky <mail@jkozlovsky.cz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #58100|0 |1 is obsolete| | --- Comment #8 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- Created attachment 58101 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=58101&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #10 from Josef Moravec <josef.moravec@gmail.com> --- 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' -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #11 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- Created attachment 58993 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=58993&action=edit QA followup test plan unchanged -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #13 from Josef Moravec <josef.moravec@gmail.com> --- Also, unit test are needed I think -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #15 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #17 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- Created attachment 60133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60133&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #18 from Josef Moravec <josef.moravec@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Web services |REST api CC| |jonathan.druart@bugs.koha-c | |ommunity.org QA Contact|testopia@bugs.koha-communit | |y.org | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #21 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #58101|0 |1 is obsolete| | Attachment #58993|0 |1 is obsolete| | Attachment #60133|0 |1 is obsolete| | --- Comment #22 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 90171 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90171&action=edit Implemented suggestions REST API -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #23 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 90172 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90172&action=edit Bug 17314 : Migration from Swagger2 to OpenApi -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #24 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #26 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |tomascohen@gmail.com --- Comment #28 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #30 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 91545 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91545&action=edit Bug 17314 : Added _to_model and _to_api methods -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91545|0 |1 is obsolete| | --- Comment #32 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 91546 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91546&action=edit Bug 17314 : Added _to_model and _to_api methods and changed attirbutes naming -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #34 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90176|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90171|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #35 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 93761 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93761&action=edit Bug 17314: Routes to handle purchase suggestions -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #37 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 93763 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93763&action=edit Bug 17314: QA Followup Test plan unchanged -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|REST API: Add API route to |Routes to create, list and |create, list and delete a |delete a purchase |purchase suggestion |suggestion -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #40 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 93765 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93765&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #41 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #43 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #45 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 109973 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109973&action=edit Bug 17314: Migration from Swagger2 to OpenApi This patch migrates the original implementation so it uses the new OpenAPI plugin. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #47 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 109975 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109975&action=edit Bug 17314: Added _to_model and _to_api methods and changed attirbutes naming -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #49 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 109977 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109977&action=edit Bug 17314: (QA follow-up) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Needs Signoff |Patch doesn't apply --- Comment #52 from Victor Grousset/tuxayo <victor@tuxayo.net> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #109972|0 |1 is obsolete| | Attachment #109973|0 |1 is obsolete| | Attachment #109974|0 |1 is obsolete| | Attachment #109975|0 |1 is obsolete| | Attachment #109976|0 |1 is obsolete| | Attachment #109977|0 |1 is obsolete| | --- Comment #53 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 114113 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114113&action=edit Bug 17314: Routes to handle purchase suggestions -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #55 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 114115 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114115&action=edit Bug 17314: (QA follow-up) Test plan unchanged -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #57 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 114117 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114117&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #59 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- does patch still apply? anyone willing to test? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114113|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114115|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114117|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #61 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120111|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #63 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120133&action=edit Bug 17314: Make TestBuilder set good defaults for Suggestions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #65 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120135 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120135&action=edit Bug 17314: OpenAPI spec Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #67 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120133|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120135|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #69 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 125098 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125098&action=edit Bug 17314: Unit tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #71 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 125100 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125100&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #125097|0 |1 is obsolete| | --- Comment #72 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 125101 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125101&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #125099|0 |1 is obsolete| | --- Comment #74 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 125103 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125103&action=edit Bug 17314: OpenAPI spec Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #76 from 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 jmbroust <jean-manuel.broust@univ-lyon2.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jean-manuel.broust@univ-lyo | |n2.fr -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #78 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- suggestions.suggestedby - Koha is setting it to the logged in user. Shouldn't we do the same here? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #79 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #82 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.11.00 |21.11.00,21.05.05 released in| | --- Comment #84 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.05 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #85 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Depends on| |28189 --- Comment #87 from Fridolin Somers <fridolin.somers@biblibre.com> --- Ah patches does not apply in 20.11.x because of Bug 28189 Can you try to provide rebased patches ? Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28189 [Bug 28189] Move the base swagger file to YAML -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #88 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 127533 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127533&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #90 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 127535 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127535&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17314 --- Comment #92 from Tomás Cohen Arazi <tomascohen@gmail.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. -- You are receiving this mail because: You are watching all bug changes.