Bug 37429 - Can't edit bibliographic records anymore (empty form)
Summary: Can't edit bibliographic records anymore (empty form)
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P1 - high major
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 36192 37499 37947
  Show dependency treegraph
 
Reported: 2024-07-23 09:19 UTC by zefanjas
Modified: 2024-11-14 14:38 UTC (History)
8 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,24.05.04
Circulation function:


Attachments
Bug 37429: Set default value for global variables (3.05 KB, patch)
2024-08-15 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37429: Set default value for global variables (3.10 KB, patch)
2024-08-15 11:35 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37429: Set default value for global variables (3.20 KB, patch)
2024-08-16 06:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description zefanjas 2024-07-23 09:19:38 UTC
While cataloging new books the edit / add biblio form is sometimes empty. This happens also when I want to edit an existing biblio or import one via Z.39.50. Sometime I can add 5-10 new items before it happens, sometimes it happens after every new record. 

The only "fix" right now for me is to do a `service koha-common restart`. The log files don't show any error.

To reproduce the issue:

1. Open an existing biblio (detail.pl)
2. Click "Edit record"
3. Change the framework (no need to change any details)
4. Change the framework back to the original one (no need to save anything)
5. Go back to the biblio overview page (detail.pl)
6. Click "Edit record" again → mask / form is empty
Comment 1 Jan Kissig 2024-07-23 11:20:51 UTC
This behavior is caused by $changed_framework with a value = 1 even when calling addbiblio.pl without an param op:

our($tagslib,$authorised_values_sth,$is_a_modif,$usedTagsLib,$mandatory_z3950,$op,$changed_framework);
Comment 2 Andreas Roussos 2024-07-23 22:11:02 UTC
git bisect points to commit 3f2a8ecf3c9f3421260657bfc779029e6ae75f2c

Bug 34478: Regression - fix change framework

https://git.koha-community.org/Koha-community/Koha/commit/3f2a8ecf3c9f3421260657bfc779029e6ae75f2c
Comment 3 Andreas Roussos 2024-07-24 12:55:56 UTC
Interestingly, I cannot reproduce this if I disable and then
stop Plack (followed by a restart of memcached and apache2).

The following quick and dirty patch seems to fix things
(even when Plack and memcached are enabled):

diff --git a/cataloguing/addbiblio.pl b/cataloguing/addbiblio.pl
index 99c20580b5..754aaf175a 100755
--- a/cataloguing/addbiblio.pl
+++ b/cataloguing/addbiblio.pl
@@ -518,6 +518,7 @@ my $fa_duedatespec        = $input->param('duedatespec');

 my $userflags = 'edit_catalogue';

+$changed_framework = 0;
 if ( $op eq 'cud-change-framework' ) {
     $op = $input->param('original_op');
     $changed_framework = 1;

Not sure about the quality of the above solution, though?
Comment 4 Emily Lamancusa (emlam) 2024-07-26 16:48:19 UTC
(In reply to Andreas Roussos from comment #3)
> Interestingly, I cannot reproduce this if I disable and then
> stop Plack (followed by a restart of memcached and apache2).
> 
> The following quick and dirty patch seems to fix things
> (even when Plack and memcached are enabled):
> 
> diff --git a/cataloguing/addbiblio.pl b/cataloguing/addbiblio.pl
> index 99c20580b5..754aaf175a 100755
> --- a/cataloguing/addbiblio.pl
> +++ b/cataloguing/addbiblio.pl
> @@ -518,6 +518,7 @@ my $fa_duedatespec        = $input->param('duedatespec');
> 
>  my $userflags = 'edit_catalogue';
> 
> +$changed_framework = 0;
>  if ( $op eq 'cud-change-framework' ) {
>      $op = $input->param('original_op');
>      $changed_framework = 1;
> 
> Not sure about the quality of the above solution, though?

The bigger issue is that $changed_framework (as well as others) shouldn't really be global variables in the first place. I don't quite understand Perl's scoping rules well enough to be able to say how big of a refactor it would be to fix that, though.

In the meantime, I'd say your patch is worth doing to fix the major bug. We already do that with $is_a_modif, and it's a simple change. I'll file a follow-up bug to address the underlying issue.
Comment 5 Jonathan Druart 2024-08-15 10:24:19 UTC
Created attachment 170346 [details] [review]
Bug 37429: Set default value for global variables

The global variables needs to be assigned with a default value, or the
value from the previous request will be used.

Global variables are persistent from one request to another, in memory.

This patch:
* groups default value for global variables together
* removes $authorised_values_sth from this list (it is not a global var)
* set $changed_framework to 0 (which fix the issue)

Test plan:
1. Open an existing biblio (detail.pl)
2. Click "Edit record"
3. Change the framework (no need to change any details)
4. Change the framework back to the original one (no need to save anything)
5. Go back to the biblio overview page (detail.pl)
=> The form is populated
Comment 6 Paul Derscheid 2024-08-15 11:35:06 UTC
Created attachment 170356 [details] [review]
Bug 37429: Set default value for global variables

The global variables needs to be assigned with a default value, or the
value from the previous request will be used.

Global variables are persistent from one request to another, in memory.

This patch:
* groups default value for global variables together
* removes $authorised_values_sth from this list (it is not a global var)
* set $changed_framework to 0 (which fix the issue)

Test plan:
1. Open an existing biblio (detail.pl)
2. Click "Edit record"
3. Change the framework (no need to change any details)
4. Change the framework back to the original one (no need to save anything)
5. Go back to the biblio overview page (detail.pl)
=> The form is populated

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 7 Marcel de Rooy 2024-08-16 06:29:45 UTC
Looking here
Comment 8 Marcel de Rooy 2024-08-16 06:33:07 UTC
Created attachment 170424 [details] [review]
Bug 37429: Set default value for global variables

The global variables needs to be assigned with a default value, or the
value from the previous request will be used.

Global variables are persistent from one request to another, in memory.

This patch:
* groups default value for global variables together
* removes $authorised_values_sth from this list (it is not a global var)
* set $changed_framework to 0 (which fix the issue)

Test plan:
1. Open an existing biblio (detail.pl)
2. Click "Edit record"
3. Change the framework (no need to change any details)
4. Change the framework back to the original one (no need to save anything)
5. Go back to the biblio overview page (detail.pl)
=> The form is populated

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Katrin Fischer 2024-08-16 15:37:16 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 10 Lucas Gass (lukeg) 2024-09-11 16:40:14 UTC
Backported to 24.05.x for upcoming 24.05.04
Comment 11 Fridolin Somers 2024-10-21 12:10:33 UTC
Looks like this is mainly for $op.
Since in 23.11.x $op is not global, is this needed.
If yes please provide rebased patches.