Bug 20581 - Allow manual selection of custom ILL request statuses
Summary: Allow manual selection of custom ILL request statuses
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andrew Isherwood
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on: 20600 7317
Blocks: 20639
  Show dependency treegraph
 
Reported: 2018-04-13 15:27 UTC by Andrew Isherwood
Modified: 2018-10-19 09:46 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20581: Modifications to database schema (3.87 KB, patch)
2018-04-17 13:54 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Provide status_alias on IllRequest (1.74 KB, patch)
2018-04-17 13:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: API provide status_alias embed (2.63 KB, patch)
2018-04-17 13:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Add display of status_alias to staff vw (7.52 KB, patch)
2018-04-17 13:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Add support for status_alias to OPAC view (2.11 KB, patch)
2018-04-17 13:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Unit tests for status_alias (2.66 KB, patch)
2018-04-17 16:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Modifications to database schema (3.87 KB, patch)
2018-05-15 14:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: DO NOT PUSH: Updated Schema Files. (2.95 KB, patch)
2018-05-15 14:23 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Display status name (2.17 KB, patch)
2018-06-05 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Modifications to database schema (3.87 KB, patch)
2018-08-15 14:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Provide status_alias on IllRequest (1.74 KB, patch)
2018-08-15 14:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: API provide status_alias embed (2.63 KB, patch)
2018-08-15 14:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Add display of status_alias to staff vw (7.52 KB, patch)
2018-08-15 14:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Add support for status_alias to OPAC view (2.11 KB, patch)
2018-08-15 14:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Unit tests for status_alias (2.66 KB, patch)
2018-08-15 14:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: DO NOT PUSH: Updated Schema Files. (2.95 KB, patch)
2018-08-15 14:56 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Display status name (2.17 KB, patch)
2018-08-15 14:57 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Modifications to database schema (3.87 KB, patch)
2018-09-03 13:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Provide status_alias on IllRequest (1.74 KB, patch)
2018-09-03 13:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: API provide status_alias embed (2.63 KB, patch)
2018-09-03 13:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Add display of status_alias to staff vw (7.66 KB, patch)
2018-09-03 13:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Add support for status_alias to OPAC view (2.17 KB, patch)
2018-09-03 13:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Unit tests for status_alias (2.66 KB, patch)
2018-09-03 13:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: DO NOT PUSH: Updated Schema Files. (2.95 KB, patch)
2018-09-03 13:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Display status name (2.19 KB, patch)
2018-09-03 13:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Fix display of status alias in OPAC (2.11 KB, patch)
2018-09-03 13:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Fix display of status alias in OPAC (2.12 KB, patch)
2018-09-05 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Modifications to database schema (3.92 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: Provide status_alias on IllRequest (1.79 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: API provide status_alias embed (2.68 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: Add display of status_alias to staff vw (7.71 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: Add support for status_alias to OPAC view (2.22 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: Unit tests for status_alias (2.71 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: DO NOT PUSH: Updated Schema Files. (3.00 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: (follow-up) Display status name (2.24 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: Fix display of status alias in OPAC (2.17 KB, patch)
2018-09-07 13:18 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20581: Modifications to database schema (3.92 KB, patch)
2018-10-09 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Provide status_alias on IllRequest (1.79 KB, patch)
2018-10-09 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: API provide status_alias embed (2.68 KB, patch)
2018-10-09 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Add display of status_alias to staff vw (7.72 KB, patch)
2018-10-09 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Add support for status_alias to OPAC view (2.22 KB, patch)
2018-10-09 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Unit tests for status_alias (2.71 KB, patch)
2018-10-09 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Display status name (2.25 KB, patch)
2018-10-09 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: Fix display of status alias in OPAC (2.17 KB, patch)
2018-10-09 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Modify DB updgrade (3.43 KB, patch)
2018-10-12 13:32 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Fix statusalias return (1.11 KB, patch)
2018-10-12 13:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Allow for undefined lib (1.13 KB, patch)
2018-10-12 14:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Modify DB updgrade (3.88 KB, patch)
2018-10-12 14:12 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Fix statusalias return (1.12 KB, patch)
2018-10-12 14:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Fix fallback status_alias (1.24 KB, patch)
2018-10-15 13:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Modify POD (806 bytes, patch)
2018-10-15 13:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20581: (follow-up) Fix AV population (2.14 KB, patch)
2018-10-15 13:49 UTC, Andrew Isherwood
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2018-04-13 15:27:12 UTC
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
Comment 1 Andrew Isherwood 2018-04-17 13:50:20 UTC
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
Comment 2 Andrew Isherwood 2018-04-17 13:54:04 UTC
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.
Comment 3 Andrew Isherwood 2018-04-17 13:55:30 UTC
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
Comment 4 Andrew Isherwood 2018-04-17 13:56:04 UTC
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
Comment 5 Andrew Isherwood 2018-04-17 13:56:31 UTC
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.
Comment 6 Andrew Isherwood 2018-04-17 13:56:54 UTC
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
Comment 7 Andrew Isherwood 2018-04-17 16:06:08 UTC
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
Comment 8 M. Tompsett 2018-04-18 01:31:34 UTC
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.
Comment 9 Andrew Isherwood 2018-04-18 09:02:12 UTC
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?
Comment 10 Katrin Fischer 2018-04-18 11:10:03 UTC
(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.
Comment 11 Andrew Isherwood 2018-04-18 13:02:18 UTC
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
Comment 12 Andrew Isherwood 2018-05-15 14:20:12 UTC
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.
Comment 13 Andrew Isherwood 2018-05-15 14:22:03 UTC
Added an updated patch "Bug 20581: Modifications to database schema" which fixes the filename of the atomicupdate file from .sql to .perl
Comment 14 Andrew Isherwood 2018-05-15 14:23:06 UTC
Created attachment 75334 [details] [review]
Bug 20581: DO NOT PUSH: Updated Schema Files.
Comment 15 Andrew Isherwood 2018-06-05 11:21:07 UTC
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
Comment 16 Andrew Isherwood 2018-08-15 14:56:24 UTC
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.
Comment 17 Andrew Isherwood 2018-08-15 14:56:33 UTC
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
Comment 18 Andrew Isherwood 2018-08-15 14:56:39 UTC
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
Comment 19 Andrew Isherwood 2018-08-15 14:56:44 UTC
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.
Comment 20 Andrew Isherwood 2018-08-15 14:56:49 UTC
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
Comment 21 Andrew Isherwood 2018-08-15 14:56:54 UTC
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
Comment 22 Andrew Isherwood 2018-08-15 14:56:59 UTC
Created attachment 77825 [details] [review]
Bug 20581: DO NOT PUSH: Updated Schema Files.
Comment 23 Andrew Isherwood 2018-08-15 14:57:05 UTC
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
Comment 24 Andrew Isherwood 2018-08-15 14:57:34 UTC
Rebased against current master
Comment 25 Andrew Isherwood 2018-09-03 13:18:27 UTC
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.
Comment 26 Andrew Isherwood 2018-09-03 13:18:33 UTC
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
Comment 27 Andrew Isherwood 2018-09-03 13:18:39 UTC
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
Comment 28 Andrew Isherwood 2018-09-03 13:18:44 UTC
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.
Comment 29 Andrew Isherwood 2018-09-03 13:18:49 UTC
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
Comment 30 Andrew Isherwood 2018-09-03 13:18:54 UTC
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
Comment 31 Andrew Isherwood 2018-09-03 13:19:00 UTC
Created attachment 78382 [details] [review]
Bug 20581: DO NOT PUSH: Updated Schema Files.
Comment 32 Andrew Isherwood 2018-09-03 13:19:07 UTC
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
Comment 33 Andrew Isherwood 2018-09-03 13:19:12 UTC
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.
Comment 34 Andrew Isherwood 2018-09-03 13:20:46 UTC
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
Comment 35 Andrew Isherwood 2018-09-03 13:21:11 UTC
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
Comment 36 Andrew Isherwood 2018-09-05 09:55:15 UTC
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.
Comment 37 PTFS Europe Sandboxes 2018-09-07 13:18:07 UTC
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
Comment 38 PTFS Europe Sandboxes 2018-09-07 13:18:11 UTC
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
Comment 39 PTFS Europe Sandboxes 2018-09-07 13:18:15 UTC
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
Comment 40 PTFS Europe Sandboxes 2018-09-07 13:18:19 UTC
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
Comment 41 PTFS Europe Sandboxes 2018-09-07 13:18:23 UTC
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
Comment 42 PTFS Europe Sandboxes 2018-09-07 13:18:27 UTC
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
Comment 43 PTFS Europe Sandboxes 2018-09-07 13:18:31 UTC
Created attachment 78521 [details] [review]
Bug 20581: DO NOT PUSH: Updated Schema Files.

Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Comment 44 PTFS Europe Sandboxes 2018-09-07 13:18:35 UTC
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
Comment 45 PTFS Europe Sandboxes 2018-09-07 13:18:39 UTC
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
Comment 46 Josef Moravec 2018-10-09 14:15:49 UTC
Could you rebase please?
Comment 47 Andrew Isherwood 2018-10-09 14:36:25 UTC
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
Comment 48 Andrew Isherwood 2018-10-09 14:36:30 UTC
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
Comment 49 Andrew Isherwood 2018-10-09 14:36:34 UTC
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
Comment 50 Andrew Isherwood 2018-10-09 14:36:39 UTC
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
Comment 51 Andrew Isherwood 2018-10-09 14:36:44 UTC
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
Comment 52 Andrew Isherwood 2018-10-09 14:36:48 UTC
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
Comment 53 Andrew Isherwood 2018-10-09 14:36:52 UTC
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
Comment 54 Andrew Isherwood 2018-10-09 14:36:57 UTC
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
Comment 55 Andrew Isherwood 2018-10-09 14:37:24 UTC
Done, thanks Josef!
Comment 56 Josef Moravec 2018-10-12 10:41:44 UTC
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 57 Josef Moravec 2018-10-12 11:00:31 UTC
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 58 Josef Moravec 2018-10-12 11:15:43 UTC
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 59 Josef Moravec 2018-10-12 11:22:21 UTC
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?
Comment 60 Josef Moravec 2018-10-12 12:16:17 UTC
Overall it is looking OK, just few relatively small issues I found (see above comments)

Great work.
Comment 61 Andrew Isherwood 2018-10-12 13:32:55 UTC
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
Comment 62 Andrew Isherwood 2018-10-12 13:33:00 UTC
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
Comment 63 Andrew Isherwood 2018-10-12 13:56:22 UTC
(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!
Comment 64 Andrew Isherwood 2018-10-12 14:09:35 UTC
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 65 Josef Moravec 2018-10-12 14:09:57 UTC
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
Comment 66 Andrew Isherwood 2018-10-12 14:12:37 UTC
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
Comment 67 Andrew Isherwood 2018-10-12 14:16:21 UTC
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 68 Josef Moravec 2018-10-12 14:41:43 UTC
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 69 Josef Moravec 2018-10-12 14:43:00 UTC
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
Comment 70 Josef Moravec 2018-10-12 15:14:28 UTC
One more think on authorised values: generaly it is better to take them in from AuthorisedValues template plugin
Comment 71 Andrew Isherwood 2018-10-15 13:34:35 UTC
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
Comment 72 Andrew Isherwood 2018-10-15 13:34:40 UTC
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
Comment 73 Andrew Isherwood 2018-10-15 13:49:53 UTC
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