This bug is for tracking follow-ups missed in the initial review. Please see the "Code review missed" section on the framapad here: https://annuel.framapad.org/p/koha_34478_remaining Additional entries can be added there, and should be crossed out when completed Patches should be attached here This bug can be closed when that section of the pad is empty
Created attachment 162684 [details] [review] Bug 36193: Add more request information on Koha::Middleware::CSRF output Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I'm not sure if I fully understand. It looks like bug 34478 accidentally broke the Oauth/OIDC SSO, so I've fixed it on bug 36219. Should I have added that patch here instead?
(In reply to David Cook from comment #2) > I'm not sure if I fully understand. It looks like bug 34478 accidentally > broke the Oauth/OIDC SSO, so I've fixed it on bug 36219. Should I have added > that patch here instead? The idea was simply to reduce the number of bugs filed, for what will likely be similar patches - it is perfectly okay that you filed a new bug, and it is for a distinct issue, and you linked it to the others. This is going to be a bit messy, so we just do our best to keep things clear and simple, and I think you did.
Created attachment 162733 [details] [review] Bug 36193: Fix acq receiving This patch addresses the CSRF error when receiving in acquisitions. To test: 1. Have at least one order to receive 2. Follow the steps to receive them 3. Have the logs open: $ ktd --shell k$ tail -f /var/log/koha/kohadev/*.log 4. Click to confirm receipt => FAIL: An error modal is displayed => FAIL: There's an error about missing CSRF token in POST 5. Apply this patch 6. Reload everything: k$ restart_all 7. Repeat 1-4 => SUCCESS: Receipt works :-D => SUCCESS: No error log 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162734 [details] [review] Bug 36193: cud- treatment for Acq order receipt cancelling Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162749 [details] [review] Bug 36193: cud- treatment for cash_registers.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162750 [details] [review] Bug 36193: cud- treatment for credit_types.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162751 [details] [review] Bug 36193: cud- treatment for dedit_types.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162752 [details] [review] Bug 36193: cud- treatment for reset_confirmed on mappings.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162755 [details] [review] Bug 36193: cud- treatment for restrictions.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162769 [details] [review] Bug 36193: Add more request information on Koha::Middleware::CSRF output Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162770 [details] [review] Bug 36193: Fix acq receiving This patch addresses the CSRF error when receiving in acquisitions. To test: 1. Have at least one order to receive 2. Follow the steps to receive them 3. Have the logs open: $ ktd --shell k$ tail -f /var/log/koha/kohadev/*.log 4. Click to confirm receipt => FAIL: An error modal is displayed => FAIL: There's an error about missing CSRF token in POST 5. Apply this patch 6. Reload everything: k$ restart_all 7. Repeat 1-4 => SUCCESS: Receipt works :-D => SUCCESS: No error log 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162771 [details] [review] Bug 36193: cud- treatment for Acq order receipt cancelling Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162772 [details] [review] Bug 36193: cud- treatment for cash_registers.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162773 [details] [review] Bug 36193: cud- treatment for credit_types.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162774 [details] [review] Bug 36193: cud- treatment for dedit_types.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162775 [details] [review] Bug 36193: cud- treatment for reset_confirmed on mappings.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162776 [details] [review] Bug 36193: cud- treatment for restrictions.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162777 [details] [review] Bug 36193: (follow-up) Use buttons and don't nest forms When changing action links into forms, we should use a submit button rather than JS to submit Receipt cancelling needed a form embedded in the page as we cannot nest forms Fixed searching on invoice.pl Fixed resetting mappings (original button did not need cud-) Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162784 [details] [review] Bug 36193: (follow-up) Use buttons and don't nest forms When changing action links into forms, we should use a submit button rather than JS to submit Receipt cancelling needed a form embedded in the page as we cannot nest forms Fixed searching on invoice.pl Fixed resetting mappings (original button did not need cud-) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162830 [details] [review] Bug 36193: Make restore icon consistent in cash registers Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162831 [details] [review] Bug 36193: cud- treatment for catalogue/stockrotation.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 162837 [details] [review] Bug 36193: cud- treatment for tools/stockrotation.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed all these to master. Handling is a little big finicky. I'd prefer separate bugs from now on if that's OK.
Pushed for 24.05! Well done everyone, thank you!
Depends on Bug 34478 no ?
(In reply to Fridolin Somers from comment #26) > Depends on Bug 34478 no ? Yep. I've just added it as a dependency.
Depends on Bug 34478 not in 23.11.x