Bug 37263 - Creating default article request fees is not working
Summary: Creating default article request fees is not working
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major
Assignee: Pedro Amorim
QA Contact: Marcel de Rooy
URL:
Keywords: rel_24_05_candidate, rel_24_11_candidate
Depends on: 36192 36672 37290
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-05 14:41 UTC by Pedro Amorim
Modified: 2024-09-06 16:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 37263: Fix handling of 'All' values on 'default article request fees' (1.43 KB, patch)
2024-07-05 14:42 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37263: Fix URL param retrieval (982 bytes, patch)
2024-07-08 12:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37263: Fix deletion of 'All' patron category in Default article request fees (1.60 KB, patch)
2024-07-08 15:13 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37263: Fix handling of 'All' values on 'default article request fees' (1.43 KB, patch)
2024-07-09 13:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37263: Fix URL param retrieval (982 bytes, patch)
2024-07-09 13:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37263: Fix deletion of 'All' patron category in Default article request fees (1.60 KB, patch)
2024-07-09 13:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37263: Fix handling of 'All' values on 'default article request fees' (1.48 KB, patch)
2024-07-09 17:57 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 37263: Fix URL param retrieval (1.01 KB, patch)
2024-07-09 17:57 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 37263: Fix deletion of 'All' patron category in Default article request fees (1.65 KB, patch)
2024-07-09 17:57 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 37263: Fix handling of 'All' values on 'default article request fees' (1.54 KB, patch)
2024-07-10 05:45 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 37263: Fix URL param retrieval (1.07 KB, patch)
2024-07-10 05:45 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 37263: Fix deletion of 'All' patron category in Default article request fees (1.71 KB, patch)
2024-07-10 05:46 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 37263: Fix handling of 'All' values on 'default article request fees' (1.63 KB, patch)
2024-08-02 06:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37263: Fix URL param retrieval (1.16 KB, patch)
2024-08-02 06:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37263: Fix deletion of 'All' patron category in Default article request fees (1.80 KB, patch)
2024-08-02 06:57 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 Pedro Amorim 2024-07-05 14:41:10 UTC
To reproduce:
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.
Comment 1 Pedro Amorim 2024-07-05 14:42:48 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.
Comment 2 Roman Dolny 2024-07-05 21:37:54 UTC
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.
Comment 3 Emmi Takkinen 2024-07-08 07:31:59 UTC
(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.
Comment 4 Pedro Amorim 2024-07-08 12:45:19 UTC
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
Comment 5 Pedro Amorim 2024-07-08 12:46:43 UTC
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.
Comment 6 Emmi Takkinen 2024-07-08 12:52:44 UTC
(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.
Comment 7 Pedro Amorim 2024-07-08 13:03:49 UTC
(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.
Comment 8 Pedro Amorim 2024-07-08 15:13:03 UTC
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.
Comment 9 Roman Dolny 2024-07-08 19:11:58 UTC
(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.
Comment 10 Janusz Kaczmarek 2024-07-09 11:28:58 UTC
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?
Comment 11 Pedro Amorim 2024-07-09 13:44:01 UTC
(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.
Comment 12 Pedro Amorim 2024-07-09 13:49:10 UTC
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.
Comment 13 Pedro Amorim 2024-07-09 13:49:13 UTC
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
Comment 14 Pedro Amorim 2024-07-09 13:49:15 UTC
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.
Comment 15 Pedro Amorim 2024-07-09 13:52:45 UTC
(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.
Comment 16 Roman Dolny 2024-07-09 17:57:13 UTC
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>
Comment 17 Roman Dolny 2024-07-09 17:57:16 UTC
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>
Comment 18 Roman Dolny 2024-07-09 17:57:19 UTC
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>
Comment 19 Janusz Kaczmarek 2024-07-09 18:22:18 UTC
(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! :)
Comment 20 Emmi Takkinen 2024-07-10 05:45:06 UTC
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>
Comment 21 Emmi Takkinen 2024-07-10 05:45:34 UTC
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>
Comment 22 Emmi Takkinen 2024-07-10 05:46:06 UTC
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>
Comment 23 Emily Lamancusa 2024-07-31 15:26:22 UTC
Increasing the severity on this one and flagging it for backport, since it crashes the page.
Comment 24 Marcel de Rooy 2024-08-02 06:35:58 UTC
Looking here
Comment 25 Marcel de Rooy 2024-08-02 06:57:37 UTC
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>
Comment 26 Marcel de Rooy 2024-08-02 06:57:39 UTC
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>
Comment 27 Marcel de Rooy 2024-08-02 06:57:42 UTC
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>
Comment 28 Marcel de Rooy 2024-08-02 07:00:03 UTC
Switching to major. Critical seems to be overkill ?
Comment 29 Katrin Fischer 2024-08-02 17:01:33 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 30 Emily Lamancusa 2024-08-05 16:56:16 UTC
(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.
Comment 31 Lucas Gass 2024-09-06 16:13:24 UTC
doesnt apply clean to 24.05.x, please rebase if you think this is needed
Comment 32 Lucas Gass 2024-09-06 16:25:00 UTC
Maybe the last patch is not needed in 24.05?