Created attachment 80841[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.

Created attachment 82155[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.

Created attachment 82156[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)

Created attachment 82506[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.

Created attachment 82507[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)

Created attachment 84405[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.

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.

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)

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.

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

(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

(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.

(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.

(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?

(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...

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

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

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>

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>