Bug 21503 - Update AuthorisedValues.pm to fall back to code if description doesn't exist
Summary: Update AuthorisedValues.pm to fall back to code if description doesn't exist
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Christopher Brannon
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-05 15:30 UTC by Christopher Brannon
Modified: 2020-11-30 21:48 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 21503: Fail authorised vaulues better/to code (2.04 KB, patch)
2018-10-05 22:33 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Fail authorised values better/to code (2.04 KB, patch)
2018-10-05 22:37 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Tests for the existing behaviour (4.41 KB, patch)
2018-10-08 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21503: Tests for the exiting behavior (3.65 KB, patch)
2018-10-11 22:28 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Tests for the existing behavior (5.41 KB, patch)
2018-10-12 15:37 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Tests for the existing behavior (5.41 KB, patch)
2018-10-12 18:11 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Tests for the existing behavior (5.41 KB, patch)
2018-10-12 18:17 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Fix AV to fail to other values (914 bytes, patch)
2018-10-12 19:01 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Tests for the existing behavior (5.36 KB, patch)
2018-10-12 19:21 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Fix AV to fail to other values (1.64 KB, patch)
2018-10-12 19:28 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Fix AV to fail to other values (1.64 KB, patch)
2018-10-12 19:38 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Tests cleanup and highlight a missing case (6.03 KB, patch)
2018-10-15 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Tests clarification for debugging (2.34 KB, patch)
2018-10-17 21:18 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Changed logic for checking values (1.29 KB, patch)
2018-10-17 21:21 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Simplify If Else logic (867 bytes, patch)
2018-10-17 21:53 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21503: Tests for the existing behavior (5.42 KB, patch)
2020-03-21 11:16 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21503: Fix AV to fail to other values (1.70 KB, patch)
2020-03-21 11:17 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21503: Tests cleanup and highlight a missing case (6.08 KB, patch)
2020-03-21 11:17 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21503: Tests clarification for debugging (2.40 KB, patch)
2020-03-21 11:17 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21503: Changed logic for checking values (1.35 KB, patch)
2020-03-21 11:18 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21503: Simplify If Else logic (929 bytes, patch)
2020-03-21 11:18 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21503: Tests for the existing behavior (5.49 KB, patch)
2020-03-24 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Fix AV to fail to other values (1.78 KB, patch)
2020-03-24 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Tests cleanup and highlight a missing case (6.16 KB, patch)
2020-03-24 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Tests clarification for debugging (2.47 KB, patch)
2020-03-24 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Changed logic for checking values (1.49 KB, patch)
2020-03-24 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Restore existing ternary operator logic (2.58 KB, patch)
2020-03-24 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Tests clarification for debugging (2.46 KB, patch)
2020-03-24 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Changed logic for checking values (1.49 KB, patch)
2020-03-24 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21503: Restore existing ternary operator logic (2.58 KB, patch)
2020-03-24 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2018-10-05 15:30:43 UTC
If depending on AuthorisedValues.pm (sub GetDescriptionByKohaField) for an authorised value, if a description doesn't exist, the description is left blank instead of failing to code.
Comment 1 Christopher Brannon 2018-10-05 22:33:18 UTC
Created attachment 80151 [details] [review]
Bug 21503: Fail authorised vaulues better/to code

To Test (Test both detail.pl and opac-detail.pl for functionality):
1) Apply patch (restart_all)
2) Look up an item that has a shelving location with a description
    defined (opac description too).
3) Remove opac description from shelving location.  Refresh
    opac-detail.pl and note that description fails back to the
    regular description.  Refresh detail.pl and note no change.
4) Remove regular description from shelving location.  Refresh
    opac-detail.pl and detail.pl.  Note that both default to the
    shelving location code.
5) Remove the shelving location from the LOC table all together.
    Refresh opac-detail.pl and detail.pl.  Note that both default to a
    blank shelving location.
Comment 2 Christopher Brannon 2018-10-05 22:37:07 UTC
Created attachment 80152 [details] [review]
Bug 21503: Fail authorised values better/to code

To Test (Test both detail.pl and opac-detail.pl for functionality):
1) Apply patch (restart_all)
2) Look up an item that has a shelving location with a description
    defined (opac description too).
3) Remove opac description from shelving location.  Refresh
    opac-detail.pl and note that description fails back to the
    regular description.  Refresh detail.pl and note no change.
4) Remove regular description from shelving location.  Refresh
    opac-detail.pl and detail.pl.  Note that both default to the
    shelving location code.
5) Remove the shelving location from the LOC table all together.
    Refresh opac-detail.pl and detail.pl.  Note that both default to a
    blank shelving location.
Comment 3 Tomás Cohen Arazi 2018-10-08 19:22:56 UTC
Created attachment 80233 [details] [review]
Bug 21503: Tests for the existing behaviour

This patch introduces tests for the current behaviour for the
AuthorisedValues template plugin.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Template/Plugin/AuthorisedValues.t
=> SUCCESS: Tests pass, all green!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Jonathan Druart 2018-10-09 17:21:42 UTC
I'd flush the cache again at the end of the tests to avoid side-effects in the following tests.
Comment 5 Christopher Brannon 2018-10-11 22:28:24 UTC
Created attachment 80439 [details] [review]
Bug 21503: Tests for the exiting behavior
Comment 6 Christopher Brannon 2018-10-11 22:51:51 UTC
Tomas, I have either failed at writing a good test or failed at my patch.  Would you take a look a the test and tell me what you think.  It seems to fail where it should with the master, but it is failing in other ways with the patch, yet the patch does what it is supposed to.  So, I don't know where I have gone astray.
Comment 7 Christopher Brannon 2018-10-11 22:53:30 UTC
Tomas, see comment 6.
Comment 8 Tomás Cohen Arazi 2018-10-12 00:47:51 UTC
(In reply to Christopher Brannon from comment #6)
> Tomas, I have either failed at writing a good test or failed at my patch. 
> Would you take a look a the test and tell me what you think.  It seems to
> fail where it should with the master, but it is failing in other ways with
> the patch, yet the patch does what it is supposed to.  So, I don't know
> where I have gone astray.

I wrote tests for the existing behavior, trying to cover all the use cases. The last two tests are for the situation in which the requested av doesn't exist.

What I understand is that you want to change the behavior so those two tests return the passed value instead of the empty string. So all you need to do (if I didn't miss something) is change those two tests, so they 'expect' the result is the passed value.

We can talk about it tomorrow!
Comment 9 Christopher Brannon 2018-10-12 15:37:16 UTC
Created attachment 80521 [details] [review]
Bug 21503: Tests for the existing behavior

This patch introduces 7 tests for the current behaviour for the
AuthorisedValues template plugin.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Template/Plugin/AuthorisedValues.t
=> SUCCESS: Tests pass, all green!
Comment 10 Christopher Brannon 2018-10-12 18:11:37 UTC
Created attachment 80544 [details] [review]
Bug 21503: Tests for the existing behavior

This patch introduces 7 tests for the current behaviour for the
AuthorisedValues template plugin.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Template/Plugin/AuthorisedValues.t
=> SUCCESS: Tests pass, all green!
Comment 11 Christopher Brannon 2018-10-12 18:17:36 UTC
Created attachment 80545 [details] [review]
Bug 21503: Tests for the existing behavior

This patch introduces 7 tests for the current behaviour for the
AuthorisedValues template plugin.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Template/Plugin/AuthorisedValues.t
=> SUCCESS: Tests pass, all green!
Comment 12 Christopher Brannon 2018-10-12 19:01:34 UTC
Created attachment 80558 [details] [review]
Bug 21503: Fix AV to fail to other values
Comment 13 Christopher Brannon 2018-10-12 19:21:26 UTC
Created attachment 80561 [details] [review]
Bug 21503: Tests for the existing behavior

This patch introduces 7 tests for the current behaviour for the
AuthorisedValues template plugin.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Template/Plugin/AuthorisedValues.t
=> SUCCESS: Tests pass, all green!
Comment 14 Christopher Brannon 2018-10-12 19:28:39 UTC
Created attachment 80562 [details] [review]
Bug 21503: Fix AV to fail to other values

To Test (Test both detail.pl and opac-detail.pl for functionality):
    1) Apply patch (restart_all)
    2) Look up an item that has a shelving location with a description
        defined (opac description too).
    3) Remove opac description from shelving location.  Refresh
        opac-detail.pl and note that description fails back to the
        regular description.  Refresh detail.pl and note no change.
    4) Remove regular description from shelving location.  Refresh
        opac-detail.pl and detail.pl.  Note that both default to the
        shelving location code.
    5) Remove the shelving location from the LOC table all together.
        Refresh opac-detail.pl and detail.pl.  Note that both default to
        the requested selving location code.
Comment 15 Christopher Brannon 2018-10-12 19:38:29 UTC
Created attachment 80563 [details] [review]
Bug 21503: Fix AV to fail to other values

To Test (Test both detail.pl and opac-detail.pl for functionality):
    1) Apply patch (restart_all)
    2) Look up an item that has a shelving location with a description
        defined (opac description too).
    3) Remove opac description from shelving location.  Refresh
        opac-detail.pl and note that description fails back to the
        regular description.  Refresh detail.pl and note no change.
    4) Remove regular description from shelving location.  Refresh
        opac-detail.pl and detail.pl.  Note that both default to the
        shelving location code.
    5) Remove the shelving location from the LOC table all together.
        Refresh opac-detail.pl and detail.pl.  Note that both default to
        the requested shelving location code.
Comment 16 Jonathan Druart 2018-10-15 13:50:08 UTC
Created attachment 80601 [details] [review]
Bug 21503: Tests cleanup and highlight a missing case

This patch does the following changes:
- Use build_object to create the MSS
- Store the generate AVs. That was not the case before and so we were
not testing the right things
- Split the tests into 2 parts: staff and OPAC, for readability
- Update the tests' descriptions
- Highlight a problem: If descriptions are missing for both OPAC and
staff, undef is returned. Is that what we expect? It seems that it is
not what were expecting the tests.
Comment 17 Jonathan Druart 2018-10-15 13:52:10 UTC
Hi Christopher, 2 things:
- I think I found a missing case in the previous patches
- You should not modify the patch from someone else. If you want to add/remove/alter lines from a previous patch you should attach a new patch
Comment 18 Christopher Brannon 2018-10-15 14:32:05 UTC
(In reply to Jonathan Druart from comment #16)
> Created attachment 80601 [details] [review] [review]
> Bug 21503: Tests cleanup and highlight a missing case
> 
> This patch does the following changes:
> - Use build_object to create the MSS
> - Store the generate AVs. That was not the case before and so we were
> not testing the right things
> - Split the tests into 2 parts: staff and OPAC, for readability
> - Update the tests' descriptions
> - Highlight a problem: If descriptions are missing for both OPAC and
> staff, undef is returned. Is that what we expect? It seems that it is
> not what were expecting the tests.

Perhaps the problem was with the fact that we weren't testing the right things?  I could not get a test cast to return null or empty if the authorised_value didn't exist.  It seemed to always return the authorised_value if there was nothing eles.  I assumed it was the non-existant parameter being searched, and so I left it at that.

Thank you for your help.  This is the firt time I've touched a unit test.

I think the expectation is that if there are no descriptions, then the authorised_value is displayed.  If there is no av, then it should either return the parameter that was searched or be blank.  Not sure which.
Comment 19 Christopher Brannon 2018-10-15 14:33:57 UTC
(In reply to Jonathan Druart from comment #17)
> Hi Christopher, 2 things:
> - I think I found a missing case in the previous patches
> - You should not modify the patch from someone else. If you want to
> add/remove/alter lines from a previous patch you should attach a new patch

I had removed that case, because I could not get it to ever be true.  Perhaps you fixed this.  I will check things.

Sorry, I didn't know.  I'll be more careful.  Tomas made the patch as an example for me, since this was my first time.
Comment 20 Christopher Brannon 2018-10-17 21:18:26 UTC
Created attachment 80758 [details] [review]
Bug 21503: Tests clarification for debugging
Comment 21 Christopher Brannon 2018-10-17 21:21:08 UTC
Created attachment 80759 [details] [review]
Bug 21503: Changed logic for checking values
Comment 22 Christopher Brannon 2018-10-17 21:53:47 UTC
Created attachment 80762 [details] [review]
Bug 21503: Simplify If Else logic
Comment 23 Fridolin Somers 2020-03-21 11:16:40 UTC
Created attachment 101323 [details] [review]
Bug 21503: Tests for the existing behavior

This patch introduces 7 tests for the current behaviour for the
AuthorisedValues template plugin.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Template/Plugin/AuthorisedValues.t
=> SUCCESS: Tests pass, all green!

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 24 Fridolin Somers 2020-03-21 11:17:04 UTC
Created attachment 101324 [details] [review]
Bug 21503: Fix AV to fail to other values

To Test (Test both detail.pl and opac-detail.pl for functionality):
    1) Apply patch (restart_all)
    2) Look up an item that has a shelving location with a description
        defined (opac description too).
    3) Remove opac description from shelving location.  Refresh
        opac-detail.pl and note that description fails back to the
        regular description.  Refresh detail.pl and note no change.
    4) Remove regular description from shelving location.  Refresh
        opac-detail.pl and detail.pl.  Note that both default to the
        shelving location code.
    5) Remove the shelving location from the LOC table all together.
        Refresh opac-detail.pl and detail.pl.  Note that both default to
        the requested shelving location code.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 25 Fridolin Somers 2020-03-21 11:17:34 UTC
Created attachment 101325 [details] [review]
Bug 21503: Tests cleanup and highlight a missing case

This patch does the following changes:
- Use build_object to create the MSS
- Store the generate AVs. That was not the case before and so we were
not testing the right things
- Split the tests into 2 parts: staff and OPAC, for readability
- Update the tests' descriptions
- Highlight a problem: If descriptions are missing for both OPAC and
staff, undef is returned. Is that what we expect? It seems that it is
not what were expecting the tests.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 26 Fridolin Somers 2020-03-21 11:17:52 UTC
Created attachment 101326 [details] [review]
Bug 21503: Tests clarification for debugging

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 27 Fridolin Somers 2020-03-21 11:18:11 UTC
Created attachment 101327 [details] [review]
Bug 21503: Changed logic for checking values

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 28 Fridolin Somers 2020-03-21 11:18:33 UTC
Created attachment 101328 [details] [review]
Bug 21503: Simplify If Else logic

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 29 Fridolin Somers 2020-03-21 11:21:11 UTC
Works as expected. This behavior makes sense to me.

Note for QA :

Some lines are missing ending ";"non ? Like : 
  return $av_check
Comment 30 Jonathan Druart 2020-03-24 10:06:46 UTC
Created attachment 101524 [details] [review]
Bug 21503: Tests for the existing behavior

This patch introduces 7 tests for the current behaviour for the
AuthorisedValues template plugin.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Template/Plugin/AuthorisedValues.t
=> SUCCESS: Tests pass, all green!

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2020-03-24 10:06:50 UTC
Created attachment 101525 [details] [review]
Bug 21503: Fix AV to fail to other values

To Test (Test both detail.pl and opac-detail.pl for functionality):
    1) Apply patch (restart_all)
    2) Look up an item that has a shelving location with a description
        defined (opac description too).
    3) Remove opac description from shelving location.  Refresh
        opac-detail.pl and note that description fails back to the
        regular description.  Refresh detail.pl and note no change.
    4) Remove regular description from shelving location.  Refresh
        opac-detail.pl and detail.pl.  Note that both default to the
        shelving location code.
    5) Remove the shelving location from the LOC table all together.
        Refresh opac-detail.pl and detail.pl.  Note that both default to
        the requested shelving location code.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2020-03-24 10:06:54 UTC
Created attachment 101526 [details] [review]
Bug 21503: Tests cleanup and highlight a missing case

This patch does the following changes:
- Use build_object to create the MSS
- Store the generate AVs. That was not the case before and so we were
not testing the right things
- Split the tests into 2 parts: staff and OPAC, for readability
- Update the tests' descriptions
- Highlight a problem: If descriptions are missing for both OPAC and
staff, undef is returned. Is that what we expect? It seems that it is
not what were expecting the tests.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2020-03-24 10:06:58 UTC
Created attachment 101527 [details] [review]
Bug 21503: Tests clarification for debugging

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 34 Jonathan Druart 2020-03-24 10:07:02 UTC
Created attachment 101528 [details] [review]
Bug 21503: Changed logic for checking values

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Bug 21503: Simplify If Else logic

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 35 Jonathan Druart 2020-03-24 10:07:07 UTC
Created attachment 101529 [details] [review]
Bug 21503: Restore existing ternary operator logic

This syntax is much more readable IMO. Precendent patch also missed the
case where the parameter was undef.
I think we are good now.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 36 Jonathan Druart 2020-03-24 10:25:24 UTC
Created attachment 101533 [details] [review]
Bug 21503: Tests clarification for debugging

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 37 Jonathan Druart 2020-03-24 10:25:28 UTC
Created attachment 101534 [details] [review]
Bug 21503: Changed logic for checking values

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Bug 21503: Simplify If Else logic

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 38 Jonathan Druart 2020-03-24 10:25:32 UTC
Created attachment 101535 [details] [review]
Bug 21503: Restore existing ternary operator logic

This syntax is much more readable IMO. Precendent patch also missed the
case where the parameter was undef.
I think we are good now.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 39 Martin Renvoize 2020-03-24 11:33:42 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 40 Joy Nelson 2020-04-03 21:56:21 UTC
not backported to 19.11.x