Bug 14367 - History for MARC records. Roll back changes on a timeline or per field.
Summary: History for MARC records. Roll back changes on a timeline or per field.
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 5 votes (vote)
Assignee: Martin Persson
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-09 08:48 UTC by Viktor Sarge
Modified: 2018-04-16 22:10 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14367: Add MARC record history (35.30 KB, patch)
2015-08-18 21:30 UTC, Martin Persson
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (37.70 KB, patch)
2016-10-18 02:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: [FOLLOW-UP] Fixing merge conflicts... (4.33 KB, patch)
2017-06-13 01:29 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Viktor Sarge 2015-06-09 08:48:59 UTC
Changes to MARC records are currently not tracked. We suggest creating functionality that tracks all changes to records and lets the users roll them back either in batch to a previous point in the history or separately for each field (so you can roll back changes to a field without changing fields that may have been changed after this one).
Comment 1 Martin Persson 2015-08-18 21:30:07 UTC
Created attachment 41642 [details] [review]
Bug 14367: Add MARC record history

This is a proof-of-concept implementation for adding history
support to MARC records. Every time a change is made a complete
copy of the record is stored along with the date/time and user
identity. The changes are listed under each field in the MARC
record editor and can be reverted with a click.

The changes are stored as a JSON array in a new column named
'history' in the database. The array is re-read from the database
before updating the record to prevent old data lingering in
the session from overwriting newer changes made by other users.

If we decide to implement this feature it might be better to
simply create a new table altogether and link it rather than
the clumsy JSON solution. That would eliminate a lot of bulky
code that transforms MARC-KOHA-JSON and back while ensuring
data integrity.

Also, there are plans to add permissions to the MARC records;
this likely requires more complex interactions that will scale
badly with the current JSON solution.

At present, the history is hardcoded to 10 entries.
This can easily be made into a syspref.

The current implementation should probably be refactored
into a 'BiblioHistory' class before deploying.

Documentation of the functions/methods are also needed.

Icon is ugly and needs to be improved.

Sponsored-By: Halland County Library

Test plan:
* Log into OPAC, search for a title, chose to edit it's MARC record.
* Chose a MARC field and modify it, press Save.
* Open the MARC editor again for the same title.
* Next to the edited field a new icon should appear, looking like
  a clock face with a encircling arrow; the history icon.
* Clicking the icon should open a table showing all changes done
  to the record, including value change, date/time and user name.
Comment 2 David Cook 2015-11-23 00:23:55 UTC
Comment on attachment 41642 [details] [review]
Bug 14367: Add MARC record history

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

::: C4/Biblio.pm
@@ +3531,4 @@
>  sub ModBiblioMarc {
>      # pass the MARC::Record to this function, and it will create the records in
>      # the marc field
> +    my ( $record, $history, $biblionumber, $frameworkcode ) = @_;

Why would you add $history as a 2nd positional parameter here? Wouldn't it make more sense to add it as the 4th, so that you wouldn't affect any existing calls to ModBiblioMarc that don't require the history?

I notice $history was added as the last positional parameter on other functions which use history, so doing that here would be more consistent as well...
Comment 3 David Cook 2015-11-23 00:25:54 UTC
Comment on attachment 41642 [details] [review]
Bug 14367: Add MARC record history

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

::: C4/Biblio.pm
@@ +141,5 @@
>        &TransformHtmlToXml
>        prepare_host_field
>      );
> +
> +    # History functions

Are we adding new functions to C4::Biblio, or should these have gone into something like Koha::Metadata::History?

::: cataloguing/addbiblio.pl
@@ +299,5 @@
>      my $index_subfield = CreateKey(); # create a specifique key for each subfield
>  
> +    my $taghistory = $history->{$tag}->{$subfield};
> +    if (defined $taghistory ) {
> +		warn "TAG HIST: $tag/$subfield", Dumper($taghistory);

This looks like debugging code, no?

@@ +523,4 @@
>      my @BIG_LOOP;
>      my %seen;
>      my @tab_data; # all tags to display
> +    #print STDERR "usedTagsLibs: ", Dumper(@$usedTagsLib);

Also looks like debugging code
Comment 4 Owen Leonard 2016-04-04 18:39:34 UTC
Based on comment 2 and comment 3 I think this should be 'Failed QA'
Comment 5 Aleisha Amohia 2016-10-18 02:21:05 UTC
Created attachment 56613 [details] [review]
Bug 14367: Add MARC record history

This is a proof-of-concept implementation for adding history
support to MARC records. Every time a change is made a complete
copy of the record is stored along with the date/time and user
identity. The changes are listed under each field in the MARC
record editor and can be reverted with a click.

The changes are stored as a JSON array in a new column named
'history' in the database. The array is re-read from the database
before updating the record to prevent old data lingering in
the session from overwriting newer changes made by other users.

If we decide to implement this feature it might be better to
simply create a new table altogether and link it rather than
the clumsy JSON solution. That would eliminate a lot of bulky
code that transforms MARC-KOHA-JSON and back while ensuring
data integrity.

Also, there are plans to add permissions to the MARC records;
this likely requires more complex interactions that will scale
badly with the current JSON solution.

At present, the history is hardcoded to 10 entries.
This can easily be made into a syspref.

The current implementation should probably be refactored
into a 'BiblioHistory' class before deploying.

Documentation of the functions/methods are also needed.

Icon is ugly and needs to be improved.

Sponsored-By: Halland County Library & Catalyst IT

Test plan:
* Run updatedatabase.pl
* Log into Staff interface, search for a title, choose to edit it's MARC record.
* Chose a MARC field and modify it, press Save.
* Open the MARC editor again for the same title.
* Next to the edited field a new icon should appear, looking like
  a clock face with a encircling arrow; the history icon.
* Clicking the icon should open a table showing all changes done
  to the record, including value change, date/time and user name.
* Clicking a record in the history log should revert that field to the
  clicked value (a previous value)
* Run prove t/db_dependent/BiblioHistory.t and confirm all tests pass
Comment 6 David Cook 2016-11-29 23:29:20 UTC
Not sure about the implementation of this idea, but I like the idea of storing a history for records...
Comment 7 Eric 2017-03-23 15:59:30 UTC
Patch didn't apply in the sandboxes.
Comment 8 Eric 2017-03-23 16:01:40 UTC
(In reply to Eric from comment #7)
> Patch didn't apply in the sandboxes.

Auto-merging koha-tmpl/intranet-tmpl/prog/js/cataloging.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/css/addbiblio.css
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/css/addbiblio.css
Auto-merging cataloguing/addbiblio.pl
Auto-merging C4/Biblio.pm
CONFLICT (content): Merge conflict in C4/Biblio.pm
Patch failed at 0001 Bug 14367: Add MARC record history
The copy of the patch that failed is found in: .git/rebase-apply/patch
Comment 9 Chris 2017-04-07 08:15:04 UTC
I like the idea of storing history with timestamp and author. But storing whole record seems like overkill to me. Especially for huge libraries.

So instead of whole history, just a "delta" of data changed (somewhat git does) and as mentioned - not JSON but TABLE.
Comment 10 David Cook 2017-04-10 07:01:37 UTC
(In reply to Chris from comment #9)
> I like the idea of storing history with timestamp and author. But storing
> whole record seems like overkill to me. Especially for huge libraries.
> 
> So instead of whole history, just a "delta" of data changed (somewhat git
> does) and as mentioned - not JSON but TABLE.

I like the idea of storing deltas, although I wonder what that looks like in practice.
Comment 11 David Cook 2017-04-11 01:19:16 UTC
(In reply to Chris from comment #9)
> I like the idea of storing history with timestamp and author. But storing
> whole record seems like overkill to me. Especially for huge libraries.
> 
> So instead of whole history, just a "delta" of data changed (somewhat git
> does) and as mentioned - not JSON but TABLE.

After doing some reading, it appears that Git actually does save the whole file during a commit. It will compress it using zlib, but it stores the whole file. However, when it does its garbage collection, it may discard old whole files and replace them with deltas for more efficient storage.

Using Compress::Zlib, which is a core Perl module, I compressed a 6.7KB 153 line MARCXML file to 1.3KB. 

Let's say we have 1,000,000 old versions... that's 976MB or .95GB. Not super huge but not tiny either. 

If we used GNU diff/patch or even 'git diff --no-index file1 file2', we could get diffs and store those. Using diff, I made a 467B patch for a one line change, and with git diff I got a 474B patch for the same one line change. 

Presuming 1,000,000 uncompressed patches of the same size, that's 445MB or .434GB.

If you compress the patch, you can get it down to 227B, so 216MB or .211GB for 1,000,000 compressed patches. That seems pretty decent to me.

So Compress::Zlib is easy to use and it's a core module. Awesome. 

I think making the deltas may be harder. You can use Git.pm to take advantage of 'git diff --no-index file1 file2', although git diff returns non zero for a diff, so Git.pm actually throws an exception which you have to catch and then you get the diff from there. Not that elegant... plus you'd have to write the records to temporary files (same with using GNU diff).

I've seen lots of diff modules on CPAN... and it seems that Text::Diff gets recommended a fair bit (https://metacpan.org/pod/Text::Diff) and shows up in the O'Reilly book it seems (http://docstore.mik.ua/orelly/perl4/cook/ch08_23.htm). I also found it was already installed on my Koha server, although I don't use the Debian packages, so I can't attest to that. It looks like it's required by Test::Differences and that lots of modules use that, so that's probably why it's on my system. I've just tried it out and it's easy to use. Looks like we'd want to use Text::Patch for applying the diff to rebuild a record... although I haven't tried it.

Using Text::Diff I got a patch that was 378B uncompressed.
Comment 12 David Cook 2017-04-11 01:25:05 UTC
Using whole records, it would be easy to recover an older version on a timeline. 

Per field changes would be harder... but you could always load the older version and then do your own diff algorithm for diffing MARC::Record objects... then choose which differences to apply... but that would be work.

Actually, a person could look at XML::Diff as well... as that could make computing the difference a lot easier at the field level. 

Personally, I like the idea of just using the whole record. It's less granular but it would be easier.
Comment 13 Aleisha Amohia 2017-06-13 01:29:47 UTC
Created attachment 64243 [details] [review]
Bug 14367: [FOLLOW-UP] Fixing merge conflicts...

...and other fixes to make patch work.
Comment 14 Hugo Agud 2017-08-22 08:44:35 UTC
56613 - Bug 14367: Add MARC record history
64243 - Bug 14367: [FOLLOW-UP] Fixing merge conflicts...

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14367: Add MARC record history
Using index info to reconstruct a base tree...
M	C4/Biblio.pm
M	cataloguing/addbiblio.pl
M	koha-tmpl/intranet-tmpl/prog/css/addbiblio.css
M	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
M	koha-tmpl/intranet-tmpl/prog/js/cataloging.js
<stdin>:376: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/js/cataloging.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/css/addbiblio.css
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/css/addbiblio.css
Auto-merging cataloguing/addbiblio.pl
CONFLICT (content): Merge conflict in cataloguing/addbiblio.pl
Auto-merging C4/Biblio.pm
CONFLICT (content): Merge conflict in C4/Biblio.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 14367: Add MARC record history
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-14367-Add-MARC-record-history-6jWpQA.patch
Comment 15 M. Tompsett 2017-08-25 03:05:14 UTC
The patch needs major intervention in reworking with the introduction of metadata in C4::Biblio.
Comment 16 Chris Cormack 2018-04-16 22:10:10 UTC
I am going to be working on this over the next few months