|
Description
Pedro Amorim
2024-07-05 14:41:10 UTC
Created attachment 168553 [details] [review] Bug 37263: Fix handling of 'All' values on 'default article request fees' To test: 1. Enable ArticleRequests sys pref 2. Visit <staff_url>/cgi-bin/koha/admin/smart-rules.pl 3. Under 'Default article request fees' on 'Patron category' select 'All' and input a numeric Free 4. Click 'Add'. Notice it blows up. 5. Apply patch. Repeat. I found a few problems during testing in KTD, but not sure if they should be new bugs or fixed in this one. Before applying a patch problem is also with adding fee for any single patron category - Koha blows up. But after applying a patch there is another problem - if there is rule for 'All' one can not add rule for specific patron category. If there is no rule for 'All' new rule is added not for selected category but for 'All'. Trying to add new rule (for specific category) with fee value different from fee for 'All' doesn't work, but updates fee value for 'All'. Delete button does not work. (In reply to Roman Dolny from comment #2) > I found a few problems during testing in KTD, but not sure if they should be > new bugs or fixed in this one. > > Before applying a patch problem is also with adding fee for any single > patron category - Koha blows up. > > But after applying a patch there is another problem - if there is rule for > 'All' one can not add rule for specific patron category. If there is no rule > for 'All' new rule is added not for selected category but for 'All'. > > Trying to add new rule (for specific category) with fee value different from > fee for 'All' doesn't work, but updates fee value for 'All'. > > Delete button does not work. Can confirm, "Default article request fees" is always saved to database with categorycode as NULL no matter which patron category is used. That's why when you try to save new rule with different patron category, it doesn't appear to the rule list. So now in master any saved rule leads to Koha blowing up. Can also confirm deleting rule with patron category as "All" doesn't work. I changed categorytype in database manually as existing patron category type and after that I was able to delete the rule. My guess is that since "All" value is sent to deleted script as empty value ("") it can't be found from database and thus deleting it fails. Created attachment 168591 [details] [review] Bug 37263: Fix URL param retrieval This was introduced by bug 34478. To test: Follow the test plan as before, but test for several different patron categories Hi both, thank you for testing. I believe the 2nd patch should fix the follow-up issues, if you can please test again. Thank you. (In reply to Pedro Amorim from comment #5) > Hi both, thank you for testing. I believe the 2nd patch should fix the > follow-up issues, if you can please test again. Thank you. Tested and works as described. However deleting rule is still not working at least for me. (In reply to Emmi Takkinen from comment #6) > (In reply to Pedro Amorim from comment #5) > > Hi both, thank you for testing. I believe the 2nd patch should fix the > > follow-up issues, if you can please test again. Thank you. > > Tested and works as described. However deleting rule is still not working at > least for me. Right, it seems to work if a category is defined, but not for 'All'. I'll revisit this when I next find the chance. Thanks. Created attachment 168593 [details] [review] Bug 37263: Fix deletion of 'All' patron category in Default article request fees Follow same test plan as before, but attempt to delete a 'All' entry in 'Default article request fees'. Notice it blows up before this patch. It works as expected after the patch. (In reply to Pedro Amorim from comment #8) > Created attachment 168593 [details] [review] [review] > Bug 37263: Fix deletion of 'All' patron category in Default article request > fees > > Follow same test plan as before, but attempt to delete a 'All' entry in > 'Default article request fees'. > Notice it blows up before this patch. It works as expected after the patch. Thank you for patches, it works as described - doesn't blow up after applying a patch, adding/deleting rule for category or for 'All' works. Notice one more thing - "Delete" button works when there is "Standard rules for all libraries" selected, but doesn't work when specific library is selected. There is still a problem in the smart-rules.tt file, causing the settings for all libraries to be removed instead of the selected one. (You are trying to remove settings for e.g. Midway, but in fact they remain untouched and the defaults are removed). What should be corrected is--in 8 spots in smart-rules.tt: - ... data-branch"[% current_branch | html %]"> ... + ... data-branch="[% current_branch | html %]"> ... I'm not filling a new bug. Pedro, could you please include this in yours? (In reply to Janusz Kaczmarek from comment #10) > There is still a problem in the smart-rules.tt file, causing the settings > for all libraries to be removed instead of the selected one. (You are > trying to remove settings for e.g. Midway, but in fact they remain untouched > and the defaults are removed). > > What should be corrected is--in 8 spots in smart-rules.tt: > > - ... data-branch"[% current_branch | html %]"> ... > + ... data-branch="[% current_branch | html %]"> ... > > I'm not filling a new bug. Pedro, could you please include this in yours? I've opened bug 37290 for this as the scope is clearly wider there. (This current bug 37263 is about article requests specifically) Janusz I've submitted the patch in your name and signed-off. I'll rebase these patches here on top of bug 37290 to avoid unecessary conflicts. Created attachment 168650 [details] [review] Bug 37263: Fix handling of 'All' values on 'default article request fees' To test: 1. Enable ArticleRequests sys pref 2. Visit <staff_url>/cgi-bin/koha/admin/smart-rules.pl 3. Under 'Default article request fees' on 'Patron category' select 'All' and input a numeric Free 4. Click 'Add'. Notice it blows up. 5. Apply patch. Repeat. Created attachment 168651 [details] [review] Bug 37263: Fix URL param retrieval This was introduced by bug 34478. To test: Follow the test plan as before, but test for several different patron categories Created attachment 168652 [details] [review] Bug 37263: Fix deletion of 'All' patron category in Default article request fees Follow same test plan as before, but attempt to delete a 'All' entry in 'Default article request fees'. Notice it blows up before this patch. It works as expected after the patch. (In reply to Roman Dolny from comment #9) > Notice one more thing - "Delete" button works when there is "Standard rules > for all libraries" selected, but doesn't work when specific library is > selected. This should be fixed by bug 37290, please check there. Created attachment 168675 [details] [review] Bug 37263: Fix handling of 'All' values on 'default article request fees' To test: 1. Enable ArticleRequests sys pref 2. Visit <staff_url>/cgi-bin/koha/admin/smart-rules.pl 3. Under 'Default article request fees' on 'Patron category' select 'All' and input a numeric Free 4. Click 'Add'. Notice it blows up. 5. Apply patch. Repeat. Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Created attachment 168676 [details] [review] Bug 37263: Fix URL param retrieval This was introduced by bug 34478. To test: Follow the test plan as before, but test for several different patron categories Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Created attachment 168677 [details] [review] Bug 37263: Fix deletion of 'All' patron category in Default article request fees Follow same test plan as before, but attempt to delete a 'All' entry in 'Default article request fees'. Notice it blows up before this patch. It works as expected after the patch. Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> (In reply to Pedro Amorim from comment #11) > I've opened bug 37290 for this as the scope is clearly wider there. (This > current bug 37263 is about article requests specifically) > Janusz I've submitted the patch in your name and signed-off. Indeed, my discovery turned out to be wider than just article request fee deletion problem. Thank you, Pedro, for your extra work! :) Created attachment 168690 [details] [review] Bug 37263: Fix handling of 'All' values on 'default article request fees' To test: 1. Enable ArticleRequests sys pref 2. Visit <staff_url>/cgi-bin/koha/admin/smart-rules.pl 3. Under 'Default article request fees' on 'Patron category' select 'All' and input a numeric Free 4. Click 'Add'. Notice it blows up. 5. Apply patch. Repeat. Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Created attachment 168691 [details] [review] Bug 37263: Fix URL param retrieval This was introduced by bug 34478. To test: Follow the test plan as before, but test for several different patron categories Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Created attachment 168692 [details] [review] Bug 37263: Fix deletion of 'All' patron category in Default article request fees Follow same test plan as before, but attempt to delete a 'All' entry in 'Default article request fees'. Notice it blows up before this patch. It works as expected after the patch. Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Increasing the severity on this one and flagging it for backport, since it crashes the page. Looking here Created attachment 169983 [details] [review] Bug 37263: Fix handling of 'All' values on 'default article request fees' To test: 1. Enable ArticleRequests sys pref 2. Visit <staff_url>/cgi-bin/koha/admin/smart-rules.pl 3. Under 'Default article request fees' on 'Patron category' select 'All' and input a numeric Free 4. Click 'Add'. Notice it blows up. 5. Apply patch. Repeat. Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 169984 [details] [review] Bug 37263: Fix URL param retrieval This was introduced by bug 34478. To test: Follow the test plan as before, but test for several different patron categories Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 169985 [details] [review] Bug 37263: Fix deletion of 'All' patron category in Default article request fees Follow same test plan as before, but attempt to delete a 'All' entry in 'Default article request fees'. Notice it blows up before this patch. It works as expected after the patch. Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Switching to major. Critical seems to be overkill ? Pushed for 24.11! Well done everyone, thank you! (In reply to Marcel de Rooy from comment #28) > Switching to major. Critical seems to be overkill ? Sorry if I caused undue alarm! I went for critical based on the phrasing "Koha or component crashes" on the wiki, but of course there's always nuance to these things. doesnt apply clean to 24.05.x, please rebase if you think this is needed Maybe the last patch is not needed in 24.05? Backported to 24.05.x for upcoming 24.05.06 |