|
Description
Jonathan Druart
2019-05-14 15:54:46 UTC
All this code is awful, I have no idea how to fix it simply and without introduce regressions. All of this is very confusing, it's full of bugs. Play with 'Organize by'=Library and you will get a strong headache quickly. 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. Mind well and truly melted.. I'm not entirely confident in this fix.. it'll need a thorough testing for side effects :S 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 I am confused. If I apply the patches from bug 22907, I cannot recreate the original issue. (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! 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 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 Created attachment 89871 [details] [review] Bug 22905: (QA follow-up) remove bad warn Signed-off-by: Liz Rea <wizzyrea@gmail.com> 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> (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. 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). (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. 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 (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 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> 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> KohaCon19 PQA Awesome work all! Pushed to master for 19.05 Bug with enhancement not in 18.11.x series. |