Bug 38287 - Saving default checkout, hold and return policy with empty bookings values causes error
Summary: Saving default checkout, hold and return policy with empty bookings values ca...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Pedro Amorim
QA Contact: Aleisha Amohia
URL:
Keywords:
Depends on: 34440
Blocks:
  Show dependency treegraph
 
Reported: 2024-10-29 13:59 UTC by Pedro Amorim
Modified: 2024-12-05 22:04 UTC (History)
7 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:
24.11.00
Circulation function: bookings


Attachments
Bug 38287: Add missing rule_value access (1.94 KB, patch)
2024-10-29 14:02 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38287: Clean-up unecessary input values (1.61 KB, patch)
2024-10-29 14:03 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38287: Add missing rule_value access (2.00 KB, patch)
2024-10-29 16:30 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 38287: Clean-up unecessary input values (1.67 KB, patch)
2024-10-29 16:30 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 38287: Add missing rule_value access (2.06 KB, patch)
2024-10-31 04:36 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 38287: Clean-up unecessary input values (1.73 KB, patch)
2024-10-31 04:36 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-10-29 13:59:52 UTC

    
Comment 1 Pedro Amorim 2024-10-29 14:02:22 UTC
Created attachment 173648 [details] [review]
Bug 38287: Add missing rule_value access

Test plan:
1) Access circuation rules:
http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
2) Scroll down to "Default checkout, hold and return policy". Enter nothing. Hit 'Save'.
3) Notice on page reload there is a Koha:: object on the bookings inputs
4) Save again -> ERROR
5) Apply patch. Repeat.
Comment 2 Pedro Amorim 2024-10-29 14:03:08 UTC
Created attachment 173649 [details] [review]
Bug 38287: Clean-up unecessary input values

These are inputs for new entries, no values are shown or needed here
Comment 3 Biblibre Sandboxes 2024-10-29 16:30:54 UTC
Created attachment 173681 [details] [review]
Bug 38287: Add missing rule_value access

Test plan:
1) Access circuation rules:
http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
2) Scroll down to "Default checkout, hold and return policy". Enter nothing. Hit 'Save'.
3) Notice on page reload there is a Koha:: object on the bookings inputs
4) Save again -> ERROR
5) Apply patch. Repeat.

Signed-off-by: Sam Sowanick <sam.sowanick@corvallisoregon.gov>
Comment 4 Biblibre Sandboxes 2024-10-29 16:30:56 UTC
Created attachment 173682 [details] [review]
Bug 38287: Clean-up unecessary input values

These are inputs for new entries, no values are shown or needed here

Signed-off-by: Sam Sowanick <sam.sowanick@corvallisoregon.gov>
Comment 5 Aleisha Amohia 2024-10-31 04:36:55 UTC
Created attachment 173743 [details] [review]
Bug 38287: Add missing rule_value access

Test plan:
1) Access circuation rules:
http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
2) Scroll down to "Default checkout, hold and return policy". Enter nothing. Hit 'Save'.
3) Notice on page reload there is a Koha:: object on the bookings inputs
4) Save again -> ERROR
5) Apply patch. Repeat.

Signed-off-by: Sam Sowanick <sam.sowanick@corvallisoregon.gov>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 6 Aleisha Amohia 2024-10-31 04:36:57 UTC
Created attachment 173744 [details] [review]
Bug 38287: Clean-up unecessary input values

These are inputs for new entries, no values are shown or needed here

Signed-off-by: Sam Sowanick <sam.sowanick@corvallisoregon.gov>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 7 Katrin Fischer 2024-10-31 09:48:51 UTC
I wonder if we are quite there yet here. I had added a rule leaving one field empty, but now the tests fail. Can you please check, Martin? Maybe there is more needed here.

    #   Failed test 'Koha::Item->find_booking returns undefined when the current booking is cancelled and the future booking is out of range'
    #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3022.
     #          got: 'Koha::Booking=HASH(0x55691b45d918)'
    #     expected: undef
 
        #   Failed test 'Koha::Item->find_booking returns undefined when the current booking is deleted and the future booking is out of range and there's no lead period rule'
        #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3040.
         #          got: 'Koha::Booking=HASH(0x55691b30fc00)'
        #     expected: undef
         # Looks like you failed 1 test of 3.
 
    #   Failed test 'Preparation period handling'
    #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3074.
     # Looks like you failed 2 tests of 8.
Comment 8 Pedro Amorim 2024-11-04 10:25:20 UTC
(In reply to Katrin Fischer from comment #7)
> I wonder if we are quite there yet here. I had added a rule leaving one
> field empty, but now the tests fail. Can you please check, Martin? Maybe
> there is more needed here.
> 
>     #   Failed test 'Koha::Item->find_booking returns undefined when the
> current booking is cancelled and the future booking is out of range'
>     #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3022.
>      #          got: 'Koha::Booking=HASH(0x55691b45d918)'
>     #     expected: undef
>  
>         #   Failed test 'Koha::Item->find_booking returns undefined when the
> current booking is deleted and the future booking is out of range and
> there's no lead period rule'
>         #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3040.
>          #          got: 'Koha::Booking=HASH(0x55691b30fc00)'
>         #     expected: undef
>          # Looks like you failed 1 test of 3.
>  
>     #   Failed test 'Preparation period handling'
>     #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3074.
>      # Looks like you failed 2 tests of 8.

This does not seem related. Tests pass for me before and applying the patches here.
Comment 9 Katrin Fischer 2024-11-07 13:22:29 UTC
(In reply to Pedro Amorim from comment #8)
> (In reply to Katrin Fischer from comment #7)
> > I wonder if we are quite there yet here. I had added a rule leaving one
> > field empty, but now the tests fail. Can you please check, Martin? Maybe
> > there is more needed here.
> > 
> >     #   Failed test 'Koha::Item->find_booking returns undefined when the
> > current booking is cancelled and the future booking is out of range'
> >     #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3022.
> >      #          got: 'Koha::Booking=HASH(0x55691b45d918)'
> >     #     expected: undef
> >  
> >         #   Failed test 'Koha::Item->find_booking returns undefined when the
> > current booking is deleted and the future booking is out of range and
> > there's no lead period rule'
> >         #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3040.
> >          #          got: 'Koha::Booking=HASH(0x55691b30fc00)'
> >         #     expected: undef
> >          # Looks like you failed 1 test of 3.
> >  
> >     #   Failed test 'Preparation period handling'
> >     #   at /kohadevbox/koha/t/db_dependent/Koha/Item.t line 3074.
> >      # Looks like you failed 2 tests of 8.
> 
> This does not seem related. Tests pass for me before and applying the
> patches here.

You need to add data, not just run the tests.
With the patch applied, add a rule that leaves these fields empty. 
Before the patch, the rule would not save, but now it does.
Run the test after.
Comment 10 Pedro Amorim 2024-11-07 13:55:01 UTC
(In reply to Katrin Fischer from comment #9)
> 
> You need to add data, not just run the tests.
> With the patch applied, add a rule that leaves these fields empty. 
> Before the patch, the rule would not save, but now it does.
> Run the test after.

I'm sorry, still can't reproduce.

$ prove t/db_dependent/Koha/Item.t

This is always passing for me. Before and after applying patch. Before and after adding "Default checkout, hold and return policy" with empty values for bookings input and different values for "Hold policy" and "Hold pickup library match".

I'm clearly missing something, can you please clarify further.
I want to understand how changes to a .tt file would break (or cause any change) to a Koha class .t tests file.
Comment 11 Katrin Fischer 2024-11-07 16:55:07 UTC
I've retested and I can't replicate the error now. 
Maybe it was resolved by some of the other bookings work pushed?

What I was thinking is:
* Before the patch you are prevented from adding empty cells (they don't save)
* After the patch they can be saved.
* So I had assumed that the data was breaking the test as we have no validation for these fields. I have filed a separate bug asking for validation as you can currently also add "a" as a value. (bug 38308)
Comment 12 Katrin Fischer 2024-11-07 17:51:28 UTC
Pushed for 24.11!

Well done everyone, thank you!