From cec1cccd97c8499e2493abde5724700f9b1594f5 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Fri, 12 Aug 2022 12:32:58 +0000
Subject: [PATCH] Bug 31109: Add ability to lock bibliographic record or alert
 staff when another user is in the same bibliographic record

Koha, till now, lacks the controll of concurrent modification of data.
Perhaps the most urgent case is parallel modifications of a bibliograhic
record by two independent agents.

The proposed procedure, collision detection when saving a record, comes
from Nick, who is also the author of the original version of the patch.
This idea is simpler to implement than introducing a record lock.  The
idea of using a checksum to confirm that a record has not changed during
editing comes from Jonathan.  The other elements, including the
implementation of redirecting to the merge page after a failed attempt to
modify a record, come from Janusz.

The checksum is passed to C4::Biblio::ModBiblio as an optional
original_digest parameter, which in the future should perhaps be
mandatory for all record modifications, including authority records and
other Koha objects.

The patch prevents also a possible collision between edit and merge.

Test plan:
==========
1. In two independent browsers, A and B, open in parallel for edit the same
   bibliographic record.  Make different changes in each browser.  Save
   first A, then B.  Reload the record in each browser.  You should see,
   in both browsers, the version of the record modified in B.  Note that you
   have lost the changes made in browser A without any warning.  This is
   a serious problem.
2. Apply the patch; restart_all.
3. Repeat p. 1.  While trying to save the record in browser B, you will
   be redirect to a merge page, with a warning about a conflict in
   modification, and with a possibility to merge your modification to the
   current version of the record, and, possibly, amend the record again
   in the regular editor.
---
 C4/Biblio.pm                                  | 81 +++++++++++-------
 cataloguing/addbiblio.pl                      | 83 +++++++++++++++++--
 cataloguing/merge.pl                          | 81 +++++++++++++++++-
 .../prog/en/includes/merge-record.inc         |  6 +-
 .../prog/en/modules/cataloguing/addbiblio.tt  | 15 +++-
 .../prog/en/modules/cataloguing/merge.tt      | 14 ++++
 6 files changed, 239 insertions(+), 41 deletions(-)

diff --git a/C4/Biblio.pm b/C4/Biblio.pm
index 4b32ad43d5..2d8b577c4a 100644
--- a/C4/Biblio.pm
+++ b/C4/Biblio.pm
@@ -116,6 +116,7 @@ use Koha::SearchEngine::Indexer;
 use Koha::SimpleMARC;
 use Koha::Libraries;
 use Koha::Util::MARC;
+use Koha::Util::Misc;
 
 =head1 NAME
 
@@ -377,9 +378,13 @@ Used when the indexing schedulling will be handled by the caller
 
 Set as the record source when saving the record.
 
+=item C<original_digest>
+
+Digest of the original version of the modified record.
+
 =back
 
-Returns 1 on success 0 on failure
+Returns 1 on success, -1 in case of modification collision, 0 on other failure.
 
 =cut
 
@@ -401,20 +406,6 @@ sub ModBiblio {
         return 0;
     }
 
-    if ( C4::Context->preference("CataloguingLog") ) {
-        my $biblio = Koha::Biblios->find($biblionumber);
-        my $record;
-        my $decoding_error = "";
-        eval { $record = $biblio->metadata->record };
-        if ($@) {
-            my $exception = $@;
-            $exception->rethrow unless ( $exception->isa('Koha::Exceptions::Metadata::Invalid') );
-            $decoding_error = "There was an error with this bibliographic record: " . $exception;
-            $record         = $biblio->metadata->record_strip_nonxml;
-        }
-        logaction( "CATALOGUING", "MODIFY", $biblionumber, "biblio $decoding_error BEFORE=>" . $record->as_formatted );
-    }
-
     if ( !$options->{disable_autolink} && C4::Context->preference('AutoLinkBiblios') ) {
         BiblioAutoLink( $record, $frameworkcode );
     }
@@ -465,27 +456,57 @@ sub ModBiblio {
     # update MARC subfield that stores biblioitems.cn_sort
     _koha_marc_update_biblioitem_cn_sort( $record, $oldbiblio, $frameworkcode );
 
-    # update the MARC record (that now contains biblio and items) with the new record data
-    ModBiblioMarc( $record, $biblionumber, $mod_biblio_marc_options );
+    my $schema = Koha::Database->schema;
+    my $result = 0;
+    $schema->txn_do(
+        sub {
 
-    # modify the other koha tables
-    _koha_modify_biblio( $dbh, $oldbiblio, $frameworkcode );
-    _koha_modify_biblioitem_nonmarc( $dbh, $oldbiblio );
+            my $biblio = Koha::Biblios->find($biblionumber);
+            my $orig_rec;
+            my $decoding_error = "";
+            eval { $orig_rec = $biblio->metadata->record };
+            if ($@) {
+                my $exception = $@;
+                $exception->rethrow unless ( $exception->isa('Koha::Exceptions::Metadata::Invalid') );
+                $decoding_error = "There was an error with this bibliographic record: " . $exception;
+                $orig_rec       = $biblio->metadata->record_strip_nonxml;
+            }
+            if ( !$options->{original_digest} || $options->{original_digest} eq digest($orig_rec) ) {
 
-    _after_biblio_action_hooks({ action => 'modify', biblio_id => $biblionumber });
+                if ( C4::Context->preference("CataloguingLog") ) {
+                    logaction(
+                        "CATALOGUING", "MODIFY", $biblionumber,
+                        "biblio $decoding_error BEFORE=>" . $orig_rec->as_formatted
+                    );
+                }
 
-    # update OAI-PMH sets
-    if(C4::Context->preference("OAI-PMH:AutoUpdateSets")) {
-        C4::OAI::Sets::UpdateOAISetsBiblio($biblionumber, $record);
-    }
+                # update the MARC record (that now contains biblio and items) with the new record data
+                ModBiblioMarc( $record, $biblionumber, $mod_biblio_marc_options );
 
-    Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue(
-        {
-            biblio_ids => [ $biblionumber ]
+                # modify the other koha tables
+                _koha_modify_biblio( $dbh, $oldbiblio, $frameworkcode );
+                _koha_modify_biblioitem_nonmarc( $dbh, $oldbiblio );
+
+                _after_biblio_action_hooks( { action => 'modify', biblio_id => $biblionumber } );
+
+                # update OAI-PMH sets
+                if ( C4::Context->preference("OAI-PMH:AutoUpdateSets") ) {
+                    C4::OAI::Sets::UpdateOAISetsBiblio( $biblionumber, $record );
+                }
+
+                Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( { biblio_ids => [$biblionumber] } )
+                    unless $options->{skip_holds_queue}
+                    or !C4::Context->preference('RealTimeHoldsQueue');
+
+                $result = 1;
+
+            } else {
+                $result = -1;
+            }
         }
-    ) unless $options->{skip_holds_queue} or !C4::Context->preference('RealTimeHoldsQueue');
+    );
 
-    return 1;
+    return $result;
 }
 
 =head2 _strip_item_fields
diff --git a/cataloguing/addbiblio.pl b/cataloguing/addbiblio.pl
index 99c20580b5..ac8437e059 100755
--- a/cataloguing/addbiblio.pl
+++ b/cataloguing/addbiblio.pl
@@ -57,6 +57,7 @@ use Koha::Libraries;
 use Koha::BiblioFrameworks;
 use Koha::Patrons;
 use Koha::UI::Form::Builder::Biblio;
+use Koha::Util::Misc;
 
 use MARC::File::USMARC;
 use MARC::File::XML;
@@ -603,6 +604,7 @@ my (
 	$biblioitemnumber
 );
 
+my $digest;
 if ( $biblio && !$breedingid ) {
     eval { $record = $biblio->metadata->record };
     if ($@) {
@@ -611,6 +613,7 @@ if ( $biblio && !$breedingid ) {
         $record = $biblio->metadata->record_strip_nonxml;
         $template->param( INVALID_METADATA => $exception );
     }
+    $digest = digest($record) if $record;
 }
 if ($breedingid) {
     ( $record, $encoding ) = MARCfindbreeding( $breedingid ) ;
@@ -701,11 +704,12 @@ if ( $op eq "cud-addbiblio" ) {
         ( $duplicatebiblionumber, $duplicatetitle ) = FindDuplicate($record);
     }
     my $confirm_not_duplicate = $input->param('confirm_not_duplicate');
+    my $digest_orig = $input->param('digest');
     # it is not a duplicate (determined either by Koha itself or by user checking it's not a duplicate)
     if ( !$duplicatebiblionumber or $confirm_not_duplicate ) {
         my $oldbibitemnum;
         if ( $is_a_modif ) {
-            ModBiblio(
+            my $success = ModBiblio(
                 $record,
                 $biblionumber,
                 $frameworkcode,
@@ -714,9 +718,75 @@ if ( $op eq "cud-addbiblio" ) {
                         source       => $z3950 ? 'z3950' : 'intranet',
                         categorycode => $logged_in_patron->categorycode,
                         userid       => $logged_in_patron->userid,
-                    }
+                    },
+                    original_digest => $digest_orig,
                 }
             );
+
+            # in case of unsuccessful record modification let's redirect to merge
+            unless ( $success > 0 ) {
+                $success ||= 1;
+
+                ( $template, undef, undef ) = get_template_and_user(
+                    {
+                        template_name => "cataloguing/merge.tt",
+                        query         => $input,
+                        type          => "intranet",
+                        flagsrequired => { editcatalogue => 'edit_catalogue' },
+                    }
+                );
+
+                my $framework_database = GetFrameworkCode($biblionumber);
+
+                # Getting MARC Structure
+                my $tagslib = GetMarcStructure( 1, $framework_database );
+
+                my $marcflavour = lc( C4::Context->preference('marcflavour') );
+
+                # Creating a loop for display
+                my @records;
+
+                for my $i ( 0 .. 1 ) {
+                    my $biblio;
+                    my $marcrecord;
+                    my $frmwkcode;
+                    my $template_record;
+                    if ( $i == 0 ) {    #database version
+                        $biblio                       = Koha::Biblios->find($biblionumber);
+                        $marcrecord                   = $biblio->record;
+                        $frmwkcode                    = $framework_database;
+                        $template_record->{reference} = 1;
+                        $template->param( ref_record => $template_record );
+                        $template_record->{recordid} = $biblionumber;
+                    } else {            #modified version
+                        $marcrecord                  = $record;
+                        $frmwkcode                   = $frameworkcode;
+                        $template_record->{recordid} = -1;  # NOTE for the modified version
+                    }
+
+                    my $recordObj = Koha::MetadataRecord->new( { 'record' => $marcrecord, schema => $marcflavour } );
+                    $template_record->{record}        = $marcrecord;
+                    $template_record->{frameworkcode} = $frmwkcode;
+                    $template_record->{display}       = $recordObj->createMergeHash($tagslib);
+                    push @records, $template_record;
+                }
+
+                my ($biblionumbertag) = GetMarcFromKohaField('biblio.biblionumber');
+
+                # Parameters
+                $template->param(
+                    ref_biblionumber  => $biblionumber,
+                    records           => \@records,
+                    ref_record        => $records[0],
+                    framework         => $framework_database,
+                    biblionumbertag   => $biblionumbertag,
+                    mid_air_collision => 1,
+                    digest            => digest( $records[0]->{record} ),
+                );
+
+                output_html_with_http_headers $input, $cookie, $template->output;
+                exit;
+            }
         }
         else {
             ( $biblionumber, $oldbibitemnum ) = AddBiblio( $record, $frameworkcode );
@@ -845,13 +915,14 @@ elsif ( $op eq "cud-delete" ) {
     build_tabs( $template, $record, $dbh, $encoding,$input );
     $template->param(
         biblionumber             => $biblionumber,
-        biblionumbertagfield        => $biblionumbertagfield,
-        biblionumbertagsubfield     => $biblionumbertagsubfield,
+        biblionumbertagfield     => $biblionumbertagfield,
+        biblionumbertagsubfield  => $biblionumbertagsubfield,
         biblioitemnumtagfield    => $biblioitemnumtagfield,
         biblioitemnumtagsubfield => $biblioitemnumtagsubfield,
         biblioitemnumber         => $biblioitemnumber,
-	hostbiblionumber	=> $hostbiblionumber,
-	hostitemnumber		=> $hostitemnumber
+        hostbiblionumber         => $hostbiblionumber,
+        hostitemnumber           => $hostitemnumber,
+        digest                   => $digest,
     );
 }
 
diff --git a/cataloguing/merge.pl b/cataloguing/merge.pl
index 9fe7bc4f8b..6026f24fa2 100755
--- a/cataloguing/merge.pl
+++ b/cataloguing/merge.pl
@@ -32,14 +32,16 @@ use C4::Biblio qw(
     ModBiblio
     TransformHtmlToMarc
 );
-use C4::Serials qw( CountSubscriptionFromBiblionumber );
-use C4::Reserves qw( MergeHolds );
+use C4::Serials     qw( CountSubscriptionFromBiblionumber );
+use C4::Reserves    qw( MergeHolds );
 use C4::Acquisition qw( ModOrder GetOrdersByBiblionumber );
+use C4::Search      qw( enabled_staff_search_views );
 
 use Koha::BiblioFrameworks;
 use Koha::Biblios;
 use Koha::Items;
 use Koha::MetadataRecord;
+use Koha::Util::Misc;
 
 my $input = CGI->new;
 my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
@@ -88,8 +90,80 @@ if ($op eq 'cud-merge') {
     #Take new framework code
     my $frameworkcode = $input->param('frameworkcode');
 
+    my $digest = $input->param('digest');
+
     # Modifying the reference record
-    ModBiblio($record, $ref_biblionumber, $frameworkcode);
+    my $result = ModBiblio( $record, $ref_biblionumber, $frameworkcode, { original_digest => $digest } );
+
+    if ( $result == -1 ) {
+        my $framework_database = GetFrameworkCode($ref_biblionumber);
+
+        # Getting MARC Structure
+        my $tagslib = GetMarcStructure( 1, $framework_database );
+
+        my $marcflavour = lc( C4::Context->preference('marcflavour') );
+
+        # Creating a loop for display
+        my @records;
+
+        for my $i ( 0 .. 1 ) {
+            my $biblio;
+            my $marcrecord;
+            my $frmwkcode;
+            my $template_record;
+            if ( $i == 0 ) {    #database version
+                $biblio                       = Koha::Biblios->find($ref_biblionumber);
+                $marcrecord                   = $biblio->record;
+                $frmwkcode                    = $framework_database;
+                $template_record->{reference} = 1;
+                $template->param( ref_record => $template_record );
+                $template_record->{recordid} = $ref_biblionumber;
+            } else {            #modified version
+                $marcrecord                  = $record;
+                $frmwkcode                   = $frameworkcode;
+                $template_record->{recordid} = -1;               # NOTE for the modified version
+            }
+
+            my $recordObj = Koha::MetadataRecord->new( { 'record' => $marcrecord, schema => $marcflavour } );
+            $template_record->{record}        = $marcrecord;
+            $template_record->{frameworkcode} = $frmwkcode;
+            $template_record->{display}       = $recordObj->createMergeHash($tagslib);
+            push @records, $template_record;
+        }
+
+        my ($biblionumbertag) = GetMarcFromKohaField('biblio.biblionumber');
+
+        # Parameters
+        $template->param(
+            ref_biblionumber  => $ref_biblionumber,
+            records           => \@records,
+            ref_record        => $records[0],
+            framework         => $framework_database,
+            biblionumbertag   => $biblionumbertag,
+            mid_air_collision => 1,
+            digest            => digest( $records[0]->{record} ),
+        );
+
+        output_html_with_http_headers $input, $cookie,
+            $template->output;
+        exit;
+    }
+
+    # This is a merge after a mid-air collision, not a real merge
+    if ( $biblionumbers[0] == -1 ) {
+        my $defaultview = C4::Context->preference('IntranetBiblioDefaultView');
+        my $views       = {C4::Search::enabled_staff_search_views};
+        if ( $defaultview eq 'isbd' && $views->{can_view_ISBD} ) {
+            print $input->redirect("/cgi-bin/koha/catalogue/ISBDdetail.pl?biblionumber=$ref_biblionumber");
+        } elsif ( $defaultview eq 'marc' && $views->{can_view_MARC} ) {
+            print $input->redirect("/cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=$ref_biblionumber");
+        } elsif ( $defaultview eq 'labeled_marc' && $views->{can_view_labeledMARC} ) {
+            print $input->redirect("/cgi-bin/koha/catalogue/labeledMARCdetail.pl?biblionumber=$ref_biblionumber");
+        } else {
+            print $input->redirect("/cgi-bin/koha/catalogue/detail.pl?biblionumber=$ref_biblionumber");
+        }
+        exit;
+    }
 
     my $report_header = {};
     foreach my $biblionumber ($ref_biblionumber, @biblionumbers) {
@@ -174,6 +248,7 @@ if ($op eq 'cud-merge') {
             if ($ref_biblionumber and $ref_biblionumber == $biblionumber) {
                 $record->{reference} = 1;
                 $template->param(ref_record => $record);
+                $template->param(digest => digest($marcrecord));
                 unshift @records, $record;
             } else {
                 push @records, $record;
diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc
index cdf3aefb54..dc47b82707 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc
@@ -49,7 +49,11 @@
         [% WRAPPER tabs_nav %]
             [% FOREACH record IN sourcerecords %]
                 [% WRAPPER tab_item tabname= "tabrecord${record.recordid}" %]
-                    [% record.recordid | html %]
+                    [% IF record.recordid == -1 %]
+                        modif
+                    [% ELSE %]
+                        [% record.recordid | html %]
+                    [% END %]
                     [% IF record.reference %]<span>(ref)</span>[% END %]
                 [% END %]
             [% END %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
index 85b41b2daf..c5e7443e75 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
@@ -879,7 +879,7 @@ $(document).ready(function(){
                     [% END %]
                 </h1>
 
-                [% UNLESS ( number ) %]
+                [% UNLESS ( number && !mod_error ) %]
                     <!-- show duplicate warning on tab 0 only -->
                     [% IF ( duplicatebiblionumber ) %]
                         <div class="dialog alert">
@@ -905,6 +905,18 @@ $(document).ready(function(){
                             </form>
                         </div> <!-- /.dialog.alert -->
                     [% END # /IF duplicatebiblionumber %]
+                    [% IF (mod_error == -1 ) %]
+                        <div class="dialog alert">
+                            <h3>Record edited elsewhere</h3>
+                            <p>Someone else beat you to it!</p>
+                        </div>
+                    [% ELSIF (mod_error == 1) %]
+                        <div class="dialog alert">
+                            <h3>Error</h3>
+                            <p>Something is not right!</p>
+                        </div>
+                    [% END %]
+
                 [% END # /UNLESS number %]
 
                 [% IF ( done ) %]
@@ -920,6 +932,7 @@ $(document).ready(function(){
                         <input type="hidden" value="" id="current_tab" name="current_tab" />
                         <input type="hidden" name="original_op" value="[% op | html %]" />
                         <input type="hidden" value="0" id="confirm_not_duplicate" name="confirm_not_duplicate" />
+                        <input type="hidden" value="[% digest | html %]" id="digest" name="digest" />
                 [% END %]
 
                 <div id="toolbar" class="btn-toolbar">
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt
index 6688d4e828..f1c9d06b7b 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt
@@ -40,6 +40,13 @@ div#result { margin-top: 1em; }
             [% INCLUDE 'messages.inc' %]
 
 <h1>Merging records</h1>
+    [% IF mid_air_collision %]
+        <div class="dialog alert">
+            <h3>Record edited elsewhere</h3>
+            You can try to merge your version (marked as modif) with the current version of the record (marked as ref) or cancel
+        </div>
+    [% END %]
+
 [% IF ( result ) %]
     [% IF ( errors.size ) %]
        [% FOREACH error IN errors %]
@@ -179,6 +186,7 @@ div#result { margin-top: 1em; }
 </div> <!-- .col-sm-6 -->
 
 <input type="hidden" name="ref_biblionumber" value="[% ref_biblionumber | html %]" />
+<input type="hidden" id="digest" name="digest" value="[% digest | html %]" />
 [% FOREACH record IN records %]
     <input type="hidden" name="biblionumber" value="[% record.recordid | html %]" />
 [% END %]
@@ -187,9 +195,15 @@ div#result { margin-top: 1em; }
 <fieldset class="action">
     <input type="hidden" name="op" value="cud-merge" />
     <input type="submit" name="merge" class="btn btn-primary" value="Merge" />
+    [% IF ! mid_air_collision %]
     <label for="report_fields">Fields to display in report:</label>
     <input type="text" name="report_fields" id="report_fields" value="[% MergeReportFields | html %]" />
     <span class="hint">(Example: "001,245ab,600")</span>
+    [% ELSE %]
+        <div class="btn-group">
+            <a href="[% PROCESS biblio_a_href biblionumber => ref_biblionumber %]" class="btn btn-link" id="cancel">Cancel</a>
+        </div>
+    [% END %]
 </fieldset>
 </form>
 [% END %]
-- 
2.39.2