Bug 35685 - ILL - OPAC request creation error if submitted empty while ILLModuleDisclaimerByType is in use
Summary: ILL - OPAC request creation error if submitted empty while ILLModuleDisclaime...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Pedro Amorim
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 33716
Blocks:
  Show dependency treegraph
 
Reported: 2024-01-03 11:54 UTC by Pedro Amorim
Modified: 2024-04-05 09:52 UTC (History)
9 users (show)

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


Attachments
Bug 35685: Fix after creation actions (2.24 KB, patch)
2024-01-03 12:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35685: Fix after creation actions (2.29 KB, patch)
2024-01-04 23:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 35685: Fix after creation actions (2.43 KB, patch)
2024-01-26 10:40 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35685: Fix after creation actions (2.53 KB, patch)
2024-04-05 08:25 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-01-03 11:54:33 UTC
To reproduce:

1) Install FreeForm and enable ILLModule, run:
  bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Configure ILLModuleDisclaimerByType, visit:
  /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType
  Click "edit" and paste the example code into the sys pref value
3) Create a new OPAC ILL request, visit:
  /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm
4) Dont put a type or branch, click 'Create', on the next screen click 'Submit'
5) Notice it explodes
6) Apply patch and repeat. It no longer explodes and now shows an error message.
Comment 1 Pedro Amorim 2024-01-03 12:01:51 UTC
Created attachment 160456 [details] [review]
Bug 35685: Fix after creation actions

Test plan:

1) Install FreeForm and enable ILLModule, run:
  bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Configure ILLModuleDisclaimerByType, visit:
  /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType
  Click "edit" and paste the example code into the sys pref value
3) Create a new OPAC ILL request, visit:
  /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm
4) Dont put a type or branch, click 'Create', on the next screen click 'Submit'
5) Notice it explodes
6) Apply patch and repeat. It no longer explodes and now shows an error message.
Comment 2 David Nind 2024-01-04 23:37:01 UTC
Created attachment 160563 [details] [review]
Bug 35685: Fix after creation actions

Test plan:

1) Install FreeForm and enable ILLModule, run:
  bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Configure ILLModuleDisclaimerByType, visit:
  /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType
  Click "edit" and paste the example code into the sys pref value
3) Create a new OPAC ILL request, visit:
  /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm
4) Dont put a type or branch, click 'Create', on the next screen click 'Submit'
5) Notice it explodes
6) Apply patch and repeat. It no longer explodes and now shows an error message.

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2024-01-04 23:38:00 UTC
This does fix the issue, and I have signed off.

However, some comments - not sure how hard these are to do or whether it depends on the ILL plugin (I'm assuming this is the case):

1. If the "Destination library" is not selected, then the message at the top says: "Please Note: Branch is a mandatory field.". In my view the error should match the UI, that is, instead of "Branch" it should say "Destination library is required"

2. The normal UI for forms in the OPAC (from what I can see) is to have "Required" under the field name, and then a message if mandatory fields are not completed. I assume that this depends on how the ILL plugin works/coded though.

Testing notes (using KTD):

1. Make the ILL request using the OPAC!
   . Go to the OPAC
   . Log in as a patron
   . Make the request using the "Interlibrary loan requests" tab/section
Comment 4 Pedro Amorim 2024-01-05 09:37:02 UTC
(In reply to David Nind from comment #3)
> This does fix the issue, and I have signed off.
> 
> However, some comments - not sure how hard these are to do or whether it
> depends on the ILL plugin (I'm assuming this is the case):
> 
> 1. If the "Destination library" is not selected, then the message at the top
> says: "Please Note: Branch is a mandatory field.". In my view the error
> should match the UI, that is, instead of "Branch" it should say "Destination
> library is required"
> 
> 2. The normal UI for forms in the OPAC (from what I can see) is to have
> "Required" under the field name, and then a message if mandatory fields are
> not completed. I assume that this depends on how the ILL plugin works/coded
> though.

Hi David, thanks for testing!
Your assumptions are correct. The form and its behavior is dependent on the specific ILL backend which the request is being created to, in this example it's FreeForm. 

In bug 35570 I'm submitting work to add FreeForm to Koha core, to become a backend available out of the box instead of it being required to install an external one in order to make use of the ILL module.
I mention bug 35570 because once that's gone through, these sort of enhancements can be worked directly into Koha, and have at least the Koha native backend be consistent with everything else.
Comment 5 David Nind 2024-01-05 10:07:50 UTC
(In reply to Pedro Amorim from comment #4)
...
> Hi David, thanks for testing!
> Your assumptions are correct. The form and its behavior is dependent on the
> specific ILL backend which the request is being created to, in this example
> it's FreeForm. 
> 
> In bug 35570 I'm submitting work to add FreeForm to Koha core, to become a
> backend available out of the box instead of it being required to install an
> external one in order to make use of the ILL module.
> I mention bug 35570 because once that's gone through, these sort of
> enhancements can be worked directly into Koha, and have at least the Koha
> native backend be consistent with everything else.

Thanks Pedro!

I'll look at signing that one off over the next few days. I'll keep an eye out for other ILL bugs.
Comment 6 Marcel de Rooy 2024-01-26 09:56:55 UTC
                print $query->redirect('/cgi-bin/koha/opac-illrequests.pl?message=2');

                # After creation actions
                if ( $params->{type_disclaimer_submitted} ) {
                    $type_disclaimer->after_request_created( $params, $request );
                    print $query->redirect('/cgi-bin/koha/opac-illrequests.pl?message=2');


Possibly, two redirects in a row ?
Comment 7 Pedro Amorim 2024-01-26 10:40:50 UTC
Created attachment 161503 [details] [review]
Bug 35685: Fix after creation actions

Test plan:

1) Install FreeForm and enable ILLModule, run:
  bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Configure ILLModuleDisclaimerByType, visit:
  /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType
  Click "edit" and paste the example code into the sys pref value
3) Create a new OPAC ILL request, visit:
  /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm
4) Dont put a type or branch, click 'Create', on the next screen click 'Submit'
5) Notice it explodes
6) Apply patch and repeat. It no longer explodes and now shows an error message.

7) (After FQA): Create a normal request, input type and library, submit the type disclaimer
8) Run:
echo "select * from illrequestattributes;" | koha-mysql kohadev
9) Notice type_disclaimer_date and type_disclaimer_value are correctly saved

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Pedro Amorim 2024-01-26 10:42:34 UTC
(In reply to Marcel de Rooy from comment #6)
>                 print
> $query->redirect('/cgi-bin/koha/opac-illrequests.pl?message=2');
> 
>                 # After creation actions
>                 if ( $params->{type_disclaimer_submitted} ) {
>                     $type_disclaimer->after_request_created( $params,
> $request );
>                     print
> $query->redirect('/cgi-bin/koha/opac-illrequests.pl?message=2');
> 
> 
> Possibly, two redirects in a row ?

Thank you Marcel, certainly not great code! Thanks for pointing it out. I've amended the original patch and updated the test plan to reinforce that there are no regressions in how the data is saved.
Comment 9 Marcel de Rooy 2024-04-05 08:25:18 UTC
Created attachment 164457 [details] [review]
Bug 35685: Fix after creation actions

Test plan:

1) Install FreeForm and enable ILLModule, run:
  bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Configure ILLModuleDisclaimerByType, visit:
  /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType
  Click "edit" and paste the example code into the sys pref value
3) Create a new OPAC ILL request, visit:
  /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm
4) Dont put a type or branch, click 'Create', on the next screen click 'Submit'
5) Notice it explodes
6) Apply patch and repeat. It no longer explodes and now shows an error message.

7) (After FQA): Create a normal request, input type and library, submit the type disclaimer
8) Run:
echo "select * from illrequestattributes;" | koha-mysql kohadev
9) Notice type_disclaimer_date and type_disclaimer_value are correctly saved

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Katrin Fischer 2024-04-05 09:52:22 UTC
Pushed for 24.05!

Well done everyone, thank you!