Bug 37870 - UI/Form/Builder/Item.t and Biblio.t are still failing randomly (cn_source sort)
Summary: UI/Form/Builder/Item.t and Biblio.t are still failing randomly (cn_source sort)
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords:
: 37867 (view as bug list)
Depends on: 32710
Blocks: 25551 39462 40003
  Show dependency treegraph
 
Reported: 2024-09-09 14:36 UTC by Katrin Fischer
Modified: 2025-05-27 08:40 UTC (History)
7 users (show)

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


Attachments
Bug 37870: Remove warning if biblionumber is not passed (1.11 KB, patch)
2024-09-10 09:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37870: Fix sort order of class sources (6.39 KB, patch)
2024-09-10 09:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37870: Run tests from ClassSources.t within transactions (1.97 KB, patch)
2024-09-10 09:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37870: Remove warning if biblionumber is not passed (1.17 KB, patch)
2024-09-10 19:38 UTC, CJ Lynce
Details | Diff | Splinter Review
Bug 37870: Fix sort order of class sources (6.44 KB, patch)
2024-09-10 19:39 UTC, CJ Lynce
Details | Diff | Splinter Review
Bug 37870: Run tests from ClassSources.t within transactions (2.02 KB, patch)
2024-09-10 19:40 UTC, CJ Lynce
Details | Diff | Splinter Review
Bug 37870: Remove warning if biblionumber is not passed (1.22 KB, patch)
2024-09-11 13:12 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37870: Fix sort order of class sources (6.50 KB, patch)
2024-09-11 13:12 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37870: Run tests from ClassSources.t within transactions (2.08 KB, patch)
2024-09-11 13:12 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37870: Tidy (1.59 KB, patch)
2024-09-11 13:12 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2024-09-09 14:36:59 UTC
Jenkins is failing on:
t/db_dependent/Koha/UI/Form/Builder

D10 #1626
D12 #3027
Comment 1 Katrin Fischer 2024-09-09 14:37:21 UTC
Tests passing in ktd for me locally, but haven't tried with specific OS.
Comment 2 Jonathan Druart 2024-09-10 07:57:08 UTC
More info on bug 32710.
Comment 3 Jonathan Druart 2024-09-10 09:04:36 UTC
Created attachment 171242 [details] [review]
Bug 37870: Remove warning if biblionumber is not passed

Not directly related to the failure, only a bit of cleaning before
starting.
Comment 4 Jonathan Druart 2024-09-10 09:04:39 UTC
Created attachment 171243 [details] [review]
Bug 37870: Fix sort order of class sources

There are several things going on here.
The tests are failing randomly when some additional class sources are in the
DB. This should never happen on Jenkins, but it happened, see the third
patch of this patch set (spoiler: tests not run within a txn)

There were also a sorting problem: by default sort will show
uppercases first:
  A, B, C, a, b, c
However we want:
  a, A, b, B, c, C
which is what fc does (https://perldoc.perl.org/functions/fc)

Test plan:
0. Checkout the main branch, without patches from this patchset.
1. Run t/db_dependent/ClassSources.t several times
=> Notice that new entries in the DB table 'class_sources' are created
2. Run t/db_dependent/Koha/UI/Form/Builder/Biblio.t and
t/db_dependent/Koha/UI/Form/Builder/Item.t
=> They fail (if not, run t/db_dependent/ClassSources.t again)
3. Apply the patches
4. Run t/db_dependent/ClassSources.t
=> No more additional entries in DB, tests are correctly run within
transactions
5. Run t/db_dependent/Koha/UI/Form/Builder/Biblio.t and
   t/db_dependent/Koha/UI/Form/Builder/Item.t several times
=> They always pass

Note that the sort should actually be done on the description, not the
code. But that's for another bug...
Comment 5 Jonathan Druart 2024-09-10 09:04:42 UTC
Created attachment 171244 [details] [review]
Bug 37870: Run tests from ClassSources.t within transactions
Comment 6 CJ Lynce 2024-09-10 19:38:59 UTC
Created attachment 171283 [details] [review]
Bug 37870: Remove warning if biblionumber is not passed

Not directly related to the failure, only a bit of cleaning before
starting.

Signed-off-by: CJ Lynce <cj.lynce@westlakelibrary.org>
Comment 7 CJ Lynce 2024-09-10 19:39:51 UTC
Created attachment 171284 [details] [review]
Bug 37870: Fix sort order of class sources

There are several things going on here.
The tests are failing randomly when some additional class sources are in the
DB. This should never happen on Jenkins, but it happened, see the third
patch of this patch set (spoiler: tests not run within a txn)

There were also a sorting problem: by default sort will show
uppercases first:
  A, B, C, a, b, c
However we want:
  a, A, b, B, c, C
which is what fc does (https://perldoc.perl.org/functions/fc)

Test plan:
0. Checkout the main branch, without patches from this patchset.
1. Run t/db_dependent/ClassSources.t several times
=> Notice that new entries in the DB table 'class_sources' are created
2. Run t/db_dependent/Koha/UI/Form/Builder/Biblio.t and
t/db_dependent/Koha/UI/Form/Builder/Item.t
=> They fail (if not, run t/db_dependent/ClassSources.t again)
3. Apply the patches
4. Run t/db_dependent/ClassSources.t
=> No more additional entries in DB, tests are correctly run within
transactions
5. Run t/db_dependent/Koha/UI/Form/Builder/Biblio.t and
   t/db_dependent/Koha/UI/Form/Builder/Item.t several times
=> They always pass

Note that the sort should actually be done on the description, not the
code. But that's for another bug...

Signed-off-by: CJ Lynce <cj.lynce@westlakelibrary.org>
Comment 8 CJ Lynce 2024-09-10 19:40:09 UTC
Created attachment 171285 [details] [review]
Bug 37870: Run tests from ClassSources.t within transactions

Signed-off-by: CJ Lynce <cj.lynce@westlakelibrary.org>
Comment 9 CJ Lynce 2024-09-10 19:40:54 UTC
Everything worked as described!
Comment 10 Tomás Cohen Arazi (tcohen) 2024-09-11 13:12:38 UTC
Created attachment 171307 [details] [review]
Bug 37870: Remove warning if biblionumber is not passed

Not directly related to the failure, only a bit of cleaning before
starting.

Signed-off-by: CJ Lynce <cj.lynce@westlakelibrary.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi (tcohen) 2024-09-11 13:12:41 UTC
Created attachment 171308 [details] [review]
Bug 37870: Fix sort order of class sources

There are several things going on here.
The tests are failing randomly when some additional class sources are in the
DB. This should never happen on Jenkins, but it happened, see the third
patch of this patch set (spoiler: tests not run within a txn)

There were also a sorting problem: by default sort will show
uppercases first:
  A, B, C, a, b, c
However we want:
  a, A, b, B, c, C
which is what fc does (https://perldoc.perl.org/functions/fc)

Test plan:
0. Checkout the main branch, without patches from this patchset.
1. Run t/db_dependent/ClassSources.t several times
=> Notice that new entries in the DB table 'class_sources' are created
2. Run t/db_dependent/Koha/UI/Form/Builder/Biblio.t and
t/db_dependent/Koha/UI/Form/Builder/Item.t
=> They fail (if not, run t/db_dependent/ClassSources.t again)
3. Apply the patches
4. Run t/db_dependent/ClassSources.t
=> No more additional entries in DB, tests are correctly run within
transactions
5. Run t/db_dependent/Koha/UI/Form/Builder/Biblio.t and
   t/db_dependent/Koha/UI/Form/Builder/Item.t several times
=> They always pass

Note that the sort should actually be done on the description, not the
code. But that's for another bug...

Signed-off-by: CJ Lynce <cj.lynce@westlakelibrary.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi (tcohen) 2024-09-11 13:12:43 UTC
Created attachment 171309 [details] [review]
Bug 37870: Run tests from ClassSources.t within transactions

Signed-off-by: CJ Lynce <cj.lynce@westlakelibrary.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi (tcohen) 2024-09-11 13:12:46 UTC
Created attachment 171310 [details] [review]
Bug 37870: Tidy

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Katrin Fischer 2024-09-12 11:53:48 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 15 Janusz Kaczmarek 2024-10-28 18:07:33 UTC
*** Bug 37867 has been marked as a duplicate of this bug. ***
Comment 16 Fridolin Somers 2024-12-02 09:12:10 UTC
I see it is in 24.05.x
Not needed for backport ?
Comment 17 Andrii Nugged 2025-05-25 02:46:01 UTC
commit: "Bug 37870: Remove warning if biblionumber is not passed" has:

```
    if ( exists $self->{biblionumber} ) {
        $self->{biblionumber} = $params->{biblionumber} =~ s/\D//gr;
```

why `exists`, instead of `defined`?
it is called from cataloguing/addbiblio.pl:

    my $biblio_form_builder = Koha::UI::Form::Builder::Biblio->new(
        {
            biblionumber => scalar $input->param('biblionumber'),
        }
    );

so on adding new biblio it produces warning:

[2025/05/25 05:11:02] [WARN] Use of uninitialized value in substitution (s///)
    at /usr/share/koha/lib/Koha/UI/Form/Builder/Biblio.pm line 55.
  from /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl:277
  from /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl:888
  PAGE GENERATED 200:
  GET: /intranet/cataloguing/addbiblio.pl?frameworkcode=FA
Comment 18 Jonathan Druart 2025-05-27 08:30:28 UTC
(In reply to Andrii Nugged from comment #17)
> commit: "Bug 37870: Remove warning if biblionumber is not passed" has:
> 
> ```
>     if ( exists $self->{biblionumber} ) {
>         $self->{biblionumber} = $params->{biblionumber} =~ s/\D//gr;
> ```
> 
> why `exists`, instead of `defined`?
> it is called from cataloguing/addbiblio.pl:
> 
>     my $biblio_form_builder = Koha::UI::Form::Builder::Biblio->new(
>         {
>             biblionumber => scalar $input->param('biblionumber'),
>         }
>     );
> 
> so on adding new biblio it produces warning:
> 
> [2025/05/25 05:11:02] [WARN] Use of uninitialized value in substitution
> (s///)
>     at /usr/share/koha/lib/Koha/UI/Form/Builder/Biblio.pm line 55.
>   from /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl:277
>   from /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl:888
>   PAGE GENERATED 200:
>   GET: /intranet/cataloguing/addbiblio.pl?frameworkcode=FA

Well, the main problem I'd say is that $self->{biblionumber} actually never exists!
Comment 19 Jonathan Druart 2025-05-27 08:32:25 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Andrii Nugged from comment #17)
> > commit: "Bug 37870: Remove warning if biblionumber is not passed" has:
> > 
> > ```
> >     if ( exists $self->{biblionumber} ) {
> >         $self->{biblionumber} = $params->{biblionumber} =~ s/\D//gr;
> > ```
> > 
> > why `exists`, instead of `defined`?
> > it is called from cataloguing/addbiblio.pl:
> > 
> >     my $biblio_form_builder = Koha::UI::Form::Builder::Biblio->new(
> >         {
> >             biblionumber => scalar $input->param('biblionumber'),
> >         }
> >     );
> > 
> > so on adding new biblio it produces warning:
> > 
> > [2025/05/25 05:11:02] [WARN] Use of uninitialized value in substitution
> > (s///)
> >     at /usr/share/koha/lib/Koha/UI/Form/Builder/Biblio.pm line 55.
> >   from /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl:277
> >   from /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl:888
> >   PAGE GENERATED 200:
> >   GET: /intranet/cataloguing/addbiblio.pl?frameworkcode=FA
> 
> Well, the main problem I'd say is that $self->{biblionumber} actually never
> exists!

Sorry forget that, we have a follow-up in main (replacing "$self" with "$params")

Bug 39462: (bug 37870 follow-up) Default values from framework are inserted into existing record while editing
Comment 20 Jonathan Druart 2025-05-27 08:39:25 UTC
(In reply to Andrii Nugged from comment #17)
> commit: "Bug 37870: Remove warning if biblionumber is not passed" has:
> 
> ```
>     if ( exists $self->{biblionumber} ) {
>         $self->{biblionumber} = $params->{biblionumber} =~ s/\D//gr;
> ```
> 
> why `exists`, instead of `defined`?
> it is called from cataloguing/addbiblio.pl:
> 
>     my $biblio_form_builder = Koha::UI::Form::Builder::Biblio->new(
>         {
>             biblionumber => scalar $input->param('biblionumber'),
>         }
>     );
> 
> so on adding new biblio it produces warning:
> 
> [2025/05/25 05:11:02] [WARN] Use of uninitialized value in substitution
> (s///)
>     at /usr/share/koha/lib/Koha/UI/Form/Builder/Biblio.pm line 55.
>   from /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl:277
>   from /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl:888
>   PAGE GENERATED 200:
>   GET: /intranet/cataloguing/addbiblio.pl?frameworkcode=FA

Opened bug 40003