Bug 17314 - Routes to create, list and delete a purchase suggestion
Summary: Routes to create, list and delete a purchase suggestion
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on: 26636 28189
Blocks: 16652
  Show dependency treegraph
 
Reported: 2016-09-18 14:59 UTC by Jiri Kozlovsky
Modified: 2022-06-06 20:27 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05


Attachments
Implemented suggestions REST API (26.09 KB, patch)
2016-12-10 20:22 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Implemented suggestions REST API (26.09 KB, patch)
2016-12-11 22:13 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Implemented suggestions REST API (26.10 KB, patch)
2016-12-11 22:16 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
QA followup (2.04 KB, patch)
2017-01-14 22:26 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Bug 17314 - QA followup 2nd (5.00 KB, patch)
2017-02-12 17:48 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Implemented suggestions REST API (24.71 KB, patch)
2019-05-28 23:58 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314 : Migration from Swagger2 to OpenApi (14.92 KB, patch)
2019-05-28 23:59 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314 : QA Followup (4.23 KB, patch)
2019-05-29 08:59 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314 : Added _to_model and _to_api methods (4.51 KB, patch)
2019-07-16 10:26 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314 : Added _to_model and _to_api methods and changed attirbutes naming (8.99 KB, patch)
2019-07-16 10:37 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: Routes to handle purchase suggestions (24.69 KB, patch)
2019-10-04 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Migration from Swagger2 to OpenApi (14.99 KB, patch)
2019-10-04 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: QA Followup (4.23 KB, patch)
2019-10-04 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314 : Added _to_model and _to_api methods and changed attirbutes naming (8.98 KB, patch)
2019-10-04 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: (QA follow-up) Adjust spec (6.89 KB, patch)
2019-10-04 20:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Routes to handle purchase suggestions (24.83 KB, patch)
2020-08-17 07:28 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: Routes to handle purchase suggestions (24.85 KB, patch)
2020-09-12 00:29 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: Migration from Swagger2 to OpenApi (15.00 KB, patch)
2020-09-12 00:29 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: (QA follow-up) (4.23 KB, patch)
2020-09-12 00:30 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: Added _to_model and _to_api methods and changed attirbutes naming (8.99 KB, patch)
2020-09-12 00:30 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: (QA follow-up) Adjust spec (6.90 KB, patch)
2020-09-12 00:30 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: (QA follow-up) (20.51 KB, patch)
2020-09-12 00:30 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: Routes to handle purchase suggestions (24.85 KB, patch)
2020-12-02 20:52 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: Migration from Swagger2 to OpenApi (15.00 KB, patch)
2020-12-02 20:53 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: (QA follow-up) (4.23 KB, patch)
2020-12-02 20:53 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: Added _to_model and _to_api methods and changed attirbutes naming (8.99 KB, patch)
2020-12-02 20:53 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: (QA follow-up) Adjust spec (6.90 KB, patch)
2020-12-02 20:53 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: (QA follow-up) (20.51 KB, patch)
2020-12-02 20:53 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17314: Add routes for purchase suggestions (21.28 KB, patch)
2021-04-23 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Make TestBuilder set good defaults for Suggestions (780 bytes, patch)
2021-04-23 17:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Unit tests (11.69 KB, patch)
2021-04-23 17:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: OpenAPI spec (14.95 KB, patch)
2021-04-23 17:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Implement /suggestions routes (7.01 KB, patch)
2021-04-23 17:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Make TestBuilder set good defaults for Suggestions (781 bytes, patch)
2021-09-21 12:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Unit tests (11.68 KB, patch)
2021-09-21 12:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: OpenAPI spec (13.40 KB, patch)
2021-09-21 12:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Implement /suggestions routes (7.51 KB, patch)
2021-09-21 12:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Make TestBuilder set good defaults for Suggestions (848 bytes, patch)
2021-09-21 12:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17314: Unit tests (11.75 KB, patch)
2021-09-21 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17314: OpenAPI spec (13.48 KB, patch)
2021-09-21 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17314: Implement /suggestions routes (7.58 KB, patch)
2021-09-21 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17314: Make TestBuilder set good defaults for Suggestions [20.11.x] (857 bytes, patch)
2021-11-11 11:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Unit tests [20.11.x] (11.76 KB, patch)
2021-11-11 11:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: OpenAPI spec [20.11.x] (12.95 KB, patch)
2021-11-11 11:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17314: Implement /suggestions routes [20.11.x] (7.58 KB, patch)
2021-11-11 11:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jiri Kozlovsky 2016-09-18 14:59:45 UTC
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
Comment 1 Katrin Fischer 2016-09-18 15:13:23 UTC
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?
Comment 2 Jiri Kozlovsky 2016-09-18 15:32:01 UTC
(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.
Comment 3 Katrin Fischer 2016-09-18 15:49:28 UTC
> > - 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?
Comment 4 Jiri Kozlovsky 2016-12-04 23:40:22 UTC
(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.
Comment 5 Jiri Kozlovsky 2016-12-10 20:22:55 UTC
Created attachment 58098 [details] [review]
Implemented suggestions REST API
Comment 6 Jiri Kozlovsky 2016-12-10 22:28:16 UTC
(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 !
Comment 7 Jiri Kozlovsky 2016-12-11 22:13:15 UTC
Created attachment 58100 [details] [review]
Implemented suggestions REST API

Added HTTP Codes for remaining errors
Comment 8 Jiri Kozlovsky 2016-12-11 22:16:11 UTC
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.
Comment 9 Josef Moravec 2017-01-11 08:50:34 UTC
Started testing...

For first koha-qa complains:
 FAIL	Koha/REST/V1/Suggestions.pm
   FAIL	  valid
		Smartmatch is experimental
Comment 10 Josef Moravec 2017-01-11 08:53:08 UTC
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'
Comment 11 Jiri Kozlovsky 2017-01-14 22:26:12 UTC
Created attachment 58993 [details] [review]
QA followup

test plan unchanged
Comment 12 Josef Moravec 2017-01-17 19:45:18 UTC
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
Comment 13 Josef Moravec 2017-01-17 19:47:13 UTC
Also, unit test are needed I think
Comment 14 Jiri Kozlovsky 2017-02-12 16:53:39 UTC
> 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.
Comment 15 Jiri Kozlovsky 2017-02-12 17:12:48 UTC
(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.
Comment 16 Jiri Kozlovsky 2017-02-12 17:19:21 UTC
(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.
Comment 17 Jiri Kozlovsky 2017-02-12 17:48:27 UTC
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
Comment 18 Josef Moravec 2017-04-27 11:40:16 UTC
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.
Comment 19 Jon Knight 2017-08-07 17:01:07 UTC
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?
Comment 20 Jon Knight 2017-11-01 11:14:05 UTC
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?
Comment 21 Jiri Kozlovsky 2017-11-04 20:27:34 UTC
(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.
Comment 22 Arthur Suzuki 2019-05-28 23:58:04 UTC
Created attachment 90171 [details] [review]
Implemented suggestions REST API
Comment 23 Arthur Suzuki 2019-05-28 23:59:50 UTC
Created attachment 90172 [details] [review]
Bug 17314 : Migration from Swagger2 to OpenApi
Comment 24 Jonathan Druart 2019-05-29 00:25:55 UTC
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.
Comment 25 Arthur Suzuki 2019-05-29 08:59:32 UTC
Created attachment 90176 [details] [review]
Bug 17314 : QA Followup

Test plan unchanged
Comment 26 Arthur Suzuki 2019-05-29 09:18:04 UTC
(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!
Comment 27 Arthur Suzuki 2019-05-29 23:44:20 UTC
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)
Comment 28 Tomás Cohen Arazi 2019-06-10 15:45:59 UTC
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
Comment 29 Arthur Suzuki 2019-07-15 11:49:40 UTC
Added RFC :
https://wiki.koha-community.org/wiki/Suggestions_endpoint_RFC

Work still in progress to make the patch conform with RFC.
Comment 30 Arthur Suzuki 2019-07-16 10:26:05 UTC
Created attachment 91545 [details] [review]
Bug 17314 : Added _to_model and _to_api methods
Comment 31 Michal Denar 2019-07-16 10:29:31 UTC
Hi Arthur,
nice to see progress in this bug. I'm ready to test :-)
Comment 32 Arthur Suzuki 2019-07-16 10:37:33 UTC
Created attachment 91546 [details] [review]
Bug 17314 : Added _to_model and _to_api methods and changed attirbutes naming
Comment 33 Arthur Suzuki 2019-07-16 10:40:34 UTC
Hi Michal,
great to see your interest! feel free to test and let me know :)
Comment 34 Tomás Cohen Arazi 2019-09-10 21:13:46 UTC
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!
Comment 35 Tomás Cohen Arazi 2019-10-04 19:45:09 UTC
Created attachment 93761 [details] [review]
Bug 17314: Routes to handle purchase suggestions
Comment 36 Tomás Cohen Arazi 2019-10-04 19:45:14 UTC
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.
Comment 37 Tomás Cohen Arazi 2019-10-04 19:45:18 UTC
Created attachment 93763 [details] [review]
Bug 17314: QA Followup

Test plan unchanged
Comment 38 Tomás Cohen Arazi 2019-10-04 19:45:22 UTC
Created attachment 93764 [details] [review]
Bug 17314 : Added _to_model and _to_api methods and changed attirbutes naming
Comment 39 Tomás Cohen Arazi 2019-10-04 19:46:04 UTC
Rebased it on top of current master.
Comment 40 Tomás Cohen Arazi 2019-10-04 20:01:46 UTC
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
Comment 41 Arthur Suzuki 2020-07-07 00:48:24 UTC
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>
Comment 42 Arthur Suzuki 2020-08-17 07:28:43 UTC
Created attachment 108380 [details] [review]
Bug 17314: Routes to handle purchase suggestions
Comment 43 Arthur Suzuki 2020-08-17 09:03:20 UTC
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>
Comment 44 Arthur Suzuki 2020-09-12 00:29:34 UTC
Created attachment 109972 [details] [review]
Bug 17314: Routes to handle purchase suggestions
Comment 45 Arthur Suzuki 2020-09-12 00:29:55 UTC
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.
Comment 46 Arthur Suzuki 2020-09-12 00:30:03 UTC
Created attachment 109974 [details] [review]
Bug 17314: (QA follow-up)

Test plan unchanged
Comment 47 Arthur Suzuki 2020-09-12 00:30:09 UTC
Created attachment 109975 [details] [review]
Bug 17314: Added _to_model and _to_api methods and changed attirbutes naming
Comment 48 Arthur Suzuki 2020-09-12 00:30:15 UTC
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
Comment 49 Arthur Suzuki 2020-09-12 00:30:22 UTC
Created attachment 109977 [details] [review]
Bug 17314: (QA follow-up)
Comment 50 Arthur Suzuki 2020-09-12 00:32:24 UTC
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.
Comment 51 Arthur Suzuki 2020-10-16 14:25:41 UTC
up? This one need QA :)
Comment 52 Victor Grousset/tuxayo 2020-11-27 16:10:55 UTC
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.
Comment 53 Arthur Suzuki 2020-12-02 20:52:51 UTC
Created attachment 114113 [details] [review]
Bug 17314: Routes to handle purchase suggestions
Comment 54 Arthur Suzuki 2020-12-02 20:53:09 UTC
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.
Comment 55 Arthur Suzuki 2020-12-02 20:53:16 UTC
Created attachment 114115 [details] [review]
Bug 17314: (QA follow-up)

Test plan unchanged
Comment 56 Arthur Suzuki 2020-12-02 20:53:23 UTC
Created attachment 114116 [details] [review]
Bug 17314: Added _to_model and _to_api methods and changed attirbutes naming
Comment 57 Arthur Suzuki 2020-12-02 20:53:33 UTC
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
Comment 58 Arthur Suzuki 2020-12-02 20:53:40 UTC
Created attachment 114118 [details] [review]
Bug 17314: (QA follow-up)
Comment 59 Arthur Suzuki 2021-01-04 15:18:24 UTC
does patch still apply? anyone willing to test?
Comment 60 David Nind 2021-01-25 22:22:31 UTC
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)
Comment 61 Tomás Cohen Arazi 2021-04-23 15:08:13 UTC
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.
Comment 62 Tomás Cohen Arazi 2021-04-23 15:15:15 UTC Comment hidden (obsolete)
Comment 63 Tomás Cohen Arazi 2021-04-23 17:54:55 UTC
Created attachment 120133 [details] [review]
Bug 17314: Make TestBuilder set good defaults for Suggestions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Tomás Cohen Arazi 2021-04-23 17:55:17 UTC
Created attachment 120134 [details] [review]
Bug 17314: Unit tests
Comment 65 Tomás Cohen Arazi 2021-04-23 17:55:27 UTC
Created attachment 120135 [details] [review]
Bug 17314: OpenAPI spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 66 Tomás Cohen Arazi 2021-04-23 17:55:35 UTC
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
Comment 67 Arthur Suzuki 2021-05-18 09:28:48 UTC
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>
Comment 68 Tomás Cohen Arazi 2021-09-21 12:35:44 UTC
Created attachment 125097 [details] [review]
Bug 17314: Make TestBuilder set good defaults for Suggestions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Tomás Cohen Arazi 2021-09-21 12:35:53 UTC
Created attachment 125098 [details] [review]
Bug 17314: Unit tests
Comment 70 Tomás Cohen Arazi 2021-09-21 12:35:59 UTC
Created attachment 125099 [details] [review]
Bug 17314: OpenAPI spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 71 Tomás Cohen Arazi 2021-09-21 12:36:05 UTC
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
Comment 72 Martin Renvoize 2021-09-21 12:39:57 UTC
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>
Comment 73 Martin Renvoize 2021-09-21 12:40:02 UTC
Created attachment 125102 [details] [review]
Bug 17314: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 74 Martin Renvoize 2021-09-21 12:40:08 UTC
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>
Comment 75 Martin Renvoize 2021-09-21 12:40:12 UTC
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>
Comment 76 Martin Renvoize 2021-09-21 12:42:18 UTC
* Agree with field names
* Tested manually and all works as expected
* Tests pass
* QA script passes

Going straight for PQA.
Comment 77 Jonathan Druart 2021-10-28 14:20:43 UTC
+    # FIXME: This should be handled in Koha::Suggestion->store
+    $body->{'status'} = 'ASKED'
+        unless defined $body->{'status'};

Please open a separate bug report for that.
Comment 78 Jonathan Druart 2021-10-28 14:27:57 UTC
suggestions.suggestedby - Koha is setting it to the logged in user.
Shouldn't we do the same here?
Comment 79 Jonathan Druart 2021-10-28 15:44:41 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 80 Arthur Suzuki 2021-10-28 16:04:51 UTC
Great \o/ thanks everyone!
Hope this one can go to stable and old-stable as well :)
Comment 81 Victor Grousset/tuxayo 2021-10-31 17:46:31 UTC
> 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?
Comment 82 Arthur Suzuki 2021-11-01 18:48:03 UTC
(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.
Comment 83 Jonathan Druart 2021-11-02 10:59:56 UTC
"Just add routes" is not as trivial as it sounds. We could introduce permission issues.
Comment 84 Kyle M Hall 2021-11-02 12:43:19 UTC
Pushed to 21.05.x for 21.05.05
Comment 85 Victor Grousset/tuxayo 2021-11-03 20:06:26 UTC
(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?
Comment 86 Arthur Suzuki 2021-11-07 21:22:03 UTC
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.
Comment 87 Fridolin Somers 2021-11-11 00:07:18 UTC
Ah patches does not apply in 20.11.x because of Bug 28189
Can you try to provide rebased patches ?
Comment 88 Tomás Cohen Arazi 2021-11-11 11:36:52 UTC
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>
Comment 89 Tomás Cohen Arazi 2021-11-11 11:37:03 UTC
Created attachment 127534 [details] [review]
Bug 17314: Unit tests [20.11.x]

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 90 Tomás Cohen Arazi 2021-11-11 11:37:10 UTC
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>
Comment 91 Tomás Cohen Arazi 2021-11-11 11:37:17 UTC
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>
Comment 92 Tomás Cohen Arazi 2021-11-11 11:39:51 UTC
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.