Bug 37365 - Bad redirect when adding a patron message from members/files.pl
Summary: Bad redirect when adding a patron message from members/files.pl
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Eric Garcia
QA Contact: Aleisha Amohia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-07-15 15:27 UTC by Lucas Gass (lukeg)
Modified: 2024-11-07 16:33 UTC (History)
6 users (show)

See Also:
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
Circulation function:

Attachments
Bug 37365: Fixed redirect when adding a patron message from members/files.pl (1.12 KB, patch)
2024-07-15 20:29 UTC, Eric Garcia 		Details | Diff | Splinter Review
Bug 37365: Fixed redirect when adding a patron message from members/files.pl (1.17 KB, patch)
2024-07-16 03:53 UTC, David Nind 		Details | Diff | Splinter Review
Bug 37365: (QA follow-up) Add borrowernumber to referrer url (1.05 KB, patch)
2024-10-04 09:07 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 37365: (QA follow-up) Add error checking around AddFile call (2.33 KB, patch)
2024-10-04 09:17 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 37365: (QA follow-up) Add borrowernumber to referrer url (1.07 KB, patch)
2024-11-06 03:19 UTC, Aleisha Amohia 		Details | Diff | Splinter Review
Bug 37365: (QA follow-up) Add error checking around AddFile call (2.35 KB, patch)
2024-11-06 03:19 UTC, Aleisha Amohia 		Details | Diff | Splinter Review
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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!