Description
Jonathan Druart
2023-03-01 10:01:02 UTC
Created attachment 149509 [details] [review] Bug 33105: DB changes Created attachment 149510 [details] [review] Bug 33105: DBIC schema changes Created attachment 149511 [details] [review] Bug 33105: DBIC specific Created attachment 149512 [details] [review] Bug 33105: Add Koha object classes Created attachment 149513 [details] [review] Bug 33105: Add vendor issues This patch is the main patch of this patch set, it contains the controller acqui/vendor_issues.pl, its corresponding template, and some links to this script. It adds: * A new DB table aqbookseller_issues linked with the aqbooksellers table * A new subpermission acquisition.issue_manage * A new authorised value category VENDOR_ISSUE_TYPE and two examples MAINTENANCE and OUTAGE * A new controller couple acqui/vendor_issues.[pl,tt] Test plan: 0. Apply the patches, run updatedatabase and restart_all 1. Go to the acquisition module, create a new vendor or use an existing one 2. Create a couple of issues for this vendor 3. Edit/Delete and search for those issues This is the basics for tracking issues with vendors. Suggestions welcome, on follow-up bug reports. Created attachment 149514 [details] [review] Bug 33105: REST API specs Created attachment 149515 [details] [review] Bug 33105: Add tests Deletion is currently failing due to a typo in vendor_issues.pl cgi-bin/koha/acqui/vendor_iddues.pl?op=delete_confirm&issue_id=1 iddues instead of issues. Looking good otherwise! Created attachment 149530 [details] [review] Bug 33105: Add vendor issues This patch is the main patch of this patch set, it contains the controller acqui/vendor_issues.pl, its corresponding template, and some links to this script. It adds: * A new DB table aqbookseller_issues linked with the aqbooksellers table * A new subpermission acquisition.issue_manage * A new authorised value category VENDOR_ISSUE_TYPE and two examples MAINTENANCE and OUTAGE * A new controller couple acqui/vendor_issues.[pl,tt] Test plan: 0. Apply the patches, run updatedatabase and restart_all 1. Go to the acquisition module, create a new vendor or use an existing one 2. Create a couple of issues for this vendor 3. Edit/Delete and search for those issues This is the basics for tracking issues with vendors. Suggestions welcome, on follow-up bug reports. Created attachment 149531 [details] [review] Bug 33105: REST API specs Created attachment 149532 [details] [review] Bug 33105: Add tests (In reply to Jonathan Field from comment #8) > Deletion is currently failing due to a typo in vendor_issues.pl oops, should be fixed now! Created attachment 149556 [details] [review] Bug 33105: DB changes Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 149557 [details] [review] Bug 33105: DBIC schema changes Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 149558 [details] [review] Bug 33105: DBIC specific Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 149559 [details] [review] Bug 33105: Add Koha object classes Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 149560 [details] [review] Bug 33105: Add vendor issues This patch is the main patch of this patch set, it contains the controller acqui/vendor_issues.pl, its corresponding template, and some links to this script. It adds: * A new DB table aqbookseller_issues linked with the aqbooksellers table * A new subpermission acquisition.issue_manage * A new authorised value category VENDOR_ISSUE_TYPE and two examples MAINTENANCE and OUTAGE * A new controller couple acqui/vendor_issues.[pl,tt] Test plan: 0. Apply the patches, run updatedatabase and restart_all 1. Go to the acquisition module, create a new vendor or use an existing one 2. Create a couple of issues for this vendor 3. Edit/Delete and search for those issues This is the basics for tracking issues with vendors. Suggestions welcome, on follow-up bug reports. Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 149561 [details] [review] Bug 33105: REST API specs Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 149562 [details] [review] Bug 33105: Add tests Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 152894 [details] [review] Bug 33105: DB changes Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 152895 [details] [review] Bug 33105: Add Koha object classes Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 152896 [details] [review] Bug 33105: Add vendor issues This patch is the main patch of this patch set, it contains the controller acqui/vendor_issues.pl, its corresponding template, and some links to this script. It adds: * A new DB table aqbookseller_issues linked with the aqbooksellers table * A new subpermission acquisition.issue_manage * A new authorised value category VENDOR_ISSUE_TYPE and two examples MAINTENANCE and OUTAGE * A new controller couple acqui/vendor_issues.[pl,tt] Test plan: 0. Apply the patches, run updatedatabase and restart_all 1. Go to the acquisition module, create a new vendor or use an existing one 2. Create a couple of issues for this vendor 3. Edit/Delete and search for those issues This is the basics for tracking issues with vendors. Suggestions welcome, on follow-up bug reports. Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 152897 [details] [review] Bug 33105: REST API specs Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 152898 [details] [review] Bug 33105: Add tests Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Created attachment 152899 [details] [review] Bug 33105: [DBIx] Schema changes Former patches did not apply anymore. Refreshed them. Created attachment 152900 [details] [review] Bug 33105: DBIC specific Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Not able to finish now. Rebased/refreshed dbix stuff. (In reply to Marcel de Rooy from comment #27) > Not able to finish now. Rebased/refreshed dbix stuff. Continuing here acqui/vendor_issues.pl demonstrates a point that is larger than this script: $issue = Koha::Acquisition::Bookseller::Issues->find($issue_id); We have a name clash with the issues table, so it would have been nicer to specify vendor_issue here in naming variables. No blocker just noting. vendor_id: type: - string started_on: type: - string ended_on: type: - string Was expecting a numeric id and dates ? Plural or singular? Confusion ! + vendor_issue: + $ref: ./definitions/vendor_issue.yaml + "/acquisitions/vendors/{vendor_id}/issues": + $ref: "./paths/acquisitions_vendor_issues.yaml#/~1acquisitions~1vendors~1{vendor_id}~1issues" Wny vendor and acquisition_vendor btw ? TODO kohastructure Can't call method "issues" on an undefined value at /usr/share/koha/acqui/vendor_issues.pl line 103. at /usr/share/koha/acqui/vendor_issues.pl line 102 When there is no vendor. Tried to use output_and_exit. But it needs more attention. Created attachment 153117 [details] [review] Bug 33105: (QA follow-up) Small changes in dbrev Created attachment 153118 [details] [review] Bug 33105: (QA follow-up) Changes for Koha objects Created attachment 153119 [details] [review] Bug 33105: (QA follow-up) REST API Includes removing q_header. Inconsistent use of quotes for response codes. This is not ready yet. Wonder why this got signed off? Created attachment 153122 [details] [review] Bug 33105: Fix vendor api spec vendor ID must be integer and date attributes should specify the format accordingly. Created attachment 153123 [details] [review] Bug 33105: Redirect to 404 if no vendor exists (In reply to Marcel de Rooy from comment #29) > acqui/vendor_issues.pl demonstrates a point that is larger than this script: > $issue = Koha::Acquisition::Bookseller::Issues->find($issue_id); > We have a name clash with the issues table, so it would have been nicer to > specify vendor_issue here in naming variables. > No blocker just noting. well, 'issue' in the code should be checkout when it's about checkouts. If you see $vendor->issue you know given the context that it's about vendor's issues, not checkout. I can rename if you want but I don't think it's needed. (In reply to Marcel de Rooy from comment #30) > vendor_id: > type: > - string > started_on: > type: > - string > ended_on: > type: > - string > > Was expecting a numeric id and dates ? vendor_id must be integer, yes. dates have a type=string. Could have a format=date however, I totally overlooked that. Done in commit "Fix vendor api spec" (In reply to Marcel de Rooy from comment #31) > Plural or singular? Confusion ! Where? vendor_issue.yaml is the definition file for a given issue, hence the singular. acquisitions_vendor_issues.yaml is the path for the /issues endpoint. It's following the convention. > + vendor_issue: > + $ref: ./definitions/vendor_issue.yaml > + "/acquisitions/vendors/{vendor_id}/issues": > + $ref: > "./paths/acquisitions_vendor_issues.yaml#/ > ~1acquisitions~1vendors~1{vendor_id}~1issues" > > Wny vendor and acquisition_vendor btw ? "acquisitions_vendor_issues" to copy "acquisitions_orders.yaml" and keep consistency. Same for the definition file: vendor.yaml vendor_alias.yaml vendor_issue.yaml order.yaml We could/should all prefixed them with 'acquisitions', but that's not for this bug. (In reply to Marcel de Rooy from comment #32) > TODO kohastructure What's missing there? (In reply to Marcel de Rooy from comment #33) > Can't call method "issues" on an undefined value at > /usr/share/koha/acqui/vendor_issues.pl line 103. at > /usr/share/koha/acqui/vendor_issues.pl line 102 > When there is no vendor. Tried to use output_and_exit. But it needs more > attention. Done in "Redirect to 404 if no vendor exists" (In reply to Marcel de Rooy from comment #37) > This is not ready yet. Wonder why this got signed off? Because it was working according to the test plan? :) Thanks for the follow-ups, they all make sense! Revisiting > (In reply to Marcel de Rooy from comment #37)
> > This is not ready yet. Wonder why this got signed off?
>
> Because it was working according to the test plan? :)
Hmm. A test plan can be incomplete.
Here is another problem:
Add an issue. Delete it. Say no. Response is 404. That is not good.
Created attachment 153125 [details] [review] Bug 33105: Redirect to 404 if no vendor exists (In reply to Jonathan Druart from comment #40) > (In reply to Marcel de Rooy from comment #32) > > TODO kohastructure > > What's missing there? > Nothing. Now I remember that I was searching for vendor_issues in it. And it should be aqbookseller_issues, so nice ;) REST API problem /api/v1/acquisitions/vendors/7/issues is working fine (you use it on the form) but /api/v1/acquisitions/vendors/7/issues/4 or the single GET does not work. Responds with 404. Same problem for DELETE via REST API Created attachment 153126 [details] [review] Bug 33105: DB changes Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153127 [details] [review] Bug 33105: Add Koha object classes Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153128 [details] [review] Bug 33105: Add vendor issues This patch is the main patch of this patch set, it contains the controller acqui/vendor_issues.pl, its corresponding template, and some links to this script. It adds: * A new DB table aqbookseller_issues linked with the aqbooksellers table * A new subpermission acquisition.issue_manage * A new authorised value category VENDOR_ISSUE_TYPE and two examples MAINTENANCE and OUTAGE * A new controller couple acqui/vendor_issues.[pl,tt] Test plan: 0. Apply the patches, run updatedatabase and restart_all 1. Go to the acquisition module, create a new vendor or use an existing one 2. Create a couple of issues for this vendor 3. Edit/Delete and search for those issues This is the basics for tracking issues with vendors. Suggestions welcome, on follow-up bug reports. Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153129 [details] [review] Bug 33105: REST API specs Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153130 [details] [review] Bug 33105: Add tests Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153131 [details] [review] Bug 33105: [DBIx] Schema changes Former patches did not apply anymore. Refreshed them. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153132 [details] [review] Bug 33105: DBIC specific Signed-off-by: Jonathan Field <jonathan.fieeld@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153133 [details] [review] Bug 33105: (QA follow-up) Small changes in dbrev Similar edit in kohastructure added. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153134 [details] [review] Bug 33105: (QA follow-up) Changes for Koha objects Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153135 [details] [review] Bug 33105: (QA follow-up) REST API Includes removing q_header. Inconsistent use of quotes for response codes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153136 [details] [review] Bug 33105: Fix vendor api spec vendor ID must be integer and date attributes should specify the format accordingly. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153137 [details] [review] Bug 33105: Redirect to 404 if no vendor exists Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Not going to block this, but we added the 'OPAC concerns' and now the 'vendor issues'. Should we think of a more generic approach? (In reply to Tomás Cohen Arazi from comment #59) > Not going to block this, but we added the 'OPAC concerns' and now the > 'vendor issues'. Should we think of a more generic approach? Hum I don't think so, they are two completely different things. One is linked with the acquisition module for external entities, OPAC concerns is for OPAC users. I don't see how we could merge them. (In reply to Jonathan Druart from comment #60) > (In reply to Tomás Cohen Arazi from comment #59) > > Not going to block this, but we added the 'OPAC concerns' and now the > > 'vendor issues'. Should we think of a more generic approach? > > Hum I don't think so, they are two completely different things. One is > linked with the acquisition module for external entities, OPAC concerns is > for OPAC users. I don't see how we could merge them. Besides the UI differences, I see 'ticket type X' and 'ticket type Y'. I had the same concerns about the 'OPAC record concerns ' feature as being too specific by design. (In reply to Tomás Cohen Arazi from comment #61) > (In reply to Jonathan Druart from comment #60) > > (In reply to Tomás Cohen Arazi from comment #59) > > > Not going to block this, but we added the 'OPAC concerns' and now the > > > 'vendor issues'. Should we think of a more generic approach? > > > > Hum I don't think so, they are two completely different things. One is > > linked with the acquisition module for external entities, OPAC concerns is > > for OPAC users. I don't see how we could merge them. > > Besides the UI differences, I see 'ticket type X' and 'ticket type Y'. I had > the same concerns about the 'OPAC record concerns ' feature as being too > specific by design. The table name was not specific: "tickets" :D (In reply to Jonathan Druart from comment #62) > (In reply to Tomás Cohen Arazi from comment #61) > > (In reply to Jonathan Druart from comment #60) > > > (In reply to Tomás Cohen Arazi from comment #59) > > > > Not going to block this, but we added the 'OPAC concerns' and now the > > > > 'vendor issues'. Should we think of a more generic approach? > > > > > > Hum I don't think so, they are two completely different things. One is > > > linked with the acquisition module for external entities, OPAC concerns is > > > for OPAC users. I don't see how we could merge them. > > > > Besides the UI differences, I see 'ticket type X' and 'ticket type Y'. I had > > the same concerns about the 'OPAC record concerns ' feature as being too > > specific by design. > > The table name was not specific: "tickets" :D The table name got the right name, that's true. But we have concerns.pl concerns.tt and all the tools are named 'catalog_concern.*'. Including the admin pages. Not complaining, just saying that we might need to think of a more general approach at some point. I think I left follow-up bugs to generalise that.. I was heading towards a more generalised enquiry management flow but I had to also keep the sponsor happy.. hense the API design trying hard to generalise and then my aim was to migrate to vue and a new 'enquiries' module once some of the vue stuff had settled (at the time I coded catalogue concerns, we'd not made anywhere near the progress in vue). I'd love to see a much more generalise enquiries/workflows entity and am happy to contribute in that space.. we're currently working on sponsorship for it here at ptfs-e. (In reply to Jonathan Druart from comment #60) > (In reply to Tomás Cohen Arazi from comment #59) > > Not going to block this, but we added the 'OPAC concerns' and now the > > 'vendor issues'. Should we think of a more generic approach? > > Hum I don't think so, they are two completely different things. One is > linked with the acquisition module for external entities, OPAC concerns is > for OPAC users. I don't see how we could merge them. A generic approach would only make sense tome if the workflow around such concerns and issues is kind of the same. The fact that we have e.g. lists and patron lists does not mean that they should be merged in one table. Etc. I feel like the 'OPAC problems' and 'Catalog concerns' are a closer match than the vendor issues. At the moment the vendor issues are not about communicating problems, but about tracking them, while the other two features are both about reporting something to the library with the ability to reply/resolve. I think it would make sense merging them first and leave this be. Pushed to master for 23.11. Nice work everyone, thanks! Created attachment 153730 [details] [review] Bug 33105: (follow-up) DB changes boooh! Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 153731 [details] [review] Bug 33105: Fix Auth/Permissions.t (In reply to Jonathan Druart from comment #68) > Created attachment 153730 [details] [review] [review] > Bug 33105: (follow-up) DB changes > > boooh! > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> This has been pushed. (In reply to Jonathan Druart from comment #69) > Created attachment 153731 [details] [review] [review] > Bug 33105: Fix Auth/Permissions.t Still need to be pushed. I am not sure those tests are really useful... (In reply to Jonathan Druart from comment #71) > (In reply to Jonathan Druart from comment #69) > > Created attachment 153731 [details] [review] [review] [review] > > Bug 33105: Fix Auth/Permissions.t > > Still need to be pushed. > > I am not sure those tests are really useful... Pushed, thanks. New feature not pushed to 23.05.x |