Bug 28932 - Backend overriding status_graph element causes duplicate actions
Summary: Backend overriding status_graph element causes duplicate actions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Andrew Isherwood
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-31 13:32 UTC by Andrew Isherwood
Modified: 2022-06-06 20:31 UTC (History)
7 users (show)

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


Attachments
Bug 28932: Fix duplicate next_action & prev_action (7.30 KB, patch)
2021-08-31 14:14 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 28932: Regression tests (5.98 KB, patch)
2022-01-05 13:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28932: Fix duplicate next_action & prev_action (1.76 KB, patch)
2022-01-05 13:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2021-08-31 13:32:43 UTC
Koha::Illrequest::_status_graph_union merges the core status graph and one provided by a backend. Although not officially supported...

> Whilst it is not intended that this works, you can override entries in $origin
> with entries with the same key in $new_graph.

...a backend should be able to override a core method. This was necessary when implementing the RapidILL backend. However, this revealed a bug in which any next_actions and prev_actions declared in both would end up being duplicated, leading to duplicate buttons in the toolbar.
Comment 1 Andrew Isherwood 2021-08-31 14:14:15 UTC
Created attachment 124309 [details] [review]
Bug 28932: Fix duplicate next_action & prev_action

This commit fixes the problem described in Bug 28932.

Test plan:
- Run the unit tests: prove t/db_dependent/Illrequests.t
=> TEST: Ensure all tests pass
Comment 2 Tomás Cohen Arazi 2022-01-05 13:09:00 UTC
Created attachment 129020 [details] [review]
Bug 28932: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2022-01-05 13:09:04 UTC
Created attachment 129021 [details] [review]
Bug 28932: Fix duplicate next_action & prev_action

This patch fixes the duplicate buttons issue.
To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Illrequests.t
=> FAIL: Tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2022-01-05 13:13:44 UTC
I found this bug when trying an ILL backend recently. My previous attempts all used a completely decoupled graph, so never saw this before.

What I was seeing was that buttons were showed randomly. With this patch most of that is gone, I still see only one button show/hide on refresh.

Will try to dig a bit more. It is probably a problem with my graph, or another use case not covered by this bug.
Comment 5 Tomás Cohen Arazi 2022-01-05 13:14:37 UTC
I splitted the patch in two, so it is easier to spot the problem  and how the patch fixes it.
Comment 6 Martin Renvoize 2022-01-07 20:28:37 UTC
Being bold here and upgrading Tomas's signoff to QA.

There aren't many people capable of testing ILL to the degree Tomas clearly has.. and we're using this in production already here ;)
Comment 7 Martin Renvoize 2022-01-07 20:28:48 UTC
Being bold here and upgrading Tomas's signoff to QA.

There aren't many people capable of testing ILL to the degree Tomas clearly has.. and we're using this in production already here ;)
Comment 8 Tomás Cohen Arazi 2022-01-07 20:42:45 UTC
(In reply to Martin Renvoize from comment #7)
> Being bold here and upgrading Tomas's signoff to QA.
> 
> There aren't many people capable of testing ILL to the degree Tomas clearly
> has.. and we're using this in production already here ;)

I agree. I hesitated because of the issue I was seeing on my backend, but it turned out it was because of a bad override if REQ
Comment 9 Fridolin Somers 2022-01-18 00:56:19 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 10 Kyle M Hall 2022-02-04 18:57:46 UTC
Pushed to 21.11.x for 21.11.03
Comment 11 Andrew Fuerste-Henry 2022-02-10 16:58:48 UTC
Pushed to 21.05.x for 21.05.11
Comment 12 Victor Grousset/tuxayo 2022-02-18 21:01:12 UTC
Backported: Pushed to 20.11.x branch for 20.11.15
Comment 13 wainuiwitikapark 2022-02-21 03:05:44 UTC
Not backported to 19.11.x for 19.11.26.

Please request it if you need it.