37263 – Creating default article request fees is not working

Bug 37263 - Creating default article request fees is not working

Summary: Creating default article request fees is not working

Status:	RESOLVED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	Fines and fees (show other bugs)
Version:	unspecified
Hardware:	All All

Importance:	P5 - low major
Assignee:	Pedro Amorim
QA Contact:	Marcel de Rooy

URL:
Keywords:

Depends on:	36192 37290 36672
Blocks:
	Show dependency tree / graph

Reported:	2024-07-05 14:41 UTC by Pedro Amorim
Modified:	2024-12-16 02:07 UTC (History)
CC List:	8 users (show)

See Also:	37255 37531
Change sponsored?:	---
Patch complexity:	Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:	This fixes adding and deleting values for 'Default article request fees', so this works as expected (Administration > Circulation and fine rules > Default article request fees (when ArticleRequests enabled)). (This is related to the CSRF changes added in Koha 24.05 to improve form security.)
Version(s) released in:	24.11.00,24.05.06
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
Show Obsolete (12) View All Add an attachment (proposed patch, testcase, etc.)

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 (emlam) 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 (emlam) 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 (lukeg) 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 (lukeg) 2024-09-06 16:25:00 UTC

Maybe the last patch is not needed in 24.05?

Comment 33 Lucas Gass (lukeg) 2024-11-18 18:37:57 UTC

Backported to 24.05.x for upcoming 24.05.06

Comment 34 Fridolin Somers 2024-12-06 08:52:34 UTC

Not for 23.11.x

Comment 35 David Nind 2024-12-16 02:07:42 UTC

This is a bug fix and there is no change to the user interface - no changes to the manual required.