Bug 22905 - Cannot update the status of suggestions if the branchcode filter is set to all
Summary: Cannot update the status of suggestions if the branchcode filter is set to all
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 22368
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-14 15:54 UTC by Jonathan Druart
Modified: 2019-05-30 19:40 UTC (History)
4 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.05.00


Attachments
Bug 22905: A quick, but horrible, fix. (1.35 KB, patch)
2019-05-16 11:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22905: Fix update of suggestion's status if branchcode filter is 'all' (4.27 KB, patch)
2019-05-16 18:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22905: Fix update of suggestion's status if branchcode filter is 'all' (4.37 KB, patch)
2019-05-16 18:47 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22905: (QA follow-up) remove bad warn (805 bytes, patch)
2019-05-16 18:47 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22905: Counterpatch removing just branchcode (1.17 KB, patch)
2019-05-17 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22905: Fix update of suggestion's status if branchcode filter is 'all' (4.46 KB, patch)
2019-05-21 07:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22905: (QA follow-up) remove bad warn (903 bytes, patch)
2019-05-21 07:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-05-14 15:54:46 UTC
Because of the "Library" filter on the left of the "Suggestions management" screen, there is something wrong happening:

1. Create a suggestion from library A, login from library B
2. Go to  Home › Acquisitions › Suggestions management 
=> The suggestion does not appear - OK
3. In the filter on the left, select "all library"
=> The suggestion appears on the pending tab - KO
4. Select the suggestion and mark is as 'Accepted'
=> => The suggestion still appears on the pending tab - Failure

The log says:
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`suggestions`, CONSTRAINT `suggestions_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) [for Statement "UPDATE `suggestions` SET `accepteddate` = ?, `branchcode` = ?, `currency` = ?, `manageddate` = ?, `price` = ?, `reason` = ?, `suggesteddate` = ?, `total` = ? WHERE ( `suggestionid` = ? )" with ParamValues: 0='2019-05-14T15:48:18', 1="", 2="CAD", 3='2019-05-14T15:48:18', 4="0.00", 5="", 6='2019-05-14T00:00:00', 7="0.00", 8=3] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
Comment 1 Jonathan Druart 2019-05-14 16:19:39 UTC
All this code is awful, I have no idea how to fix it simply and without introduce regressions.
Comment 2 Jonathan Druart 2019-05-14 16:26:38 UTC
All of this is very confusing, it's full of bugs. Play with 'Organize by'=Library and you will get a strong headache quickly.
Comment 3 Martin Renvoize 2019-05-16 11:17:31 UTC
Created attachment 89828 [details] [review]
Bug 22905: A quick, but horrible, fix.

'branchcode' appears to be used as a uri param if displayby is set to
branchcode. However, this also matches a field name in the suggestions
table so the reverse mapping of params to suggestions fields catches it
incorrectly.

I believe this is the correct method for removing the incorrect
assignment but the better fix would be to clean up the whole script and
use unique params for filters.
Comment 4 Martin Renvoize 2019-05-16 11:18:10 UTC
Mind well and truly melted.. I'm not entirely confident in this fix.. it'll need a thorough testing for side effects :S
Comment 5 Liz Rea 2019-05-16 14:54:54 UTC
Interesting, this works if you edit the individual suggestion (use the edit button - I did it beccause I was curious) and accept it there, but not if you check it and use the "mark selected as" from the bottom of the suggestions page. The behaviour is exactly the same with or without this patch?

Liz
Comment 6 Jonathan Druart 2019-05-16 18:04:20 UTC
I am confused. If I apply the patches from bug 22907, I cannot recreate the original issue.
Comment 7 Jonathan Druart 2019-05-16 18:06:29 UTC
(In reply to Jonathan Druart from comment #6)
> I am confused. If I apply the patches from bug 22907, I cannot recreate the
> original issue.

Scratch that, it's wrong!
Comment 8 Jonathan Druart 2019-05-16 18:24:07 UTC
Created attachment 89868 [details] [review]
Bug 22905: Fix update of suggestion's status if branchcode filter is 'all'

Because of the "Library" filter on the left of the "Suggestions management" screen, there is something wrong happening:

1. Create a suggestion from library A, login from library B
2. Go to  Home › Acquisitions › Suggestions management
=> The suggestion does not appear - OK
3. In the filter on the left, select "all library"
=> The suggestion appears on the pending tab - KO
4. Select the suggestion and mark is as 'Accepted'
=> The suggestion still appears on the pending tab - Failure

The log says:
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`suggestions`, CONSTRAINT `suggestions_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) [for Statement "UPDATE `suggestions` SET `accepteddate` = ?, `branchcode` = ?, `currency` = ?, `manageddate` = ?, `price` = ?, `reason` = ?, `suggesteddate` = ?, `total` = ? WHERE ( `suggestionid` = ? )" with ParamValues: 0='2019-05-14T15:48:18', 1="", 2="CAD", 3='2019-05-14T15:48:18', 4="0.00", 5="", 6='2019-05-14T00:00:00', 7="0.00", 8=3] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.

Let forget what could have happened earlier in the script and do it the regular
way, from $input. Then call ModSuggestion with only what we need.

Test plan:
Confirm that the steps described before work as expected once this patch is applied
Comment 9 Liz Rea 2019-05-16 18:47:03 UTC
Created attachment 89870 [details] [review]
Bug 22905: Fix update of suggestion's status if branchcode filter is 'all'

Because of the "Library" filter on the left of the "Suggestions management" screen, there is something wrong happening:

1. Create a suggestion from library A, login from library B
2. Go to  Home › Acquisitions › Suggestions management
=> The suggestion does not appear - OK
3. In the filter on the left, select "all library"
=> The suggestion appears on the pending tab - KO
4. Select the suggestion and mark is as 'Accepted'
=> The suggestion still appears on the pending tab - Failure

The log says:
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`suggestions`, CONSTRAINT `suggestions_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) [for Statement "UPDATE `suggestions` SET `accepteddate` = ?, `branchcode` = ?, `currency` = ?, `manageddate` = ?, `price` = ?, `reason` = ?, `suggesteddate` = ?, `total` = ? WHERE ( `suggestionid` = ? )" with ParamValues: 0='2019-05-14T15:48:18', 1="", 2="CAD", 3='2019-05-14T15:48:18', 4="0.00", 5="", 6='2019-05-14T00:00:00', 7="0.00", 8=3] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.

Let forget what could have happened earlier in the script and do it the regular
way, from $input. Then call ModSuggestion with only what we need.

Test plan:
Confirm that the steps described before work as expected once this patch is applied

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=22907
Comment 10 Liz Rea 2019-05-16 18:47:11 UTC
Created attachment 89871 [details] [review]
Bug 22905: (QA follow-up) remove bad warn

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 11 Marcel de Rooy 2019-05-17 09:57:15 UTC
Created attachment 89884 [details] [review]
Bug 22905: Counterpatch removing just branchcode

The suggested patches replace using $suggestion_only, making much more
changes than needed. I would opt for removing the offending branchcode.
In this case __ANY__. Apart from strict SQL mode, it is just wrong.

This patch touches just one line.

Same test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2019-05-17 09:58:20 UTC
(In reply to Jonathan Druart from comment #1)
> All this code is awful, I have no idea how to fix it simply and without
> introduce regressions.

Yes, this definitely is horrible. I suggest a simpler fix.
Comment 13 Jonathan Druart 2019-05-17 13:50:31 UTC
I think my patch is much more regression proof. It makes the code does what it must do. There are much more variables in the filters that have the same name: budgetid, title, etc.

I wanted to provide a patch in which we can trust. I do not trust yours, because I did not spend 2 hours to make sure there are no more bugs (interference with variable's name).
Comment 14 Marcel de Rooy 2019-05-18 17:08:58 UTC
(In reply to Jonathan Druart from comment #13)
> I think my patch is much more regression proof. 
Sure. Any dev should be convinced of that ;)

> I do not trust yours.
Good to hear.
Comment 15 Liz Rea 2019-05-20 14:07:16 UTC
Well, I'm going to say I've signed off Jonathan's, it fixes the problem and I prefer the approach there.

I'd like to get this fixed and we have a patch, let's have another QA person look at it and decide.

Liz
Comment 16 Marcel de Rooy 2019-05-20 20:42:44 UTC
(In reply to Liz Rea from comment #15)
> Well, I'm going to say I've signed off Jonathan's, it fixes the problem and
> I prefer the approach there.
> 
> I'd like to get this fixed and we have a patch, let's have another QA person
> look at it and decide.
> 
> Liz

Dont worry
Comment 17 Marcel de Rooy 2019-05-21 07:07:14 UTC
Created attachment 89930 [details] [review]
Bug 22905: Fix update of suggestion's status if branchcode filter is 'all'

Because of the "Library" filter on the left of the "Suggestions management" screen, there is something wrong happening:

1. Create a suggestion from library A, login from library B
2. Go to  Home › Acquisitions › Suggestions management
=> The suggestion does not appear - OK
3. In the filter on the left, select "all library"
=> The suggestion appears on the pending tab - KO
4. Select the suggestion and mark is as 'Accepted'
=> The suggestion still appears on the pending tab - Failure

The log says:
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`suggestions`, CONSTRAINT `suggestions_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) [for Statement "UPDATE `suggestions` SET `accepteddate` = ?, `branchcode` = ?, `currency` = ?, `manageddate` = ?, `price` = ?, `reason` = ?, `suggesteddate` = ?, `total` = ? WHERE ( `suggestionid` = ? )" with ParamValues: 0='2019-05-14T15:48:18', 1="", 2="CAD", 3='2019-05-14T15:48:18', 4="0.00", 5="", 6='2019-05-14T00:00:00', 7="0.00", 8=3] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.

Let forget what could have happened earlier in the script and do it the regular
way, from $input. Then call ModSuggestion with only what we need.

Test plan:
Confirm that the steps described before work as expected once this patch is applied

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=22907

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2019-05-21 07:07:19 UTC
Created attachment 89931 [details] [review]
Bug 22905: (QA follow-up) remove bad warn

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2019-05-21 07:08:02 UTC
KohaCon19 PQA
Comment 20 Nick Clemens 2019-05-23 15:02:48 UTC
Awesome work all!

Pushed to master for 19.05
Comment 21 Martin Renvoize 2019-05-30 19:40:35 UTC
Bug with enhancement not in 18.11.x series.