If we used Scruct::Diff, we should keep the old values, but omit unchanged values

$diff = diff($x, $y, noU => 1 ); # omit unchanged items

This would give:
{
  "D": {
    "cardnumber": {
      "N": "9876543"
      "O": "12345"
    },
    "nickname": {
      "A": "K-dog"
    },
    "suffix": {
      "R": "Jr."
    }
  }
}

So we can display the old and new values for changed fields.

Yet another benefit would be the ability to run queries against the action log's data if/when the DBMS is JSON aware ( mysql 5.7 / maraia 10.2.3 ).

I like this.

+100 on this idea. Standardizing action logs across modules would be great. Storing both before and after values would be great. Making them all use JSON for easy reporting would be great.

This would be so great.  I often have a hard time interpreting the action logs - easily seeing the before and after values would be really helpful.

I'm going to propose we begin by adding a new column named "diff" to the action_logs table to store this json formatted diff. Once that's in we can work on the viewer as a followup.

(In reply to Kyle M Hall from comment #6)
> I'm going to propose we begin by adding a new column named "diff" to the
> action_logs table to store this json formatted diff. Once that's in we can
> work on the viewer as a followup.

That seems like a good low-bar option to get this moving.

I think it's a good idea, our current logs all log a little differently. A 'fresh start' in a new column makes sense to get out of the situation incrementally.

Created attachment 153600 [details] [review]
Bug 25159: DB Update

Created attachment 153601 [details] [review]
Bug 25159: DB Update

Created attachment 153602 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]

Created attachment 153603 [details] [review]
Bug 15159: Add ability to specify a pre-modified version of action log data and store as diff

Test Plan:
1) sudo cpanm Struct::Diff
2) prove t/db_dependent/Log.t

Created attachment 153604 [details] [review]
Bug 15159: Implement diffs in action logs for holds

Created attachment 153606 [details] [review]
Bug 25159: DB Update

Created attachment 153607 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]

Created attachment 153608 [details] [review]
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff

Test Plan:
1) sudo cpanm Struct::Diff
2) prove t/db_dependent/Log.t

Created attachment 153609 [details] [review]
Bug 25159: Implement diffs in action logs for holds

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Enable HoldsLog
5) Perform various hold related actions
6) Observe the diff column is populated by a JSON string
   of the diff format generated by Struct::Diff

Created attachment 153611 [details] [review]
Bug 25159: DB Update

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>

Created attachment 153612 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>

Created attachment 153613 [details] [review]
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff

Test Plan:
1) sudo cpanm Struct::Diff
2) prove t/db_dependent/Log.t

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>

Created attachment 153614 [details] [review]
Bug 25159: Implement diffs in action logs for holds

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Enable HoldsLog
5) Perform various hold related actions
6) Observe the diff column is populated by a JSON string
   of the diff format generated by Struct::Diff

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>

Those signoffs are actually mine, produced on a sandbox. Thanks!

(In reply to Andrew Fuerste-Henry from comment #22)
> Those signoffs are actually mine, produced on a sandbox. Thanks!

I'll get those fixed!

Created attachment 153637 [details] [review]
Bug 25159: DB Update

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Created attachment 153643 [details] [review]
Bug 25159: DB Update

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Created attachment 153644 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Created attachment 153645 [details] [review]
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff

Test Plan:
1) sudo cpanm Struct::Diff
2) prove t/db_dependent/Log.t

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Created attachment 153646 [details] [review]
Bug 25159: Implement diffs in action logs for holds

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Enable HoldsLog
5) Perform various hold related actions
6) Observe the diff column is populated by a JSON string
   of the diff format generated by Struct::Diff

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

1. I am not sure, but maybe we could pass $self and log $self->get_from_store as the original (?)
Maybe we will lose flexibility however.

2. What about the switch? Should we continue to log in 'info'?

Please give feedback when questions are raised.

(In reply to Jonathan Druart from comment #29)
> 1. I am not sure, but maybe we could pass $self and log
> $self->get_from_store as the original (?)
> Maybe we will lose flexibility however.
> 
> 2. What about the switch? Should we continue to log in 'info'?

I don't know enough about 1 to speak to that, but as far as 2 goes, I think yes, we continue logging in info, at least for now as it will show information that isn't changed and the diff will only show updated information.

(In reply to Jonathan Druart from comment #29)
> 1. I am not sure, but maybe we could pass $self and log
> $self->get_from_store as the original (?)
> Maybe we will lose flexibility however.

I don't think I understand your question. Could you give me some more detail?

> 2. What about the switch? Should we continue to log in 'info'?

Yes, I think for the time being we should continue to log in `info`. We can EOL the info column at some point in the future, but we should give everyone plenty of advance notice if we ever choose to do so.

(In reply to Kyle M Hall from comment #32)
> (In reply to Jonathan Druart from comment #29)
> > 1. I am not sure, but maybe we could pass $self and log
> > $self->get_from_store as the original (?)
> > Maybe we will lose flexibility however.
> 
> I don't think I understand your question. Could you give me some more detail?

Replace:
+    my $original = C4::Context->preference('HoldsLog') ? $self->unblessed : undef;
+    logaction( 'HOLDS', 'SUSPEND', $self->reserve_id, $self, undef, $original )

With
+    logaction( 'HOLDS', 'SUSPEND', $self->reserve_id, $self, undef )

And, in logaction, $original = $self->get_from_storage.

> Replace:
> +    my $original = C4::Context->preference('HoldsLog') ? $self->unblessed :
> undef;
> +    logaction( 'HOLDS', 'SUSPEND', $self->reserve_id, $self, undef,
> $original )
> 
> With
> +    logaction( 'HOLDS', 'SUSPEND', $self->reserve_id, $self, undef )
> 
> And, in logaction, $original = $self->get_from_storage.

I like it! Let's support both since $infos can be either!

Created attachment 159758 [details] [review]
Bug 25159: (QA follow-up) Allow passing of 'original' as hashref or Koha::Object

Created attachment 164597 [details] [review]
Bug 25159: DB Update

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Created attachment 164598 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Created attachment 164599 [details] [review]
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff

Test Plan:
1) sudo cpanm Struct::Diff
2) prove t/db_dependent/Log.t

Created attachment 164600 [details] [review]
Bug 25159: Implement diffs in action logs for holds

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Enable HoldsLog
5) Perform various hold related actions
6) Observe the diff column is populated by a JSON string
   of the diff format generated by Struct::Diff

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Created attachment 164601 [details] [review]
Bug 25159: (QA follow-up) Allow passing of 'original' as hashref or Koha::Object

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Tests for this bug are OK but prove t/db_dependent/Log.t fails on following error:

#   Failed test 'Reduce log size by unblessing Koha objects'
#   at t/db_dependent/Log.t line 195.
encountered object 't::lib::TestBuilder=HASH(0x556b88394790)', but neither allow_blessed, convert_blessed nor allow_tags settings are enabled (or TO_JSON/FREEZE method missing) at /home/emmi/Koha/C4/Log.pm line 137.

(In reply to Emmi Takkinen from comment #41)
> Tests for this bug are OK but prove t/db_dependent/Log.t fails on following
> error:
> 
> #   Failed test 'Reduce log size by unblessing Koha objects'
> #   at t/db_dependent/Log.t line 195.
> encountered object 't::lib::TestBuilder=HASH(0x556b88394790)', but neither
> allow_blessed, convert_blessed nor allow_tags settings are enabled (or
> TO_JSON/FREEZE method missing) at /home/emmi/Koha/C4/Log.pm line 137.

That unit test is passing for me!

(In reply to Kyle M Hall from comment #42)
> (In reply to Emmi Takkinen from comment #41)
> > Tests for this bug are OK but prove t/db_dependent/Log.t fails on following
> > error:
> > 
> > #   Failed test 'Reduce log size by unblessing Koha objects'
> > #   at t/db_dependent/Log.t line 195.
> > encountered object 't::lib::TestBuilder=HASH(0x556b88394790)', but neither
> > allow_blessed, convert_blessed nor allow_tags settings are enabled (or
> > TO_JSON/FREEZE method missing) at /home/emmi/Koha/C4/Log.pm line 137.
> 
> That unit test is passing for me!

I did suspect that this might be a problem in my test environment. Thank you for confirming it :D

Created attachment 165527 [details] [review]
Bug 25159: DB Update

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165528 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165529 [details] [review]
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff

Test Plan:
1) sudo cpanm Struct::Diff
2) prove t/db_dependent/Log.t

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165530 [details] [review]
Bug 25159: Implement diffs in action logs for holds

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Enable HoldsLog
5) Perform various hold related actions
6) Observe the diff column is populated by a JSON string
   of the diff format generated by Struct::Diff

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165531 [details] [review]
Bug 25159: (QA follow-up) Allow passing of 'original' as hashref or Koha::Object

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165532 [details] [review]
Bug 25159: (QA follow-up) Tidy changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Long overdue enhancements that Kyle and I discussed way back in 2020!  All working well and I'm looking forward to display improvements at the other end.

Passing QA

This is quite an odd one, I can't seem to run QA tools on the patch set.

FAIL	C4/Log.pm
   FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   FAIL	  valid
		Can't locate Struct/Diff.pm in @INC (you may need to install the Struct::Diff module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/lib /kohadevbox/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.36.0 /usr/local/share/perl/5.36.0 /usr/lib/x86_64-linux-gnu/perl5/5.36 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.36 /usr/share/perl/5.36 /usr/local/lib/site_perl)

...

The same tests worked fine all day. Only happens once the patches are applied?

Ok, this introduces a new dependency. We will need the packaging team to have a look at this.

+use Struct::Diff   qw( diff );

(In reply to Katrin Fischer from comment #52)
> Ok, this introduces a new dependency. We will need the packaging team to
> have a look at this.
> 
> +use Struct::Diff   qw( diff );

i'll have a go at packaging this one, its missing from debian.org

(In reply to Mason James from comment #53)
> (In reply to Katrin Fischer from comment #52)
> > Ok, this introduces a new dependency. We will need the packaging team to
> > have a look at this.
> > 
> > +use Struct::Diff   qw( diff );
> 
> i'll have a go at packaging this one, its missing from debian.org

Thanks Mason!

(In reply to Mason James from comment #53)
> (In reply to Katrin Fischer from comment #52)
> > Ok, this introduces a new dependency. We will need the packaging team to
> > have a look at this.
> > 
> > +use Struct::Diff   qw( diff );
> 
> i'll have a go at packaging this one, its missing from debian.org

package is pushed to koha-staging repo, ready for testing

Pushed for 24.05!

Well done everyone, thank you!

Why did we choose here for a perl module that has no Debian package?

Just glancing over cpan I am seeing for instance Data::Compare with a Debian package libdata-compare-perl.
And note that Struct::Diff is from 2019 with bus factor 0 and Data::Compare is from 2023 with bus factor 1.

I think that we should not just start packaging without asking for good reasons to  go for an 'unpackaged' perl module.. (Noting that I do not see any discussion about the choice for this module on the report.)

I asked the question to myself whilst QAing and couldn't find any comparable module..


Data::Compare isn't one I came across but a quick look now suggests it has very different output and use case.. i.e. it's a simple boolean result of are these things the same whilst Struct::Diff is rather different in that it offers a structured diff output highlight what's changed where.

I have actually done this before though come to think about it.. I should see what I used back then.. I was generating JSON::Patch patches.

Looks like I maintained my own JSON::Patch library back then. 8 years ago, wow.. I should really have upstreamed it to cpan.

Anywho, I think Struct::Diff, whilst not the nicest output for humans, works pretty well and isn't as badly maintained as you suggest.. it's got a few things depending on it etc.im happy to submit my JSON::Patch library is people would prefer however

I chose Struct::Diff because if generated the best format for the use case. I doubt a library like Struct::Diff requires much, if anything, in the way of maintenance. That said I see no problem replacing it with an alternative as long as we don't lose any functionality!

Created attachment 166219 [details] [review]
Bug 25159: (QA follow-up) Make dbrev idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Not backported to 23.11.x

If i understand correctly, this doesn't change anything in the interface, correct? If that is the case, there is nothing to change or edit in the manual. Please reopen with more details if I'm wrong!

(In reply to Caroline Cyr La Rose from comment #63)
> If i understand correctly, this doesn't change anything in the interface,
> correct? If that is the case, there is nothing to change or edit in the
> manual. Please reopen with more details if I'm wrong!

That is correct! Eventually we will want to build a viewer to see these diffs in the action logs viewer, but for now it's good to have them for devops to access.

Coming from 38531.

-    my $diff = ( $original && ref $updated eq 'HASH' ) ? encode_json( diff( $original, $updated, noU => 1 ) ) : undef;
+    my $diff = undef;
+    $diff = encode_json( diff( $original->unblessed, $updated, noU => 1 ) )
+        if blessed($original) && $original->isa('Koha::Object');
+    $diff //= encode_json( diff( $original, $updated, noU => 1 ) )
+        if $original && ref $updated eq 'HASH';

Would be nice to add some POD for this change? Bit obscure now imo.

Also note that this earlier line does not check $original:
        if ( $infos->isa('Koha::Item') && $modulename eq 'CATALOGUING' && $actionname eq 'MODIFY' ) {
            $infos = "item " . Dumper( $original->unblessed );

Feels suboptimal?