Bug 37365

Summary: Bad redirect when adding a patron message from members/files.pl
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: PatronsAssignee: Eric Garcia <cubingguy714>
Status: Pushed to oldoldstable --- QA Contact: Aleisha Amohia <aleisha>
Severity: minor    
Priority: P5 - low CC: aleisha, cubingguy714, david, fridolin.somers, gmcharlt, kyle.m.hall, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes a redirect when adding a patron message straight after uploading a patron file (when EnableBorrowerFiles is enabled). Before this fix, an error message "Patron not found. Return to search" was displayed if you added a message straight after you finished uploading a file (the "Add message" option on other pages worked as expected).
Version(s) released in:
24.11.00,24.05.06,23.11.11
Circulation function:
Attachments: Bug 37365: Fixed redirect when adding a patron message from members/files.pl
Bug 37365: Fixed redirect when adding a patron message from members/files.pl
Bug 37365: (QA follow-up) Add borrowernumber to referrer url
Bug 37365: (QA follow-up) Add error checking around AddFile call
Bug 37365: (QA follow-up) Add borrowernumber to referrer url
Bug 37365: (QA follow-up) Add error checking around AddFile call

Description Lucas Gass (lukeg) 2024-07-15 15:27:35 UTC
To recreate:

1. Set 'EnableBorrowerFiles' to 'Do'.
2. Find a patron record
3. Upload a file
4. Once the upload completes 'Add message' and save 
5. You are redirected to /cgi-bin/koha/members/files.pl with a message like: "Patron not found. Return to search"
Comment 1 Eric Garcia 2024-07-15 20:29:03 UTC
Created attachment 169028 [details] [review]
Bug 37365: Fixed redirect when adding a patron message from members/files.pl

To test:
1. Set 'EnableBorrowerFiles' to 'Do'
2. Find a patron record
3. Upload a file
4. Once the uploaed completes 'Add message' and save
5. You are redirected to /cgi-bin/koha/members/files.pl with a message like: "Patron not found. Return to search"
6. Apply patch and restart_all
7. Repeat steps 1-4 and notice you are redirected to the correct page '/cgi-bin/koha/members/files.pl?borrowernumber='
Comment 2 David Nind 2024-07-16 03:53:14 UTC
Created attachment 169034 [details] [review]
Bug 37365: Fixed redirect when adding a patron message from members/files.pl

To test:
1. Set 'EnableBorrowerFiles' to 'Do'
2. Find a patron record
3. Upload a file
4. Once the uploaed completes 'Add message' and save
5. You are redirected to /cgi-bin/koha/members/files.pl with a message like: "Patron not found. Return to search"
6. Apply patch and restart_all
7. Repeat steps 1-4 and notice you are redirected to the correct page '/cgi-bin/koha/members/files.pl?borrowernumber='

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Marcel de Rooy 2024-07-19 09:17:30 UTC
I cannot reproduce the original problem. I do not see the "Patron not found" message. Please clarify.

After a print redirect, I would expect an exit statement.

Failed QA
Comment 4 David Nind 2024-07-22 02:37:52 UTC
This is how to reproduce:

1. Set the EnableBorrowerFiles system preference to 'Do'.
2. Go to a patron record (I used Mary Burton).
3. Go to the 'Files' section for the patron (in the left hand-side navigation menu between statistics and purchase suggestions).
4. Upload any file:
   4.1 Add a description
   4.2 Choose a file
   4.3 Click 'Upload file'
   Result ==> You are on a page that lists files in a table (Name, Type, Description, Uploaded > Action column with a Delete button) and the upload new file form below it. The URL is /cgi-bin/koha/members/files.pl
5. Without changing pages, click 'Add message':
   Result ==> The pop-up form appears for adding a message.
6. Enter a message and click 'Save'
7. You get an error message: "Patron not found. Return to search":
   7.1 The URL is /cgi-bin/koha/members/files.pl
   7.2 The breadcrumb is Home > Patrons >  > Files 
   7.3 The browser title is "Files for > Patrons > Koha - [your browser name]"

It took me a while to figure it out - the 'Add message' button appears to work correctly from all the other patron pages.

It works OK when you first go to the Files page - it only generates the error after you click on 'Add message' and the 'Save' straight after you upload a file.
Comment 5 Marcel de Rooy 2024-10-04 08:12:09 UTC
(In reply to David Nind from comment #4)
> This is how to reproduce:

Thx David. I saw it now.
Comment 6 Marcel de Rooy 2024-10-04 09:07:02 UTC
Created attachment 172381 [details] [review]
Bug 37365: (QA follow-up) Add borrowernumber to referrer url

Since we post to add_message, we need to ensure that the
borrowernumber is added to the referrer url. This 'tric' is
already done there for another form.

Test plan:
See original plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2024-10-04 09:07:49 UTC
Now lets pay attention to the first patch still.
Comment 8 Marcel de Rooy 2024-10-04 09:17:22 UTC
(In reply to Marcel de Rooy from comment #7)
> Now lets pay attention to the first patch still.

Actually we need to remove that redirect now. We could add some missing error checking around calling AddFile though.
Comment 9 Marcel de Rooy 2024-10-04 09:17:56 UTC
Created attachment 172382 [details] [review]
Bug 37365: (QA follow-up) Add error checking around AddFile call

Test plan:
Add a return; in AddFile to simulate a failing db insert.
Verify that an alert pops up.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Katrin Fischer 2024-10-11 10:00:14 UTC
It looks like this bug is still assigned to Eric, but both patches are from Marcel and only signed by Marcel. What about the first patch that is now obsoleted?

Can we get one extra set of eyes here? Eric - do you agree with the solution?
Comment 11 Marcel de Rooy 2024-10-11 10:06:55 UTC
(In reply to Katrin Fischer from comment #10)
> It looks like this bug is still assigned to Eric, but both patches are from
> Marcel and only signed by Marcel. What about the first patch that is now
> obsoleted?

Correct. We dont need it anymore. Please see above.

> Can we get one extra set of eyes here? Eric - do you agree with the solution?

Agreed. But FQA is the way to forget this one completely. Moving back to SO.
Comment 12 Katrin Fischer 2024-10-11 10:09:48 UTC
It's a way to get the developer's attention to clarify :) SO is perfect thanks.
Comment 13 Katrin Fischer 2024-10-11 10:11:18 UTC
(In reply to Katrin Fischer from comment #12)
> It's a way to get the developer's attention to clarify :) SO is perfect
> thanks.

SO = Signed off
Comment 14 Aleisha Amohia 2024-11-06 03:19:26 UTC
Created attachment 174061 [details] [review]
Bug 37365: (QA follow-up) Add borrowernumber to referrer url

Since we post to add_message, we need to ensure that the
borrowernumber is added to the referrer url. This 'tric' is
already done there for another form.

Test plan:
See original plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 15 Aleisha Amohia 2024-11-06 03:19:29 UTC
Created attachment 174062 [details] [review]
Bug 37365: (QA follow-up) Add error checking around AddFile call

Test plan:
Add a return; in AddFile to simulate a failing db insert.
Verify that an alert pops up.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 16 Katrin Fischer 2024-11-07 16:33:42 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 17 Lucas Gass (lukeg) 2024-12-05 21:49:32 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 18 Fridolin Somers 2024-12-18 10:23:45 UTC
Pushed to 23.11.x for 23.11.11