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.
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..
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
+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...
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
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.)
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).
Created attachment 167374 [details] [review] Bug 31109: Unit tests for Koha::Util::Misc::digest
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.
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>
Created attachment 167416 [details] [review] Bug 31109: Unit tests for Koha::Util::Misc::digest Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
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 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 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
Failing for the export from a Koha:: module.
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.
Also, why create Koha::Util::Misc when you could just use Digest::MD5 on the MARCXML representation of the record returned by $biblio->metadata?
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.
(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.
(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.
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).
Created attachment 167437 [details] [review] Bug 31109: Unit tests for Koha::Util::Misc::digest
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
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
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>
Created attachment 167493 [details] [review] Bug 31109: Unit tests for Koha::Util::Misc::digest Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
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>
(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 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.
Going to Fail QA again for the use of the Exporter. Curious to get other QAers/RM input about Koha::Util::Misc.
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.
(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.
(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...
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.
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).
Created attachment 168353 [details] [review] Bug 31109: Unit tests for Koha::Util::Misc::digest
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
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>
Created attachment 168473 [details] [review] Bug 31109: Unit tests for Koha::Util::Misc::digest Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
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>
(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.
(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
(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.
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