I love the idea.

(In reply to Pedro Amorim from comment #0)
> Thoughts?

Sounds like a great idea!

Created attachment 159942 [details] [review]
Bug 35570: Put 'FreeForm' backend into core as 'Standard'

FreeForm copied as is from HEAD 369dffb159f1e70162b685b473dcf26c76f7e7e7
At https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/

Only edits made were 'freeform' -> 'standard' in the markup of the .inc files
and some occurrences of FreeForm in Standard.pm (previous Base.pm) file.

I have named the new core backend 'Standard' but I don't feel strongly about this, opinions/suggestions welcomed.

Created attachment 160654 [details] [review]
Bug 35570: Put 'FreeForm' backend into core as 'Standard'

FreeForm copied as is from HEAD 369dffb159f1e70162b685b473dcf26c76f7e7e7
At https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/

Only edits made were 'freeform' -> 'standard' in the markup of the .inc files
and some occurrences of FreeForm in Standard.pm (previous Base.pm) file.

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

Created attachment 160655 [details] [review]
Bug 35570: Consider 'Standard' core backend

Added File::Basename qw( dirname )
Sorted Koha:: dependencies alphabetically
DRYed load_backend a bit and included 'Standard' backend check

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

Created attachment 160656 [details] [review]
Bug 35570: Atomicupdate

Move stuff out of 'FreeForm' into 'Standard'

This atomicupdate will printout the changes it made, if any.
It will also print out the report IDs of reports that may contain occurrences of 'FreeForm'.
This is to give the sys admin the chance to fix those reports before users.

To test, empty k-t-d:
1) Run updatedatabase, notice nothing happens
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Create a new 'FreeForm' ILL batch (requires a metadata enrichment plugin like https://github.com/PTFS-Europe/koha-plugin-api-pubmed)
4) Add '123' to the pubmedid list of identifiers and finish creating the batch
5) Upon creating the batch, you will now have 1 request, 1 batch and a few illrequestattributes in the 'FreeForm' backend
6) Run the updatedatabase again, notice the print outs.
7) Create a saved sql report like:
'Select * from illrequests where backend="FreeForm";'
8) Run the updatedatabase again, notice you get a warning for the above report

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

Created attachment 160657 [details] [review]
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed

Test plan:
1) This patch should not be applied in order to be able to go through the previous patch's test plan
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Please note that you need to be using FreeForm's reorganize_ILL branch at:
https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/
4) Visit ILL page:
/cgi-bin/koha/ill/ill-requests.pl
5) Notice there's a configuration error preventing the use of the ILL module.
6) Click the 'about page' link and verify there is a warning about
   FreeForm
7) Remove FreeForm from /kohadevbox/koha/Koha/Illbackends and refresh the page
8) Repeat 4), notice the requests are listed as expected and you can now
   use the ILL module again.

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

Created attachment 160658 [details] [review]
Bug 35570: (QA follow-up):

Make QA script green, fix:
- Variables declared in condition
- Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840)
- missing_filter
- forbidden pattern: Do not use line breaks inside template tags (bug 18675)
- Attribute type should not be used for script tags (bug 20053)
- File must have the exec flag
- forbidden pattern: trailing space char
- valid_template: /shared-includes/custom_fields.inc: not found

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

Created attachment 160659 [details] [review]
Bug 35570: QA follow-up:

Update updatedatabase report colors to base on bug 35681

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

Created attachment 160660 [details] [review]
Bug 35570: (QA follow-up):

Add FreeForm fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/9c9da5ff2815fb6ce0af0181341dea5ce8c3bb2e into this work.
This is to keep this work up to date with latest FreeForm's fixes.
See bug 35685 for context and test plan.

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

(In reply to Pedro Amorim from comment #14)
...
> 
> I believe the error you're getting is because you may have skipped this step:
> 3) Please note that you need to be using FreeForm's reorganize_ILL branch at:
> https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/
> 
> After you git checkout reorganize_ILL in FreeForm you may have to koha-plack
> --restart. Would you please try again? Reach out to me on IRC, I'm always
> available. Thanks!

Thanks Pedro. That was the issue!

Testing notes (using KTD):

1. Apply the first three patches:
   - Bug 35570: Put 'FreeForm' backend into core as 'Standard'
   - Bug 35570: Consider 'Standard' core backend
   - Bug 35570: Atomicupdate
   
   Notes:
   - Apply all the dependent bugs
   - For 35570 (this bug) use i (for interactive) and add a # hash in front of the patches not to apply at this time

2. Run the database update: updatedatabase
   => Output:

Upgrade to 23.12.00.003  [18:41:32]: Bug 35681 - Test DB Rev output
	Something in red is a warning
	Somthing in yellow is a call to action
	Something in blue is for information only
	You could use 'WHITE' too?
	Or leave color off and stick to default
DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35570.pl  [18:41:32]: Bug 35570 - Update 'FreeForm' ILL backend to 'Standard'
	Bug 35570: Finished database update.
DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35681.pl  [18:41:32]: Bug 35681 - Test Atomic update output
	Something in red is a warning
	Somthing in yellow is a call to action
	Something in blue is for information only
	You could use 'WHITE' too?
	Or leave color off and stick to default

3. Install the FreeForm backend. Run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)

4. Change the FreeForm branch to reorganize_ILL. From within the Koha/Illbackends/FreeForm directory: git checkout reorganize_ILL

5. Run flush_memcached and restart_all (not sure whether these are required here, but I ran them anyway).

6. Install the Pubmed plugin: 
   - Download from https://github.com/PTFS-Europe/koha-plugin-api-pubmed
   - Upload the plugin: Koha administration > Plugins > Manage plugins
   - Run restart_all

7. Create a new 'FreeForm' ILL batch:
   - + New ILL requests batch
   - Select the 'FreeForm' option
   - Batch details:
     . Batch name: BZ35570
     . Card number...: 42 (koha)
     . Library: Centerville
    - Select 'Continue'
    - Add '123' to the PubMed ID list of identifiers
    - Click 'Process identifiers'
    - Once processed, click 'Add items to batch'
    - Click 'Finish and view batch'
    => You now have 1 request, 1 batch and a few ILL request attributes in the 'FreeForm' backend

8. Run updatedatabase again, notice that there are now extra messages from the previous database update - "Updated ILL batches from 'FreeForm' to 'Standard'...":

DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35570.pl  [19:26:57]: Bug 35570 - Update 'FreeForm' ILL backend to 'Standard'
	Bug 35570: Updated ILL batches from 'FreeForm' to 'Standard
	Bug 35570: Updated ILL request attributes from 'FreeForm' to 'Standard
	Bug 35570: Updated ILL requests from 'FreeForm' to 'Standard
	Bug 35570: Finished database update.
DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35681.pl  [19:26:57]: Bug 35681 - Test Atomic update output
	Something in red is a warning
	Somthing in yellow is a call to action
	Something in blue is for information only
	You could use 'WHITE' too?
	Or leave color off and stick to default

9. Create a saved SQL report with this SQL (note that if you run the report you get no results): Select * from illrequests where backend="FreeForm"

10. Run the updatedatabase again. Note that there is a warning about the report you created "Bug 35570: **ACTION REQUIRED**: Saved SQL reports containing occurrences of 'FreeForm' were found.....":

DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35570.pl  [19:30:47]: Bug 35570 - Update 'FreeForm' ILL backend to 'Standard'
	Bug 35570: **ACTION REQUIRED**: Saved SQL reports containing occurrences of 'FreeForm' were found.
	The following reports MUST be updated accordingly ('FreeForm' -> 'Standard'):
		Report ID: 1 | Edit link: http://kohadev-intra.koha-testing:8081/cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Edit%20SQL
DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35681.pl  [19:30:47]: Bug 35681 - Test Atomic update output
	Something in red is a warning
	Somthing in yellow is a call to action
	Something in blue is for information only
	You could use 'WHITE' too?
	Or leave color off and stick to default

11. Apply the remainder of the patches for this bug:
    - Say no for all the dependencies, as they are already installed
    - For 35570 (this bug) use i (for interactive) and add a # hash in front of the first three patches
    - There is no need to run the databaseupdate again
    - Run flush_memcached and restart_all (not sure if this is required, but is what I normally do after applying a patch)

12. Go to the ILL page: Administration > ILL requests

13. Notice that there is a configuration error preventing the use of the ILL module. The message displayed is "ILL module configuration problem. Take a look at the about page".

14. Click the 'about page' link and verify there is a warning about the FreeForm backend:

     The ILL module is enabled, but the deprecated 'FreeForm' backend is installed.
     'FreeForm' has since become part of core Koha as 'Standard'.
     All 'FreeForm' ILL content should've been migrated to 'Standard' upon upgrading Koha to 24.05.
     To avoid confusion or redundancy, please uninstall the 'FreeForm' backend before using the ILL module.

15. Remove the FreeForm backend from /kohadevbox/koha/Koha/Illbackends (sudo rm -R /kohadevbox/koha/Koha/Illbackends/FreeForm) and refresh the page.

16. Repeat step 12. Note that:
    - The requests are listed as expected, and you can now use the ILL module again.
    - For '+ New ILL requests batch', 'Standard' is now the only option shown.

Additional testing
==================

I created a new ILL request and tested the various options available on the ILL requests page (this wasn't part of the test plan):

- Confirm request: page not found (404) error when confirming request (URL was /cgi-bin/koha/ill/%2Fcgi-bin%2Fkoha%2Fill%2Fill-requests.pl%3Fmethod%3Dconfirm%26stage%3Dconfirm%26illrequest_id%3D2) - presume something else is required to configure this?

- All the other areas seemed to work as expected:
  . Edit request
  . Place request with partner libraries (add a couple of patrons to the Inter-Library Loan patron category, make sure they have an email address)
  . Delete request
  . Edit item metadata
  . Send notice to patron (I added an email to the patron used to create the request, updated patron message preferences, and added an email address to the various email system preferences)
  . Display supplier metadata (found this confusing - it shows the request metadata, but this is the existing terminology)
  . ILL request log (changes made to the request show after enabling the IllLog system preference)

Created attachment 160686 [details] [review]
Bug 35570: (QA follow-up): Fix action button links. Use url filter instead of uri.

Before applying this patch:
1) Click 'Manage Request' for a request, then on the top toolbar click 'Confirm request'.
2) Click the 'Confirm request' button on the bottom of the text. Notice it fails with a 404. Use the browser to go back.
3) Click the 'Cancel' button on the bottom of the text. Notice it fails with a 404. Use the browser to go back.

4) Set all the requests to status to 'REQ' (relevant for next test), ktd kshell, run:
   echo 'update illrequests set status = 'REQ';' | koha-mysql kohadev
5) Click 'Manage Request' for a request, then on the top toolbar click 'Revert request'.
6) Click the 'Revert request' button on the bottom of the text. Notice it fails with a 404. Use the browser to go back.
7) Click the 'Cancel' button on the bottom of the text. Notice it fails with a 404. Use the browser to go back.

8) Apply patch. Repeat test plan. Notice all the action buttons now work.

(In reply to David Nind from comment #24)
> Additional testing
> ==================
> 
> I created a new ILL request and tested the various options available on the
> ILL requests page (this wasn't part of the test plan):
> 
> - Confirm request: page not found (404) error when confirming request (URL
> was
> /cgi-bin/koha/ill/%2Fcgi-bin%2Fkoha%2Fill%2Fill-requests.
> pl%3Fmethod%3Dconfirm%26stage%3Dconfirm%26illrequest_id%3D2) - presume
> something else is required to configure this?

Hi David, nice catch. This was an oversight ('uri' instead of 'url') and an actual bug. 
I've submitted a patch. Thank you so much for your extensive testing!

(In reply to Pedro Amorim from comment #26)
> 
> Hi David, nice catch. This was an oversight ('uri' instead of 'url') and an
> actual bug. 
> I've submitted a patch. Thank you so much for your extensive testing!

Thanks Pedro! Can confirm that this now works as expected.

Could we do a cleanup here first?
Seeing various (QA follow-up) without any further text. Does not look good.

(In reply to Marcel de Rooy from comment #28)
> Could we do a cleanup here first?
> Seeing various (QA follow-up) without any further text. Does not look good.

Hi Marcel, thank you. I agree, I'll rewrite the commit messages to have proper titles and resubmit. There's a minor rebase needed as well. Having said that, here's an explanation of the 3 qa follow-up commits without further text on the title:

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160658&action=diff
First qa follow-up is required because it records all the QA work done on FreeForm after it has become Standard in Koha core.

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160659&action=diff
Second qa follow-up was an update on the work done in bug 35681 (I need to revisit this patch again)

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160660&action=diff
Third qa follow-upn was a bugfix submitted to the FreeForm repo, but I'm also adding here to keep both in sync while this is not pushed to core.

(In reply to Marcel de Rooy from comment #28)
> Could we do a cleanup here first?
> Seeing various (QA follow-up) without any further text. Does not look good.

Hi Marcel, I've improved the commit messages titles and squashed 2 QA follow-ups to their respective original commits. Kept 2 of the QA follow-up commits as they are important history.

Rebased after recent changes to bug 19605

Rebase after a follow-up submitted to bug 19605 was required.

Created attachment 165286 [details] [review]
Bug 35570: Put 'FreeForm' backend into core as 'Standard'

FreeForm copied as is from HEAD 369dffb159f1e70162b685b473dcf26c76f7e7e7
At https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/

Only edits made were 'freeform' -> 'standard' in the markup of the .inc files
and some occurrences of FreeForm in Standard.pm (previous Base.pm) file.

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

Created attachment 165287 [details] [review]
Bug 35570: Consider 'Standard' core backend

Added File::Basename qw( dirname )
Sorted Koha:: dependencies alphabetically
DRYed load_backend a bit and included 'Standard' backend check

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

Created attachment 165288 [details] [review]
Bug 35570: Atomicupdate

Move stuff out of 'FreeForm' into 'Standard'

This atomicupdate will printout the changes it made, if any.
It will also print out the report IDs of reports that may contain occurrences of 'FreeForm'.
This is to give the sys admin the chance to fix those reports before users.

To test, empty k-t-d:
1) Run updatedatabase, notice nothing happens
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Create a new 'FreeForm' ILL batch (requires a metadata enrichment plugin like https://github.com/PTFS-Europe/koha-plugin-api-pubmed)
4) Add '123' to the pubmedid list of identifiers and finish creating the batch
5) Upon creating the batch, you will now have 1 request, 1 batch and a few illrequestattributes in the 'FreeForm' backend
6) Run the updatedatabase again, notice the print outs.
7) Create a saved sql report like:
'Select * from illrequests where backend="FreeForm";'
8) Run the updatedatabase again, notice you get a warning for the above report

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

Created attachment 165289 [details] [review]
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed

Test plan:
1) This patch should not be applied in order to be able to go through the previous patch's test plan
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Please note that you need to be using FreeForm's reorganize_ILL branch at:
https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/
4) Visit ILL page:
/cgi-bin/koha/ill/ill-requests.pl
5) Notice there's a configuration error preventing the use of the ILL module.
6) Click the 'about page' link and verify there is a warning about
   FreeForm
7) Remove FreeForm from /kohadevbox/koha/Koha/Illbackends and refresh the page
8) Repeat 4), notice the requests are listed as expected and you can now
   use the ILL module again.

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

Created attachment 165290 [details] [review]
Bug 35570: (QA follow-up): Follow QA script on Standard backend

Make QA script green, fix:
- Variables declared in condition
- Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840)
- missing_filter
- forbidden pattern: Do not use line breaks inside template tags (bug 18675)
- Attribute type should not be used for script tags (bug 20053)
- File must have the exec flag
- forbidden pattern: trailing space char
- valid_template: /shared-includes/custom_fields.inc: not found

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

Created attachment 165291 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing

Add FreeForm fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/9c9da5ff2815fb6ce0af0181341dea5ce8c3bb2e into this work.
This is to keep this work up to date with latest FreeForm's fixes.
See bug 35685 for context and test plan.

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

Created attachment 165292 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility

Add FreeForm CSRF fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/c34be16ba34c840193ed5e043e6736fa0b1cb646 into this work.
This is to keep this work up to date with latest FreeForm's fixes.

Created attachment 165293 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF

Add FreeForm CSRF fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/6a37ce0daba8aab13130dacd055c9ca5876b7df4 into this work.
This is to keep this work up to date with latest FreeForm's fixes.

The test plan to demonstrate the issue this patch fixes:
Test plan, k-t-d:

1) Install FreeForm, enable ILL module, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) You'll have to switch the FreeForm branch to the one compatible with this work, i.e.:
cd /kohadevbox/koha/Koha/Illbackends/FreeForm
git checkout b_36243
3) Restart plack:
koha-plack --restart kohadev
4) Go to OPAC ILL requests, login and visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl
5) Click "Create new request"
6) Change the 'type'. Notice you get a 403 error. Logs say:
Programming error - op 'add_form' must start with 'cud-' for POST
7) Apply patch. Restart plack. Repeat 6)
8) Notice the change type works as expected. Click "Create".
9) Notice the request is created as expected.

Rebased required after bug 35151 and bug 32693pushed to main.

The patchset includes the following string:

All 'FreeForm' ILL content should've been migrated to 'Standard' upon upgrading Koha to 24.05.

This needs to be revisited/update if not pushed in time for 24.05.

Created attachment 167051 [details] [review]
Bug 35570: (QA follow-up): Rebase on final state of bug 35681

Hi Pedro, just wanted to QA Bug 35725 but this one doesn't apply anymore unfortunately.

Huh, this one was already pushed to main? Sorry for the noise then.

(In reply to Paul Derscheid from comment #78)
> Huh, this one was already pushed to main? Sorry for the noise then.

No, this has not yet been pushed to main. I'm putting this back to 'Patch doesn't apply' to look back at tomorrow. Thanks.

Created attachment 171312 [details] [review]
Bug 35570: Put 'FreeForm' backend into core as 'Standard'

FreeForm copied as is from HEAD 369dffb159f1e70162b685b473dcf26c76f7e7e7
At https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/

Only edits made were 'freeform' -> 'standard' in the markup of the .inc files
and some occurrences of FreeForm in Standard.pm (previous Base.pm) file.

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

Created attachment 171313 [details] [review]
Bug 35570: Consider 'Standard' core backend

Added File::Basename qw( dirname )
Sorted Koha:: dependencies alphabetically
DRYed load_backend a bit and included 'Standard' backend check

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

Created attachment 171314 [details] [review]
Bug 35570: Atomicupdate

Move stuff out of 'FreeForm' into 'Standard'

This atomicupdate will printout the changes it made, if any.
It will also print out the report IDs of reports that may contain occurrences of 'FreeForm'.
This is to give the sys admin the chance to fix those reports before users.

To test, empty k-t-d:
1) Run updatedatabase, notice nothing happens
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Create a new 'FreeForm' ILL batch (requires a metadata enrichment plugin like https://github.com/PTFS-Europe/koha-plugin-api-pubmed)
4) Add '123' to the pubmedid list of identifiers and finish creating the batch
5) Upon creating the batch, you will now have 1 request, 1 batch and a few illrequestattributes in the 'FreeForm' backend
6) Run the updatedatabase again, notice the print outs.
7) Create a saved sql report like:
'Select * from illrequests where backend="FreeForm";'
8) Run the updatedatabase again, notice you get a warning for the above report

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

Created attachment 171315 [details] [review]
Bug 35570: Put 'FreeForm' backend into core as 'Standard'

FreeForm copied as is from HEAD 369dffb159f1e70162b685b473dcf26c76f7e7e7
At https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/

Only edits made were 'freeform' -> 'standard' in the markup of the .inc files
and some occurrences of FreeForm in Standard.pm (previous Base.pm) file.

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

Created attachment 171316 [details] [review]
Bug 35570: Consider 'Standard' core backend

Added File::Basename qw( dirname )
Sorted Koha:: dependencies alphabetically
DRYed load_backend a bit and included 'Standard' backend check

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

Created attachment 171317 [details] [review]
Bug 35570: Atomicupdate

Move stuff out of 'FreeForm' into 'Standard'

This atomicupdate will printout the changes it made, if any.
It will also print out the report IDs of reports that may contain occurrences of 'FreeForm'.
This is to give the sys admin the chance to fix those reports before users.

To test, empty k-t-d:
1) Run updatedatabase, notice nothing happens
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Create a new 'FreeForm' ILL batch (requires a metadata enrichment plugin like https://github.com/PTFS-Europe/koha-plugin-api-pubmed)
4) Add '123' to the pubmedid list of identifiers and finish creating the batch
5) Upon creating the batch, you will now have 1 request, 1 batch and a few illrequestattributes in the 'FreeForm' backend
6) Run the updatedatabase again, notice the print outs.
7) Create a saved sql report like:
'Select * from illrequests where backend="FreeForm";'
8) Run the updatedatabase again, notice you get a warning for the above report

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

Created attachment 171318 [details] [review]
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed

Test plan:
1) This patch should not be applied in order to be able to go through the previous patch's test plan
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Please note that you need to be using FreeForm's reorganize_ILL branch at:
https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/
4) Visit ILL page:
/cgi-bin/koha/ill/ill-requests.pl
5) Notice there's a configuration error preventing the use of the ILL module.
6) Click the 'about page' link and verify there is a warning about
   FreeForm
7) Remove FreeForm from /kohadevbox/koha/Koha/Illbackends and refresh the page
8) Repeat 4), notice the requests are listed as expected and you can now
   use the ILL module again.

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

Created attachment 171319 [details] [review]
Bug 35570: (QA follow-up): Follow QA script on Standard backend

Make QA script green, fix:
- Variables declared in condition
- Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840)
- missing_filter
- forbidden pattern: Do not use line breaks inside template tags (bug 18675)
- Attribute type should not be used for script tags (bug 20053)
- File must have the exec flag
- forbidden pattern: trailing space char
- valid_template: /shared-includes/custom_fields.inc: not found

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

Created attachment 171320 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing

Add FreeForm fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/9c9da5ff2815fb6ce0af0181341dea5ce8c3bb2e into this work.
This is to keep this work up to date with latest FreeForm's fixes.
See bug 35685 for context and test plan.

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

Created attachment 171321 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility

Add FreeForm CSRF fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/c34be16ba34c840193ed5e043e6736fa0b1cb646 into this work.
This is to keep this work up to date with latest FreeForm's fixes.

Created attachment 171322 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF

Add FreeForm CSRF fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/6a37ce0daba8aab13130dacd055c9ca5876b7df4 into this work.
This is to keep this work up to date with latest FreeForm's fixes.

The test plan to demonstrate the issue this patch fixes:
Test plan, k-t-d:

1) Install FreeForm, enable ILL module, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) You'll have to switch the FreeForm branch to the one compatible with this work, i.e.:
cd /kohadevbox/koha/Koha/Illbackends/FreeForm
git checkout b_36243
3) Restart plack:
koha-plack --restart kohadev
4) Go to OPAC ILL requests, login and visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl
5) Click "Create new request"
6) Change the 'type'. Notice you get a 403 error. Logs say:
Programming error - op 'add_form' must start with 'cud-' for POST
7) Apply patch. Restart plack. Repeat 6)
8) Notice the change type works as expected. Click "Create".
9) Notice the request is created as expected.

Created attachment 171323 [details] [review]
Bug 35570: (QA follow-up): Rebase on final state of bug 35681

Created attachment 172218 [details] [review]
Bug 35570: (QA Follow-up: Standard backend: Biblio creation: Add UNIMARC support

Add FreeForm UNIMARC enhancement from https://github.com/PTFS-Europe/koha-ill-freeform/commit/5d07c68f8058a9a1f35e3193699605240741ff0c into this work.
This is to keep this work up to date with latest FreeForm's work.

To test, k-t-d:
1) (Skip if using sandboxes) Start off by ensuring this is a UNIMARC installation, run:
$ reset_all_unimarc
2) (Skip if using sandboxes) Apply this bug 35570
3) Create a new Standard ILL request:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=FreeForm
4) Select type: book, input different values for title+author+isbn (the remaining fields are optional
   for this test)
5) Put in a cardnumber ('42' is kohaadmin if k-t-d/sandboxes). Select any library.
   Click 'Create'.
6) You should now see the ILL request details page. Click on the
   'Bibliographic record ID' link.
7) Ensure the newly created biblio record has the title+author+isbn from
   the metadata originally submitted.

Raised by Sonia Bouis at KohaCon's 2024 ILL workshop

Created attachment 172219 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Biblio creation: Add UNIMARC support

Add FreeForm UNIMARC enhancement from https://github.com/PTFS-Europe/koha-ill-freeform/commit/5d07c68f8058a9a1f35e3193699605240741ff0c into this work.
This is to keep this work up to date with latest FreeForm's work.

To test, k-t-d:
1) (Skip if using sandboxes) Start off by ensuring this is a UNIMARC installation, run:
$ reset_all_unimarc
2) (Skip if using sandboxes) Apply this bug 35570
3) Create a new Standard ILL request:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=FreeForm
4) Select type: book, input different values for title+author+isbn (the remaining fields are optional
   for this test)
5) Put in a cardnumber ('42' is kohaadmin if k-t-d/sandboxes). Select any library.
   Click 'Create'.
6) You should now see the ILL request details page. Click on the
   'Bibliographic record ID' link.
7) Ensure the newly created biblio record has the title+author+isbn from
   the metadata originally submitted.

Raised by Sonia Bouis at KohaCon's 2024 ILL workshop

Created attachment 172722 [details] [review]
Bug 35570: Put 'FreeForm' backend into core as 'Standard'

FreeForm copied as is from HEAD 369dffb159f1e70162b685b473dcf26c76f7e7e7
At https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/

Only edits made were 'freeform' -> 'standard' in the markup of the .inc files
and some occurrences of FreeForm in Standard.pm (previous Base.pm) file.

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

Created attachment 172723 [details] [review]
Bug 35570: Consider 'Standard' core backend

Added File::Basename qw( dirname )
Sorted Koha:: dependencies alphabetically
DRYed load_backend a bit and included 'Standard' backend check

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

Created attachment 172724 [details] [review]
Bug 35570: Atomicupdate

Move stuff out of 'FreeForm' into 'Standard'

This atomicupdate will printout the changes it made, if any.
It will also print out the report IDs of reports that may contain occurrences of 'FreeForm'.
This is to give the sys admin the chance to fix those reports before users.

To test, empty k-t-d:
1) Run updatedatabase, notice nothing happens
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Create a new 'FreeForm' ILL batch (requires a metadata enrichment plugin like https://github.com/PTFS-Europe/koha-plugin-api-pubmed)
4) Add '123' to the pubmedid list of identifiers and finish creating the batch
5) Upon creating the batch, you will now have 1 request, 1 batch and a few illrequestattributes in the 'FreeForm' backend
6) Run the updatedatabase again, notice the print outs.
7) Create a saved sql report like:
'Select * from illrequests where backend="FreeForm";'
8) Run the updatedatabase again, notice you get a warning for the above report

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

Created attachment 172725 [details] [review]
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed

Test plan:
1) This patch should not be applied in order to be able to go through the previous patch's test plan
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Please note that you need to be using FreeForm's reorganize_ILL branch at:
https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/
4) Visit ILL page:
/cgi-bin/koha/ill/ill-requests.pl
5) Notice there's a configuration error preventing the use of the ILL module.
6) Click the 'about page' link and verify there is a warning about
   FreeForm
7) Remove FreeForm from /kohadevbox/koha/Koha/Illbackends and refresh the page
8) Repeat 4), notice the requests are listed as expected and you can now
   use the ILL module again.

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

Created attachment 172726 [details] [review]
Bug 35570: (QA follow-up): Follow QA script on Standard backend

Make QA script green, fix:
- Variables declared in condition
- Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840)
- missing_filter
- forbidden pattern: Do not use line breaks inside template tags (bug 18675)
- Attribute type should not be used for script tags (bug 20053)
- File must have the exec flag
- forbidden pattern: trailing space char
- valid_template: /shared-includes/custom_fields.inc: not found

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

Created attachment 172727 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing

Add FreeForm fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/9c9da5ff2815fb6ce0af0181341dea5ce8c3bb2e into this work.
This is to keep this work up to date with latest FreeForm's fixes.
See bug 35685 for context and test plan.

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

Created attachment 172728 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility

Add FreeForm CSRF fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/c34be16ba34c840193ed5e043e6736fa0b1cb646 into this work.
This is to keep this work up to date with latest FreeForm's fixes.

Created attachment 172729 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF

Add FreeForm CSRF fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/6a37ce0daba8aab13130dacd055c9ca5876b7df4 into this work.
This is to keep this work up to date with latest FreeForm's fixes.

The test plan to demonstrate the issue this patch fixes:
Test plan, k-t-d:

1) Install FreeForm, enable ILL module, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) You'll have to switch the FreeForm branch to the one compatible with this work, i.e.:
cd /kohadevbox/koha/Koha/Illbackends/FreeForm
git checkout b_36243
3) Restart plack:
koha-plack --restart kohadev
4) Go to OPAC ILL requests, login and visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl
5) Click "Create new request"
6) Change the 'type'. Notice you get a 403 error. Logs say:
Programming error - op 'add_form' must start with 'cud-' for POST
7) Apply patch. Restart plack. Repeat 6)
8) Notice the change type works as expected. Click "Create".
9) Notice the request is created as expected.

Created attachment 172730 [details] [review]
Bug 35570: (QA follow-up): Rebase on final state of bug 35681

Created attachment 172731 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Biblio creation: Add UNIMARC support

Add FreeForm UNIMARC enhancement from https://github.com/PTFS-Europe/koha-ill-freeform/commit/5d07c68f8058a9a1f35e3193699605240741ff0c into this work.
This is to keep this work up to date with latest FreeForm's work.

To test, k-t-d:
1) (Skip if using sandboxes) Start off by ensuring this is a UNIMARC installation, run:
$ reset_all_unimarc
2) (Skip if using sandboxes) Apply this bug 35570
3) Create a new Standard ILL request:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=FreeForm
4) Select type: book, input different values for title+author+isbn (the remaining fields are optional
   for this test)
5) Put in a cardnumber ('42' is kohaadmin if k-t-d/sandboxes). Select any library.
   Click 'Create'.
6) You should now see the ILL request details page. Click on the
   'Bibliographic record ID' link.
7) Ensure the newly created biblio record has the title+author+isbn from
   the metadata originally submitted.

Raised by Sonia Bouis at KohaCon's 2024 ILL workshop

Created attachment 172732 [details] [review]
Bug 35570: (QA follow-up) Tidy

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172733 [details] [review]
Bug 35570: Make strings in Standard.pm translatable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172734 [details] [review]
Bug 35570: Use ->extended_attributes()

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172735 [details] [review]
Bug 35570: Do not unbless iterator

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Hi, nice job :-D

The only 'failure' I found is that choosing DVD when using the form makes it blow. I couldn't find the `dvd.inc` file in the original backend repo so I leave it to you.

Some remarks:

* Some of the code might fit somewhere else. For example the code for creating the record, that has those mappings for ISBNs, etc. BUT that's for another day and another project.
* I'm not sure how we're gonna make those templates translatable. For the backends I worked with, this was the backend's implementation business, but being core we need to find a solution.

Please provide the dvd.inc and mark as PQA

It also feels a bit out of place to see the 'New request' and 'List requests' buttons when editing the request. But might be out of the scope of this bug.

(In reply to Tomás Cohen Arazi (tcohen) from comment #108)
> Please provide the dvd.inc and mark as PQA

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36197#c4

I'll provide the fix asap.

Created attachment 172740 [details] [review]
Bug 35570: Put 'FreeForm' backend into core as 'Standard'

FreeForm copied as is from HEAD 369dffb159f1e70162b685b473dcf26c76f7e7e7
At https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/

Only edits made were 'freeform' -> 'standard' in the markup of the .inc files
and some occurrences of FreeForm in Standard.pm (previous Base.pm) file.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172741 [details] [review]
Bug 35570: Consider 'Standard' core backend

Added File::Basename qw( dirname )
Sorted Koha:: dependencies alphabetically
DRYed load_backend a bit and included 'Standard' backend check

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172742 [details] [review]
Bug 35570: Atomicupdate

Move stuff out of 'FreeForm' into 'Standard'

This atomicupdate will printout the changes it made, if any.
It will also print out the report IDs of reports that may contain occurrences of 'FreeForm'.
This is to give the sys admin the chance to fix those reports before users.

To test, empty k-t-d:
1) Run updatedatabase, notice nothing happens
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Create a new 'FreeForm' ILL batch (requires a metadata enrichment plugin like https://github.com/PTFS-Europe/koha-plugin-api-pubmed)
4) Add '123' to the pubmedid list of identifiers and finish creating the batch
5) Upon creating the batch, you will now have 1 request, 1 batch and a few illrequestattributes in the 'FreeForm' backend
6) Run the updatedatabase again, notice the print outs.
7) Create a saved sql report like:
'Select * from illrequests where backend="FreeForm";'
8) Run the updatedatabase again, notice you get a warning for the above report

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172743 [details] [review]
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed

Test plan:
1) This patch should not be applied in order to be able to go through the previous patch's test plan
2) Install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
3) Please note that you need to be using FreeForm's reorganize_ILL branch at:
https://github.com/PTFS-Europe/koha-ill-freeform/commits/reorganize_ILL/
4) Visit ILL page:
/cgi-bin/koha/ill/ill-requests.pl
5) Notice there's a configuration error preventing the use of the ILL module.
6) Click the 'about page' link and verify there is a warning about
   FreeForm
7) Remove FreeForm from /kohadevbox/koha/Koha/Illbackends and refresh the page
8) Repeat 4), notice the requests are listed as expected and you can now
   use the ILL module again.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172744 [details] [review]
Bug 35570: (QA follow-up): Follow QA script on Standard backend

Make QA script green, fix:
- Variables declared in condition
- Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840)
- missing_filter
- forbidden pattern: Do not use line breaks inside template tags (bug 18675)
- Attribute type should not be used for script tags (bug 20053)
- File must have the exec flag
- forbidden pattern: trailing space char
- valid_template: /shared-includes/custom_fields.inc: not found

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172745 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing

Add FreeForm fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/9c9da5ff2815fb6ce0af0181341dea5ce8c3bb2e into this work.
This is to keep this work up to date with latest FreeForm's fixes.
See bug 35685 for context and test plan.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172746 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility

Add FreeForm CSRF fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/c34be16ba34c840193ed5e043e6736fa0b1cb646 into this work.
This is to keep this work up to date with latest FreeForm's fixes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172747 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF

Add FreeForm CSRF fix from https://github.com/PTFS-Europe/koha-ill-freeform/commit/6a37ce0daba8aab13130dacd055c9ca5876b7df4 into this work.
This is to keep this work up to date with latest FreeForm's fixes.

The test plan to demonstrate the issue this patch fixes:
Test plan, k-t-d:

1) Install FreeForm, enable ILL module, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) You'll have to switch the FreeForm branch to the one compatible with this work, i.e.:
cd /kohadevbox/koha/Koha/Illbackends/FreeForm
git checkout b_36243
3) Restart plack:
koha-plack --restart kohadev
4) Go to OPAC ILL requests, login and visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl
5) Click "Create new request"
6) Change the 'type'. Notice you get a 403 error. Logs say:
Programming error - op 'add_form' must start with 'cud-' for POST
7) Apply patch. Restart plack. Repeat 6)
8) Notice the change type works as expected. Click "Create".
9) Notice the request is created as expected.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172748 [details] [review]
Bug 35570: (QA follow-up): Rebase on final state of bug 35681

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172749 [details] [review]
Bug 35570: (QA follow-up): Standard backend: Biblio creation: Add UNIMARC support

Add FreeForm UNIMARC enhancement from https://github.com/PTFS-Europe/koha-ill-freeform/commit/5d07c68f8058a9a1f35e3193699605240741ff0c into this work.
This is to keep this work up to date with latest FreeForm's work.

To test, k-t-d:
1) (Skip if using sandboxes) Start off by ensuring this is a UNIMARC installation, run:
$ reset_all_unimarc
2) (Skip if using sandboxes) Apply this bug 35570
3) Create a new Standard ILL request:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=FreeForm
4) Select type: book, input different values for title+author+isbn (the remaining fields are optional
   for this test)
5) Put in a cardnumber ('42' is kohaadmin if k-t-d/sandboxes). Select any library.
   Click 'Create'.
6) You should now see the ILL request details page. Click on the
   'Bibliographic record ID' link.
7) Ensure the newly created biblio record has the title+author+isbn from
   the metadata originally submitted.

Raised by Sonia Bouis at KohaCon's 2024 ILL workshop

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172750 [details] [review]
Bug 35570: (QA follow-up) Tidy

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172751 [details] [review]
Bug 35570: Make strings in Standard.pm translatable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172752 [details] [review]
Bug 35570: Use ->extended_attributes()

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 172753 [details] [review]
Bug 35570: Do not unbless iterator

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

(In reply to Pedro Amorim from comment #92)
> Created attachment 172218 [details] [review] [review]
> Bug 35570: (QA Follow-up: Standard backend: Biblio creation: Add UNIMARC
> support

Hi,
Thanks Pedro for this addition! I've just tested this part.
The actions from the test plan are OK but, I wonder if we should not put the author in the field 200$f instead of 700$a because 700$a is only for the surname, while you can put different elements of the name in 200$f.

I have also tried to create a book ILL request from the OPAC and then, only the title was added in the biblio record (no author nor isbn).

Strange thing, the request id from the one created from the OPAC had an prefix ILL- and not the one created from the staff. 

Last thought, in addition of book, the request for DVD or Thesis should also create biblio records as it's often physical documents. 

Sonia

Created attachment 172780 [details] [review]
Bug 35570: Revert some strings translatability

This patch reverts making core field descriptions translatable. It was
an obvious move and a needed one. But it turns out our core templates
are using those keys ('Author' and 'Title' in particular) and as such
translating them would break display (at least in the OPAC). Without
this patch, the ILL requests table in the OPAC doesn't display the title
and author columns values (i.e. displays 'N/A' instead)

Added a FIXME in the code, and will file a bug soo we fix this properly.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Thank you Sonia for your testing, please read my replies below:

(In reply to Sonia Bouis from comment #125)
> The actions from the test plan are OK but, I wonder if we should not put the
> author in the field 200$f instead of 700$a because 700$a is only for the
> surname, while you can put different elements of the name in 200$f.

Fair point, I'll provide a follow-up correcting this. Thank you.

(In reply to Sonia Bouis from comment #125)
> I have also tried to create a book ILL request from the OPAC and then, only
> the title was added in the biblio record (no author nor isbn).

Not able to reproduce this. Running on an UNIMARC database:
1) Create a new OPAC ILL request:
<opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard
2) Pick Type:Book. Put a title, author and isbn, cardnumber + library. Click 'Create'.
3) On the detail screen. Click on the 'Bibliographic record ID'.
4) On the biblio detail page, confirm there's an entry for author and ISBN.
5) Click "Marc preview:Show". Confirm the MARC data is as expected.

What I think you may be experiencing (on k-t-d/sandboxes):
From step 4), if you click "Edit -> Edit record", you'll be shown the "Fast Add Framework". This framework does not have fields 010 and 700, so they're effectively stripped. If you then change to default, these fields don't carry over, because they've been effectivelly stripped by the "Fast Add Framework".
To prove my point, access the edit biblio page using the Default frameworkcode directly from the URL, like below (update staff_url and change '21651' to your test biblionumber):
<staff_url>/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=21651&frameworkcode=Default

You'll notice that in this view, all MARC data is shown, including ISBN+Author. From here, if you switch to "Fast Add Framework" and then go back to "Default", you'll notice those fields are gone again. I believe this has always been the case in Koha since I can remember, but I may be wrong.
Please let me know if this makes sense and describes what you're experiencing. I've done this exercise creating an ILL request from either Staff or OPAC, it's no different from my testing.

(In reply to Sonia Bouis from comment #125)
> Strange thing, the request id from the one created from the OPAC had an
> prefix ILL- and not the one created from the staff. 

This is because there's a <prefix> configured in koha-conf.xml for 'CPL' (Centerville library on stock k-t-d database). This 'ILL-' prefix will happen for any ILL request created for Centerville, regardless if created in the OPAC or Staff. Only Centerville has this configured, so any other library will not have a prefix. I believe this is working as designed.

(In reply to Sonia Bouis from comment #125)
> Last thought, in addition of book, the request for DVD or Thesis should also
> create biblio records as it's often physical documents. 

My suggestion is that we file this as a follow-up bug, if you agree. This is additional functionality and in my opinion goes beyond the scope of this bug.

Created attachment 172809 [details] [review]
Bug 35570: (QA follow-up): Fix DVD type

I've added some DVD specific fields: language, format, genre, studio.
Happy to have this discussed and/or updated but hoping this is good enough for now.
To test:
1) Create an ILL request of 'DVD' type on both OPAC and Staff
2) Verify the form is shown without issues
3) Fill all fields and verify all fields are stored and shown correctly on both OPAC and STaff

(In reply to Tomás Cohen Arazi (tcohen) from comment #108)
> Please provide the dvd.inc and mark as PQA

Done.

Created attachment 172811 [details] [review]
Bug 35570: (QA follow-up): Update UNIMARC author to 200$f

Reworked the logic here a bit to ensure the following scenario for UNIMARC:
If both title and author exist, the result is:
200$aTitle$fAuthor

In the previous logic, it would become:
200$aTitle
200$fAuthor

I'm assuming the desired result is the former and not the latter.
Test following the same test plan, but try having only author, or only title, and verify that the resulted MARC data is as expected.

(In reply to Pedro Amorim from comment #127)
> Thank you Sonia for your testing, please read my replies below:
> 
> (In reply to Sonia Bouis from comment #125)
> > The actions from the test plan are OK but, I wonder if we should not put the
> > author in the field 200$f instead of 700$a because 700$a is only for the
> > surname, while you can put different elements of the name in 200$f.
> 
> Fair point, I'll provide a follow-up correcting this. Thank you.

Hi Sonia, I've now provided a patch that adds the author field information to 200$f instead of 700$a for UNIMARC. Please test when you're available. Thank you!

Hello
I wanted to ask why DVD or Thesis requests did not create biblio records, but I see Sonia has already made this remark... It believe it could be a nice improvement after this patch is pushed.

Pedro, if you use 200$f for the author in UNIMARC, would not it make sense to use 245$c in MARC21 (instead of 700$a), as this field have the same meaning?

I tested on a UNIMARC sandbox. Everything is fine in the record (010$a, 200$a, 200$f are created as expected)

But we should not have a 942$n field. It is not a defined in UNIMARC Koha standard framework.

However the proper handling of this information in UNIMARC is problematic in UNIMARC (see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37600). So I suppose we can fix the code for ILL form for UNIMARC once a solution is pushed for bug 37600

(In reply to Mathieu Saby from comment #132)
> Hello
> I wanted to ask why DVD or Thesis requests did not create biblio records,
> but I see Sonia has already made this remark... It believe it could be a
> nice improvement after this patch is pushed.

Hi Mathieu, thank you for testing and adding to the discussion!
I'm fully on board with this and surely others are too.
I'd prefer to keep the scope of this bug limited to just adding FreeForm into core Koha, even though we've added some enhancements here, I think it'd be better to add have any other additional enhancements to follow-up bugs instead.

(In reply to Mathieu Saby from comment #133)
> Pedro, if you use 200$f for the author in UNIMARC, would not it make sense
> to use 245$c in MARC21 (instead of 700$a), as this field have the same
> meaning?

A very good question. When I added the UNIMARC support for biblio creation, it did not cross my mind to review the MARC21 mapping. As above, something to discuss on a follow-up bug imo.

Hi Pedro,
Just to let you know that I've tested again the unimarc biblio creation and that it's ok (perhaps the change made by Tomas in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35570#c126 resolved the problem of author and isbn in biblio created in the OPAC).

Just wondering, is the status really passed QA? I don't see QA analysis on this.

Thank you again for this job.
Sonia

(In reply to Sonia Bouis from comment #137)
> Just wondering, is the status really passed QA? I don't see QA analysis on
> this.
> 
> Thank you again for this job.
> Sonia

(In reply to Pedro Amorim from comment #129)
> (In reply to Tomás Cohen Arazi (tcohen) from comment #108)
> > Please provide the dvd.inc and mark as PQA
> 
> Done.

Still applies, continueing work here tomorrow.

Please check:

 FAIL	Koha/ILL/Backend/Standard.pm
   FAIL	  critic
		"$marc_isbn" is declared but not used at line 1179, column 5. Unused variables clutter code and make it harder to read.
		"$marc_author" is declared but not used at line 1180, column 5. Unused variables clutter code and make it harder to read.
		"$marc_title" is declared but not used at line 1181, column 5. Unused variables clutter code and make it harder to read.

There are some issues here that will require follow-ups:

1) Translatability

a) Using __() in a .pm (nothing to do here)

I am glad I double checked these, because I thought they would not work, but they do get parsed into the po files. 

Koha/ILL/Backend/Standard.pm
+            name           => __('Switched provider'),
+            ui_method_name => __('Switch provider'),



b) But this... hm:

+    # FIXME: This should all be made translatable as is basically a
+    #        code => description mapping. The metadata() sub is using this
+    #        descriptions as keys. I thought this was a bug on this backend
+    #        but it turns out opac-illrequests.tt is using those descriptions
+    #        as keys. for example:

c) JS (definitely needs fixing):

+            '</span>Delete</button></li>';


2) Unused variables (to fix, see QA script complaint):

+    my $marc_isbn;
+    my $marc_author;
+    my $marc_title;

3) Terminology (please fix)

+<p><em>Please Note:</em> Branch is a mandatory field.</p>
+[% ELSIF whole.status == 'invalid_borrower' %]
+<p><em>Please Note:</em> The borrower details you entered are invalid.</p>
+[% ELSIF whole.status == 'invalid_branch' %]
+<p><em>Please Note:</em> The branch you chose is invalid.</p>
+[% ELSE %]

This repeats in several files, where the field itself has already been renamed to "Destination library".

branch => library

+        <label class="required" for="brw">Borrower</label>

Borrower => Patron

4) Database update (fixed)

a) No need to repeat the bug number for each message, it will be at the beginning of the paragraph. 

+                "Bug 35570: Updated ILL request attributes from 'FreeForm' to 'Standard'"

I removed these when moving the database update to dbrevs.

b) Can you explain this one? The English strings make me a little suspicious (question)

+        my $reports = join(
+            "\n",
+            map( "\tReport ID: "
+                    . $_->id
+                    . ' | Edit link: '
+                    . C4::Context->preference('staffClientBaseURL')
+                    . '/cgi-bin/koha/reports/guided_reports.pl?reports='
+                    . $_->id
+                    . "&phase=Edit%20SQL",
+                Koha::Reports->search( { savedsql => { -like => "%FreeForm%" } } )->as_list )
+        );


5) Wrong version (fixed)

+                                    <p>All 'FreeForm' ILL content should've been migrated to 'Standard' upon upgrading Koha to 24.05.</p>

Changed to 24.11 in follow-up.


6) Unit tests (separate bug?)

There are no unit tests, but it feels like there should be some...
Maybe for a separate bug at this point.

7) Styling (separate bug)

Some of the pages and elements in the forms have unusual styling. For example we are missing the "yellow" buttons for the main actions.

Pushed for 24.11!

Well done everyone, thank you!

Created attachment 173916 [details] [review]
Bug 35570: (QA follow-up): Fix styling of confirmation buttons

Created attachment 173917 [details] [review]
Bug 35570: (QA follow-up): Fix terminology

Created attachment 173918 [details] [review]
Bug 35570: (QA follow-up): Fix translatability of 'Delete'

This is also fixing the delete->trash icon and spacing between label+icon

Hi Katrin, thank you very much for your work here and apologies not replying sooner:

1.a) Non-issue
1.b) Opened bug 38339 for this.
1.c) Patch submitted. Additional translatability work filed for bug 38340.
2) Non-issue, fixed upstream by commit 217b004. Thanks!
3) Patch submitted.
4.a) Thanks!
4.b) Yes, this produces the following warning if a report is found containing the word 'FreeForm'. This is to help administrators to update their reports reporting on FreeForm:

        Bug 35570: **ACTION REQUIRED**: Saved SQL reports containing occurrences of 'FreeForm' were found. The following reports MUST be updated accordingly ('FreeForm' -> 'Standard'):
                Report ID: 1 | Edit link: http://kohadev-intra.myDNSname.org:8081/cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Edit%20SQL

I did not consider translatability here (sorry!). I'm not entirely sure how to do it here, or if worth the investment. Let me know!

5) Fixed upstream. Thanks!
6) Yes, bug 38341
7) Patch submitted

I am sorry Pedro, this one escaped me and I missed your great follow-ups.
I'll try to merge the work on bug 36221 and here.