Bug 21063 - Allow columns in intranet ILL request datatable to be customisable
Summary: Allow columns in intranet ILL request datatable to be customisable
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andrew Isherwood
QA Contact: Josef Moravec
URL:
Keywords:
: 20883 22268 (view as bug list)
Depends on: 20750
Blocks: 18837 25944
  Show dependency treegraph
 
Reported: 2018-07-12 09:22 UTC by Andrew Isherwood
Modified: 2020-07-15 15:37 UTC (History)
7 users (show)

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


Attachments
Bug 21063: Add "Columns settings" for ILL (2.76 KB, patch)
2018-07-13 10:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add ability to show / hide columns (12.50 KB, patch)
2018-07-13 10:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add "Columns settings" for ILL (2.76 KB, patch)
2018-07-13 10:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add ability to show / hide columns (12.50 KB, patch)
2018-07-13 10:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add "Columns settings" for ILL (2.81 KB, patch)
2018-09-28 07:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 21063: Add ability to show / hide columns (12.59 KB, patch)
2018-09-28 07:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 21063: Add "Columns settings" for ILL (2.84 KB, patch)
2018-10-17 09:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add ability to show / hide columns (12.46 KB, patch)
2018-10-17 09:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Fix bad merge (1.52 KB, patch)
2018-10-18 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Add user ID to column list (2.74 KB, patch)
2018-10-18 13:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add "Columns settings" for ILL (2.84 KB, patch)
2018-11-09 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add ability to show / hide columns (14.30 KB, patch)
2018-11-09 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Add user ID to column list (3.02 KB, patch)
2018-11-09 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Amendments for rebase (12.08 KB, patch)
2018-11-09 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add "Columns settings" for ILL (2.84 KB, patch)
2018-11-19 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add ability to show / hide columns (14.30 KB, patch)
2018-11-19 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Add user ID to column list (3.02 KB, patch)
2018-11-19 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Amendments for rebase (10.93 KB, patch)
2018-11-19 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Add comments to column list (1.75 KB, patch)
2019-01-25 12:04 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add "Columns settings" for ILL (2.84 KB, patch)
2019-02-05 14:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add ability to show / hide columns (14.38 KB, patch)
2019-02-05 14:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Add user ID to column list (3.02 KB, patch)
2019-02-05 14:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Amendments for rebase (10.93 KB, patch)
2019-02-05 14:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Add comments to column list (1.75 KB, patch)
2019-02-05 14:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Sanitize datatable data (4.05 KB, patch)
2019-02-08 11:15 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Fix bad rebase in javascript (985 bytes, patch)
2019-02-28 12:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Ill request list (34.33 KB, image/png)
2019-02-28 12:16 UTC, Josef Moravec
Details
Bug 21063: (follow-up) Fix rebase problems (1.83 KB, patch)
2019-02-28 13:49 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add more tests (1.41 KB, patch)
2019-03-01 08:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) borrowernumber -> patron_id (1.04 KB, patch)
2019-03-01 09:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Fix more rebase problems (1.50 KB, patch)
2019-03-01 11:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Make patron col consistent (7.04 KB, patch)
2019-03-01 11:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Make patron col consistent (7.53 KB, patch)
2019-03-01 11:49 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: (follow-up) Fix missing table values (7.03 KB, patch)
2019-03-05 12:14 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21063: Add "Columns settings" for ILL (2.88 KB, patch)
2019-03-05 16:58 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: Add ability to show / hide columns (14.43 KB, patch)
2019-03-05 16:58 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Add user ID to column list (3.06 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Amendments for rebase (10.97 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Add comments to column list (1.79 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Sanitize datatable data (4.05 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Fix bad rebase in javascript (1.01 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Fix rebase problems (1.87 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: Add more tests (1.46 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) borrowernumber -> patron_id (1.08 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Fix more rebase problems (1.54 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Make patron col consistent (7.57 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (follow-up) Fix missing table values (7.06 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (QA follow-up) Fix terminology (1.24 KB, patch)
2019-03-05 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (QA follow-up) Remove debug message (956 bytes, patch)
2019-03-05 17:00 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21063: (QA follow-up) Fix test api/v1/illrequest.t (916 bytes, patch)
2019-03-07 08:33 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2018-07-12 09:22:02 UTC
Currently the columns in this datatable are static, they should be customisable.
Comment 1 Andrew Isherwood 2018-07-13 10:16:11 UTC Comment hidden (obsolete)
Comment 2 Andrew Isherwood 2018-07-13 10:16:23 UTC Comment hidden (obsolete)
Comment 3 Andrew Isherwood 2018-07-13 10:33:53 UTC Comment hidden (obsolete)
Comment 4 Andrew Isherwood 2018-07-13 10:34:05 UTC Comment hidden (obsolete)
Comment 5 PTFS Europe Sandboxes 2018-09-28 07:18:24 UTC Comment hidden (obsolete)
Comment 6 PTFS Europe Sandboxes 2018-09-28 07:18:47 UTC Comment hidden (obsolete)
Comment 7 Andrew Isherwood 2018-10-17 09:11:19 UTC Comment hidden (obsolete)
Comment 8 Andrew Isherwood 2018-10-17 09:11:23 UTC Comment hidden (obsolete)
Comment 9 Andrew Isherwood 2018-10-17 09:11:50 UTC
Rebased on master
Comment 10 Andrew Isherwood 2018-10-18 10:59:32 UTC Comment hidden (obsolete)
Comment 11 Andrew Isherwood 2018-10-18 13:35:19 UTC Comment hidden (obsolete)
Comment 12 Andrew Isherwood 2018-10-18 13:38:33 UTC
*** Bug 20883 has been marked as a duplicate of this bug. ***
Comment 13 Andrew Isherwood 2018-11-09 10:52:15 UTC Comment hidden (obsolete)
Comment 14 Andrew Isherwood 2018-11-09 10:52:25 UTC Comment hidden (obsolete)
Comment 15 Andrew Isherwood 2018-11-09 10:52:29 UTC Comment hidden (obsolete)
Comment 16 Andrew Isherwood 2018-11-09 10:52:32 UTC Comment hidden (obsolete)
Comment 17 Andrew Isherwood 2018-11-09 10:52:36 UTC Comment hidden (obsolete)
Comment 18 Andrew Isherwood 2018-11-09 10:53:24 UTC
Rebased on top of dependency tree
Comment 19 Andrew Isherwood 2018-11-19 14:30:33 UTC Comment hidden (obsolete)
Comment 20 Andrew Isherwood 2018-11-19 14:30:37 UTC Comment hidden (obsolete)
Comment 21 Andrew Isherwood 2018-11-19 14:30:41 UTC Comment hidden (obsolete)
Comment 22 Andrew Isherwood 2018-11-19 14:30:45 UTC Comment hidden (obsolete)
Comment 23 Andrew Isherwood 2019-01-25 12:04:30 UTC Comment hidden (obsolete)
Comment 24 Andrew Isherwood 2019-02-05 14:50:09 UTC
Created attachment 84786 [details] [review]
Bug 21063: Add "Columns settings" for ILL

This patch adds the "Columns settings" values for the illrequests table

Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Comment 25 Andrew Isherwood 2019-02-05 14:50:13 UTC
Created attachment 84787 [details] [review]
Bug 21063: Add ability to show / hide columns

This patch adds the "Column visibility" functionality to the main ILL
request list table.

To test:
- Ensure ILL is enabled and you have some requests
- Apply patch
- From the "Koha administration screen, select "Configure columns"
- In the "Columns settings" page:
  => TEST: Ensure an "Interlibrary loans" category exists
  - Upon expanding the category:
  => TEST: Ensure a table is displayed showing columns
  => TEST: Ensure the "action" column has "Cannot be toggled"
  pre-selected
  => TEST: Change values for columns and ensure they're saved
- From the main staff menu, select "ILL requests"
- In the table:
  => TEST: Click the "Column visibility" button and ensure a modal
  containing all columns (except "Action")  is displayed
  => TEST: Select various columns and ensure they are shown and hidden

Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Comment 26 Andrew Isherwood 2019-02-05 14:50:16 UTC
Created attachment 84788 [details] [review]
Bug 21063: (follow-up) Add user ID to column list

As originally specified in bug 20883, there is a requirement for some
users to be able to display the user ID (borrowernumber) in the UI.

This patch adds that ability to this bug, 20883 will be marked as a
duplicate of this one.
Comment 27 Andrew Isherwood 2019-02-05 14:50:20 UTC
Created attachment 84789 [details] [review]
Bug 21063: (follow-up) Amendments for rebase

Modify to add the additional changes required now we're rebasing on top
of the dependency tree. Includes adding additional columns (and changing
indexes for search/filter where appropriate)
Comment 28 Andrew Isherwood 2019-02-05 14:50:23 UTC
Created attachment 84790 [details] [review]
Bug 21063: (follow-up) Add comments to column list

Since this bug is now dependent on Bug 18591 (Allow an arbitrary number
of comments on ILLs) we need to add the comments column to this table
and the list of selectable columns. This patch does this.
Comment 29 Andrew Isherwood 2019-02-08 11:15:30 UTC
Created attachment 84862 [details] [review]
Bug 21063: (follow-up) Sanitize datatable data

This mitigates bug 22268 by sanitizing data prior to display using the
built in $.fn.dataTable.render.text() helper provided by Datatables.

The patch was added here, rather that in 22268 since this is the bug
that introduced the problem by increasing the number of fields that are
displayed in the table, some of which could contain user provided
malicious data
Comment 30 Andrew Isherwood 2019-02-08 11:17:35 UTC
*** Bug 22268 has been marked as a duplicate of this bug. ***
Comment 31 Katrin Fischer 2019-02-09 22:27:29 UTC
This might a little change soon depending on Owen's work on bug 22015.
Comment 32 Josef Moravec 2019-02-28 12:14:20 UTC
Created attachment 85854 [details] [review]
Bug 21063: (follow-up) Fix bad rebase in javascript
Comment 33 Josef Moravec 2019-02-28 12:16:28 UTC
Created attachment 85855 [details]
Ill request list

The table has some minor rendering issues.

Also, why to have three columns for patron identity?
Comment 34 Andrew Isherwood 2019-02-28 13:49:22 UTC
Created attachment 85869 [details] [review]
Bug 21063: (follow-up) Fix rebase problems

A couple of minor rebase problems resolved
Comment 35 Andrew Isherwood 2019-02-28 13:50:41 UTC
(In reply to Josef Moravec from comment #33)
> Created attachment 85855 [details]
> Ill request list
> 
> The table has some minor rendering issues.
These should now be fixed
> Also, why to have three columns for patron identity?
We had different customers asking for different fields (for sorting and filtering), so it was easiest just to include all three, then they can be turned on or off as needed
Comment 36 Josef Moravec 2019-03-01 08:42:50 UTC
Comment on attachment 84788 [details] [review]
Bug 21063: (follow-up) Add user ID to column list

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

::: Koha/REST/V1/Illrequests.pm
@@ +125,4 @@
>          foreach my $p(@{$patron_arr}) {
>              if ($p->{borrowernumber} == $req->borrowernumber) {
>                  $to_push->{patron} = {
> +                    borrowernumber => $p->{borrowernumber},

according to our api name conventions, this should be patron_id
Comment 37 Josef Moravec 2019-03-01 08:44:03 UTC
(In reply to Andrew Isherwood from comment #35)
> (In reply to Josef Moravec from comment #33)
> > Also, why to have three columns for patron identity?

> We had different customers asking for different fields (for sorting and
> filtering), so it was easiest just to include all three, then they can be
> turned on or off as needed

I don't like this, it makes inconsistancy in system, we should use standard pattern here: Firstname Surname (cardnumber) with link to moremember.pl. Ideally generated by patron-title.inc template.
Comment 38 Josef Moravec 2019-03-01 08:44:34 UTC
Created attachment 85889 [details] [review]
Bug 21063: Add more tests
Comment 39 Andrew Isherwood 2019-03-01 09:21:16 UTC
Created attachment 85893 [details] [review]
Bug 21063: (follow-up) borrowernumber -> patron_id

As per comment #36 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21063#c36
Comment 40 Andrew Isherwood 2019-03-01 09:27:53 UTC
(In reply to Josef Moravec from comment #36)
> Comment on attachment 84788 [details] [review] [review]
> Bug 21063: (follow-up) Add user ID to column list
> 
> Review of attachment 84788 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/Illrequests.pm
> @@ +125,4 @@
> >          foreach my $p(@{$patron_arr}) {
> >              if ($p->{borrowernumber} == $req->borrowernumber) {
> >                  $to_push->{patron} = {
> > +                    borrowernumber => $p->{borrowernumber},
> 
> according to our api name conventions, this should be patron_id

Thanks for that Josef, this is now done. It has introduced a slight inconsistency in that we now have the following in the API response:

Request object:
{
    [...]
    borrowernumber: 123,
    patron: {
        patron_id: 123
    }
    [...]
}

The borrowernumber in the request object comes directly from the column name in the request table. It feels potentially error prone to start introducing mapping from borrowernumber->patron_id on egress and patron_id->borrowernumber on ingress. What do you think?

I don't know, in my mind, it's not an ideal situation to be in having a naming convention for API routes that differs from the naming convention everywhere else, but that's how it is, so I guess we just need to work with it.
Comment 41 Josef Moravec 2019-03-01 09:39:07 UTC
(In reply to Andrew Isherwood from comment #40)
> (In reply to Josef Moravec from comment #36)
> > Comment on attachment 84788 [details] [review] [review] [review]
> > Bug 21063: (follow-up) Add user ID to column list
> > 
> > Review of attachment 84788 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: Koha/REST/V1/Illrequests.pm
> > @@ +125,4 @@
> > >          foreach my $p(@{$patron_arr}) {
> > >              if ($p->{borrowernumber} == $req->borrowernumber) {
> > >                  $to_push->{patron} = {
> > > +                    borrowernumber => $p->{borrowernumber},
> > 
> > according to our api name conventions, this should be patron_id
> 
> Thanks for that Josef, this is now done. It has introduced a slight
> inconsistency in that we now have the following in the API response:
> 
> Request object:
> {
>     [...]
>     borrowernumber: 123,
>     patron: {
>         patron_id: 123
>     }
>     [...]
> }
> 
> The borrowernumber in the request object comes directly from the column name
> in the request table. It feels potentially error prone to start introducing
> mapping from borrowernumber->patron_id on egress and
> patron_id->borrowernumber on ingress. What do you think?
> 
> I don't know, in my mind, it's not an ideal situation to be in having a
> naming convention for API routes that differs from the naming convention
> everywhere else, but that's how it is, so I guess we just need to work with
> it.

We should take the rfc to development meeting, vote it and then update the endpoint as it was done with /holds endpoint
Comment 42 Andrew Isherwood 2019-03-01 10:22:08 UTC
(In reply to Josef Moravec from comment #37)
> (In reply to Andrew Isherwood from comment #35)
> > (In reply to Josef Moravec from comment #33)
> > > Also, why to have three columns for patron identity?
> 
> > We had different customers asking for different fields (for sorting and
> > filtering), so it was easiest just to include all three, then they can be
> > turned on or off as needed
> 
> I don't like this, it makes inconsistancy in system, we should use standard
> pattern here: Firstname Surname (cardnumber) with link to moremember.pl.
> Ideally generated by patron-title.inc template.

I think this raises a bigger question of whether the current convention of Firstname Surname (cardnumber) addresses users' requirements. In our experience here, it doesn't. I think it's maybe worth having a separate discussion of whether we should be reconsidering the current situation and whether we should be making other DataTable instances in line with what we have here. I think it's certainly worth at least discussing.

Which does beg the question of what we do to achieve PQA on this bug. We don't want it to be dependent on a potentially far reaching discussion. Do you think we should keep it as is, pending the discussion, or make it consistent with elsewhere, then address as appropriate following the discussion?
Comment 43 Andrew Isherwood 2019-03-01 10:23:23 UTC
Adding Katrin and Owen to this so they're aware of the issue detailed in comment #42
Comment 44 Josef Moravec 2019-03-01 10:38:39 UTC
(In reply to Andrew Isherwood from comment #42)
> (In reply to Josef Moravec from comment #37)
> > (In reply to Andrew Isherwood from comment #35)
> > > (In reply to Josef Moravec from comment #33)
> > > > Also, why to have three columns for patron identity?
> > 
> > > We had different customers asking for different fields (for sorting and
> > > filtering), so it was easiest just to include all three, then they can be
> > > turned on or off as needed
> > 
> > I don't like this, it makes inconsistancy in system, we should use standard
> > pattern here: Firstname Surname (cardnumber) with link to moremember.pl.
> > Ideally generated by patron-title.inc template.
> 
> I think this raises a bigger question of whether the current convention of
> Firstname Surname (cardnumber) addresses users' requirements. In our
> experience here, it doesn't. I think it's maybe worth having a separate
> discussion of whether we should be reconsidering the current situation and
> whether we should be making other DataTable instances in line with what we
> have here. I think it's certainly worth at least discussing.
> 
> Which does beg the question of what we do to achieve PQA on this bug. We
> don't want it to be dependent on a potentially far reaching discussion. Do
> you think we should keep it as is, pending the discussion, or make it
> consistent with elsewhere, then address as appropriate following the
> discussion?

My opinion is, to make it consistent with current system. As you pointed out, such a discussion could take 'some' time... And is definitely out of scope of this report...
Comment 45 Andrew Isherwood 2019-03-01 11:47:06 UTC
Created attachment 85894 [details] [review]
Bug 21063: (follow-up) Fix more rebase problems

More minor rebase problems fixed
Comment 46 Andrew Isherwood 2019-03-01 11:48:11 UTC
Created attachment 85895 [details] [review]
Bug 21063: (follow-up) Make patron col consistent

As per comment #44 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21063#c44
Comment 47 Andrew Isherwood 2019-03-01 11:49:54 UTC
Created attachment 85896 [details] [review]
Bug 21063: (follow-up) Make patron col consistent

As per comment #44 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21063#c44
Comment 48 Josef Moravec 2019-03-04 11:29:00 UTC
Thanks Andrew for quick follow-ups. I continue testing, but found functional issue - some metadata are not shown in ill request list table, so far i found problem with:
pages
comments
article (part title), it is shown in title instead
completed on
cost
Comment 49 Andrew Isherwood 2019-03-05 12:14:30 UTC
Created attachment 86060 [details] [review]
Bug 21063: (follow-up) Fix missing table values

As per comment #48 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21063#c48

>pages
This was a misnamed property in colums_settings.yml

>comments
This was a missing embed in the API call, probably a rebase error

>article (part title)
This was a bug. A combination of a misnamed property in
columns_settings.yml and a missing "render" method on the datatable

>completed on
This was an oversight and had apparently never worked. It has now been
added

>cost
I cannot replicate this, cost is displaying fine for me
Comment 50 Josef Moravec 2019-03-05 16:52:13 UTC
(In reply to Andrew Isherwood from comment #49)
> >cost
> I cannot replicate this, cost is displaying fine for me

My mistake, cost is ok
Comment 51 Josef Moravec 2019-03-05 16:58:53 UTC
Created attachment 86092 [details] [review]
Bug 21063: Add "Columns settings" for ILL

This patch adds the "Columns settings" values for the illrequests table

Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 52 Josef Moravec 2019-03-05 16:58:59 UTC
Created attachment 86093 [details] [review]
Bug 21063: Add ability to show / hide columns

This patch adds the "Column visibility" functionality to the main ILL
request list table.

To test:
- Ensure ILL is enabled and you have some requests
- Apply patch
- From the "Koha administration screen, select "Configure columns"
- In the "Columns settings" page:
  => TEST: Ensure an "Interlibrary loans" category exists
  - Upon expanding the category:
  => TEST: Ensure a table is displayed showing columns
  => TEST: Ensure the "action" column has "Cannot be toggled"
  pre-selected
  => TEST: Change values for columns and ensure they're saved
- From the main staff menu, select "ILL requests"
- In the table:
  => TEST: Click the "Column visibility" button and ensure a modal
  containing all columns (except "Action")  is displayed
  => TEST: Select various columns and ensure they are shown and hidden

Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 53 Josef Moravec 2019-03-05 16:59:03 UTC
Created attachment 86094 [details] [review]
Bug 21063: (follow-up) Add user ID to column list

As originally specified in bug 20883, there is a requirement for some
users to be able to display the user ID (borrowernumber) in the UI.

This patch adds that ability to this bug, 20883 will be marked as a
duplicate of this one.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 54 Josef Moravec 2019-03-05 16:59:08 UTC
Created attachment 86095 [details] [review]
Bug 21063: (follow-up) Amendments for rebase

Modify to add the additional changes required now we're rebasing on top
of the dependency tree. Includes adding additional columns (and changing
indexes for search/filter where appropriate)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 55 Josef Moravec 2019-03-05 16:59:13 UTC
Created attachment 86096 [details] [review]
Bug 21063: (follow-up) Add comments to column list

Since this bug is now dependent on Bug 18591 (Allow an arbitrary number
of comments on ILLs) we need to add the comments column to this table
and the list of selectable columns. This patch does this.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 56 Josef Moravec 2019-03-05 16:59:18 UTC
Created attachment 86097 [details] [review]
Bug 21063: (follow-up) Sanitize datatable data

This mitigates bug 22268 by sanitizing data prior to display using the
built in $.fn.dataTable.render.text() helper provided by Datatables.

The patch was added here, rather that in 22268 since this is the bug
that introduced the problem by increasing the number of fields that are
displayed in the table, some of which could contain user provided
malicious data

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 57 Josef Moravec 2019-03-05 16:59:23 UTC
Created attachment 86098 [details] [review]
Bug 21063: (follow-up) Fix bad rebase in javascript

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 58 Josef Moravec 2019-03-05 16:59:28 UTC
Created attachment 86099 [details] [review]
Bug 21063: (follow-up) Fix rebase problems

A couple of minor rebase problems resolved

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 59 Josef Moravec 2019-03-05 16:59:33 UTC
Created attachment 86100 [details] [review]
Bug 21063: Add more tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 60 Josef Moravec 2019-03-05 16:59:39 UTC
Created attachment 86101 [details] [review]
Bug 21063: (follow-up) borrowernumber -> patron_id

As per comment #36 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21063#c36

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 61 Josef Moravec 2019-03-05 16:59:44 UTC
Created attachment 86102 [details] [review]
Bug 21063: (follow-up) Fix more rebase problems

More minor rebase problems fixed

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 62 Josef Moravec 2019-03-05 16:59:48 UTC
Created attachment 86103 [details] [review]
Bug 21063: (follow-up) Make patron col consistent

As per comment #44 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21063#c44

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 63 Josef Moravec 2019-03-05 16:59:54 UTC
Created attachment 86104 [details] [review]
Bug 21063: (follow-up) Fix missing table values

As per comment #48 here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21063#c48

>pages
This was a misnamed property in colums_settings.yml

>comments
This was a missing embed in the API call, probably a rebase error

>article (part title)
This was a bug. A combination of a misnamed property in
columns_settings.yml and a missing "render" method on the datatable

>completed on
This was an oversight and had apparently never worked. It has now been
added

>cost
I cannot replicate this, cost is displaying fine for me

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 64 Josef Moravec 2019-03-05 16:59:59 UTC
Created attachment 86105 [details] [review]
Bug 21063: (QA follow-up) Fix terminology

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 65 Josef Moravec 2019-03-05 17:00:05 UTC
Created attachment 86106 [details] [review]
Bug 21063: (QA follow-up) Remove debug message

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 66 Josef Moravec 2019-03-07 08:33:10 UTC
Created attachment 86235 [details] [review]
Bug 21063: (QA follow-up) Fix test api/v1/illrequest.t
Comment 67 Nick Clemens 2019-03-15 19:59:18 UTC
Awesome work all!

Pushed (squashed) to master for 19.05
Comment 68 Martin Renvoize 2019-03-19 10:44:21 UTC
Enhancement, will not be backported to 18.11.x series.