[Bug 22280] New: The ILL module assumes every status needs a next/previous status

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] New: The ILL module assumes every status needs a next/previous status

bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

            Bug ID: 22280
           Summary: The ILL module assumes every status needs a
                    next/previous status
 Change sponsored?: ---
           Product: Koha
           Version: master
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5 - low
         Component: ILL
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

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?

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] The ILL module assumes every status needs a next/previous status

bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

Andrew Isherwood <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrew.isherwood@ptfs-europ
                   |                            |e.com

--- Comment #1 from Andrew Isherwood <[hidden email]> ---
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.

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] The ILL module assumes every status needs a next/previous status

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

--- Comment #2 from Magnus Enger <[hidden email]> ---
(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. :-)

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] The ILL module assumes every status needs a next/previous status

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

--- Comment #3 from Magnus Enger <[hidden email]> ---
Now I can't reproduce this in master. Any chance it got solved as a side effect
of something else?

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] The ILL module assumes every status needs a next/previous status

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

--- Comment #4 from Tomás Cohen Arazi <[hidden email]> ---
Created attachment 91232
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91232&action=edit
Bug 22280: Fix typo in _status_graph_union

Signed-off-by: Tomas Cohen Arazi <[hidden email]>

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] The ILL module assumes every status needs a next/previous status

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |Needs Signoff
           Assignee|[hidden email]-commun |[hidden email]
                   |ity.org                     |
                 CC|                            |[hidden email]

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] The ILL module assumes every status needs a next/previous status

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

Martin Renvoize <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |martin.renvoize@ptfs-europe
                   |                            |.com

--- Comment #5 from Martin Renvoize <[hidden email]> ---
Slightly confused by the status of this.. is Tomas's patch achieving the same
end goal?

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] The ILL module assumes every status needs a next/previous status

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

M. Tompsett <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #6 from M. Tompsett <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 22280] The ILL module assumes every status needs a next/previous status

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://bugs.koha-community
                   |                            |.org/bugzilla3/show_bug.cgi
                   |                            |?id=20923

--- Comment #7 from Tomás Cohen Arazi <[hidden email]> ---
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.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/