Description
Andrew Isherwood
2018-07-12 09:22:02 UTC
Created attachment 76940 [details] [review] Bug 21063: Add "Columns settings" for ILL This patch adds the "Columns settings" values for the illrequests table Created attachment 76941 [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 Created attachment 76942 [details] [review] Bug 21063: Add "Columns settings" for ILL This patch adds the "Columns settings" values for the illrequests table Created attachment 76943 [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 Created attachment 79559 [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 Created attachment 79560 [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 Created attachment 80690 [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 Created attachment 80691 [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 Rebased on master Created attachment 80802 [details] [review] Bug 21063: (follow-up) Fix bad merge At some point an incomplete merge had crept in 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. *** Bug 20883 has been marked as a duplicate of this bug. *** Comment on attachment 80802 [details] [review] Bug 21063: (follow-up) Fix bad merge Obsoleted by additional rebase patch Created attachment 82153 [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 Created attachment 82154 [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 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) Rebased on top of dependency tree Created attachment 82504 [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 Created attachment 82505 [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 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 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 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 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 *** Bug 22268 has been marked as a duplicate of this bug. *** This might a little change soon depending on Owen's work on bug 22015. Created attachment 85854 [details] [review] Bug 21063: (follow-up) Fix bad rebase in javascript Created attachment 85855 [details]
Ill request list
The table has some minor rendering issues.
Also, why to have three columns for patron identity?
Created attachment 85869 [details] [review] Bug 21063: (follow-up) Fix rebase problems A couple of minor rebase problems resolved (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 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 (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. Created attachment 85889 [details] [review] Bug 21063: Add more tests 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 (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 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 (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? Adding Katrin and Owen to this so they're aware of the issue detailed in comment #42 (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... Created attachment 85894 [details] [review] Bug 21063: (follow-up) Fix more rebase problems More minor rebase problems fixed 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 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 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 (In reply to Andrew Isherwood from comment #49) > >cost > I cannot replicate this, cost is displaying fine for me My mistake, cost is ok 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> 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> 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 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> 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> 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> Created attachment 86098 [details] [review] Bug 21063: (follow-up) Fix bad rebase in javascript Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> Created attachment 86100 [details] [review] Bug 21063: Add more tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> 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> 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> 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> Created attachment 86105 [details] [review] Bug 21063: (QA follow-up) Fix terminology Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86106 [details] [review] Bug 21063: (QA follow-up) Remove debug message Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86235 [details] [review] Bug 21063: (QA follow-up) Fix test api/v1/illrequest.t Awesome work all! Pushed (squashed) to master for 19.05 Enhancement, will not be backported to 18.11.x series. |