Bug 36735 - Cannot revert the waiting status of a hold
Summary: Cannot revert the waiting status of a hold
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Emily Lamancusa
QA Contact: Testopia
URL:
Keywords: RM_priority
Depends on: 36246
Blocks: 36192
  Show dependency treegraph
 
Reported: 2024-04-30 15:40 UTC by Emily Lamancusa
Modified: 2024-05-22 12:47 UTC (History)
4 users (show)

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


Attachments
Bug 36735: Fix revert hold button (3.39 KB, patch)
2024-04-30 19:57 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 36735: Fix revert hold button (3.44 KB, patch)
2024-04-30 21:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 36735: (follow-up) Fix TT filters and add a comment (2.89 KB, patch)
2024-05-08 13:40 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 36735: Fix revert hold button (3.50 KB, patch)
2024-05-10 12:56 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36735: (follow-up) Fix TT filters and add a comment (2.95 KB, patch)
2024-05-10 12:56 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa 2024-04-30 15:40:43 UTC
To reproduce:
1. Place a hold on a biblio record
2. Check an item in and confirm the hold
3. On the holds tab for the biblio record, click the "Revert waiting status" button for that hold
--> The page reloads but the hold is still waiting

Looking at the code for the button, I'm pretty sure it just needs to be updated for CSRF. I think this is one of the use cases bug 36246 is for, if I'm understanding it correctly?
Comment 1 Emily Lamancusa 2024-04-30 19:57:19 UTC
Created attachment 165930 [details] [review]
Bug 36735: Fix revert hold button

Update the revert hold button to use the new include for submitting
forms from link data with a POST request

To test:
1. Place a hold on a biblio record
2. Check an item in to fill the hold
3. On the holds tab for the biblio record, click the "Revert waiting
   status" button for that hold
--> The page reloads but the hold is still waiting
4. Apply patchset
5. Click to another page and then return to the holds tab (we don't want
   to refresh the page and resend the request)
6. Click the "Revert waiting status" button for that hold
--> The hold should be reverted to pending status
Comment 2 Emily Lamancusa 2024-04-30 20:49:47 UTC
Submitting for feedback - this seems to work, but there are a couple of things I'm not sure about:
- I kept the uri TT filters from the existing code. Should those be changed to html instead since they're not being passed in the URL anymore?

- I feel like there has to be a better way to handle the dependency between the .inc file and the JS file, but including the JS asset directly in the .inc file breaks things (I think because the include is added in the middle of the page in request.tt, which violates the coding guideline of including JS assets at the end?)
Comment 3 David Nind 2024-04-30 21:14:36 UTC
Created attachment 165931 [details] [review]
Bug 36735: Fix revert hold button

Update the revert hold button to use the new include for submitting
forms from link data with a POST request

To test:
1. Place a hold on a biblio record
2. Check an item in to fill the hold
3. On the holds tab for the biblio record, click the "Revert waiting
   status" button for that hold
--> The page reloads but the hold is still waiting
4. Apply patchset
5. Click to another page and then return to the holds tab (we don't want
   to refresh the page and resend the request)
6. Click the "Revert waiting status" button for that hold
--> The hold should be reverted to pending status

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2024-04-30 21:16:22 UTC
(In reply to Emily Lamancusa from comment #2)
> Submitting for feedback - this seems to work, but there are a couple of
> things I'm not sure about:
> - I kept the uri TT filters from the existing code. Should those be changed
> to html instead since they're not being passed in the URL anymore?
> 
> - I feel like there has to be a better way to handle the dependency between
> the .inc file and the JS file, but including the JS asset directly in the
> .inc file breaks things (I think because the include is added in the middle
> of the page in request.tt, which violates the coding guideline of including
> JS assets at the end?)

I don't have any feedback on these points, as I don't know enough to knowledgeably comment 8-)

However, I have signed off the bug, as I can replicate the issue and it is fixed as per the test plan.
Comment 5 Nick Clemens (kidclamp) 2024-05-03 11:26:45 UTC
(In reply to Emily Lamancusa from comment #2)
> Submitting for feedback - this seems to work, but there are a couple of
> things I'm not sure about:
> - I kept the uri TT filters from the existing code. Should those be changed
> to html instead since they're not being passed in the URL anymore?

Correct, they should all be html filters now. In the end we are building a form and submitting, so they won't be in a URL


> - I feel like there has to be a better way to handle the dependency between
> the .inc file and the JS file, but including the JS asset directly in the
> .inc file breaks things (I think because the include is added in the middle
> of the page in request.tt, which violates the coding guideline of including
> JS assets at the end?)

I think I understand the issue, it seems odd to have the JS asset in the script and not in the include directly? In this case, the include is only used on this page, but perhaps adding a comment to the include to specify that it needs the asset would be helpful?
Comment 6 Emily Lamancusa 2024-05-08 13:38:27 UTC
(In reply to Nick Clemens (kidclamp) from comment #5)

Thanks for the feedback!

> Correct, they should all be html filters now. In the end we are building a
> form and submitting, so they won't be in a URL

In retrospect this was a dumb question. ^^" Fixing...


> I think I understand the issue, it seems odd to have the JS asset in the
> script and not in the include directly? In this case, the include is only
> used on this page, but perhaps adding a comment to the include to specify
> that it needs the asset would be helpful?

Basically, yes. Thanks, just wanted another pair of eyes in case there was a better solution that I wasn't finding.

Keeping this as signed-off and attaching a follow-up since both of the above are trivial changes.
Comment 7 Emily Lamancusa 2024-05-08 13:40:31 UTC
Created attachment 166418 [details] [review]
Bug 36735: (follow-up) Fix TT filters and add a comment

holds_table.inc depends on form-submit.js, but importing the asset
directly breaks the script. Add a comment stating that any script that
imports holds_table.inc must import form-submit.js too.

Also change uri filters to html
Comment 8 Nick Clemens (kidclamp) 2024-05-10 12:56:40 UTC
Created attachment 166552 [details] [review]
Bug 36735: Fix revert hold button

Update the revert hold button to use the new include for submitting
forms from link data with a POST request

To test:
1. Place a hold on a biblio record
2. Check an item in to fill the hold
3. On the holds tab for the biblio record, click the "Revert waiting
   status" button for that hold
--> The page reloads but the hold is still waiting
4. Apply patchset
5. Click to another page and then return to the holds tab (we don't want
   to refresh the page and resend the request)
6. Click the "Revert waiting status" button for that hold
--> The hold should be reverted to pending status

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens (kidclamp) 2024-05-10 12:56:42 UTC
Created attachment 166553 [details] [review]
Bug 36735: (follow-up) Fix TT filters and add a comment

holds_table.inc depends on form-submit.js, but importing the asset
directly breaks the script. Add a comment stating that any script that
imports holds_table.inc must import form-submit.js too.

Also change uri filters to html

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Katrin Fischer 2024-05-10 15:47:08 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 11 Fridolin Somers 2024-05-22 12:47:32 UTC
Depends on Bug 36246 not in 23.11.x