[Bug 22280] New: The ILL module assumes every status needs a next/previous status
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: koha-bugs@lists.koha-community.org Reporter: magnus@libriotech.no 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Andrew Isherwood <andrew.isherwood@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew.isherwood@ptfs-europ | |e.com --- Comment #1 from Andrew Isherwood <andrew.isherwood@ptfs-europe.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #2 from Magnus Enger <magnus@libriotech.no> --- (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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #3 from Magnus Enger <magnus@libriotech.no> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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 <tomascohen@theke.io> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- (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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=20923 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Andrew Isherwood <andrew.isherwood@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91232|0 |1 is obsolete| | --- Comment #8 from Andrew Isherwood <andrew.isherwood@ptfs-europe.com> --- Created attachment 93125 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93125&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- *** Bug 20923 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Taking Andrews SO as a QA here as he's the topic expert when it comes to ILL. :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93125|0 |1 is obsolete| | --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 93127 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93127&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 93128 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93128&action=edit Bug 22280: (RM follow-up) Correction to POD Minor correction to mislabled POD section for private method. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I'd really like to see a unit test added for this however ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #14 from Magnus Enger <magnus@libriotech.no> --- Created attachment 93354 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93354&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #15 from Magnus Enger <magnus@libriotech.no> --- OK, I tried to add a unit test. Hopes it makes sense! Also reset status to PQA. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |19.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work! Pushed to master for 19.11.00 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 --- Comment #17 from Magnus Enger <magnus@libriotech.no> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|19.11.00 |19.11.00,19.05.05 released in| | Status|Pushed to master |Pushed to stable --- Comment #18 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 19.05.x for 19.05.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22280 Andrew Isherwood <bugzilla@warmlight.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|bugzilla@warmlight.co.uk | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org