Bug 35263 - Cannot update patron categories
Summary: Cannot update patron categories
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Pedro Amorim
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 18203
Blocks: 35556
  Show dependency treegraph
 
Reported: 2023-11-05 12:23 UTC by Katrin Fischer
Modified: 2024-01-05 05:51 UTC (History)
4 users (show)

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


Attachments
Bug 35263: Add selenium tests (1.53 KB, patch)
2023-11-06 10:16 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35263: Default 'can_place_ill_in_opac' to 1 (1.56 KB, patch)
2023-11-06 10:16 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35263: Add selenium tests (1.59 KB, patch)
2023-11-06 19:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35263: Default 'can_place_ill_in_opac' to 1 (1.62 KB, patch)
2023-11-06 19:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35263: Add selenium tests (1.64 KB, patch)
2023-11-06 21:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 35263: Default 'can_place_ill_in_opac' to 1 (1.67 KB, patch)
2023-11-06 21:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 35263: QA follow-up: Update code (1.48 KB, patch)
2023-11-07 10:29 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35263: Add selenium tests (1.64 KB, patch)
2023-11-07 21:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35263: Default 'can_place_ill_in_opac' to 1 (1.67 KB, patch)
2023-11-07 21:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35263: (QA follow-up) Update code (1.48 KB, patch)
2023-11-07 21:53 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-11-05 12:23:13 UTC
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.
Comment 1 Pedro Amorim 2023-11-06 10:16:46 UTC
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.
Comment 2 Pedro Amorim 2023-11-06 10:16:48 UTC
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.
Comment 3 Katrin Fischer 2023-11-06 19:19:48 UTC
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>
Comment 4 Katrin Fischer 2023-11-06 19:19:50 UTC
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>
Comment 5 David Nind 2023-11-06 20:18:51 UTC
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...
Comment 6 Katrin Fischer 2023-11-06 20:55:38 UTC
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!
Comment 7 Katrin Fischer 2023-11-06 20:59:00 UTC
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
Comment 8 David Nind 2023-11-06 21:12:00 UTC
(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).
Comment 9 David Nind 2023-11-06 21:12:48 UTC
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>
Comment 10 David Nind 2023-11-06 21:12:51 UTC
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>
Comment 11 Jonathan Druart 2023-11-07 09:33:48 UTC
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;
Comment 12 Pedro Amorim 2023-11-07 10:29:27 UTC
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.'
Comment 13 Katrin Fischer 2023-11-07 21:51:37 UTC
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.
Comment 14 Katrin Fischer 2023-11-07 21:53:12 UTC
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>
Comment 15 Katrin Fischer 2023-11-07 21:53:15 UTC
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>
Comment 16 Katrin Fischer 2023-11-07 21:53:18 UTC
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.'
Comment 17 Katrin Fischer 2023-11-07 21:55:29 UTC
Thanks a lot for the commit message, I also learned the difference today :)
Comment 18 Tomás Cohen Arazi 2023-11-08 13:00:15 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 19 Fridolin Somers 2023-11-10 01:54:17 UTC
Depends on Bug 18203 not in 23.05.x