Bug 25159 - Action logs should be stored in JSON (and as a diff of the change)
Summary: Action logs should be stored in JSON (and as a diff of the change)
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 11 votes (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords: dependency, release-notes-needed
Depends on:
Blocks: 36698 36699
  Show dependency treegraph
 
Reported: 2020-04-15 13:37 UTC by Kyle M Hall
Modified: 2024-05-03 16:50 UTC (History)
17 users (show)

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


Attachments
Bug 25159: DB Update (1.78 KB, patch)
2023-07-18 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: DB Update (1.78 KB, patch)
2023-07-18 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: Update Schema File [DO NOT PUSH] (1.49 KB, patch)
2023-07-18 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15159: Add ability to specify a pre-modified version of action log data and store as diff (5.08 KB, patch)
2023-07-18 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15159: Implement diffs in action logs for holds (5.09 KB, patch)
2023-07-18 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: DB Update (1.78 KB, patch)
2023-07-18 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: Update Schema File [DO NOT PUSH] (1.49 KB, patch)
2023-07-18 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff (5.07 KB, patch)
2023-07-18 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: Implement diffs in action logs for holds (5.27 KB, patch)
2023-07-18 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: DB Update (1.83 KB, patch)
2023-07-18 15:12 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25159: Update Schema File [DO NOT PUSH] (1.55 KB, patch)
2023-07-18 15:12 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff (5.12 KB, patch)
2023-07-18 15:12 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25159: Implement diffs in action logs for holds (5.33 KB, patch)
2023-07-18 15:12 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25159: DB Update (1.89 KB, patch)
2023-07-19 10:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: DB Update (1.89 KB, patch)
2023-07-19 11:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: Update Schema File [DO NOT PUSH] (1.60 KB, patch)
2023-07-19 11:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff (5.18 KB, patch)
2023-07-19 11:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: Implement diffs in action logs for holds (5.39 KB, patch)
2023-07-19 11:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: (QA follow-up) Allow passing of 'original' as hashref or Koha::Object (2.81 KB, patch)
2023-12-12 17:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25159: DB Update (1.96 KB, patch)
2024-04-10 10:56 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 25159: Update Schema File [DO NOT PUSH] (1.67 KB, patch)
2024-04-10 10:56 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff (5.24 KB, patch)
2024-04-10 10:57 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 25159: Implement diffs in action logs for holds (5.45 KB, patch)
2024-04-10 10:57 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 25159: (QA follow-up) Allow passing of 'original' as hashref or Koha::Object (2.87 KB, patch)
2024-04-10 10:58 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 25159: DB Update (2.02 KB, patch)
2024-04-25 11:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25159: Update Schema File [DO NOT PUSH] (1.73 KB, patch)
2024-04-25 11:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25159: Add ability to specify a pre-modified version of action log data and store as diff (5.30 KB, patch)
2024-04-25 11:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25159: Implement diffs in action logs for holds (5.51 KB, patch)
2024-04-25 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25159: (QA follow-up) Allow passing of 'original' as hashref or Koha::Object (2.93 KB, patch)
2024-04-25 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25159: (QA follow-up) Tidy changes (6.08 KB, patch)
2024-04-25 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-04-15 13:37:04 UTC
Action logs would be much more readable if we changed the format from a raw Data::Dumper::Dumper string to JSON which we could format nicely in the interface.

In addition, we could store changes to database rows ( and other data structures ) as a diff stored in JSON. I think the format from Struct::Diff::diff is nice.

Given:
my $x = {
    surname => 'Hall',
    firstname => 'Kyle',
    cardnumber => '12345',
    suffix => "Jr.",
};

my $y = {
    surname => 'Hall',
    firstname => 'Kyle',
    cardnumber => '9876543',
    nickname => "K-dog"
};

The diff looks like:

{
  "D": {
    "cardnumber": {
      "N": "9876543"
    },
    "nickname": {
      "A": "K-dog"
    },
    "suffix": {
      "R": "Jr."
    }
  }
}

Where the D key is the difference, and indicates cardnumber has changed with an N, nickname has been added with an A, and suffix has been removed with an R.

With this data we can easily construct a nice diff view ( perhaps green for added, red for removed, and yellow for changes showing the old and new value ).
Comment 1 Kyle M Hall 2020-04-15 13:58:12 UTC
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.
Comment 2 Kyle M Hall 2020-04-15 14:01:27 UTC
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 ).
Comment 3 Tomás Cohen Arazi 2020-04-15 15:55:19 UTC
I like this.
Comment 4 Andrew Fuerste-Henry 2022-12-16 14:18:02 UTC
+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.
Comment 5 Barbara Johnson 2023-07-11 16:05:52 UTC
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.
Comment 6 Kyle M Hall 2023-07-11 16:10:24 UTC
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.
Comment 7 Andrew Fuerste-Henry 2023-07-11 17:28:36 UTC
(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.
Comment 8 Katrin Fischer 2023-07-16 10:27:51 UTC
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.
Comment 9 Kyle M Hall 2023-07-18 13:08:34 UTC
Created attachment 153600 [details] [review]
Bug 25159: DB Update
Comment 10 Kyle M Hall 2023-07-18 13:08:53 UTC
Created attachment 153601 [details] [review]
Bug 25159: DB Update
Comment 11 Kyle M Hall 2023-07-18 13:09:18 UTC
Created attachment 153602 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]
Comment 12 Kyle M Hall 2023-07-18 13:09:20 UTC
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
Comment 13 Kyle M Hall 2023-07-18 13:09:23 UTC
Created attachment 153604 [details] [review]
Bug 15159: Implement diffs in action logs for holds
Comment 14 Kyle M Hall 2023-07-18 13:40:53 UTC
Created attachment 153606 [details] [review]
Bug 25159: DB Update
Comment 15 Kyle M Hall 2023-07-18 13:41:00 UTC
Created attachment 153607 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]
Comment 16 Kyle M Hall 2023-07-18 13:41:02 UTC
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
Comment 17 Kyle M Hall 2023-07-18 13:41:04 UTC
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
Comment 18 ByWater Sandboxes 2023-07-18 15:12:30 UTC
Created attachment 153611 [details] [review]
Bug 25159: DB Update

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Comment 19 ByWater Sandboxes 2023-07-18 15:12:32 UTC
Created attachment 153612 [details] [review]
Bug 25159: Update Schema File [DO NOT PUSH]

Signed-off-by: Kyle Hall <kyle@bywatersolutions.com>
Comment 20 ByWater Sandboxes 2023-07-18 15:12:34 UTC
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>
Comment 21 ByWater Sandboxes 2023-07-18 15:12:37 UTC
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>
Comment 22 Andrew Fuerste-Henry 2023-07-18 15:13:16 UTC
Those signoffs are actually mine, produced on a sandbox. Thanks!
Comment 23 Kyle M Hall 2023-07-19 10:46:13 UTC
(In reply to Andrew Fuerste-Henry from comment #22)
> Those signoffs are actually mine, produced on a sandbox. Thanks!

I'll get those fixed!
Comment 24 Kyle M Hall 2023-07-19 10:50:08 UTC
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>
Comment 25 Kyle M Hall 2023-07-19 11:57:08 UTC
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>
Comment 26 Kyle M Hall 2023-07-19 11:57:15 UTC
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>
Comment 27 Kyle M Hall 2023-07-19 11:57:17 UTC
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>
Comment 28 Kyle M Hall 2023-07-19 11:57:20 UTC
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>
Comment 29 Jonathan Druart 2023-08-03 14:50:24 UTC
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'?
Comment 30 Marcel de Rooy 2023-09-29 09:38:27 UTC
Please give feedback when questions are raised.
Comment 31 Lisette Scheer 2023-10-30 21:15:48 UTC
(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.
Comment 32 Kyle M Hall 2023-12-11 15:19:48 UTC
(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.
Comment 33 Jonathan Druart 2023-12-12 07:55:08 UTC
(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.
Comment 34 Kyle M Hall 2023-12-12 17:00:27 UTC
> 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!
Comment 35 Kyle M Hall 2023-12-12 17:04:44 UTC
Created attachment 159758 [details] [review]
Bug 25159: (QA follow-up) Allow passing of 'original' as hashref or Koha::Object
Comment 36 Emmi Takkinen 2024-04-10 10:56:07 UTC
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>
Comment 37 Emmi Takkinen 2024-04-10 10:56:33 UTC
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>
Comment 38 Emmi Takkinen 2024-04-10 10:57:07 UTC
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
Comment 39 Emmi Takkinen 2024-04-10 10:57:34 UTC
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>
Comment 40 Emmi Takkinen 2024-04-10 10:58:03 UTC
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>
Comment 41 Emmi Takkinen 2024-04-10 10:58:59 UTC
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.
Comment 42 Kyle M Hall 2024-04-11 10:25:43 UTC
(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!
Comment 43 Emmi Takkinen 2024-04-11 10:30:07 UTC
(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
Comment 44 Martin Renvoize 2024-04-25 11:54:50 UTC
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>
Comment 45 Martin Renvoize 2024-04-25 11:54:54 UTC
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>
Comment 46 Martin Renvoize 2024-04-25 11:54:57 UTC
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>
Comment 47 Martin Renvoize 2024-04-25 11:55:01 UTC
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>
Comment 48 Martin Renvoize 2024-04-25 11:55:05 UTC
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>
Comment 49 Martin Renvoize 2024-04-25 11:55:09 UTC
Created attachment 165532 [details] [review]
Bug 25159: (QA follow-up) Tidy changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2024-04-25 11:56:00 UTC
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
Comment 51 Katrin Fischer 2024-04-29 16:18:59 UTC
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?
Comment 52 Katrin Fischer 2024-04-29 16:22:52 UTC
Ok, this introduces a new dependency. We will need the packaging team to have a look at this.

+use Struct::Diff   qw( diff );
Comment 53 Mason James 2024-04-30 13:57:05 UTC
(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
Comment 54 Kyle M Hall 2024-04-30 14:35:57 UTC
(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!
Comment 55 Mason James 2024-04-30 15:01:31 UTC
(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
Comment 56 Katrin Fischer 2024-05-02 14:48:53 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 57 Marcel de Rooy 2024-05-03 06:25:44 UTC
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.)
Comment 58 Martin Renvoize 2024-05-03 16:21:26 UTC
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.
Comment 59 Martin Renvoize 2024-05-03 16:50:26 UTC
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