Bug 21877 - Show authorized value description for withdrawn in checkout
Summary: Show authorized value description for withdrawn in checkout
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin SOMERS
QA Contact: Testopia
URL:
Keywords:
Depends on: 20341
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-23 13:52 UTC by Fridolin SOMERS
Modified: 2019-02-25 09:38 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21877: Show authorized value description for withdrawn in checkout (2.52 KB, patch)
2018-11-23 14:03 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21877: Show authorized value description for withdrawn in checkout (2.58 KB, patch)
2018-11-23 22:02 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 21877: [ALTERNATIVE-PATCH] Display authorized value description for withdrawn in checkout (2.58 KB, patch)
2018-12-19 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21877: Show authorized value description for withdrawn in checkout (2.03 KB, patch)
2019-01-22 09:02 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21877: Use AuthorisedValues.GetDescriptionByKohaField for notforloan in checkout (3.52 KB, patch)
2019-01-22 09:02 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21877: Show authorized value description for withdrawn in checkout (2.09 KB, patch)
2019-01-25 20:34 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 21877: Use AuthorisedValues.GetDescriptionByKohaField for notforloan in checkout (3.58 KB, patch)
2019-01-25 20:34 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 21877: Show authorized value description for withdrawn in checkout (2.16 KB, patch)
2019-01-26 13:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21877: Use AuthorisedValues.GetDescriptionByKohaField for notforloan in checkout (3.64 KB, patch)
2019-01-26 13:31 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2018-11-23 13:52:47 UTC
During checkout the withdrawn information is displayed in a message :
"Item has been withdrawn".

Like in other pages and like notforloan, we should display the withdrawn authorized value description.
Comment 1 Fridolin SOMERS 2018-11-23 14:03:12 UTC Comment hidden (obsolete)
Comment 2 Pierre-Marc Thibault 2018-11-23 22:02:42 UTC
Created attachment 82624 [details] [review]
Bug 21877: Show authorized value description for withdrawn in checkout

During checkout the withdrawn information is displayed in a message :
"Item has been withdrawn".

Like in other pages and like notforloan, we should display the withdrawn authorized value description.

This patch adds this display.
Add <span> so that translation does not change.
Also adds class 'co-withdrawn' (like in checkin 'ci-*' classes) to ease hidding this new information via CSS.

Test plan :
1) On a catalog with items.withdrawn defined with authorized values category WITHDRAWN
2) Define in WITHDRAWN an authorized values 1 with description 'dropped in trash'
3) Define in WITHDRAWN an authorized values 2 with description empty
4) Edit an item with withdrawn=1
5) Checkout this item => You see 'Item has been withdrawn (dropped in trash)'
6) Edit an item with withdrawn=2
7) Checkout this item => You see 'Item has been withdrawn'

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Comment 3 Jonathan Druart 2018-12-05 22:11:38 UTC
Why are not you doing it the same way as notforloan?

Or provide tests, but I'd prefer consistency...
Comment 4 Fridolin SOMERS 2018-12-18 09:44:14 UTC
(In reply to Jonathan Druart from comment #3)
> Why are not you doing it the same way as notforloan?
> 
> Or provide tests, but I'd prefer consistency...

In Circulation.pm its the same :
  $needsconfirmation{item_notforloan} = $item->{'notforloan'};
  $issuingimpossible{item_withdrawn} = $item->{'withdrawn'};

In circulation.tt :

notforloan uses AuthorisedValues.GetByCode( authvalcode_notforloan ...
authvalcode_notforloan is computed in circulation.pl
this useful because it is used several times in circulation.tt.

But for withdrawn the authorised value is used only once.
Using AuthorisedValues.GetDescriptionByKohaField is better for MVC separation no ?

I could create another bug to change notforloan maybe ?
Comment 5 Jonathan Druart 2018-12-19 18:31:54 UTC
Created attachment 83400 [details] [review]
Bug 21877: [ALTERNATIVE-PATCH] Display authorized value description for withdrawn in checkout
Comment 6 Jonathan Druart 2018-12-19 18:34:01 UTC
(In reply to Jonathan Druart from comment #5)
> Created attachment 83400 [details] [review] [review]
> Bug 21877: [ALTERNATIVE-PATCH] Display authorized value description for
> withdrawn in checkout

Hi Frido,
This is what I had in mind.
It will avoid adding more stuff to C4::Circulation as well as unify the way we display/fetch the values

I would prefer this approach and provide selenium tests.

Note that another occurrence of notforloan must be replaced in circulation.tt

What do you think?
Comment 7 Fridolin SOMERS 2019-01-22 07:32:53 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Jonathan Druart from comment #5)
> > Created attachment 83400 [details] [review] [review] [review]
> > Bug 21877: [ALTERNATIVE-PATCH] Display authorized value description for
> > withdrawn in checkout
> 
> Hi Frido,
> This is what I had in mind.
> It will avoid adding more stuff to C4::Circulation as well as unify the way
> we display/fetch the values
> 
> I would prefer this approach and provide selenium tests.
> 
> Note that another occurrence of notforloan must be replaced in circulation.tt
> 
> What do you think?

OK better indeed.
I will signoff.
Comment 8 Fridolin SOMERS 2019-01-22 09:01:46 UTC
Ah in the end I review all the code and split into 2 patches :
- adding withdrawn description
- changing the way notforloan description is fetched
Comment 9 Fridolin SOMERS 2019-01-22 09:02:08 UTC
Created attachment 84273 [details] [review]
Bug 21877: Show authorized value description for withdrawn in checkout

During checkout the withdrawn information is displayed in a message :
"Item has been withdrawn".

Like in other pages and like notforloan, we should display the withdrawn authorized value description.

This patch adds this display.
Add <span> so that translation does not change.
Also adds class 'co-withdrawn' (like in checkin 'ci-*' classes) to ease hidding this new information via CSS.

Test plan :
1) On a catalog with items.withdrawn defined with authorized values category WITHDRAWN
2) Define in WITHDRAWN an authorized values 1 with description 'dropped in trash'
3) Define in WITHDRAWN an authorized values 2 with description empty
4) Edit an item with withdrawn=1
5) Checkout this item => You see 'Item has been withdrawn (dropped in trash)'
6) Edit an item with withdrawn=2
7) Checkout this item => You see 'Item has been withdrawn'
Comment 10 Fridolin SOMERS 2019-01-22 09:02:20 UTC
Created attachment 84274 [details] [review]
Bug 21877: Use AuthorisedValues.GetDescriptionByKohaField for notforloan in checkout

Using only TT plugin for authorised value description is a better code.

Test plan :
Part 1:
1.1) On a catalog with items.notforloan defined with authorized values category NOT_LOAN
1.2) Define in NOT_LOAN an authorized values 1 with description 'at the beach'
1.3) Define in NOT_LOAN an authorized values 2 with description empty
Part 2:
2.1) Set preference 'AllowNotForLoanOverride' to 'Don't allow'
2.2) Edit an item with notforloan=1
2.3) Checkout this item => You see 'Item not for loan (at the beach)'
2.4) Edit an item with notforloan=2
2.5) Checkout this item => You see 'Item not for loan'
Part 3:
3.1) Set preference 'AllowNotForLoanOverride' to 'Allow'
3.2) Edit an item with notforloan=1
3.3) Checkout this item => You see 'Item is normally not for loan (at the beach)'
3.4) Edit an item with notforloan=2
3.5) Checkout this item => You see 'Item is normally not for loan'
Comment 11 Pierre-Marc Thibault 2019-01-25 20:28:30 UTC
Both patches seems to be fine and I will probably sign them off.

When I do a test with NOT_LOAN and AV = 2, I see this : "Item not for loan .". There is a space character created by the dot no matter what the system preference setting is. 

I wonder if it can be fixed as well too.
Comment 12 Pierre-Marc Thibault 2019-01-25 20:34:56 UTC
Created attachment 84436 [details] [review]
Bug 21877: Show authorized value description for withdrawn in checkout

During checkout the withdrawn information is displayed in a message :
"Item has been withdrawn".

Like in other pages and like notforloan, we should display the withdrawn authorized value description.

This patch adds this display.
Add <span> so that translation does not change.
Also adds class 'co-withdrawn' (like in checkin 'ci-*' classes) to ease hidding this new information via CSS.

Test plan :
1) On a catalog with items.withdrawn defined with authorized values category WITHDRAWN
2) Define in WITHDRAWN an authorized values 1 with description 'dropped in trash'
3) Define in WITHDRAWN an authorized values 2 with description empty
4) Edit an item with withdrawn=1
5) Checkout this item => You see 'Item has been withdrawn (dropped in trash)'
6) Edit an item with withdrawn=2
7) Checkout this item => You see 'Item has been withdrawn'

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Comment 13 Pierre-Marc Thibault 2019-01-25 20:34:59 UTC
Created attachment 84437 [details] [review]
Bug 21877: Use AuthorisedValues.GetDescriptionByKohaField for notforloan in checkout

Using only TT plugin for authorised value description is a better code.

Test plan :
Part 1:
1.1) On a catalog with items.notforloan defined with authorized values category NOT_LOAN
1.2) Define in NOT_LOAN an authorized values 1 with description 'at the beach'
1.3) Define in NOT_LOAN an authorized values 2 with description empty
Part 2:
2.1) Set preference 'AllowNotForLoanOverride' to 'Don't allow'
2.2) Edit an item with notforloan=1
2.3) Checkout this item => You see 'Item not for loan (at the beach)'
2.4) Edit an item with notforloan=2
2.5) Checkout this item => You see 'Item not for loan'
Part 3:
3.1) Set preference 'AllowNotForLoanOverride' to 'Allow'
3.2) Edit an item with notforloan=1
3.3) Checkout this item => You see 'Item is normally not for loan (at the beach)'
3.4) Edit an item with notforloan=2
3.5) Checkout this item => You see 'Item is normally not for loan'

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Comment 14 Katrin Fischer 2019-01-26 13:31:40 UTC
Created attachment 84448 [details] [review]
Bug 21877: Show authorized value description for withdrawn in checkout

During checkout the withdrawn information is displayed in a message :
"Item has been withdrawn".

Like in other pages and like notforloan, we should display the withdrawn authorized value description.

This patch adds this display.
Add <span> so that translation does not change.
Also adds class 'co-withdrawn' (like in checkin 'ci-*' classes) to ease hidding this new information via CSS.

Test plan :
1) On a catalog with items.withdrawn defined with authorized values category WITHDRAWN
2) Define in WITHDRAWN an authorized values 1 with description 'dropped in trash'
3) Define in WITHDRAWN an authorized values 2 with description empty
4) Edit an item with withdrawn=1
5) Checkout this item => You see 'Item has been withdrawn (dropped in trash)'
6) Edit an item with withdrawn=2
7) Checkout this item => You see 'Item has been withdrawn'

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2019-01-26 13:31:44 UTC
Created attachment 84449 [details] [review]
Bug 21877: Use AuthorisedValues.GetDescriptionByKohaField for notforloan in checkout

Using only TT plugin for authorised value description is a better code.

Test plan :
Part 1:
1.1) On a catalog with items.notforloan defined with authorized values category NOT_LOAN
1.2) Define in NOT_LOAN an authorized values 1 with description 'at the beach'
1.3) Define in NOT_LOAN an authorized values 2 with description empty
Part 2:
2.1) Set preference 'AllowNotForLoanOverride' to 'Don't allow'
2.2) Edit an item with notforloan=1
2.3) Checkout this item => You see 'Item not for loan (at the beach)'
2.4) Edit an item with notforloan=2
2.5) Checkout this item => You see 'Item not for loan'
Part 3:
3.1) Set preference 'AllowNotForLoanOverride' to 'Allow'
3.2) Edit an item with notforloan=1
3.3) Checkout this item => You see 'Item is normally not for loan (at the beach)'
3.4) Edit an item with notforloan=2
3.5) Checkout this item => You see 'Item is normally not for loan'

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Nick Clemens 2019-01-28 14:14:24 UTC
Awesome work all!

Pushed to master for 19.05
Comment 17 Martin Renvoize 2019-02-01 11:58:42 UTC
Pushed to 18.11.x for 18.11.03
Comment 18 Lucas Gass 2019-02-05 23:13:20 UTC
pushed to 18.05.x for 18.05.09
Comment 19 Fridolin SOMERS 2019-02-12 15:06:50 UTC
Pushed to 17.11.x for 17.11.15