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.
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.
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.
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>
I'd flush the cache again at the end of the tests to avoid side-effects in the following tests.
Created attachment 80439 [details] [review] Bug 21503: Tests for the exiting behavior
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.
Tomas, see comment 6.
(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!
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!
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!
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!
Created attachment 80558 [details] [review] Bug 21503: Fix AV to fail to other values
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!
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.
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.
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.
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
(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.
(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.
Created attachment 80758 [details] [review] Bug 21503: Tests clarification for debugging
Created attachment 80759 [details] [review] Bug 21503: Changed logic for checking values
Created attachment 80762 [details] [review] Bug 21503: Simplify If Else logic
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>
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>
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>
Created attachment 101326 [details] [review] Bug 21503: Tests clarification for debugging Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 101327 [details] [review] Bug 21503: Changed logic for checking values Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 101328 [details] [review] Bug 21503: Simplify If Else logic Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Works as expected. This behavior makes sense to me. Note for QA : Some lines are missing ending ";"non ? Like : return $av_check
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>
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>
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>
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>
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>
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>
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>
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>
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>
Nice work everyone! Pushed to master for 20.05
not backported to 19.11.x