Bug 22280 - The ILL module assumes every status needs a next/previous status
Summary: The ILL module assumes every status needs a next/previous status
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
: 20923 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-02-06 09:57 UTC by Magnus Enger
Modified: 2019-11-15 07:43 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05


Attachments
Bug 22280: Fix typo in _status_graph_union (1.46 KB, patch)
2019-07-03 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22280: Fix typo in _status_graph_union (1.53 KB, patch)
2019-09-23 15:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22280: Fix typo in _status_graph_union (1.59 KB, patch)
2019-09-23 15:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22280: (RM follow-up) Correction to POD (937 bytes, patch)
2019-09-23 15:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22280: Add a unit test (5.30 KB, patch)
2019-10-01 09:30 UTC, Magnus Enger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2019-02-06 09:57:03 UTC
Swedish ILL is very centrally driven, so some status changes are only done by updates from the API. That means that these statuses do not need a prev_actions or a next_actions:

        IN_NEG => {
            prev_actions => [ ],
            id             => 'IN_NEG',
            name           => 'Inlån Negativt svar',
            ui_method_name => 'Negativt svar',
            method         => 'requestitem',
            next_actions   => [ ],
            ui_method_icon => 'fa-send-o'
        },

But leaving these blank as above leads to errors, around line 386 of /usr/share/koha/lib/Koha/Illrequest.pm: 

        # Update all core methods' next_actions.
        foreach my $prev_action ( @{$backend_status->{prev_actions}} ) {
            if ( grep $prev_action, @core_status_ids ) {
                my @next_actions =
                     @{$status_graph->{$prev_action}->{next_actions}}; ### ERROR HERE
                push @next_actions, $backend_status_key;
                $status_graph->{$prev_action}->{next_actions}
                    = \@next_actions;
            }
        }
        # Update all core methods' prev_actions
        foreach my $next_action ( @{$backend_status->{next_actions}} ) {
            if ( grep $next_action, @core_status_ids ) {
                my @prev_actions =
                     @{$status_graph->{$next_action}->{prev_actions}}; ### AND HERE
                push @prev_actions, $backend_status_key;
                $status_graph->{$next_action}->{prev_actions}
                    = \@prev_actions;
            }
        }

The error is something like "Can't treat an undefined value as an array". 

Changing those lines to check if an array is defined makes the error go away, and I have not seen any bad side effects:

   @{$status_graph->{$prev_action}->{next_actions}} if $status_graph->{$prev_action}->{next_actions}; ### NO MORE ERROR HERE
   @{$status_graph->{$next_action}->{prev_actions}} if $status_graph->{$next_action}->{prev_actions}; ### OR HERE

Does that change make sense to others, or am I missing some other way to work around this?
Comment 1 Andrew Isherwood 2019-02-07 11:43:14 UTC
Hi Magnuse

That's a very interesting point. It feels to me like an oversight, we should be allowing for the case where an status is isolated and doesn't come from anywhere or go anywhere.

Your solution seems like a reasonable way to address the problem.

My biggest concern at the moment would be where to place this bug fix in the ILL dependency tree. Illrequest.pm is pretty heavily modified in many of the bugs in the tree and, even though your proposed fixes are small, I'm not sure if they'd conflict. In which case, I'd be inclined to make it dependent on 18589. We already have at least one bug fix that is dependent on that bug, which is at the top of the current QA tree.
Comment 2 Magnus Enger 2019-02-11 10:02:04 UTC
(In reply to Andrew Isherwood from comment #1)
> My biggest concern at the moment would be where to place this bug fix in the
> ILL dependency tree. Illrequest.pm is pretty heavily modified in many of the
> bugs in the tree and, even though your proposed fixes are small, I'm not
> sure if they'd conflict. In which case, I'd be inclined to make it dependent
> on 18589. We already have at least one bug fix that is dependent on that
> bug, which is at the top of the current QA tree.

Sounds good to me. :-)
Comment 3 Magnus Enger 2019-04-09 12:00:02 UTC
Now I can't reproduce this in master. Any chance it got solved as a side effect of something else?
Comment 4 Tomás Cohen Arazi 2019-07-03 18:35:13 UTC
Created attachment 91232 [details] [review]
Bug 22280: Fix typo in _status_graph_union

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Martin Renvoize 2019-07-05 13:22:06 UTC
Slightly confused by the status of this.. is Tomas's patch achieving the same end goal?
Comment 6 M. Tompsett 2019-07-05 14:31:46 UTC
(In reply to Martin Renvoize from comment #5)
> Slightly confused by the status of this.. is Tomas's patch achieving the
> same end goal?

Actually, I believe it is correcting bad logic.

Next action (): 0 -- 0
Next action (undef): 0 -- 0
Next action (array): 1 -- 1 <-- the core_whatevs does have 'array'
Next action (nothing): 1 -- 0 <-- the core_whatevs doesn't have 'nothing'
Next action (ARRAY(0x55832fa45660)): 1 -- 0 <-- empty array
Next action (ARRAY(0x55832fa456f0)): 1 -- 0 <-- array with stuff in it
Next action (HASH(0x55832fa456a8)): 1 -- 0 <-- empty hash
Next action (HASH(0x55832fa456d8)): 1 -- 0 <-- hash with stuff in it

Though, I am uncomfortable with the:
    @{$back->{whatevs}}
because what if that is accidentally becoming an array of arrays? Which would always trigger true, which would certainly explode.

I'd prefer:
    my @previous_actions = $back->{whatevs};
    for my $previous_action (@previous_actions};
just to make it abundantly clear and prevent any accidental double nesting.

Though to prevent noise, I'd also prefer:
    grep { defined $next_action && $next_action eq $_ } @core_whatevs;

NOTE: I didn't get the variable names exact, but it should be clear the idea behind my comments.
Comment 7 Tomás Cohen Arazi 2019-07-12 03:31:46 UTC
Guys, this is probably a duplicate of bug 20923, which I've just found for the first time. Either solution is right, I mention it because Martin wasn't sure about the nature of the bug itself.
Comment 8 Andrew Isherwood 2019-09-23 15:34:55 UTC
Created attachment 93125 [details] [review]
Bug 22280: Fix typo in _status_graph_union

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Comment 9 Martin Renvoize 2019-09-23 15:38:26 UTC
*** Bug 20923 has been marked as a duplicate of this bug. ***
Comment 10 Martin Renvoize 2019-09-23 15:39:11 UTC
Taking Andrews SO as a QA here as he's the topic expert when it comes to ILL. :)
Comment 11 Martin Renvoize 2019-09-23 15:51:08 UTC
Created attachment 93127 [details] [review]
Bug 22280: Fix typo in _status_graph_union

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-09-23 15:51:11 UTC
Created attachment 93128 [details] [review]
Bug 22280: (RM follow-up) Correction to POD

Minor correction to mislabled POD section for private method.
Comment 13 Martin Renvoize 2019-09-23 15:52:12 UTC
I'd really like to see a unit test added for this however ;)
Comment 14 Magnus Enger 2019-10-01 09:30:26 UTC
Created attachment 93354 [details] [review]
Bug 22280: Add a unit test

This patch adds a unit test to t/db_dependent/Illrequests.t, where
a new node without any next_actions or prev_actions is added to the
core status graph.

Running the tests show a lot of warnings about "no query in
themelanguage", but that should not be related to the current bug.
Comment 15 Magnus Enger 2019-10-01 09:31:27 UTC
OK, I tried to add a unit test. Hopes it makes sense! Also reset status to PQA.
Comment 16 Martin Renvoize 2019-10-01 13:45:17 UTC
Nice work!

Pushed to master for 19.11.00
Comment 17 Magnus Enger 2019-10-22 07:06:48 UTC
(In reply to Martin Renvoize from comment #16)
> Pushed to master for 19.11.00

This is causing problems in 19.05.04, so if it could be backported to 19.05 that would be super awesome!
Comment 18 Fridolin SOMERS 2019-11-15 07:43:08 UTC
Pushed to 19.05.x for 19.05.05