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?
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
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?)
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>
(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.
(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?
(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.
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
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>
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>
Pushed for 24.05! Well done everyone, thank you!
Depends on Bug 36246 not in 23.11.x