Bug 31109 - Prevent overwriting bibliographic records in case of simultaneous modification
Summary: Prevent overwriting bibliographic records in case of simultaneous modification
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Janusz Kaczmarek
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-06 14:36 UTC by Kelly McElligott
Modified: 2024-09-27 15:06 UTC (History)
8 users (show)

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


Attachments
Bug 31109: POC (7.61 KB, patch)
2022-08-12 12:34 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 31109: POC (8.00 KB, patch)
2024-05-27 10:49 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines (2.00 KB, patch)
2024-06-04 11:35 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Unit tests for Koha::Util::Misc::digest (2.77 KB, patch)
2024-06-04 11:35 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Add ability to lock bibliographic record or alert staff when another user is in the same bibliographic record (22.16 KB, patch)
2024-06-04 11:35 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines (2.06 KB, patch)
2024-06-04 18:45 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 31109: Unit tests for Koha::Util::Misc::digest (2.82 KB, patch)
2024-06-04 18:45 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 31109: Add ability to lock bibliographic record or alert staff when another user is in the same bibliographic record (22.21 KB, patch)
2024-06-04 18:45 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines (2.01 KB, patch)
2024-06-05 11:32 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Unit tests for Koha::Util::Misc::digest (2.78 KB, patch)
2024-06-05 11:32 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Add ability to lock bibliographic record or alert staff when another user is in the same bibliographic record (21.44 KB, patch)
2024-06-05 11:32 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification (21.41 KB, patch)
2024-06-05 11:36 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines (2.07 KB, patch)
2024-06-05 19:27 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 31109: Unit tests for Koha::Util::Misc::digest (2.83 KB, patch)
2024-06-05 19:27 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification (21.46 KB, patch)
2024-06-05 19:27 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines (4.33 KB, patch)
2024-07-02 11:53 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Unit tests for Koha::Util::Misc::digest (3.01 KB, patch)
2024-07-02 11:53 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification (27.33 KB, patch)
2024-07-02 11:53 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines (4.39 KB, patch)
2024-07-03 19:05 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 31109: Unit tests for Koha::Util::Misc::digest (3.06 KB, patch)
2024-07-03 19:05 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification (27.39 KB, patch)
2024-07-03 19:05 UTC, Roman Dolny
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2022-07-06 14:36:59 UTC
When a cataloger is in bibliographic record-making changes, it would be nice to alert other staff (if they happen to be in the same bibliographic record) with a message box to indicate another staff member is also making changes.  Additionally, allow staff to view the bibliographic record but not make changes if another staff member is also in the bibliographic record. This is a concern for larger consortiums that share records to have some sort of fail-safe method of making sure edits by any staff are saved.
Comment 1 Marcel de Rooy 2022-07-06 14:50:15 UTC
How do you remove the lock or whatever mechanism implemented when the record is opened and never saved (browser crashed etc.) ? To prevent keeping it locked for no reason..
Comment 2 Nick Clemens (kidclamp) 2022-08-12 12:34:12 UTC
Created attachment 139046 [details] [review]
Bug 31109: POC

This patch tries to use the 005 to determine if the record has been altered before saving

We would need to adjust other areas, and maybe return a diff?

Just a POC to play with
Comment 3 Janusz Kaczmarek 2024-04-09 10:00:02 UTC
+10 for it!

Cf. also bug 23705. 

If I understand well the Nick's idea, the looser would only get an info after all work he has done, that his record has not been saved.  Am I right?  This would be a pity especially when the cataloger just put a lot of effort in correcting the record (for a complicated, non standard material). 

And - if I get it right - this would not work even for the first to save if he touched 005 field in the editor an so automatically updated it.

The topic is IMHO far more complex.  It includes also a parallel case of authority editing and - to complicate it further - updating an authority record used in an biblio record currently open for edition. 

And even further: checking out/in an item currently in edition (may be purely theoretical case, but could happen), or the same user modified by two agents (two librarians, which may be only theoretical, but a librarian and an external system like a faculty software interacting through API is not a theoretical case, I think). 

So - don't we need a general locking/unlocking system for everything and inserted check actions when somebody/something tries to modify a locked resource? 

For human actions - as it points Marcel - probably a timeout would be needed for a case of abandoned session?  And possibly a mechanism for extending the lock if the interactive edit takes more than the timeout time.

Looking forward to the comments...
Comment 4 Janusz Kaczmarek 2024-05-27 10:49:36 UTC
Created attachment 167192 [details] [review]
Bug 31109: POC

This patch tries to use the 005 to determine if the record has been altered before saving

We would need to adjust other areas, and maybe return a diff?

Just a POC to play with

Rebased to the current main branch state in order to continue - JK
Comment 5 David Cook 2024-05-28 00:24:46 UTC
In bug 34164 there is a need to track what page a user is/was on, so that they can be redirected back to it. We do that by storing data in the user session.

In this bug, there's a need to know who else is on the same page as the user. But we don't currently have a robust way to query every user's session at the same time...

I suppose one way to do it would be to have another table like "session_activity" which is keyed off the "sessions.id". We could populate that for all session activity or just for bib record changes to start.

In the bib editor, we could send a heartbeat message to the API every X seconds, and the API could reply saying if anyone else has been editing that records in the last Y seconds. 

(Note: We'd have cascading delete set up for the foreign key to sessions.id, so that cleanup_database.pl would automatically clean up session_activity too on a nightly basis.)
Comment 6 Janusz Kaczmarek 2024-06-04 11:35:11 UTC
Created attachment 167373 [details] [review]
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines

New module with only one function now: digest -- to calculate a md5_hex
digest of the given argument (any Perl data structure).
Comment 7 Janusz Kaczmarek 2024-06-04 11:35:13 UTC
Created attachment 167374 [details] [review]
Bug 31109: Unit tests for Koha::Util::Misc::digest
Comment 8 Janusz Kaczmarek 2024-06-04 11:35:16 UTC
Created attachment 167375 [details] [review]
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.
Comment 9 Roman Dolny 2024-06-04 18:45:01 UTC
Created attachment 167415 [details] [review]
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines

New module with only one function now: digest -- to calculate a md5_hex
digest of the given argument (any Perl data structure).

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 10 Roman Dolny 2024-06-04 18:45:04 UTC
Created attachment 167416 [details] [review]
Bug 31109: Unit tests for Koha::Util::Misc::digest

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 11 Roman Dolny 2024-06-04 18:45:07 UTC
Created attachment 167417 [details] [review]
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.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 12 David Cook 2024-06-05 00:10:14 UTC
Comment on attachment 167417 [details] [review]
Bug 31109: Add ability to lock bibliographic record or alert staff when another user is in the same bibliographic record

Review of attachment 167417 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
@@ +907,5 @@
>                      [% END # /IF duplicatebiblionumber %]
> +                    [% IF (mod_error == -1 ) %]
> +                        <div class="dialog alert">
> +                            <h3>Record edited elsewhere</h3>
> +                            <p>Someone else beat you to it!</p>

"beat you to it" is an idiomatic expression in English. I think a rephrase might be necessary to make life easier for translators.
Comment 13 David Cook 2024-06-05 00:13:44 UTC
Comment on attachment 167415 [details] [review]
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines

Review of attachment 167415 [details] [review]:
-----------------------------------------------------------------

::: Koha/Util/Misc.pm
@@ +24,5 @@
> +use parent qw( Exporter );
> +
> +our @EXPORT = qw(
> +    digest
> +);

This is an avoidable export which doesn't follow Koha's coding guidelines. 

https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL14:_Exports
Comment 14 David Cook 2024-06-05 00:15:31 UTC
Failing for the export from a Koha:: module.
Comment 15 David Cook 2024-06-05 00:16:56 UTC
Comment on attachment 167417 [details] [review]
Bug 31109: Add ability to lock bibliographic record or alert staff when another user is in the same bibliographic record

Review of attachment 167417 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt
@@ +42,5 @@
>  <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

I'm not an experienced translator, but this look like it would be difficult to translate as well.
Comment 16 David Cook 2024-06-05 00:20:00 UTC
Also, why create Koha::Util::Misc when you could just use Digest::MD5 on the MARCXML representation of the record returned by $biblio->metadata?
Comment 17 David Cook 2024-06-05 00:21:11 UTC
Overall, I think that this concept is a nice simple way of dealing with record modification collisions.

Although I don't think I'd call it "Add ability to lock bibliographic record or alert staff when another user is int he same bibliographic record", as it does neither of those things.
Comment 18 Janusz Kaczmarek 2024-06-05 10:12:41 UTC
(In reply to David Cook from comment #16)
> Also, why create Koha::Util::Misc when you could just use Digest::MD5 on the
> MARCXML representation of the record returned by $biblio->metadata?

Thanks for your comments, David.

First off all, to save time.  The proposed solution is time efficient, and generating MARCXML costs time (like 12 times more than the proposed approach).

Secondly, to have a general digest procedure, not only for MARC records, but for all types of objects.  (It works fine also for Koha::Object objects, like Koha::Patron, Koha::Item, etc.).

Also, I couldn't find an existing module suitable to house the digest function.  Hence the Koha::Util::Misc.
Comment 19 Janusz Kaczmarek 2024-06-05 10:16:19 UTC
(In reply to David Cook from comment #12)
> Comment on attachment 167417 [details] [review] [review]
> Bug 31109: Add ability to lock bibliographic record or alert staff when
> another user is in the same bibliographic record
> 
> Review of attachment 167417 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
> @@ +907,5 @@
> >                      [% END # /IF duplicatebiblionumber %]
> > +                    [% IF (mod_error == -1 ) %]
> > +                        <div class="dialog alert">
> > +                            <h3>Record edited elsewhere</h3>
> > +                            <p>Someone else beat you to it!</p>
> 
> "beat you to it" is an idiomatic expression in English. I think a rephrase
> might be necessary to make life easier for translators.

Right, but this snippet should have been removed--it is the past of old Nick's version.  My fault.
Comment 20 Janusz Kaczmarek 2024-06-05 11:32:32 UTC
Created attachment 167436 [details] [review]
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines

New module with only one function now: digest -- to calculate a md5_hex
digest of the given argument (any Perl data structure).
Comment 21 Janusz Kaczmarek 2024-06-05 11:32:35 UTC
Created attachment 167437 [details] [review]
Bug 31109: Unit tests for Koha::Util::Misc::digest
Comment 22 Janusz Kaczmarek 2024-06-05 11:32:38 UTC
Created attachment 167438 [details] [review]
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.

Sponsored-by: Ignatianum University in Cracow
Comment 23 Janusz Kaczmarek 2024-06-05 11:36:58 UTC
Created attachment 167439 [details] [review]
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification

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.

Sponsored-by: Ignatianum University in Cracow
Comment 24 Roman Dolny 2024-06-05 19:27:11 UTC
Created attachment 167492 [details] [review]
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines

New module with only one function now: digest -- to calculate a md5_hex
digest of the given argument (any Perl data structure).

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 25 Roman Dolny 2024-06-05 19:27:14 UTC
Created attachment 167493 [details] [review]
Bug 31109: Unit tests for Koha::Util::Misc::digest

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 26 Roman Dolny 2024-06-05 19:27:17 UTC
Created attachment 167494 [details] [review]
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification

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.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 27 David Cook 2024-06-06 03:17:27 UTC
(In reply to Janusz Kaczmarek from comment #18)
> (In reply to David Cook from comment #16)
> > Also, why create Koha::Util::Misc when you could just use Digest::MD5 on the
> > MARCXML representation of the record returned by $biblio->metadata?
> 
> Thanks for your comments, David.
> 
> First off all, to save time.  The proposed solution is time efficient, and
> generating MARCXML costs time (like 12 times more than the proposed
> approach).

Except that you're fetching the MARCXML from the database already, so you don't need to generate it ;). 

Instead of '$orig_rec = $biblio->metadata->record' you'd have $orig_rec = $biblio->metadata->metadata'

Theoretically, your approach should actually take longer than using the MARCXML as fetched from the DB, as it's fetching the MARCXML then turning it into a MARC::Record object, and then turning it into a binary format. That's a lot of extra steps, which aren't really necessary.

> Secondly, to have a general digest procedure, not only for MARC records, but
> for all types of objects.  (It works fine also for Koha::Object objects,
> like Koha::Patron, Koha::Item, etc.).

That sounds a bit like premature optimization to me. 
 
> Also, I couldn't find an existing module suitable to house the digest
> function.  Hence the Koha::Util::Misc.

I thought we had a bit of a prohibition against "Util" and "Misc" in the coding guidelines but apparently not. I think it might be a bit of an unspoken rule, which keeps getting broken heh.
Comment 28 David Cook 2024-06-06 03:21:32 UTC
Comment on attachment 167494 [details] [review]
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification

Review of attachment 167494 [details] [review]:
-----------------------------------------------------------------

::: C4/Biblio.pm
@@ +116,4 @@
>  use Koha::SimpleMARC;
>  use Koha::Libraries;
>  use Koha::Util::MARC;
> +use Koha::Util::Misc qw( digest );

You're still using the Exporter.

Rather than using the Exporter in Koha::Util::Misc, you have a couple options.

1. You could make an object like Koha::Util::Digester, which you then instantiate and then call a method upon (e.g. $digester->digest($rec)). Of course, keep in mind that I think Koha::Util::Misc is unnecessary and should be replaced with Digest::MD5 anyway.

2. Just call the function fully qualified down below (e.g. Koha::Util::Misc::digest($rec) ).

::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
@@ +905,4 @@
>                              </form>
>                          </div> <!-- /.dialog.alert -->
>                      [% END # /IF duplicatebiblionumber %]
> +

Adds unnecessary whitespace. Not a dealbreaker but we typically avoid unnecessary whitespace changes.
Comment 29 David Cook 2024-06-06 03:24:04 UTC
Going to Fail QA again for the use of the Exporter.

Curious to get other QAers/RM input about Koha::Util::Misc.
Comment 30 Phil Ringnalda 2024-06-06 03:41:13 UTC
Making users of the basic editor deal with merging changes after collisions while users of the advanced editor still get to just clobber changes doesn't sound like a nice idea.
Comment 31 Katrin Fischer 2024-06-23 20:25:16 UTC
(In reply to Phil Ringnalda from comment #30)
> Making users of the basic editor deal with merging changes after collisions
> while users of the advanced editor still get to just clobber changes doesn't
> sound like a nice idea.

I think maybe sometimes it's just one step after the other. Having a working concept in one place can lead to more improvements in other spots.
Comment 32 Janusz Kaczmarek 2024-06-23 20:55:17 UTC
(In reply to Katrin Fischer from comment #31)
> (In reply to Phil Ringnalda from comment #30)
> > Making users of the basic editor deal with merging changes after collisions
> > while users of the advanced editor still get to just clobber changes doesn't
> > sound like a nice idea.
> 
> I think maybe sometimes it's just one step after the other. Having a working
> concept in one place can lead to more improvements in other spots.

I am working on this in the time available.  I just needs some more time to universalize the code, solve parameter passing issue in a way suitable for both editors...
Comment 33 Phil Ringnalda 2024-06-23 21:45:13 UTC
Sweet that Bugzilla gave me a "mid-air collision detected" warning :)

Not sure what to propose other than maybe behind a pref, off-by-default.

I can absolutely see the benefit of this, say in a university library with a small number of well-trained catalogers who either know how to merge or can ask someone at the next desk over.

But for me, with a couple users of the advanced editor and a large number of users of the basic editor who mostly know how to import a record and save it, but will sometimes notice a typo ten minutes later (by which time I've corrected five autocreated authority records) it's going to be baffling. And some of those are elementary school library aides behind generic logins that I don't even have a way to contact, much less to train to use the biblio merge UI.

So whether or not it only affects basic editor users, I'll still have to find a way to make sure that for us, it pretty much never gets hit. If it lands unpreffed, I'll have to modify about 30 reports of cataloging errors to only tell me about them when the 005 says the record has been idle for several hours, and probably switch correcting bogus new authority records to leaving them bogus for at least 24 hours.
Comment 34 Janusz Kaczmarek 2024-07-02 11:53:03 UTC
Created attachment 168352 [details] [review]
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines

New module with only one function now: digest -- to calculate a md5_hex
digest of the given argument (any Perl data structure).
Comment 35 Janusz Kaczmarek 2024-07-02 11:53:07 UTC
Created attachment 168353 [details] [review]
Bug 31109: Unit tests for Koha::Util::Misc::digest
Comment 36 Janusz Kaczmarek 2024-07-02 11:53:11 UTC
Created attachment 168354 [details] [review]
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification

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.

N.B., this should work both with the basic and with the advanced editor.

Sponsored-by: Ignatianum University in Cracow
Comment 37 Roman Dolny 2024-07-03 19:05:17 UTC
Created attachment 168472 [details] [review]
Bug 31109: Koha::Util::Misc - utility class with miscellaneous routines

New module with only one function now: digest -- to calculate a md5_hex
digest of the given argument (any Perl data structure).

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 38 Roman Dolny 2024-07-03 19:05:21 UTC
Created attachment 168473 [details] [review]
Bug 31109: Unit tests for Koha::Util::Misc::digest

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 39 Roman Dolny 2024-07-03 19:05:26 UTC
Created attachment 168474 [details] [review]
Bug 31109: Prevent overwriting bibliographic records in case of simultaneous modification

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.

N.B., this should work both with the basic and with the advanced editor.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 40 David Cook 2024-07-04 00:38:52 UTC
(In reply to David Cook from comment #29)
> Curious to get other QAers/RM input about Koha::Util::Misc.

I'm still curious to hear what others think.

For me, I still think creating a Koha::Util::Misc class and using Sereal are suboptimal choices. 

The functionality added by this patch could be done more simply using $biblio->metadata and Digest::MD5 on its own. 

I also notice that "koha-tmpl/intranet-tmpl/prog/en/includes/cateditor-ui.inc" uses a new SVC API called "svc/mid_air_collision".

While I don't think we have an explicit coding guideline forbidding new SVC API endpoints, I think there is an unwritten rule about this. I think all new API endpoints must be REST API endpoints.

I'll discuss all the above with the QA team and see what they say.
Comment 41 Janusz Kaczmarek 2024-07-04 08:16:29 UTC
(In reply to David Cook from comment #40)
> The functionality added by this patch could be done more simply using
> $biblio->metadata and Digest::MD5 on its own. 

Note that we do not have a Koha::Biblio object for the modified version coming from the editor/mergeform.  We just have the MARC::Record object.

> I also notice that
> "koha-tmpl/intranet-tmpl/prog/en/includes/cateditor-ui.inc" uses a new SVC
> API called "svc/mid_air_collision".
> 
> While I don't think we have an explicit coding guideline forbidding new SVC
> API endpoints, I think there is an unwritten rule about this. I think all
> new API endpoints must be REST API endpoints.

The functionality of svc/mid_air_collision does not fit to REST API.  I'm not quite happy happy with my svc/mid_air_collision idea, but:

Feci, quod potui, faciant meliora potentes
Comment 42 David Cook 2024-07-05 00:56:06 UTC
(In reply to Janusz Kaczmarek from comment #41)
> (In reply to David Cook from comment #40)
> > The functionality added by this patch could be done more simply using
> > $biblio->metadata and Digest::MD5 on its own. 
> 
> Note that we do not have a Koha::Biblio object for the modified version
> coming from the editor/mergeform.  We just have the MARC::Record object.

You mean the output of "TransformHtmlToMarc()"? I take your point there. I think you'll still find it easier to work with 1 MARC::Record to XML operation rather than creating one MARC::Record from XML and then Sereal encoding both objects and digesting their bytes. 

> The functionality of svc/mid_air_collision does not fit to REST API.

Why do you think that is?

> Feci, quod potui, faciant meliora potentes

Mmm indeed.
Comment 43 Nick Clemens (kidclamp) 2024-09-27 15:06:53 UTC
I think I agree with David here - rather than a new svc route we should simply pass forward the record from the advanced editor and adjust merge,pl to be able to accept that as an input to compare against the existing record. If that proves very difficult I think we should instead save the record as a new record and direct to the merge, then if canceled the two records can be kept or the new one deleted.

The Util class should probably be an object method should be avoided if possible as well