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: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
: 28500 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-06-09 08:48 UTC by Viktor Sarge
Modified: 2023-07-05 21:08 UTC (History)
20 users (show)

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


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
Bug 14367: Add MARC record history (38.50 KB, patch)
2019-02-07 01:29 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: [FOLLOW-UP] Fixing merge conflicts... (3.88 KB, patch)
2019-02-07 01:29 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Rebasing to make patch work on master (5.31 KB, patch)
2019-02-07 20:16 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (38.37 KB, patch)
2019-04-26 02:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Fixing merge conflicts... (3.88 KB, patch)
2019-04-26 02:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Rebasing to make patch work on master (5.31 KB, patch)
2019-04-26 02:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (39.03 KB, patch)
2020-03-29 23:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Fixing merge conflicts... (3.90 KB, patch)
2020-03-29 23:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Rebasing to make patch work on master (6.01 KB, patch)
2020-03-29 23:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (39.08 KB, patch)
2020-04-01 08:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: (follow-up) Fixing merge conflicts... (3.95 KB, patch)
2020-04-01 08:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: (follow-up) Rebasing to make patch work on master (6.06 KB, patch)
2020-04-01 08:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (39.17 KB, patch)
2020-09-16 02:43 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Fixing merge conflicts... (3.48 KB, patch)
2020-09-16 02:43 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Rebasing to make patch work on master (7.07 KB, patch)
2020-09-16 02:44 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (39.21 KB, patch)
2020-09-16 17:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: (follow-up) Fixing merge conflicts... (3.53 KB, patch)
2020-09-16 17:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: (follow-up) Rebasing to make patch work on master (7.12 KB, patch)
2020-09-16 17:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (39.22 KB, patch)
2020-09-20 22:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Fixing merge conflicts... (3.53 KB, patch)
2020-09-20 22:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: (follow-up) Rebasing to make patch work on master (7.12 KB, patch)
2020-09-20 22:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (39.27 KB, patch)
2020-09-21 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: (follow-up) Fixing merge conflicts... (3.58 KB, patch)
2020-09-21 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: (follow-up) Rebasing to make patch work on master (7.17 KB, patch)
2020-09-21 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (41.60 KB, patch)
2021-02-10 02:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (41.39 KB, patch)
2021-05-05 05:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (41.31 KB, patch)
2021-05-17 01:14 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (41.35 KB, patch)
2021-05-17 22:10 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (41.21 KB, patch)
2021-10-18 01:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Updated schema files (1.40 KB, patch)
2021-10-18 01:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (40.91 KB, patch)
2022-03-01 23:44 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Updated schema files (1.40 KB, patch)
2022-03-01 23:45 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (40.86 KB, patch)
2023-01-25 03:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Updated schema files (1.40 KB, patch)
2023-01-25 03:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Add MARC record history (41.41 KB, patch)
2023-02-21 03:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14367: Updated schema files (2.48 KB, patch)
2023-02-21 03:30 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 Mark 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
Comment 17 Aleisha Amohia 2019-02-07 01:29:25 UTC
Created attachment 84841 [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 18 Aleisha Amohia 2019-02-07 01:29:32 UTC
Created attachment 84842 [details] [review]
Bug 14367: [FOLLOW-UP] Fixing merge conflicts...

...and other fixes to make patch work.
Comment 19 Aleisha Amohia 2019-02-07 01:29:49 UTC
Rebased
Comment 20 Aleisha Amohia 2019-02-07 20:16:10 UTC
Created attachment 84859 [details] [review]
Bug 14367: (follow-up) Rebasing to make patch work on master

Run through test plan and confirm everything still works as expected.
Comment 21 Liz Rea 2019-04-24 15:51:58 UTC
So terribly sorry Aleisha, this patch doesn't apply cleanly anymore.

Liz
Comment 22 Aleisha Amohia 2019-04-26 02:11:28 UTC
Created attachment 88827 [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 23 Aleisha Amohia 2019-04-26 02:11:36 UTC
Created attachment 88828 [details] [review]
Bug 14367: (follow-up) Fixing merge conflicts...

...and other fixes to make patch work.
Comment 24 Aleisha Amohia 2019-04-26 02:12:15 UTC
Created attachment 88829 [details] [review]
Bug 14367: (follow-up) Rebasing to make patch work on master

Run through test plan and confirm everything still works as expected.
Comment 25 Aleisha Amohia 2019-04-26 02:12:31 UTC
Rebased
Comment 26 David Nind 2020-02-04 09:58:21 UTC
Patches no longer apply.
Comment 27 Aleisha Amohia 2020-03-29 23:30:27 UTC
Created attachment 102065 [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 28 Aleisha Amohia 2020-03-29 23:30:31 UTC
Created attachment 102066 [details] [review]
Bug 14367: (follow-up) Fixing merge conflicts...

...and other fixes to make patch work.
Comment 29 Aleisha Amohia 2020-03-29 23:40:31 UTC
Created attachment 102067 [details] [review]
Bug 14367: (follow-up) Rebasing to make patch work on master

Run through test plan and confirm everything still works as expected.
Comment 30 David Nind 2020-03-30 07:39:32 UTC
Hi Aleisha.

Everything works as expected for me.

Things I noted:

1. The first change doesn't record what was originally there. For example, if the Title (245 tag a) was 'Abc' and I change it to 'XYZ' then the history table/log shows as 'XYZ', rather than 'Abc'. Later changes are as expected in the history, and can be restored.

2. The history recorded seems to be at the field level rather than the subfield level. For example, if 245 has subfield a and c and I change a, c also shows the change history. 

Happy to sign off if that is what is expected for the first iteration.

David
Comment 31 Aleisha Amohia 2020-04-01 04:40:51 UTC
I think what we have is good for a first iteration. I'm happy to work on the extra stuff once this is through as it is working.
Comment 32 David Nind 2020-04-01 08:20:13 UTC
Created attachment 102173 [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

Signed-off-by: David Nind <david@davidnind.com>
Comment 33 David Nind 2020-04-01 08:20:18 UTC
Created attachment 102174 [details] [review]
Bug 14367: (follow-up) Fixing merge conflicts...

...and other fixes to make patch work.

Signed-off-by: David Nind <david@davidnind.com>
Comment 34 David Nind 2020-04-01 08:20:24 UTC
Created attachment 102175 [details] [review]
Bug 14367: (follow-up) Rebasing to make patch work on master

Run through test plan and confirm everything still works as expected.

Signed-off-by: David Nind <david@davidnind.com>
Comment 35 Katrin Fischer 2020-06-21 15:53:18 UTC
Really interesting feature, starting with QA tools:

 FAIL	installer/data/mysql/atomicupdate/add_history.sql
   FAIL	  git manipulation
		The file has been added and deleted in the same patchset

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
   FAIL	  filters
		missing_filter at line 903 (                                                    <a href="#" class="buttonHistory" id="[%- mv.id -%]">)
		missing_filter at line 904 (                                                        <img src="[% interface %]/[% theme %]/img/icon-history.png" alt="History" title="Display history for this field" />)
		missing_filter at line 904 (                                                        <img src="[% interface %]/[% theme %]/img/icon-history.png" alt="History" title="Display history for this field" />)
		missing_filter at line 908 (                                                <table class="history_table" id="[%- mv.id -%]_history" name="[%- mv.name -%]_history">)
		missing_filter at line 908 (                                                <table class="history_table" id="[%- mv.id -%]_history" name="[%- mv.name -%]_history">)
		missing_filter at line 916 (                                                        <td><a href="#" data-tag='[%- mv.id -%]' data-history='[% hist.data %]' class='hist_rollback'>[% hist.data %]</a></td>)
		missing_filter a

 FAIL	Koha/MetadataRecord/History.pm
   FAIL	  critic
		"return" statement with explicit "undef" at line 103, column 9. See page 199 of PBP.

Can you please fix?
Comment 36 Aleisha Amohia 2020-09-16 02:43:47 UTC
Created attachment 110138 [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

Signed-off-by: David Nind <david@davidnind.com>
Comment 37 Aleisha Amohia 2020-09-16 02:43:56 UTC
Created attachment 110139 [details] [review]
Bug 14367: (follow-up) Fixing merge conflicts...

...and other fixes to make patch work.

Signed-off-by: David Nind <david@davidnind.com>
Comment 38 Aleisha Amohia 2020-09-16 02:44:03 UTC
Created attachment 110140 [details] [review]
Bug 14367: (follow-up) Rebasing to make patch work on master

Run through test plan and confirm everything still works as expected.

Signed-off-by: David Nind <david@davidnind.com>
Comment 39 David Nind 2020-09-16 17:13:21 UTC
Created attachment 110225 [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

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 David Nind 2020-09-16 17:13:25 UTC
Created attachment 110226 [details] [review]
Bug 14367: (follow-up) Fixing merge conflicts...

...and other fixes to make patch work.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 41 David Nind 2020-09-16 17:13:32 UTC
Created attachment 110227 [details] [review]
Bug 14367: (follow-up) Rebasing to make patch work on master

Run through test plan and confirm everything still works as expected.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 42 Katrin Fischer 2020-09-20 20:12:24 UTC
Tiny conflict, can you please rebase?
Comment 43 Aleisha Amohia 2020-09-20 22:17:09 UTC
Created attachment 110442 [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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 Aleisha Amohia 2020-09-20 22:17:15 UTC
Created attachment 110443 [details] [review]
Bug 14367: (follow-up) Fixing merge conflicts...

...and other fixes to make patch work.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 Aleisha Amohia 2020-09-20 22:17:21 UTC
Created attachment 110444 [details] [review]
Bug 14367: (follow-up) Rebasing to make patch work on master

Run through test plan and confirm everything still works as expected.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 46 David Nind 2020-09-21 19:53:40 UTC
Created attachment 110489 [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

Signed-off-by: David Nind <david@davidnind.com>
Comment 47 David Nind 2020-09-21 19:53:46 UTC
Created attachment 110490 [details] [review]
Bug 14367: (follow-up) Fixing merge conflicts...

...and other fixes to make patch work.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 48 David Nind 2020-09-21 19:53:53 UTC
Created attachment 110491 [details] [review]
Bug 14367: (follow-up) Rebasing to make patch work on master

Run through test plan and confirm everything still works as expected.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 49 Katrin Fischer 2020-10-11 10:42:30 UTC
The patch no longer applies, can you please rebase?

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	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Auto-merging C4/Biblio.pm
CONFLICT (content): Merge conflict in C4/Biblio.pm
error: 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: .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-X_q2HV.patch
Comment 50 Aleisha Amohia 2021-02-10 02:13:07 UTC
Created attachment 116621 [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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 51 Séverine Queune 2021-04-02 10:27:30 UTC
Sorry Aleisha, i'd be happy to test if you can rebase !

CONFLICT (content): Merge conflict in cataloguing/addbiblio.pl
Auto-merging C4/Biblio.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 14367: Add MARC record history
Comment 52 Aleisha Amohia 2021-05-05 05:17:53 UTC
Created attachment 120480 [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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 53 David Nind 2021-05-13 12:09:44 UTC
I was too slow in retesting! The patch no longer applies 8-(..
Comment 54 Aleisha Amohia 2021-05-17 01:14:48 UTC
Created attachment 121025 [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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 55 David Nind 2021-05-17 10:17:41 UTC
Thanks Aleisha.

I'm getting an error when clicking save after making a change to a record:

No property history for Koha::Biblio::Metadata at /usr/share/perl5/Exception/Class/Base.pm line 88

    in Exception::Class::Base::throw at /usr/share/perl5/Exception/Class/Base.pm line 88

       85: 
       86:     $proto->rethrow if ref $proto;
       87: 
       88:     die $proto->new(@_);
       89: }
       90: 
       91: sub rethrow {

    Show function arguments
    in Koha::Object::set at /kohadevbox/koha/Koha/Object.pm line 270

      267:     my @columns = @{$self->_columns()};
      268: 
      269:     foreach my $p ( keys %$properties ) {
      270:         unless ( grep { $_ eq $p } @columns ) {
      271:             Koha::Exceptions::Object::PropertyNotFound->throw( "No property $p for " . ref($self) );
      272:         }
      273:     }

    Show function arguments
    in Koha::Object::update at /kohadevbox/koha/Koha/Object.pm line 217

      214: sub update {
      215:     my ($self, $values) = @_;
      216:     Koha::Exceptions::Object::NotInStorage->throw unless $self->in_storage;
      217:     $self->set($values)->store();
      218: }
      219: 
      220: =head3 $object->delete();

    Show function arguments

....
Comment 56 Aleisha Amohia 2021-05-17 22:09:33 UTC
(In reply to David Nind from comment #55)
> Thanks Aleisha.
> 
> I'm getting an error when clicking save after making a change to a record:
> 
> No property history for Koha::Biblio::Metadata at
> /usr/share/perl5/Exception/Class/Base.pm line 88
> 
>     in Exception::Class::Base::throw at
> /usr/share/perl5/Exception/Class/Base.pm line 88
> 
>        85: 
>        86:     $proto->rethrow if ref $proto;
>        87: 
>        88:     die $proto->new(@_);
>        89: }
>        90: 
>        91: sub rethrow {
> 
>     Show function arguments
>     in Koha::Object::set at /kohadevbox/koha/Koha/Object.pm line 270
> 
>       267:     my @columns = @{$self->_columns()};
>       268: 
>       269:     foreach my $p ( keys %$properties ) {
>       270:         unless ( grep { $_ eq $p } @columns ) {
>       271:             Koha::Exceptions::Object::PropertyNotFound->throw(
> "No property $p for " . ref($self) );
>       272:         }
>       273:     }
> 
>     Show function arguments
>     in Koha::Object::update at /kohadevbox/koha/Koha/Object.pm line 217
> 
>       214: sub update {
>       215:     my ($self, $values) = @_;
>       216:     Koha::Exceptions::Object::NotInStorage->throw unless
> $self->in_storage;
>       217:     $self->set($values)->store();
>       218: }
>       219: 
>       220: =head3 $object->delete();
> 
>     Show function arguments
> 
> ....

Probably just my test plan missing the requirement of updating the schema files! Can you please test again?
Comment 57 Aleisha Amohia 2021-05-17 22:10:22 UTC
Created attachment 121076 [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:
* Update the database, update the schema files, restart services
* 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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 58 David Nind 2021-05-18 11:36:23 UTC
> Probably just my test plan missing the requirement of updating the schema
> files! Can you please test again?

I've done again after running dbic, but I still get the same error.

Even if I don't change anything and just click save.
Comment 59 Aleisha Amohia 2021-05-18 21:14:29 UTC
That's weird I am totally not getting that error. What steps are you taking to reproduce?
Comment 60 David Nind 2021-05-18 21:23:30 UTC
Hi Aleisha.

I'm using koha-testing-docker:

* apply patch
* databaseupdate
* dbic
* flush_memcached
* restart_all
* edit a record (tried a few different records)
* save (without making any changes)

David
Comment 61 Donna 2021-06-02 16:44:06 UTC
*** Bug 28500 has been marked as a duplicate of this bug. ***
Comment 62 Hakam Almotlak 2021-06-23 14:02:49 UTC
patch does not apply
Applying: Bug 14367: Add MARC record history
Using index info to reconstruct a base tree...
M	C4/Biblio.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Biblio.pm
CONFLICT (content): Merge conflict in C4/Biblio.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 14367: Add MARC record history
hint: Use 'git am --show-current-patch' to see the failed 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-1fUpWI.patch
Comment 63 Aleisha Amohia 2021-10-18 01:08:53 UTC
Created attachment 126413 [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:
* Update the database, update the schema files, restart services
* 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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 64 Aleisha Amohia 2021-10-18 01:08:58 UTC
Created attachment 126414 [details] [review]
Bug 14367: Updated schema files
Comment 65 Frank Hansen 2021-12-23 16:39:50 UTC
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
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
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
error: Failed to merge in the changes.
Patch failed at 0001 Bug 14367: Add MARC record history

Patch doesn't apply.
Rebase please.
Comment 66 Aleisha Amohia 2022-03-01 23:44:08 UTC
Created attachment 131214 [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:
* Update the database, update the schema files, restart services
* 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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 67 Aleisha Amohia 2022-03-01 23:45:51 UTC
Created attachment 131215 [details] [review]
Bug 14367: Updated schema files
Comment 68 Hammat wele 2022-11-21 17:55:19 UTC
Applying: Bug 14367: Add MARC record history
.git/rebase-apply/patch:393: trailing whitespace.
    
warning: 1 line adds whitespace errors.
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
.git/rebase-apply/patch:393: trailing whitespace.
    
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
CONFLICT (content): Merge conflict in 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
error: Failed to merge in the changes.
Comment 69 Aleisha Amohia 2023-01-25 03:12:41 UTC
Created attachment 145655 [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:
* Update the database, update the schema files, restart services
* 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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 70 Aleisha Amohia 2023-01-25 03:12:50 UTC
Created attachment 145656 [details] [review]
Bug 14367: Updated schema files
Comment 71 David Nind 2023-01-26 20:01:12 UTC
Notes from testing (using koha-testing-docker):

1. The tests fail:

prove -vvv t/db_dependent/BiblioHistory.t
t/db_dependent/BiblioHistory.t .. 
1..6
ok 1 - use C4::Biblio;
Error: value hash contains unrecognized columns: branchcode at /kohadevbox/koha/t/lib/TestBuilder.pm line 286.
# Looks like your test exited with 11 just after 1.
Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 5/6 subtests 

Test Summary Report
-------------------
t/db_dependent/BiblioHistory.t (Wstat: 2816 Tests: 1 Failed: 0)
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 6 tests but ran 1.
Files=1, Tests=1,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.11 cusr  0.23 csys =  1.36 CPU)
Result: FAIL

2. After applying the patch, I ran updatedatabase, flush_memcached and restart_all. I didn't run dbic as the updated schema files patch takes care of things (if I do run dbic I get an error when saving the record).

3. It is a while ago since I last tested. But I think when I made a change (for example, to a title), only the title had the history button. Now, every existing entry has one (excluding 000, 001, 005, 008). But I could be misremembering!
Comment 72 Aleisha Amohia 2023-02-21 03:30:23 UTC
Created attachment 146997 [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:
* Update the database, update the schema files, restart services
* 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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 73 Aleisha Amohia 2023-02-21 03:30:27 UTC
Created attachment 146998 [details] [review]
Bug 14367: Updated schema files