Bug 38751

Summary: Creating ILL request through OPAC openURL explodes if same attribute defined twice
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: ILLAssignee: Pedro Amorim <pedro.amorim>
Status: Pushed to main --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: minor    
Priority: P5 - low CC: martin.renvoize, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:
Bug Depends on:    
Bug Blocks: 38819    
Attachments: Bug 38751: Add tests
Bug 38751: Update Standard ILL backend add_request
Bug 38751: Update ILL::Request->extended_attributes
Bug 38751: Update ILL::Request->extended_attributes
Bug 38751: Add tests
Bug 38751: Update Standard ILL backend add_request
Bug 38751: Update ILL::Request->extended_attributes
Bug 38751: Add tests
Bug 38751: Update Standard ILL backend add_request
Bug 38751: Update Standard ILL backend migrate
Bug 38751: Update ILL::Request->extended_attributes
Bug 38751: Add tests
Bug 38751: Update Standard ILL backend add_request
Bug 38751: Update Standard ILL backend migrate

Description Pedro Amorim 2024-12-19 12:03:11 UTC

    
Comment 1 Pedro Amorim 2024-12-19 12:04:10 UTC Comment hidden (obsolete)
Comment 2 Pedro Amorim 2024-12-19 12:04:12 UTC Comment hidden (obsolete)
Comment 3 Pedro Amorim 2024-12-19 12:04:14 UTC Comment hidden (obsolete)
Comment 4 Pedro Amorim 2024-12-19 12:04:53 UTC
Created attachment 175703 [details] [review]
Bug 38751: Update ILL::Request->extended_attributes

This updates the extended_attributes method to also act as setter.
Additionally, it'll skip adding any attribute that result in error, preventing an ugly error 500.

Test plan:
1) Enable ILLModule sys pref
2) Attempt to create the following openurl request through the OPAC:
http://localhost:8080/cgi-bin/koha/opac-illrequests.pl?DOI=10.1001%2Fjama.2024.11146&atitle=Interventions%20for%20High%20Body%20Mass%20Index%20in%20Children%20and%20Adolescents%3A%20US%20Preventive%20Services%20Task%20Force%20Recommendation%20Statement.&aulast=Nicholson%2C%20Wanda%20K.&backend=FreeForm&cardnumber=53807898&date=20240716&genre=article&id=DOI%3A10.1001%2Fjama.2024.11146&issn=00987484&issue=3&method=create&opac=1&openurl=1
3) Notice it fails with a Duplicate ID error and the OPAC blows up. This is because Koha is trying to add the 'DOI' illrequestattribute twice and that is resulting in a duplicate ID
4) Apply patches, repeat test plan, notice the error no longer happens, the request is created correctly and all successful attribute have been added.
Comment 5 Pedro Amorim 2024-12-19 12:04:56 UTC
Created attachment 175704 [details] [review]
Bug 38751: Add tests

prove t/db_dependent/Koha/ILL/Request.t
Comment 6 Pedro Amorim 2024-12-19 12:04:58 UTC
Created attachment 175705 [details] [review]
Bug 38751: Update Standard ILL backend add_request

Make use of newly updated ILL::Request->extended_attributes method instead of creating the query manually
Comment 7 David Nind 2024-12-22 22:05:46 UTC
Created attachment 175903 [details] [review]
Bug 38751: Update ILL::Request->extended_attributes

This updates the extended_attributes method to also act as setter.
Additionally, it'll skip adding any attribute that result in error, preventing an ugly error 500.

Test plan:
1) Enable ILLModule sys pref
2) Attempt to create the following openurl request through the OPAC:
http://localhost:8080/cgi-bin/koha/opac-illrequests.pl?DOI=10.1001%2Fjama.2024.11146&atitle=Interventions%20for%20High%20Body%20Mass%20Index%20in%20Children%20and%20Adolescents%3A%20US%20Preventive%20Services%20Task%20Force%20Recommendation%20Statement.&aulast=Nicholson%2C%20Wanda%20K.&backend=FreeForm&cardnumber=53807898&date=20240716&genre=article&id=DOI%3A10.1001%2Fjama.2024.11146&issn=00987484&issue=3&method=create&opac=1&openurl=1
3) Notice it fails with a Duplicate ID error and the OPAC blows up. This is because Koha is trying to add the 'DOI' illrequestattribute twice and that is resulting in a duplicate ID
4) Apply patches, repeat test plan, notice the error no longer happens, the request is created correctly and all successful attribute have been added.

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2024-12-22 22:05:50 UTC
Created attachment 175904 [details] [review]
Bug 38751: Add tests

prove t/db_dependent/Koha/ILL/Request.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2024-12-22 22:05:52 UTC
Created attachment 175905 [details] [review]
Bug 38751: Update Standard ILL backend add_request

Make use of newly updated ILL::Request->extended_attributes method instead of creating the query manually

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Pedro Amorim 2025-01-03 16:07:08 UTC
Created attachment 176124 [details] [review]
Bug 38751: Update Standard ILL backend migrate

Make use of newly updated ILL::Request->extended_attributes method instead of creating the query manually

The test plan in bug 38819 fails without this patch (page explodes with duplicate ID error).
It's the same cause as the one being fixed here in bug 38751 so I'm submitting this here.
Comment 11 Martin Renvoize (ashimema) 2025-01-08 16:13:25 UTC
Created attachment 176250 [details] [review]
Bug 38751: Update ILL::Request->extended_attributes

This updates the extended_attributes method to also act as setter.
Additionally, it'll skip adding any attribute that result in error,
preventing an ugly error 500.

Test plan:
1) Enable ILLModule sys pref
2) Attempt to create the following openurl request through the OPAC:
   http://localhost:8080/cgi-bin/koha/opac-illrequests.pl?DOI=10.1001%2Fjama.2024.11146&atitle=Interventions%20for%20High%20Body%20Mass%20Index%20in%20Children%20and%20Adolescents%3A%20US%20Preventive%20Services%20Task%20Force%20Recommendation%20Statement.&aulast=Nicholson%2C%20Wanda%20K.&backend=FreeForm&cardnumber=53807898&date=20240716&genre=article&id=DOI%3A10.1001%2Fjama.2024.11146&issn=00987484&issue=3&method=create&opac=1&openurl=1
3) Notice it fails with a Duplicate ID error and the OPAC blows up.
   This is because Koha is trying to add the 'DOI' illrequestattribute
   twice and that is resulting in a duplicate ID
4) Apply patches, repeat test plan, notice the error no longer happens,
   the request is created correctly and all successful attribute have
   been added.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize (ashimema) 2025-01-08 16:13:27 UTC
Created attachment 176251 [details] [review]
Bug 38751: Add tests

prove t/db_dependent/Koha/ILL/Request.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize (ashimema) 2025-01-08 16:13:30 UTC
Created attachment 176252 [details] [review]
Bug 38751: Update Standard ILL backend add_request

Make use of newly updated ILL::Request->extended_attributes method instead of
creating the query manually

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize (ashimema) 2025-01-08 16:13:32 UTC
Created attachment 176253 [details] [review]
Bug 38751: Update Standard ILL backend migrate

Make use of newly updated ILL::Request->extended_attributes method instead of creating the query manually

The test plan in bug 38819 fails without this patch (page explodes with duplicate ID error).
It's the same cause as the one being fixed here in bug 38751 so I'm submitting this here.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize (ashimema) 2025-01-08 16:14:33 UTC
Clear bugfix and code improvement, well tested and clear.

QA scripts all happy,

Passing QA
Comment 16 Katrin Fischer 2025-01-10 18:21:16 UTC
Pushed for 25.05!

Well done everyone, thank you!