Updating patron categories in the administration module is broken. The error in the logs is: ==> /var/log/koha/kohadev/plack-intranet-error.log <== [2023/11/05 12:21:09] [WARN] DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Column 'can_place_ill_in_opac' cannot be null at /kohadevbox/koha/Koha/Object.pm line 170 ILL is not configured.
Created attachment 158495 [details] [review] Bug 35263: Add selenium tests Test plan, on k-t-d selenium (ktd --selenium up): 1) Apply just this tests patch 2) Run: prove t/db_dependent/selenium/administration_tasks.t 3) Notice it fails. Apply the other patch. Restart plack 4) Repeat step 2. Notice it passes.
Created attachment 158496 [details] [review] Bug 35263: Default 'can_place_ill_in_opac' to 1 If ILLModule sys pref is 'off', this input param is undef. The database does not allow it to be nullable causing an error. Check if its defined (i.e. the option was submitted) and use that, if not default to 1 Test plan, k-t-d: 1) Edit an existing patron category, visit: /cgi-bin/koha/admin/categories.pl?op=add_form&categorycode=B 2) Click 'Save' at the bottom of the form 3) Notice a yellow box is shown with an error message 4) Apply patch. Repeat steps 1-3. Notice it now updates successfully.
Created attachment 158543 [details] [review] Bug 35263: Add selenium tests Test plan, on k-t-d selenium (ktd --selenium up): 1) Apply just this tests patch 2) Run: prove t/db_dependent/selenium/administration_tasks.t 3) Notice it fails. Apply the other patch. Restart plack 4) Repeat step 2. Notice it passes. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158544 [details] [review] Bug 35263: Default 'can_place_ill_in_opac' to 1 If ILLModule sys pref is 'off', this input param is undef. The database does not allow it to be nullable causing an error. Check if its defined (i.e. the option was submitted) and use that, if not default to 1 Test plan, k-t-d: 1) Edit an existing patron category, visit: /cgi-bin/koha/admin/categories.pl?op=add_form&categorycode=B 2) Click 'Save' at the bottom of the form 3) Notice a yellow box is shown with an error message 4) Apply patch. Repeat steps 1-3. Notice it now updates successfully. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I was going to add my sign-off, so Katrin's could count as QA. However, the tests fail for me after the patch is applied - are there other system preferences that need enabling/disabling (tested using KTD)?: kohadev-koha@kohadevbox:koha(bz35263)$ prove t/db_dependent/selenium/administration_tasks.t t/db_dependent/selenium/administration_tasks.t .. Selenium server did not return proper status at /usr/share/perl5/Selenium/Remote/Driver.pm line 552. # Looks like your test exited with 22 before it could output anything. t/db_dependent/selenium/administration_tasks.t .. Dubious, test returned 22 (wstat 5632, 0x1600) Failed 3/3 subtests Test Summary Report ------------------- t/db_dependent/selenium/administration_tasks.t (Wstat: 5632 Tests: 0 Failed: 0) Non-zero exit status: 22 Parse errors: Bad plan. You planned 3 tests but ran 0. Files=1, Tests=0, 1 wallclock secs ( 0.00 usr 0.01 sys + 0.82 cusr 0.14 csys = 0.97 CPU) Result: FAIL I get the same result whether ILLModule is enabled or not. The patches do fix the error when editing a patron category though...
Hm, I just double checked, tests are passing nicely on a new database for me: OK admin/categories.pl OK t/db_dependent/selenium/administration_tasks.t Processing additional checks OK! Running tests (1) * Proving /kohadevbox/koha/t/db_dependent/selenium/administration_tasks.t OK!
kohadev-koha@kohadevbox:koha(71-35263-update-atron-category)$ prove t/db_dependent/selenium/administration_tasks.t t/db_dependent/selenium/administration_tasks.t .. ok All tests successful. Files=1, Tests=3, 18 wallclock secs ( 0.01 usr 0.01 sys + 1.03 cusr 0.15 csys = 1.20 CPU) Result: PASS
(In reply to David Nind from comment #5) > I was going to add my sign-off, so Katrin's could count as QA. > > However, the tests fail for me after the patch is applied - are there other > system preferences that need enabling/disabling (tested using KTD)?: > I didn't read the test plan properly - needed to start KTD with --selenium (ktd --selenium up).
Created attachment 158566 [details] [review] Bug 35263: Add selenium tests Test plan, on k-t-d selenium (ktd --selenium up): 1) Apply just this tests patch 2) Run: prove t/db_dependent/selenium/administration_tasks.t 3) Notice it fails. Apply the other patch. Restart plack 4) Repeat step 2. Notice it passes. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158567 [details] [review] Bug 35263: Default 'can_place_ill_in_opac' to 1 If ILLModule sys pref is 'off', this input param is undef. The database does not allow it to be nullable causing an error. Check if its defined (i.e. the option was submitted) and use that, if not default to 1 Test plan, k-t-d: 1) Edit an existing patron category, visit: /cgi-bin/koha/admin/categories.pl?op=add_form&categorycode=B 2) Click 'Save' at the bottom of the form 3) Notice a yellow box is shown with an error message 4) Apply patch. Repeat steps 1-3. Notice it now updates successfully. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com>
Will be better written this way IMO: my $can_place_ill_in_opac = defined $input->param('can_place_ill_in_opac') ? $input->param('can_place_ill_in_opac') : 1; => my $can_place_ill_in_opac = $input->param('can_place_ill_in_opac') // 1;
Created attachment 158588 [details] [review] Bug 35263: QA follow-up: Update code Thank you Jonathan. I first tried with '||' but it would fallback to 1 for either undef or 0 (we want the first case, but not the second). But using '//' instead, it only falls back to 1 if undef, and not when 0. Today I learned between '||' and '//'! Thanks! From perldoc.perl.org (relating to '//'): 'In fact, it's exactly the same as ||, except that it tests the left hand side's definedness instead of its truth.'
Processing additional checks * Commit title does not contain 'follow-up' correctly spelt - a0b13696d3 * Commit title does not contain '(QA follow-up)' correctly spelt - a0b13696d3 Fixing.
Created attachment 158633 [details] [review] Bug 35263: Add selenium tests Test plan, on k-t-d selenium (ktd --selenium up): 1) Apply just this tests patch 2) Run: prove t/db_dependent/selenium/administration_tasks.t 3) Notice it fails. Apply the other patch. Restart plack 4) Repeat step 2. Notice it passes. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158634 [details] [review] Bug 35263: Default 'can_place_ill_in_opac' to 1 If ILLModule sys pref is 'off', this input param is undef. The database does not allow it to be nullable causing an error. Check if its defined (i.e. the option was submitted) and use that, if not default to 1 Test plan, k-t-d: 1) Edit an existing patron category, visit: /cgi-bin/koha/admin/categories.pl?op=add_form&categorycode=B 2) Click 'Save' at the bottom of the form 3) Notice a yellow box is shown with an error message 4) Apply patch. Repeat steps 1-3. Notice it now updates successfully. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158635 [details] [review] Bug 35263: (QA follow-up) Update code Thank you Jonathan. I first tried with '||' but it would fallback to 1 for either undef or 0 (we want the first case, but not the second). But using '//' instead, it only falls back to 1 if undef, and not when 0. Today I learned between '||' and '//'! Thanks! From perldoc.perl.org (relating to '//'): 'In fact, it's exactly the same as ||, except that it tests the left hand side's definedness instead of its truth.'
Thanks a lot for the commit message, I also learned the difference today :)
Pushed to master for 23.11. Nice work everyone, thanks!
Depends on Bug 18203 not in 23.05.x