Bug 35570 - Add a generic master form in ILL
Summary: Add a generic master form in ILL
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low new feature with 10 votes (vote)
Assignee: Pedro Amorim
QA Contact:
URL:
Keywords:
Depends on: 35581 35681
Blocks: 35725
  Show dependency treegraph
 
Reported: 2023-12-14 12:57 UTC by Pedro Amorim
Modified: 2024-06-04 13:45 UTC (History)
11 users (show)

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


Attachments
Bug 35570: Put 'FreeForm' backend into core as 'Standard' (81.66 KB, patch)
2023-12-18 11:35 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Consider 'Standard' core backend (3.98 KB, patch)
2023-12-18 11:35 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Atomicupdate (4.01 KB, patch)
2023-12-18 11:35 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed (4.85 KB, patch)
2023-12-18 11:35 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): (45.37 KB, patch)
2023-12-18 11:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: QA follow-up: (2.80 KB, patch)
2024-01-03 11:19 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): (1.16 KB, patch)
2024-01-03 12:07 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Put 'FreeForm' backend into core as 'Standard' (81.68 KB, patch)
2024-01-08 20:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 35570: Consider 'Standard' core backend (4.02 KB, patch)
2024-01-08 20:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 35570: Atomicupdate (4.06 KB, patch)
2024-01-08 20:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed (4.90 KB, patch)
2024-01-08 20:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): (44.40 KB, patch)
2024-01-08 20:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 35570: QA follow-up: (2.85 KB, patch)
2024-01-08 20:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): (1.20 KB, patch)
2024-01-08 20:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Fix action button links. Use url filter instead of uri. (3.41 KB, patch)
2024-01-09 09:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Put 'FreeForm' backend into core as 'Standard' (81.72 KB, patch)
2024-01-26 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Consider 'Standard' core backend (4.02 KB, patch)
2024-01-26 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Atomicupdate (4.11 KB, patch)
2024-01-26 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed (4.91 KB, patch)
2024-01-26 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Follow QA script on Standard backend (44.18 KB, patch)
2024-01-26 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing (1.27 KB, patch)
2024-01-26 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility (2.08 KB, patch)
2024-03-07 10:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF (2.25 KB, patch)
2024-03-07 11:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Put 'FreeForm' backend into core as 'Standard' (81.72 KB, patch)
2024-03-07 11:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Consider 'Standard' core backend (4.02 KB, patch)
2024-03-07 11:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Atomicupdate (4.11 KB, patch)
2024-03-07 11:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed (4.93 KB, patch)
2024-03-07 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Follow QA script on Standard backend (44.18 KB, patch)
2024-03-07 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing (1.27 KB, patch)
2024-03-07 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility (2.08 KB, patch)
2024-03-07 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF (2.25 KB, patch)
2024-03-07 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Put 'FreeForm' backend into core as 'Standard' (81.72 KB, patch)
2024-04-02 14:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Consider 'Standard' core backend (3.94 KB, patch)
2024-04-02 14:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Atomicupdate (4.11 KB, patch)
2024-04-02 14:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed (4.93 KB, patch)
2024-04-02 14:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Follow QA script on Standard backend (44.18 KB, patch)
2024-04-02 14:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing (1.27 KB, patch)
2024-04-02 14:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility (2.08 KB, patch)
2024-04-02 14:19 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF (2.25 KB, patch)
2024-04-02 14:19 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Put 'FreeForm' backend into core as 'Standard' (81.72 KB, patch)
2024-04-19 13:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Consider 'Standard' core backend (3.96 KB, patch)
2024-04-19 13:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Atomicupdate (4.11 KB, patch)
2024-04-19 13:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed (4.93 KB, patch)
2024-04-19 13:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Follow QA script on Standard backend (44.18 KB, patch)
2024-04-19 13:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing (1.27 KB, patch)
2024-04-19 13:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility (2.08 KB, patch)
2024-04-19 13:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF (2.25 KB, patch)
2024-04-19 13:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Put 'FreeForm' backend into core as 'Standard' (81.72 KB, patch)
2024-04-22 13:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Consider 'Standard' core backend (3.99 KB, patch)
2024-04-22 13:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Atomicupdate (4.11 KB, patch)
2024-04-22 13:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: Prevent use of ILL module if deprecated FreeForm backend is installed (4.71 KB, patch)
2024-04-22 13:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Follow QA script on Standard backend (43.59 KB, patch)
2024-04-22 13:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Show appropriate error message when type is missing (1.27 KB, patch)
2024-04-22 13:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Bug 36243 compatibility (2.08 KB, patch)
2024-04-22 13:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Standard backend: Fix OPAC CSRF (2.25 KB, patch)
2024-04-22 13:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Rebase on final state of bug 35681 (3.27 KB, patch)
2024-05-22 15:57 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35570: (QA follow-up): Rebase on final state of bug 35681 (3.10 KB, patch)
2024-05-22 15:59 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-12-14 12:57:50 UTC
The idea is to:

1) Have a generic master form in ILL based on current FreeForm's form at:
https://github.com/PTFS-Europe/koha-ill-freeform
2) Deprecate the above FreeForm backend
3) Perhaps rethink the name to "Standard", "Default", something like that?
4) Migrate FreeForm requests for this new "Standard backend" built in core on upgrade

Reasoning behind this:
- The ILL module does not work out of the box, it requires at least one backend to work. This is not great as ideally enabling the system preference should be enough to make use of the module, without additional work being needed.
- This master form in core would allow for future extensions of the ILL module, could be used as baseline for future work that will optionally not require the user (Staff/OPAC) to pick the backend beforehand if several backends are installed 
 - the user may not know which backend to pick, they likely just know what they're looking for.

Thoughts?
Comment 1 Tomás Cohen Arazi 2023-12-14 13:18:52 UTC
I love the idea.
Comment 2 Magnus Enger 2023-12-18 07:56:57 UTC
(In reply to Pedro Amorim from comment #0)
> Thoughts?

Sounds like a great idea!
Comment 3 Pedro Amorim 2023-12-18 11:35:49 UTC
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.
Comment 4 Pedro Amorim 2023-12-18 11:35:52 UTC Comment hidden (obsolete)
Comment 5 Pedro Amorim 2023-12-18 11:35:54 UTC Comment hidden (obsolete)
Comment 6 Pedro Amorim 2023-12-18 11:35:58 UTC Comment hidden (obsolete)
Comment 7 Pedro Amorim 2023-12-18 11:36:02 UTC Comment hidden (obsolete)
Comment 8 Pedro Amorim 2023-12-18 11:36:39 UTC
I have named the new core backend 'Standard' but I don't feel strongly about this, opinions/suggestions welcomed.
Comment 9 David Nind 2024-01-02 21:24:53 UTC Comment hidden (obsolete)
Comment 10 Pedro Amorim 2024-01-03 09:29:20 UTC Comment hidden (obsolete)
Comment 11 Pedro Amorim 2024-01-03 11:19:04 UTC Comment hidden (obsolete)
Comment 12 Pedro Amorim 2024-01-03 12:07:50 UTC Comment hidden (obsolete)
Comment 13 David Nind 2024-01-07 23:30:30 UTC Comment hidden (obsolete)
Comment 14 Pedro Amorim 2024-01-08 09:25:00 UTC Comment hidden (obsolete)
Comment 15 David Nind 2024-01-08 20:46:41 UTC
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>
Comment 16 David Nind 2024-01-08 20:46:44 UTC
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>
Comment 17 David Nind 2024-01-08 20:46:47 UTC
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>
Comment 18 David Nind 2024-01-08 20:46:50 UTC
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>
Comment 19 David Nind 2024-01-08 20:46:54 UTC
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>
Comment 20 David Nind 2024-01-08 20:46:57 UTC
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>
Comment 21 David Nind 2024-01-08 20:47:00 UTC
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>
Comment 22 David Nind 2024-01-08 20:49:17 UTC
(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!
Comment 23 David Nind 2024-01-08 20:55:18 UTC
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.
Comment 24 David Nind 2024-01-08 20:57:59 UTC
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)
Comment 25 Pedro Amorim 2024-01-09 09:56:53 UTC
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.
Comment 26 Pedro Amorim 2024-01-09 09:58:36 UTC
(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!
Comment 27 David Nind 2024-01-10 19:24:17 UTC
(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.
Comment 28 Marcel de Rooy 2024-01-26 10:20:53 UTC
Could we do a cleanup here first?
Seeing various (QA follow-up) without any further text. Does not look good.
Comment 29 Pedro Amorim 2024-01-26 10:49:00 UTC
(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.
Comment 30 Pedro Amorim 2024-01-26 12:56:34 UTC Comment hidden (obsolete)
Comment 31 Pedro Amorim 2024-01-26 12:56:38 UTC Comment hidden (obsolete)
Comment 32 Pedro Amorim 2024-01-26 12:56:41 UTC Comment hidden (obsolete)
Comment 33 Pedro Amorim 2024-01-26 12:56:44 UTC Comment hidden (obsolete)
Comment 34 Pedro Amorim 2024-01-26 12:56:47 UTC Comment hidden (obsolete)
Comment 35 Pedro Amorim 2024-01-26 12:56:51 UTC Comment hidden (obsolete)
Comment 36 Pedro Amorim 2024-01-26 12:58:16 UTC
(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.
Comment 37 Pedro Amorim 2024-03-07 10:46:55 UTC Comment hidden (obsolete)
Comment 38 Pedro Amorim 2024-03-07 11:20:08 UTC Comment hidden (obsolete)
Comment 39 Pedro Amorim 2024-03-07 11:50:45 UTC Comment hidden (obsolete)
Comment 40 Pedro Amorim 2024-03-07 11:50:49 UTC Comment hidden (obsolete)
Comment 41 Pedro Amorim 2024-03-07 11:50:54 UTC Comment hidden (obsolete)
Comment 42 Pedro Amorim 2024-03-07 11:51:00 UTC Comment hidden (obsolete)
Comment 43 Pedro Amorim 2024-03-07 11:51:04 UTC Comment hidden (obsolete)
Comment 44 Pedro Amorim 2024-03-07 11:51:09 UTC Comment hidden (obsolete)
Comment 45 Pedro Amorim 2024-03-07 11:51:14 UTC Comment hidden (obsolete)
Comment 46 Pedro Amorim 2024-03-07 11:51:19 UTC Comment hidden (obsolete)
Comment 47 Pedro Amorim 2024-04-02 14:18:40 UTC Comment hidden (obsolete)
Comment 48 Pedro Amorim 2024-04-02 14:18:44 UTC Comment hidden (obsolete)
Comment 49 Pedro Amorim 2024-04-02 14:18:47 UTC Comment hidden (obsolete)
Comment 50 Pedro Amorim 2024-04-02 14:18:51 UTC Comment hidden (obsolete)
Comment 51 Pedro Amorim 2024-04-02 14:18:54 UTC Comment hidden (obsolete)
Comment 52 Pedro Amorim 2024-04-02 14:18:58 UTC Comment hidden (obsolete)
Comment 53 Pedro Amorim 2024-04-02 14:19:01 UTC Comment hidden (obsolete)
Comment 54 Pedro Amorim 2024-04-02 14:19:05 UTC Comment hidden (obsolete)
Comment 55 Pedro Amorim 2024-04-02 14:20:19 UTC
Rebased after recent changes to bug 19605
Comment 56 Pedro Amorim 2024-04-19 13:53:49 UTC Comment hidden (obsolete)
Comment 57 Pedro Amorim 2024-04-19 13:53:54 UTC Comment hidden (obsolete)
Comment 58 Pedro Amorim 2024-04-19 13:53:57 UTC Comment hidden (obsolete)
Comment 59 Pedro Amorim 2024-04-19 13:54:01 UTC Comment hidden (obsolete)
Comment 60 Pedro Amorim 2024-04-19 13:54:06 UTC Comment hidden (obsolete)
Comment 61 Pedro Amorim 2024-04-19 13:54:12 UTC Comment hidden (obsolete)
Comment 62 Pedro Amorim 2024-04-19 13:54:15 UTC Comment hidden (obsolete)
Comment 63 Pedro Amorim 2024-04-19 13:54:20 UTC Comment hidden (obsolete)
Comment 64 Pedro Amorim 2024-04-19 13:54:45 UTC
Rebase after a follow-up submitted to bug 19605 was required.
Comment 65 Pedro Amorim 2024-04-22 13:52:07 UTC
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>
Comment 66 Pedro Amorim 2024-04-22 13:52:11 UTC
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>
Comment 67 Pedro Amorim 2024-04-22 13:52:14 UTC
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>
Comment 68 Pedro Amorim 2024-04-22 13:52:17 UTC
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>
Comment 69 Pedro Amorim 2024-04-22 13:52:21 UTC
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>
Comment 70 Pedro Amorim 2024-04-22 13:52:25 UTC
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>
Comment 71 Pedro Amorim 2024-04-22 13:52:30 UTC
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.
Comment 72 Pedro Amorim 2024-04-22 13:52:37 UTC
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.
Comment 73 Pedro Amorim 2024-04-22 13:54:10 UTC
Rebased required after bug 35151 and bug 32693pushed to main.
Comment 74 Pedro Amorim 2024-04-22 13:54:39 UTC
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.
Comment 75 Pedro Amorim 2024-05-22 15:57:45 UTC Comment hidden (obsolete)
Comment 76 Pedro Amorim 2024-05-22 15:59:30 UTC
Created attachment 167051 [details] [review]
Bug 35570: (QA follow-up): Rebase on final state of bug 35681