Currently the displayed status of any given ILL request is strictly prescribed by where it is in the workflow, as defined in the ILL module. While this gives an accurate indication of the current status of a request, it does not allow for any local variation in the displayed status, which can be limiting for the end user. This bug proposes adding the ability for the admin, via a syspref, to define a custom set of request statuses, which can be selected for display by the manager of an ILL request. These statuses work alongside the existing "workflow" status that an ILL request has and will, if selected, override *for display* the status of that request. This proposal will allow the ILL module to continue to strictly prescribe and manage the workflow of requests, but will also allow the end user some flexibility in the display of a given request's status
Work on this bug is being submitted as distinct patches rather than one large patch. Each patch is related to a specific area of work, e.g. database schema changes, API changes, ILLrequest object changes, staff template changes & OPAC changes. Although these patches are distinct, they are all required for the end result to work. As such, I am adding QA test scenarios here rather than attaching them to the individual patches. Test custom status creation: 1) Apply this bug's patches 2) Apply database updates 3) Ensure you have at least one ILL backend available: https://wiki.koha-community.org/wiki/ILL_backends 4) Ensure you have the "ILLModule" preference enabled 5) Log in as a user with the ability to add authorised values. 6) Depending on whether you opted to accept the "Default ILL request status authorised value category name" data population during install, you may or may not have an authorised values category ILLSTATUS, if not, create it. 7) Add some authorised values to the ILLSTATUS category, these can be custom request statuses of your choice Test custom status usage: 8) From the staff interface, create an ILL request from a backend of your choice 9) On the resulting "Request details" page, observe that the request's status is "New request" 10) Click the "Edit request" button 11) In the "Request details" pane, observe that the request's status is now available in a drop down box, it should contain the current status PLUS your custom statuses. 12) Select a custom status and Submit the form 13) In the resulting "View ILL requests" page, observe that your request is now displaying your custom status. 14) Click the "Manage request" button for your new request 15) In the "Request details" page, observe that the request's status is displaying as your custom status Test custom status is overridden when request progresses through workflow: 16) Click "Confirm request", then the "Confirm request" confirmation 17) Observe that in the resulting "Request details" page, the request's status is now no longer displaying your custom status but has switched to "Requested" Test custom status is removed when authorised value is removed: 18) Observe that the request has the status "Requested", we are going to override that with a custom status, then remove that custom status authorised value 19) Click "Edit request" 20) Select a custom status and Submit the form 21) In the resulting "View ILL requests" page, observe that your request is now displaying your custom status. 22) Go to the authorised values management page 23) Delete the custom status you previously selected in step 19 24) Return to the list of ILL requests 25) Observe that the request that previously had the custom status now has it's former status of "Requested" Test API response for View ILL requests view: 26) Modify an existing request and set a custom status as discussed above 27) Navigate to the "View ILL requests" page 28) Examine the response to the API call made to the /api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias route 28) Observe that for requests with a custom status, the status_alias property contains an object for the request's authorised value containing the custom status 29) Observe that for requests without a custom status, status_alias is null Test OPAC view: 30) Modify an existing request and set a custom status as discussed above 31) In the OPAC, navigate to "your interlibrary loan requests" 32) In the "Interlibrary loan requests" view, observe that the request's status is displaying your custom value 33) Click "View" on the request with a custom value 34) In the "View Interlibrary loan request" view, observe that the request's status is displaying your custom value
Created attachment 74323 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install.
Created attachment 74324 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set
Created attachment 74325 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view
Created attachment 74326 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status.
Created attachment 74327 [details] [review] Bug 20581: Add support for status_alias to OPAC view - This patch adds the abililty for the OPAC to display a set status_alias value in preference to a request's status value. During ILL request listing or individual ILL request view, the status_alias value, if set, will be displayed in favour of the status
Created attachment 74362 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t
Let's say I install multiple languages. If the Authorized Value is not keyed partially on language, then how well will this work in a multilingual setting where one user might use one language, and another one might use some other language.
Hi Mark. That's a very good point. My knowledge of how to add multi language support to authorised values is, unfortunately, non existent. Are you aware of any "good practice" examples for this kind of thing?
(In reply to Andrew Isherwood from comment #9) > Hi Mark. That's a very good point. My knowledge of how to add multi language > support to authorised values is, unfortunately, non existent. Are you aware > of any "good practice" examples for this kind of thing? Hi Andrew, at the moment it's not possible to have multi-language authorised values. It's kind of an ongoing discussion if we should add more of them. But I think the long term solution woudl be to copy what we do for itemtype descriptions (there is a bug for that, but not spotting it right now). So the question is, if maybe there is a better way to achieve what you want to do. Could it be an option to have default values that are translatable via templates? A mix of custom values and and default values? I am not per se against adding authorised values - it's a problem we need to fix globally at some point. Even better of course if someone would be willing to have a look.
Hi Katrin Thanks for the suggestions. I'm wary about cooking up a home baked way of doing it as, at the moment, I'm using a well established method of providing an authorised list of values, which is exactly what the custom statuses need. I think I'd rather keep to a correct "Koha" way of providing this functionality than try to work around the lack of translations with a bespoke solution. It's tricky when in a transitional phase with anything, but I think having devs coming up with bespoke solutions will likely cause more pain down the line when a standard way of doing it is established and it's necessary to migrate to it. So I think, for now, it feels more "correct" to use authorised values and accept that they cannot be translated. Cheers Andrew
Created attachment 75333 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install.
Added an updated patch "Bug 20581: Modifications to database schema" which fixes the filename of the atomicupdate file from .sql to .perl
Created attachment 75334 [details] [review] Bug 20581: DO NOT PUSH: Updated Schema Files.
Created attachment 75810 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name
Created attachment 77819 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install.
Created attachment 77820 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set
Created attachment 77821 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view
Created attachment 77822 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status.
Created attachment 77823 [details] [review] Bug 20581: Add support for status_alias to OPAC view - This patch adds the abililty for the OPAC to display a set status_alias value in preference to a request's status value. During ILL request listing or individual ILL request view, the status_alias value, if set, will be displayed in favour of the status
Created attachment 77824 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t
Created attachment 77825 [details] [review] Bug 20581: DO NOT PUSH: Updated Schema Files.
Created attachment 77826 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name
Rebased against current master
Created attachment 78376 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install.
Created attachment 78377 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set
Created attachment 78378 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view
Created attachment 78379 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status.
Created attachment 78380 [details] [review] Bug 20581: Add support for status_alias to OPAC view - This patch adds the abililty for the OPAC to display a set status_alias value in preference to a request's status value. During ILL request listing or individual ILL request view, the status_alias value, if set, will be displayed in favour of the status
Created attachment 78381 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t
Created attachment 78382 [details] [review] Bug 20581: DO NOT PUSH: Updated Schema Files.
Created attachment 78383 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name
Created attachment 78384 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code.
Rebased on current master and provided an additional fix. Test custom status creation: 1) Apply this bug's patches 2) Apply database updates 3) Ensure you have at least one ILL backend available: https://wiki.koha-community.org/wiki/ILL_backends 4) Ensure you have the "ILLModule" preference enabled 5) Log in as a user with the ability to add authorised values. 6) Depending on whether you opted to accept the "Default ILL request status authorised value category name" data population during install, you may or may not have an authorised values category ILLSTATUS, if not, create it. 7) Add some authorised values to the ILLSTATUS category, these can be custom request statuses of your choice Test custom status usage: 8) From the staff interface, create an ILL request from a backend of your choice 9) On the resulting "Request details" page, observe that the request's status is "New request" 10) Click the "Edit request" button 11) In the "Request details" pane, observe that the request's status is now available in a drop down box, it should contain the current status PLUS your custom statuses. 12) Select a custom status and Submit the form 13) In the resulting "View ILL requests" page, observe that your request is now displaying your custom status. 14) Click the "Manage request" button for your new request 15) In the "Request details" page, observe that the request's status is displaying as your custom status Test custom status is overridden when request progresses through workflow: 16) Click "Confirm request", then the "Confirm request" confirmation 17) Observe that in the resulting "Request details" page, the request's status is now no longer displaying your custom status but has switched to "Requested" Test custom status is removed when authorised value is removed: 18) Observe that the request has the status "Requested", we are going to override that with a custom status, then remove that custom status authorised value 19) Click "Edit request" 20) Select a custom status and Submit the form 21) In the resulting "View ILL requests" page, observe that your request is now displaying your custom status. 22) Go to the authorised values management page 23) Delete the custom status you previously selected in step 19 24) Return to the list of ILL requests 25) Observe that the request that previously had the custom status now has it's former status of "Requested" Test API response for View ILL requests view: 26) Modify an existing request and set a custom status as discussed above 27) Navigate to the "View ILL requests" page 28) Examine the response to the API call made to the /api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias route 28) Observe that for requests with a custom status, the status_alias property contains an object for the request's authorised value containing the custom status 29) Observe that for requests without a custom status, status_alias is null Test OPAC view: 30) Modify an existing request and set a custom status as discussed above 31) In the OPAC, navigate to "your interlibrary loan requests" 32) In the "Interlibrary loan requests" view, observe that the request's status is displaying your custom value 33) Click "View" on the request with a custom value 34) In the "View Interlibrary loan request" view, observe that the request's status is displaying your custom value
Created attachment 78431 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code.
Created attachment 78515 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 78516 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 78517 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 78518 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 78519 [details] [review] Bug 20581: Add support for status_alias to OPAC view - This patch adds the abililty for the OPAC to display a set status_alias value in preference to a request's status value. During ILL request listing or individual ILL request view, the status_alias value, if set, will be displayed in favour of the status Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 78520 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 78521 [details] [review] Bug 20581: DO NOT PUSH: Updated Schema Files. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 78522 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 78523 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Could you rebase please?
Created attachment 80279 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 80280 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 80281 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 80282 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 80283 [details] [review] Bug 20581: Add support for status_alias to OPAC view - This patch adds the abililty for the OPAC to display a set status_alias value in preference to a request's status value. During ILL request listing or individual ILL request view, the status_alias value, if set, will be displayed in favour of the status Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 80284 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 80285 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 80286 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Done, thanks Josef!
Comment on attachment 80279 [details] [review] Bug 20581: Modifications to database schema Review of attachment 80279 [details] [review]: ----------------------------------------------------------------- I would not add the file ill_auth_cat.sql, but as you add the category on database update, it makes more sense to add it to mandatory/auth_val_cat.sql ::: installer/data/mysql/atomicupdate/bug_20581-add_new_illrequests_status_alias_column.perl @@ +1,4 @@ > +$DBversion = 'XXX'; # will be replaced by the RM > +if( CheckVersion( $DBversion ) ) { > + > + $dbh->do( "ALTER TABLE illrequests ADD COLUMN status_alias integer DEFAULT NULL" ); Please add a test on column existance and add AFTER statement to be sure the column order is same on all installations @@ +1,5 @@ > +$DBversion = 'XXX'; # will be replaced by the RM > +if( CheckVersion( $DBversion ) ) { > + > + $dbh->do( "ALTER TABLE illrequests ADD COLUMN status_alias integer DEFAULT NULL" ); > + $dbh->do( "ALTER TABLE illrequests ADD CONSTRAINT illrequests_safk FOREIGN KEY (status_alias) REFERENCES authorised_values(id) ON DELETE SET NULL" ); Please add ON UPDATE statement to exactly say what will happen ::: installer/data/mysql/kohastructure.sql @@ +4157,5 @@ > ON UPDATE CASCADE ON DELETE CASCADE > + CONSTRAINT `illrequests_safk` > + FOREIGN KEY (`status_alias`) > + REFERENCES `authorised_values` (`id`) > + ON DELETE SET NULL Add ON UPDATE statement
Comment on attachment 80280 [details] [review] Bug 20581: Provide status_alias on IllRequest Review of attachment 80280 [details] [review]: ----------------------------------------------------------------- ::: Koha/Illrequest.pm @@ +118,5 @@ > + return $self->status_alias ? > + Koha::AuthorisedValue->_new_from_dbic( > + scalar $self->_result->status_alias > + ) : > + undef; You shouldn't return explicit undef. And please add to pod information about return type.
Comment on attachment 80282 [details] [review] Bug 20581: Add display of status_alias to staff vw Review of attachment 80282 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt @@ +448,5 @@ > + <option value="[% alias.id %]"> > + [% END %] > + [% alias.authorised_value | html %] > + </option> > + [% END %] You could use av-build-dropbox.inc for making select from AV, then you don't even need the status_aliases param
Comment on attachment 80285 [details] [review] Bug 20581: (follow-up) Display status name Review of attachment 80285 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt @@ +129,4 @@ > // Render function for request status > var createStatus = function(data, type, row, meta) { > if (row.status_alias) { > + return row.status_alias.lib; What if lib is not defined?
Overall it is looking OK, just few relatively small issues I found (see above comments) Great work.
Created attachment 80474 [details] [review] Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56
Created attachment 80475 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57
(In reply to Josef Moravec from comment #58) > Comment on attachment 80282 [details] [review] [review] > Bug 20581: Add display of status_alias to staff vw > > Review of attachment 80282 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt > @@ +448,5 @@ > > + <option value="[% alias.id %]"> > > + [% END %] > > + [% alias.authorised_value | html %] > > + </option> > > + [% END %] > > You could use av-build-dropbox.inc for making select from AV, then you don't > even need the status_aliases param Thanks for the tip about av-build-dropbox.inc Josef. Unfortunately, in this case I don't think I can make use of it since we don't need to just display a list of AV values. We need to display a list of AV values plus a possible additional value. It looks like av-build-dropbox.inc generates the entire <select> element, rather than just the AV <option> elements, so I don't think I can make use of it on this occassion. Well worth knowing about though, thanks!
Created attachment 80501 [details] [review] Bug 20581: (follow-up) Allow for undefined lib As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c59
Comment on attachment 80475 [details] [review] Bug 20581: (follow-up) Fix statusalias return Review of attachment 80475 [details] [review]: ----------------------------------------------------------------- ::: Koha/Illrequest.pm @@ +125,5 @@ > scalar $self->_result->status_alias > + ); > + } else { > + return; > + } This could be written even simpler: return unless $self->status_alias return Koha::AuthorisedValue->_new_from_dbic( scalar $self->_result->status_alias ); But no blocker
Created attachment 80503 [details] [review] Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56
Created attachment 80505 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57
Comment on attachment 80501 [details] [review] Bug 20581: (follow-up) Allow for undefined lib Review of attachment 80501 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt @@ +129,4 @@ > // Render function for request status > var createStatus = function(data, type, row, meta) { > if (row.status_alias) { > + return row.status_alias.lib ? row.status_alias.lib : 'N/A'; I would use the code instead of 'N/A'
Comment on attachment 80505 [details] [review] Bug 20581: (follow-up) Fix statusalias return Review of attachment 80505 [details] [review]: ----------------------------------------------------------------- ::: Koha/Illrequest.pm @@ +113,5 @@ > > + my $statusalias = $request->statusalias; > + > +Return a request's status alias, if one is defined, otherwise > +return implicit undef Please add the information that status alias is of type Koha::AuthorisedValue
One more think on authorised values: generaly it is better to take them in from AuthorisedValues template plugin
Created attachment 80598 [details] [review] Bug 20581: (follow-up) Fix fallback status_alias As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c68
Created attachment 80599 [details] [review] Bug 20581: (follow-up) Modify POD As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c69
Created attachment 80600 [details] [review] Bug 20581: (follow-up) Fix AV population As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c72
Created attachment 82063 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82064 [details] [review] Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56
Created attachment 82065 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82066 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82067 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82068 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82069 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82070 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82071 [details] [review] Bug 20581: (follow-up) Allow for undefined lib As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c59
Created attachment 82072 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57
Created attachment 82073 [details] [review] Bug 20581: (follow-up) Fix fallback status_alias As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c68
Created attachment 82074 [details] [review] Bug 20581: (follow-up) Modify POD As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c69
Created attachment 82075 [details] [review] Bug 20581: (follow-up) Fix AV population As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c72
Created attachment 82076 [details] [review] Bug 20581: DO NOT PUSH: Updated Schema Files. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Rebased on top of master then the current dependency tree
Created attachment 82443 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82444 [details] [review] Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56
Created attachment 82445 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82446 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82447 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82448 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82449 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82450 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82451 [details] [review] Bug 20581: (follow-up) Allow for undefined lib As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c59
Created attachment 82452 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57
Created attachment 82453 [details] [review] Bug 20581: (follow-up) Fix fallback status_alias As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c68
Created attachment 82454 [details] [review] Bug 20581: (follow-up) Modify POD As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c69
Created attachment 82455 [details] [review] Bug 20581: (follow-up) Fix AV population As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c72
Created attachment 82456 [details] [review] Bug 20581: DO NOT PUSH: Updated Schema Files. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82457 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82458 [details] [review] Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56
Created attachment 82459 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82460 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82461 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82462 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82463 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82464 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82465 [details] [review] Bug 20581: (follow-up) Allow for undefined lib As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c59
Created attachment 82466 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57
Created attachment 82467 [details] [review] Bug 20581: (follow-up) Fix fallback status_alias As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c68
Created attachment 82468 [details] [review] Bug 20581: (follow-up) Modify POD As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c69
Created attachment 82472 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82483 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82484 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82485 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82486 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82487 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82488 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82489 [details] [review] Bug 20581: DO NOT PUSH: Updated Schema Files. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82490 [details] [review] Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56
Created attachment 82491 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 82492 [details] [review] Bug 20581: (follow-up) Allow for undefined lib As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c59
Created attachment 82493 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57
Created attachment 82494 [details] [review] Bug 20581: (follow-up) Fix fallback status_alias As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c68
Created attachment 82495 [details] [review] Bug 20581: (follow-up) Fix AV population As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c72
Created attachment 82496 [details] [review] Bug 20581: (follow-up) Modify POD As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c69
Created attachment 82733 [details] [review] Bug 20581: (follow-up) Fix typo in kohastructure Fix the missing comma
Created attachment 84202 [details] [review] Bug 20581: (follow-up) Fix QA errors This patch fixes the "missing_filter" errors on ill-requests.tt
Created attachment 84404 [details] [review] Bug 20581: (follow-up) Fix dependency requirement Since this bug is now dependent on Bug 20600 (Provide the ability for users to filter ILL requests) we need to make the status filtering aware of the custom statuses. This patch does that.
I have an issue here with the data structures: In Koha we ususally store the code (authorised_value) for referencing, this patch uses the authorised_values.id which is the numerical PK of the table. I can see where this is coming from, but as it would be the only place in Koha working like that. It will make it impossible to use our TT plugins for dealing with the descriptions etc. From the QA script: Maybe we could improve that with a bit of squashing: FAIL installer/data/mysql/en/optional/ill_auth_cat.sql FAIL git manipulation The file has been added and deleted in the same patchset FAIL installer/data/mysql/en/optional/ill_auth_cat.txt FAIL git manipulation The file has been added and deleted in the same patchset
Created attachment 84768 [details] [review] Bug 20581: (follow-up) Do not use PK for AV FK As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c133 We switch from using authorised_values.id when creating the foreign key illrequests.status_alias, we now use authorised_values.authorised_value I have added a migration from using id -> authorised_value, so when existing users of this bug get this version, their DB migrates nicely instead of just breaking.
Created attachment 84770 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Following commit was squashed into this one: Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56
Created attachment 84771 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 84772 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 84773 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 84774 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 84775 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 84776 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 84777 [details] [review] Bug 20581: (follow-up) Allow for undefined lib As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c59
Created attachment 84778 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57
Created attachment 84779 [details] [review] Bug 20581: (follow-up) Fix fallback status_alias As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c68
Created attachment 84780 [details] [review] Bug 20581: (follow-up) Fix AV population As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c72
Created attachment 84781 [details] [review] Bug 20581: (follow-up) Modify POD As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c69
Created attachment 84782 [details] [review] Bug 20581: (follow-up) Fix typo in kohastructure Fix the missing comma
Created attachment 84783 [details] [review] Bug 20581: (follow-up) Fix QA errors This patch fixes the "missing_filter" errors on ill-requests.tt
Created attachment 84784 [details] [review] Bug 20581: (follow-up) Fix dependency requirement Since this bug is now dependent on Bug 20600 (Provide the ability for users to filter ILL requests) we need to make the status filtering aware of the custom statuses. This patch does that.
Created attachment 84785 [details] [review] Bug 20581: (follow-up) Do not use PK for AV FK As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c133 We switch from using authorised_values.id when creating the foreign key illrequests.status_alias, we now use authorised_values.authorised_value I have added a migration from using id -> authorised_value, so when existing users of this bug get this version, their DB migrates nicely instead of just breaking.
Hi Katrin Thanks for that. I've now switched from using authorised_values.id to authorised_values.authorised_value. I've also squashed the commits that were causing that QA error. All should be well now. Cheers Andrew
Moving back to signed off.
Created attachment 84791 [details] [review] Bug 20581: (follow-up) Fix typo in DB upgrade IN -> ON
Comment on attachment 82489 [details] [review] Bug 20581: DO NOT PUSH: Updated Schema Files. This is too big, including more than the expected changed files. I am obsoleting it using 'dbic' on kohadevbox.
Hi Andrew, found a little issue: On a database with the sample data, create an ILLSTATUS: PROC with something like: Internal processing Add the new status to an existing or new ILL request. Check the status filter on the side of the ill requests table: It pulls the wrong description as there is also a PROC value for location: Processing Center I think you need to add the authorised value category into your look-up somewhere.
Hi Katrin, rant incoming, though it's not aimed at you! To be honest, this is actually a problem that is caused by using a foreign key that does not contain unique values. I was originally using authorised_values.id (which *is* unique) as the foreign key from illrequests.status_alias, but after you said this is the non-standard way of doing things, I changed it to use authorised_values.authorised_value, which has no unique constraint on it, as you have demonstrated. Thinking around the problem, I suspect this issue will affect any part of Koha that uses authorised values. The DBIx relationship makes the reasonable assumption that the column used as a foreign key contains unique values, so it just returns the first value it finds, which in this case is incorrect. As far as addressing it in this case. Whatever I do is going to be hackery, since it's patching over a fundamental DB design problem. Since we're using the Koha objects/DBIx::Class accessors to get the request's status, I'm going to have to provide an overloaded method that only gets the authorised values in the correct category. I do think it's worth considering in the context of the DB design error. This is a fairly nasty problem, I suspect it's just not very commonly seen since creating two authorised values with the same "Authorised value" value is probably quite rare. I'll create a bug to start a discussion on it.
No problem ;) Some of the authorised values are numeric (lost, damaged, restricted, not_for_loan), so it's very safe to assume that there is a lot of double ups actually there and in other places. I think other parts of the code MUST deal with it in some way. It's the reason why I didn't set FQA last time but "In discussion" actually. Getting some discussion on how to deal with this among the devs familiar with Koha::Object is probably best.
A first idea I had was to add prefixes... CATEGORY_AUTHVALUE, but the numerical ones are a problem there. The datatypes won't make it possible without more changes. Having a new table for each of those is also not the right way. Would a combined primary key not solve the issue? (category, authorised_value) - those ARE unique and enforced.
Hmmm, prefixes feels like a hack. Using a composite primary key as you suggest might be a step in the right direction. However, as I understand it, to then use that as a foreign key in other tables would require each table to have a corresponding column for each of the authorised_values columns that comprise the key. As an aside, I'm not convinced the category and authorised_value columns do currently have any unique constraint on them. Looking at the schema, it looks like category, authorised_value and lib are defined as keys, but I can only think that was does to create indexes on them as there appears to be no unique constraints on them. Definitely worth a discussion I think...
(In reply to Andrew Isherwood from comment #159) > Hmmm, prefixes feels like a hack. > > Using a composite primary key as you suggest might be a step in the right > direction. However, as I understand it, to then use that as a foreign key in > other tables would require each table to have a corresponding column for > each of the authorised_values columns that comprise the key. > > As an aside, I'm not convinced the category and authorised_value columns do > currently have any unique constraint on them. Looking at the schema, it > looks like category, authorised_value and lib are defined as keys, but I can > only think that was does to create indexes on them as there appears to be no > unique constraints on them. > > Definitely worth a discussion I think... It might only be enforced on GUI side. Jonathan worked on some of this as part of the move to Koha introducing the authorised_value_category table as a first step to improve the data structures. Looking at some of this code might be helpful? Note: In order to translate the code to a description you can use TT plugins. No needs to pass the descriptions usually - so that might avoid some of the issue?
Created attachment 84814 [details] [review] Bug 20581: (follow-up) Overload status_alias We need Koha::Illrequest->status_alias to return the correct authorised value. Since the foreign key uses a non-unique column, we need to take into account the authorised value category too. This patch overloads status_alias with some additional logic that considers the request's branch and the authorised value category to attempt to return the correct value. The same logic is applied to the custom statusalias method, which returns the entire AuthorisedValue object
Created attachment 84818 [details] [review] Bug 20581: (follow-up) Fix status_alias comparison status_alias will arrive in the Koha object as a string, so when we special case it, we should also pass a string
Created attachment 84819 [details] [review] Bug 20581: (follow-up) Fix unit test Since we're now storing the authorised_value column as the foreign key, rather than the id, we need to account for this in the test
Andrew, can this be switched back to Signed off = ready for more QA?
Sorry Andrew, I wish I had better news: - Correct descriptions now show But: When I edit one of the ILL requests I set to "Internal processing" and change it back to "New request" it doesn't take on saving. I stays at the custom status and I can't change it. There is no error in the plack logs, just: [Mon Feb 4 19:33:12 2019] [debug] GET "/api/v1/illrequests" [Mon Feb 4 19:33:12 2019] [debug] Routing to controller "Koha::REST::V1::Auth" and action "under" [Mon Feb 4 19:33:12 2019] [debug] Routing to controller "Koha::REST::V1::Illrequests" and action "list" [Mon Feb 4 19:33:12 2019] [debug] 200 OK (0.0728s, 13.736/s) This happens for both my FreeForm and BLDSS requests. If I confirm the request, setting "Requested" seems to work ok, But in the datbase I see -1 for alias_status. I'd say this should be NULL or "", as -1 seems unusual and a bit dangerous (considering the 'on order' authorised value is -1 for example) Not sure if this is related to the patch, but noting just in case: I can't confirm the request either, but get an error instead. This is a BLDSS request - might just be related to that as my FreeForm one behaves nicely.
I am wondering about the intended mechanics here. Looking at the test plan again maybe setting it "back" was not an intended behaviour? I am wondering if you could think of the status_alias as a sub_status to the status and in that case if it would not be better to display them both separately. If the ILL request is "new" + "custom", "confirmed" + "custom" etc. And offer an empty option in the pull down for unsetting? I might not fully grasp the use case yet :(
Hi Katrin Thanks very much for the info. A bit of context might be in order. An ILL request has, at any given time, a single "status". The workflow the request passes through is dictated by those statuses. The ILL module has the idea of a "status graph", which essentially says "if you're at this status, you can have come from this/these status(es) and you can go to this/these status(es)." This is how we can determine what actions the user can take on any given request. This bug adds the concept of status aliases, which are user defined as authorised values. These have no bearing on any of the above, the request's status is still used in the same way. The status alias is purely used for display. The display logic says "if a request has a status alias, display that, otherwise display the request's status". This enables the user to select one of their user defined status aliases, which will be displayed in favour of the request's "actual" status. This allows the user to assign a more locally meaningful status to a request, rather than rely on those provided by ILL. I mentioned above that the status alias has no bearing on the actual status. The request's status is still there, and still determines, say, which action buttons are displayed for a request, but it is not displayed if an alias is also assigned to that request. However, if a request that has a status alias is moved forward through the workflow by clicking one of the action buttons (e.g. Confirm request), we assume that the status alias is no longer applicable and nullify it in the request, causing the request's actual status to now display, until such a time as the user selects a new status alias. So, to address what you're seeing above with -1 being assigned. In the "Edit request" screen, the status dropdown contains all of the status aliases PLUS an option for the request's actual status, which effectively allows the user to nullify the request's status alias and revert the request to the actual status. You said you selected "New request", which will have been the request's actual status. But it sounds like, rather than nullifying the status alias, it set it to -1. Which is odd. I'll investigate and get back to you. Thanks again for your ongoing efforts on getting this one through!
Created attachment 84844 [details] [review] Bug 20581: (follow-up) Passed value to status_alias Now we have overloaded status_alias, we can no longer pass it undef (as it uses that to determine if it's being used as a getter or setter). So we now pass it -1 to indicate it should actually be setting undef (a hack, I know, but what are you going to do...) This patch makes sure ill-request.pl conforms to that. It also fixes the testing of the passed parameter, which was wrong.
(In reply to Katrin Fischer from comment #165) > When I edit one of the ILL requests I set to "Internal processing" and > change it back to "New request" it doesn't take on saving. I stays at the > custom status and I can't change it. This is now fixed. The hackery I had to introduce into Illrequest.pm requires a special casing for when we want to nullify status_alias, I had forgotten to introduce that here. > If I confirm the request, setting "Requested" seems to work ok, But in the > datbase I see -1 for alias_status. See above > Not sure if this is related to the patch, but noting just in case: I can't > confirm the request either, but get an error instead. This is a BLDSS > request - might just be related to that as my FreeForm one behaves nicely. This is unrelated, it does actually appear that the BLDSS API is suddenly returning an error for the confirm call :( I'll investigate...
Created attachment 84911 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Following commit was squashed into this one: Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84912 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84913 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84914 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84915 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84916 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84917 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84918 [details] [review] Bug 20581: (follow-up) Allow for undefined lib As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c59 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84919 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84920 [details] [review] Bug 20581: (follow-up) Fix fallback status_alias As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c68 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84921 [details] [review] Bug 20581: (follow-up) Fix AV population As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c72 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84922 [details] [review] Bug 20581: (follow-up) Modify POD As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c69 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84923 [details] [review] Bug 20581: (follow-up) Fix typo in kohastructure Fix the missing comma Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84924 [details] [review] Bug 20581: (follow-up) Fix QA errors This patch fixes the "missing_filter" errors on ill-requests.tt Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84925 [details] [review] Bug 20581: (follow-up) Fix dependency requirement Since this bug is now dependent on Bug 20600 (Provide the ability for users to filter ILL requests) we need to make the status filtering aware of the custom statuses. This patch does that. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84926 [details] [review] Bug 20581: (follow-up) Do not use PK for AV FK As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c133 We switch from using authorised_values.id when creating the foreign key illrequests.status_alias, we now use authorised_values.authorised_value I have added a migration from using id -> authorised_value, so when existing users of this bug get this version, their DB migrates nicely instead of just breaking. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84927 [details] [review] Bug 20581: (follow-up) Fix typo in DB upgrade IN -> ON Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84928 [details] [review] Bug 20581: (follow-up) Overload status_alias We need Koha::Illrequest->status_alias to return the correct authorised value. Since the foreign key uses a non-unique column, we need to take into account the authorised value category too. This patch overloads status_alias with some additional logic that considers the request's branch and the authorised value category to attempt to return the correct value. The same logic is applied to the custom statusalias method, which returns the entire AuthorisedValue object Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84929 [details] [review] Bug 20581: (follow-up) Fix status_alias comparison status_alias will arrive in the Koha object as a string, so when we special case it, we should also pass a string Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84930 [details] [review] Bug 20581: (follow-up) Fix unit test Since we're now storing the authorised_value column as the foreign key, rather than the id, we need to account for this in the test Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84931 [details] [review] Bug 20581: (follow-up) Passed value to status_alias Now we have overloaded status_alias, we can no longer pass it undef (as it uses that to determine if it's being used as a getter or setter). So we now pass it -1 to indicate it should actually be setting undef (a hack, I know, but what are you going to do...) This patch makes sure ill-request.pl conforms to that. It also fixes the testing of the passed parameter, which was wrong. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I still have one issue with this: I don't like -1 in illrequests.status_alias meaning 'no status set'. I'd be much happier with NULL. But I think we should be able to work this out separately from this bug and maybe at the end or on another branch of the 'ILL tree'. Maybe others can give some input then too on how to handle the situation with the Authorised values best. The functionality works now well enough for me to pass it and move on.
(In reply to Katrin Fischer from comment #191) > I don't like -1 in illrequests.status_alias meaning 'no status set'. I'd be > much happier with NULL. That's a bug, it should not be recording -1 for no status_alias, it should indeed be NULL. If it's saving -1 that's potentially very problematic. Is it definitely still doing that? The fix I did the other day should have resolved that. I'll set this to Failed QA and investigate tomorrow.
(In reply to Andrew Isherwood from comment #192) > (In reply to Katrin Fischer from comment #191) > > > I don't like -1 in illrequests.status_alias meaning 'no status set'. I'd be > > much happier with NULL. > > That's a bug, it should not be recording -1 for no status_alias, it should > indeed be NULL. If it's saving -1 that's potentially very problematic. Is it > definitely still doing that? The fix I did the other day should have > resolved that. I'll set this to Failed QA and investigate tomorrow. Thx for the reply! Yes, it still does, my illrequets are both set to -1 now. Unsetting and resetting status seem to work ok, so I assumed it was intentional at this point.
Created attachment 84964 [details] [review] Bug 20581: (follow-up) Fix status_alias -1 bug Stupid bug, we need to pass NULL not -1 when unsetting status_alias
Created attachment 85075 [details] [review] Bug 20581: Modifications to database schema - This patch provides an illrequests.status_alias column that is a foreign key to authorised_values.id. This provides the ability for an ILL request to have an optional status alias that is defined as an authorised valued. - A new ILLSTATUS authorised value category is also made available during install. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Following commit was squashed into this one: Bug 20581: (follow-up) Modify DB updgrade As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c56 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85076 [details] [review] Bug 20581: Provide status_alias on IllRequest This patch provides status_alias support for the Koha::Illrequest object. - New 'statusalias' getter method, returns the request's status_alias authorised value object if set, otherwise undef - Overloads existing 'status' method, getter/setter accessors intact, but setting the request's status now implicitly removes any status_alias that has been set Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85077 [details] [review] Bug 20581: API provide status_alias embed - This patch provides the illrequests API route with an additional embed, called 'status_alias'. Requesting this embed returns an object for the request's status_alias authorised value, or null if one is not set. - This patch also adds the new status_alias as a default embed on the API call made by the illlist view Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85078 [details] [review] Bug 20581: Add display of status_alias to staff vw - This patch adds display of the new status_alias value, when set, in favour of the request's status value. - It also allows the user to set a status_alias for an existing request. When set, this value will be displayed in preference to the status. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85079 [details] [review] Bug 20581: Fix display of status alias in OPAC Fix for status alias display in OPAC, we should be displaying the status name, not the status code. Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85080 [details] [review] Bug 20581: Unit tests for status_alias This patch adds unit tests for the specific status_alias functionality added in this bug - Creation of the ILLSTATUS authorised value - Illrequest->statusalias accessor - Illrequest->status overloading to reset status_alias To test: 1) Apply this patch 2) prove t/db_dependent/Illrequests.t Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85081 [details] [review] Bug 20581: (follow-up) Display status name Rather than display the status code for custom statuses, we should be displaying the name Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85082 [details] [review] Bug 20581: (follow-up) Allow for undefined lib As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c59 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85083 [details] [review] Bug 20581: (follow-up) Fix statusalias return As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c57 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85084 [details] [review] Bug 20581: (follow-up) Fix fallback status_alias As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c68 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85085 [details] [review] Bug 20581: (follow-up) Fix AV population As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c72 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85086 [details] [review] Bug 20581: (follow-up) Modify POD As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c69 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85087 [details] [review] Bug 20581: (follow-up) Fix typo in kohastructure Fix the missing comma Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85088 [details] [review] Bug 20581: (follow-up) Fix QA errors This patch fixes the "missing_filter" errors on ill-requests.tt Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85089 [details] [review] Bug 20581: (follow-up) Fix dependency requirement Since this bug is now dependent on Bug 20600 (Provide the ability for users to filter ILL requests) we need to make the status filtering aware of the custom statuses. This patch does that. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85090 [details] [review] Bug 20581: (follow-up) Do not use PK for AV FK As per: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20581#c133 We switch from using authorised_values.id when creating the foreign key illrequests.status_alias, we now use authorised_values.authorised_value I have added a migration from using id -> authorised_value, so when existing users of this bug get this version, their DB migrates nicely instead of just breaking. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85091 [details] [review] Bug 20581: (follow-up) Fix typo in DB upgrade IN -> ON Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85092 [details] [review] Bug 20581: (follow-up) Overload status_alias We need Koha::Illrequest->status_alias to return the correct authorised value. Since the foreign key uses a non-unique column, we need to take into account the authorised value category too. This patch overloads status_alias with some additional logic that considers the request's branch and the authorised value category to attempt to return the correct value. The same logic is applied to the custom statusalias method, which returns the entire AuthorisedValue object Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85093 [details] [review] Bug 20581: (follow-up) Fix status_alias comparison status_alias will arrive in the Koha object as a string, so when we special case it, we should also pass a string Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85094 [details] [review] Bug 20581: (follow-up) Fix unit test Since we're now storing the authorised_value column as the foreign key, rather than the id, we need to account for this in the test Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85095 [details] [review] Bug 20581: (follow-up) Passed value to status_alias Now we have overloaded status_alias, we can no longer pass it undef (as it uses that to determine if it's being used as a getter or setter). So we now pass it -1 to indicate it should actually be setting undef (a hack, I know, but what are you going to do...) This patch makes sure ill-request.pl conforms to that. It also fixes the testing of the passed parameter, which was wrong. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85096 [details] [review] Bug 20581: (follow-up) Fix status_alias -1 bug Stupid bug, we need to pass NULL not -1 when unsetting status_alias Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Awesome work all! Pushed to master for 19.05
Enhancement, will not be backported to 18.11.x series