Bug 36243 - ILL "Edit request" action is broken
Summary: ILL "Edit request" action is broken
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Pedro Amorim
QA Contact: Martin Renvoize
URL:
Keywords: regression
Depends on: 34478
Blocks: 36273 36249
  Show dependency treegraph
 
Reported: 2024-03-06 12:35 UTC by Pedro Amorim
Modified: 2024-04-17 08:46 UTC (History)
7 users (show)

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


Attachments
Bug 36243: Remove cud- from op (794 bytes, patch)
2024-03-06 13:06 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36243: Update edit_action to cud-edit_action (3.18 KB, patch)
2024-03-06 13:06 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36243: Remove cud- from op (860 bytes, patch)
2024-03-07 09:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36243: Update edit_action to cud-edit_action (3.24 KB, patch)
2024-03-07 09:43 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-03-06 12:35:26 UTC

    
Comment 1 Pedro Amorim 2024-03-06 13:06:17 UTC
Created attachment 162832 [details] [review]
Bug 36243: Remove cud- from op

'cud-' needs to be removed here because it's showing the form, not handling its POST submission
Comment 2 Pedro Amorim 2024-03-06 13:06:19 UTC
Created attachment 162833 [details] [review]
Bug 36243: Update edit_action to cud-edit_action

1) Install FreeForm, enable ILL module, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) You'll have to switch the FreeForm branch to the one compatible with this work, i.e.:
cd /kohadevbox/koha/Koha/Illbackends/FreeForm
git checkout b_36243
3) Restart plack:
koha-plack --restart kohadev
4) Create a new FreeForm request, visit:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=FreeForm
5) Set 'type' enter '42' in cardnumber and pick a library. Click 'Create'.
6) Edit that request, click "manage request" or if #1 visit:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl?op=illview&illrequest_id=1
7) Click "Edit request". Click "Submit". Notice it errors "No CSRF token passed for POST"
9) Apply patches. Restart plack.
10) Repeat 7)
Comment 3 Martin Renvoize 2024-03-07 09:43:44 UTC
Created attachment 162869 [details] [review]
Bug 36243: Remove cud- from op

'cud-' needs to be removed here because it's showing the form, not handling its POST submission

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2024-03-07 09:43:47 UTC
Created attachment 162870 [details] [review]
Bug 36243: Update edit_action to cud-edit_action

1) Install FreeForm, enable ILL module, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) You'll have to switch the FreeForm branch to the one compatible with this work, i.e.:
cd /kohadevbox/koha/Koha/Illbackends/FreeForm
git checkout b_36243
3) Restart plack:
koha-plack --restart kohadev
4) Create a new FreeForm request, visit:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=FreeForm
5) Set 'type' enter '42' in cardnumber and pick a library. Click 'Create'.
6) Edit that request, click "manage request" or if #1 visit:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl?op=illview&illrequest_id=1
7) Click "Edit request". Click "Submit". Notice it errors "No CSRF token passed for POST"
9) Apply patches. Restart plack.
10) Repeat 7)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2024-03-07 09:44:44 UTC
Follow-up fix for regression introduced.

All working as designed now and code is reasonable to follow.

Going straight for a QA and passing.
Comment 6 Tomás Cohen Arazi 2024-03-07 12:17:21 UTC
Can we add the _form suffix to the op parameter?
Comment 7 Pedro Amorim 2024-03-07 12:28:35 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> Can we add the _form suffix to the op parameter?

Yes, we can. And we should. Though it'll require a rewrite on how $op is handled in ill-requests.pl and ill-requests.tt.
The focus of this bug is to fix the regression.

ill-requests.pl handles an edit_action "form" and an edit_action "save" in the same $op eq 'edit_action', which means these would have to be decoupled to cud-edit_action and edit_action_form.
The same would have to be made for cud-create, cud-generic_confirm, cud-delete (?) and possibly others.
We'd also have to account for the fact that existing backends may have not yet adopted these new $op names (See get_op_param_deprecation in Illrequest.pm)

In short, my suggestion would be to push this refactor/clean-up work to its own bug, but I'm obviously happy to discuss if you or others disagree.
Comment 8 Tomás Cohen Arazi 2024-03-07 12:30:03 UTC
(In reply to Pedro Amorim from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > Can we add the _form suffix to the op parameter?
> 
> Yes, we can. And we should. Though it'll require a rewrite on how $op is
> handled in ill-requests.pl and ill-requests.tt.
> The focus of this bug is to fix the regression.

FAIR. Put some FIXME inline so there are no oversights on this later.
Comment 9 Pedro Amorim 2024-03-07 12:48:43 UTC
I've added bug 36273. Please take a look to see if what I wrote there makes sense.
Comment 10 Katrin Fischer 2024-03-07 15:38:22 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 11 Fridolin Somers 2024-04-17 08:46:08 UTC
Depends on Bug 34478 not in 23.11.x