Bug 18591

Summary: Allow an arbitrary number of comments on ILLs
Product: Koha Reporter: Magnus Enger <magnus>
Component: ILLAssignee: Magnus Enger <magnus>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: andrew.isherwood, gmcharlt, joe.mcglynn, katrin.fischer, kyle.m.hall, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on: 20995    
Bug Blocks: 20941    
Attachments: Bug 18591 - Allow any number of comments on ILLs
Bug 18591 - Database updates
Bug 18591 - Database updates
Update Schema - DO NOT PUSH
Bug 18591 - Allow any number of comments on ILLs
Bug 18591 - Database updates
Update Schema - DO NOT PUSH
Bug 18591 - Allow any number of comments on ILLs
Bug 18591 - Database updates
Bug 18591 - Allow any number of comments on ILLs
Update Schema - DO NOT PUSH
Bug 18591 - Database updates
Bug 18591 - Allow any number of comments on ILLs
Bug 18591: (follow-up) Add missing html filters
Bug 18591: (follow-up) Add more POD
Bug 18591: Database updates
Bug 18591: Allow any number of comments on ILLs
Bug 18591: (follow-up) Add missing html filters
Bug 18591: (QA follow-up) Fix merge error in kohastructure.sql
Bug 18591: (follow-up) Add more POD
Bug 18591: Database updates
Bug 18591: Allow any number of comments on ILLs
Bug 18591: (follow-up) Add missing html filters
Bug 18591: (follow-up) Add more POD

Description Magnus Enger 2017-05-12 11:53:28 UTC
Some libraries would like to be able to connect an arbitrary number of comments to each ILL request. This could be comments from the patron, messages between different librarians working on the same ILLS, comments about what other libraries have been tried before the current one etc. 

This would require a new table with fields like
- unique ID
- text of the comment
- borrowernumber of the librarian who made the comment
- timestamp

Maybe it would be possible to include comments from the OPAC too? Not sure if that makes sense.
Comment 1 Magnus Enger 2017-11-11 13:04:41 UTC
Two important things to decide on here: 
- Should it be possible to edit comments? 
- Should it be possible to delete comments? 

I lean towards "no" for both questions, because it makes the code simpler and it means the comments are historically correct.
Comment 2 Magnus Enger 2017-11-20 14:29:58 UTC Comment hidden (obsolete)
Comment 3 Magnus Enger 2017-11-22 12:48:43 UTC Comment hidden (obsolete)
Comment 4 Magnus Enger 2017-11-22 12:49:27 UTC Comment hidden (obsolete)
Comment 5 Magnus Enger 2017-11-22 12:49:32 UTC Comment hidden (obsolete)
Comment 6 Magnus Enger 2017-11-22 12:49:36 UTC Comment hidden (obsolete)
Comment 7 M. Tompsett 2018-03-27 00:40:21 UTC
Comment on attachment 69273 [details] [review]
Bug 18591 - Database updates

Review of attachment 69273 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/kohastructure.sql
@@ +4194,5 @@
> +    CONSTRAINT illcomments_ifk
> +      FOREIGN KEY (illrequest_id)
> +      REFERENCES illrequests ( illrequest_id )
> +      ON UPDATE CASCADE ON DELETE CASCADE
> +) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

utf8mb4, not just utf8. see bug 18336. I haven't tried yet, but this patch might not apply because of this.
Comment 8 Magnus Enger 2018-05-02 12:51:26 UTC
Created attachment 74964 [details] [review]
Bug 18591 - Database updates
Comment 9 Magnus Enger 2018-05-02 12:51:37 UTC
Created attachment 74965 [details] [review]
Update Schema - DO NOT PUSH
Comment 10 Magnus Enger 2018-05-02 12:51:48 UTC
Created attachment 74966 [details] [review]
Bug 18591 - Allow any number of comments on ILLs

This patch makes it possible to add arbitrary comments to ILL
requests. Comments are read only and displayed in chronological
order. Comments can be added by librarians, but also added automatically
based on comments in the different protocols, so that comments from the
lending library can also be added.

To test:
- Enable the ILL module ("ILLModule" syspref + config in koha-conf.xml)
- Install the Dummy backend from here:
  https://github.com/PTFS-Europe/Dummy
- Create a Dummy ILL request
- Add comments to the request, checking that the name and borrowernumber
  of the person that added the comment is displayed ok
- Check that the displayed date and time the comment was submitted follows
  the "dateformat" syspref
- On the "List requests" page, check that the correct number of comments
  is displayed in the "Comments" column
- Check that the tests pass:
  prove t/db_dependent/Illcomments.t
Comment 11 Magnus Enger 2018-05-02 12:53:32 UTC
Updated patches, using utf8mb4, are available now!
Comment 12 Andrew Isherwood 2018-09-27 14:49:50 UTC
Created attachment 79518 [details] [review]
Bug 18591 - Database updates

Signed-off-by: andrew.isherwood@ptfs-europe.com
Comment 13 Andrew Isherwood 2018-09-27 14:50:14 UTC
Created attachment 79519 [details] [review]
Bug 18591 - Allow any number of comments on ILLs

This patch makes it possible to add arbitrary comments to ILL
requests. Comments are read only and displayed in chronological
order. Comments can be added by librarians, but also added automatically
based on comments in the different protocols, so that comments from the
lending library can also be added.

To test:
- Enable the ILL module ("ILLModule" syspref + config in koha-conf.xml)
- Install the Dummy backend from here:
  https://github.com/PTFS-Europe/Dummy
- Create a Dummy ILL request
- Add comments to the request, checking that the name and borrowernumber
  of the person that added the comment is displayed ok
- Check that the displayed date and time the comment was submitted follows
  the "dateformat" syspref
- On the "List requests" page, check that the correct number of comments
  is displayed in the "Comments" column
- Check that the tests pass:
  prove t/db_dependent/Illcomments.t

Signed-off-by: andrew.isherwood@ptfs-europe.com
Comment 14 Andrew Isherwood 2018-10-05 13:07:02 UTC
Created attachment 80077 [details] [review]
Update Schema - DO NOT PUSH
Comment 15 Andrew Isherwood 2018-10-05 13:07:05 UTC
Created attachment 80078 [details] [review]
Bug 18591 - Database updates

Signed-off-by: andrew.isherwood@ptfs-europe.com
Comment 16 Andrew Isherwood 2018-10-05 13:07:09 UTC
Created attachment 80079 [details] [review]
Bug 18591 - Allow any number of comments on ILLs

This patch makes it possible to add arbitrary comments to ILL
requests. Comments are read only and displayed in chronological
order. Comments can be added by librarians, but also added automatically
based on comments in the different protocols, so that comments from the
lending library can also be added.

To test:
- Enable the ILL module ("ILLModule" syspref + config in koha-conf.xml)
- Install the Dummy backend from here:
  https://github.com/PTFS-Europe/Dummy
- Create a Dummy ILL request
- Add comments to the request, checking that the name and borrowernumber
  of the person that added the comment is displayed ok
- Check that the displayed date and time the comment was submitted follows
  the "dateformat" syspref
- On the "List requests" page, check that the correct number of comments
  is displayed in the "Comments" column
- Check that the tests pass:
  prove t/db_dependent/Illcomments.t

Signed-off-by: andrew.isherwood@ptfs-europe.com
Comment 17 Andrew Isherwood 2018-10-05 13:20:58 UTC
Created attachment 80081 [details] [review]
Bug 18591: (follow-up) Add missing html filters
Comment 18 Andrew Isherwood 2018-10-05 13:32:40 UTC
Created attachment 80084 [details] [review]
Bug 18591: (follow-up) Add more POD
Comment 19 Katrin Fischer 2018-10-05 14:20:48 UTC
Created attachment 80101 [details] [review]
Bug 18591: Database updates

Signed-off-by: andrew.isherwood@ptfs-europe.com
Comment 20 Katrin Fischer 2018-10-05 14:20:52 UTC
Created attachment 80102 [details] [review]
Bug 18591: Allow any number of comments on ILLs

This patch makes it possible to add arbitrary comments to ILL
requests. Comments are read only and displayed in chronological
order. Comments can be added by librarians, but also added automatically
based on comments in the different protocols, so that comments from the
lending library can also be added.

To test:
- Enable the ILL module ("ILLModule" syspref + config in koha-conf.xml)
- Install the Dummy backend from here:
  https://github.com/PTFS-Europe/Dummy
- Create a Dummy ILL request
- Add comments to the request, checking that the name and borrowernumber
  of the person that added the comment is displayed ok
- Check that the displayed date and time the comment was submitted follows
  the "dateformat" syspref
- On the "List requests" page, check that the correct number of comments
  is displayed in the "Comments" column
- Check that the tests pass:
  prove t/db_dependent/Illcomments.t

Signed-off-by: andrew.isherwood@ptfs-europe.com
Comment 21 Katrin Fischer 2018-10-05 14:20:56 UTC
Created attachment 80103 [details] [review]
Bug 18591: (follow-up) Add missing html filters
Comment 22 Katrin Fischer 2018-10-05 14:21:00 UTC
Created attachment 80104 [details] [review]
Bug 18591: (QA follow-up) Fix merge error in kohastructure.sql
Comment 23 Katrin Fischer 2018-10-05 14:21:05 UTC
Created attachment 80105 [details] [review]
Bug 18591: (follow-up) Add more POD
Comment 24 Andrew Isherwood 2018-10-08 16:08:15 UTC
Created attachment 80227 [details] [review]
Bug 18591: Database updates

Signed-off-by: andrew.isherwood@ptfs-europe.com

Bug 18591: (QA follow-up) Fix merge error in kohastructure.sql
Comment 25 Andrew Isherwood 2018-10-08 16:08:19 UTC
Created attachment 80228 [details] [review]
Bug 18591: Allow any number of comments on ILLs

This patch makes it possible to add arbitrary comments to ILL
requests. Comments are read only and displayed in chronological
order. Comments can be added by librarians, but also added automatically
based on comments in the different protocols, so that comments from the
lending library can also be added.

To test:
- Enable the ILL module ("ILLModule" syspref + config in koha-conf.xml)
- Install the Dummy backend from here:
  https://github.com/PTFS-Europe/Dummy
- Create a Dummy ILL request
- Add comments to the request, checking that the name and borrowernumber
  of the person that added the comment is displayed ok
- Check that the displayed date and time the comment was submitted follows
  the "dateformat" syspref
- On the "List requests" page, check that the correct number of comments
  is displayed in the "Comments" column
- Check that the tests pass:
  prove t/db_dependent/Illcomments.t

Signed-off-by: andrew.isherwood@ptfs-europe.com
Comment 26 Andrew Isherwood 2018-10-08 16:08:24 UTC
Created attachment 80229 [details] [review]
Bug 18591: (follow-up) Add missing html filters
Comment 27 Andrew Isherwood 2018-10-08 16:08:28 UTC
Created attachment 80230 [details] [review]
Bug 18591: (follow-up) Add more POD
Comment 28 Andrew Isherwood 2018-10-08 16:10:07 UTC
Comment on attachment 80104 [details] [review]
Bug 18591: (QA follow-up) Fix merge error in kohastructure.sql

This patch was squashed into https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80227
Comment 29 Andrew Isherwood 2018-10-08 16:12:21 UTC
I had to squash:

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80101

and

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80104

as the problem in the first was causing it to fail to apply before it got to the second which fixed the problem. So now they're a single patch which should apply correctly
Comment 30 Nick Clemens 2018-10-17 15:03:07 UTC
Couple issues:
1 - Requests existing before this patch throw datatables errors when viewing requests:
DataTables warning: table id=ill-requests - Requested unknown parameter 'comments' for row 0, column 8. For more information about this error, please see http://datatables.net/tn/4
2 - Instead of entering a borrowernumber, enter some letters in that field when creating a request - Internal server error
3 - Enter an invalid borrowernumner - Internal server error

Ideally I think we should use a patron search here and not rely on the user to come up with borrowernumbers
Comment 31 Andrew Isherwood 2018-10-17 15:19:16 UTC
Thanks for that Nick. I think items 2 & 3 are not related to this enhancement, they appear to be pre-existing bugs in the ILL module. I'll create bugs for them.

#1 is an interesting one, I'd expect the "comments" property to be available, even if it didn't exist when the request was available. Since it's seemingly a datatables problem, I'll take an initial look and see what's going on. If it's a problem in the original patches, I'll perhaps throw it in Magnus' direction :-)
Comment 32 Andrew Isherwood 2018-10-18 09:35:02 UTC
I've started looking into this. Problems #2 & #3 both appear to be problems with the Dummy backend. This backend hasn't seen any maintenance for over a year and has now been replaced by the FreeForm backend as a "simple, no dependency" backend.

I'll update the test plan accordingly.

Still looking at #1
Comment 33 Andrew Isherwood 2018-10-18 09:39:24 UTC
To test:
- Enable the ILL module ("ILLModule" syspref + config in koha-conf.xml)
- Install the FreeForm backend from here:
  https://github.com/PTFS-Europe/koha-ill-freeform
- Create a FreeForm ILL request
- Add comments to the request, checking that the name and borrowernumber
  of the person that added the comment is displayed ok
- Check that the displayed date and time the comment was submitted follows
  the "dateformat" syspref
- On the "List requests" page, check that the correct number of comments
  is displayed in the "Comments" column
- Check that the tests pass:
  prove t/db_dependent/Illcomments.t
Comment 34 Andrew Isherwood 2018-10-18 09:59:37 UTC
(In reply to Nick Clemens from comment #30)
> Couple issues:
> 1 - Requests existing before this patch throw datatables errors when viewing
> requests:
> DataTables warning: table id=ill-requests - Requested unknown parameter
> 'comments' for row 0, column 8. For more information about this error,
> please see http://datatables.net/tn/4

I've not been able to replicate this at all Nick. I started with no requests at all and without the bug applied. I created a couple of requests, obviously lacking comments. I then applied the bug. The request list displayed perfectly, showing the number of comments as 0 for each request. I then created a request without any problem.

Did you definitely run the database update and regenerated the schema after applying the bug? Sorry, I'm *sure* you did, but I thought it was worth mentioning.
Comment 35 Nick Clemens 2018-10-26 20:01:50 UTC
(In reply to Andrew Isherwood from comment #34)
> Did you definitely run the database update and regenerated the schema after
> applying the bug? Sorry, I'm *sure* you did, but I thought it was worth
> mentioning.

Never be too sure - I can't recreate now either, moving back to PQA
Comment 36 Nick Clemens 2018-10-27 14:26:27 UTC
Awesome work all!

Pushed to master for 18.11
Comment 37 Martin Renvoize 2018-11-08 15:20:59 UTC
Enhancement, will not be backported to 18.05.x series.