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: Needs Signoff
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: 2018-10-17 21:53 UTC (History)
4 users (show)

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


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

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