Bug 28064 - Transits are not created at check in despite user responding 'Yes, print slip' to the prompt
Summary: Transits are not created at check in despite user responding 'Yes, print slip...
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
: 28176 (view as bug list)
Depends on: 18372 24085
Blocks: 28148
  Show dependency treegraph
 
Reported: 2021-03-31 09:58 UTC by Martin Renvoize
Modified: 2021-05-24 18:30 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12,19.11.18


Attachments
Bug 28064: Add dotransfer field to slip print submission (1.76 KB, patch)
2021-03-31 12:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28064: (follow-up) Do not re-trigger transfer if it already exists (2.43 KB, patch)
2021-03-31 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28064: Add dotransfer field to slip print submission (1.83 KB, patch)
2021-03-31 16:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 28064: (follow-up) Do not re-trigger transfer if it already exists (2.50 KB, patch)
2021-03-31 16:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 28064: Add dotransfer field to slip print submission (1.88 KB, patch)
2021-04-12 01:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28064: (follow-up) Do not re-trigger transfer if it already exists (2.55 KB, patch)
2021-04-12 01:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28064: Add dotransfer field to slip print submission (1.94 KB, patch)
2021-04-14 15:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28064: (QA follow-up) Remove value from button (2.11 KB, patch)
2021-04-22 10:46 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 Martin Renvoize 2021-03-31 09:58:25 UTC
+++ This bug was initially created as a clone of Bug #18372 +++

When checking in an item from another library the user is prompted whether the item should go into transit if they respond "Yes, print slip" no transit is created and the item status is shown as Available.

This bug appears to be a resurgence of bug 18372 since we pushed bug 24085.
Comment 1 Martin Renvoize 2021-03-31 10:10:43 UTC
Test plan

* Set AutomaticItemReturn system preference is set to "Don't"
* Check in an item that belongs to another library, a dialog will ask you
if you want to transfer.
* Click 'Yes, print slip'
* Look at the item record and note the status is 'Available', it should be 'In transit to...'
Comment 2 Martin Renvoize 2021-03-31 12:10:07 UTC
Created attachment 119044 [details] [review]
Bug 28064: Add dotransfer field to slip print submission

By changing the code to only do a javascript triggered submission from
the from button we lose the original buttons name and value elements
upon submission.

This patch checks for those fields in the JS capture and triggers the
addition of a new hidden form field to contain the dotransfer data.

Test plan
1/ Set AutomaticItemReturn system preference is set to "Don't"
2/ Check in an item that belongs to another library, a dialog will ask
   you if you want to transfer.
3/ Click 'Yes, print slip'
4/ Look at the item record and note the status is 'Available'.
5/ Apply patch
6/ Follow steps 2 - 4
7/ Note the status is now 'In transit to...'
8/ Signoff
Comment 3 Martin Renvoize 2021-03-31 12:22:43 UTC
Created attachment 119045 [details] [review]
Bug 28064: (follow-up) Do not re-trigger transfer if it already exists

For some unknown reason the 'Print slip' button for when a transfer has
already been triggered also had the 'dotransfer' name attribute. It
won't have been getting passed to the form on submission due to the bug
with JS submissions and that this particular button was always a button
and not a submit in the past.  However, it does need removing to prevent
the re-triggering now.
Comment 4 ByWater Sandboxes 2021-03-31 16:10:10 UTC
Created attachment 119055 [details] [review]
Bug 28064: Add dotransfer field to slip print submission

By changing the code to only do a javascript triggered submission from
the from button we lose the original buttons name and value elements
upon submission.

This patch checks for those fields in the JS capture and triggers the
addition of a new hidden form field to contain the dotransfer data.

Test plan
1/ Set AutomaticItemReturn system preference is set to "Don't"
2/ Check in an item that belongs to another library, a dialog will ask
   you if you want to transfer.
3/ Click 'Yes, print slip'
4/ Look at the item record and note the status is 'Available'.
5/ Apply patch
6/ Follow steps 2 - 4
7/ Note the status is now 'In transit to...'
8/ Signoff

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 5 ByWater Sandboxes 2021-03-31 16:10:13 UTC
Created attachment 119056 [details] [review]
Bug 28064: (follow-up) Do not re-trigger transfer if it already exists

For some unknown reason the 'Print slip' button for when a transfer has
already been triggered also had the 'dotransfer' name attribute. It
won't have been getting passed to the form on submission due to the bug
with JS submissions and that this particular button was always a button
and not a submit in the past.  However, it does need removing to prevent
the re-triggering now.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 6 Sally 2021-03-31 16:10:30 UTC
Works as described.
Comment 7 Victor Grousset/tuxayo 2021-04-12 01:33:35 UTC
Created attachment 119440 [details] [review]
Bug 28064: Add dotransfer field to slip print submission

By changing the code to only do a javascript triggered submission from
the from button we lose the original buttons name and value elements
upon submission.

This patch checks for those fields in the JS capture and triggers the
addition of a new hidden form field to contain the dotransfer data.

Test plan
1/ Set AutomaticItemReturn system preference is set to "Don't"
2/ Check in an item that belongs to another library, a dialog will ask
   you if you want to transfer.
3/ Click 'Yes, print slip'
4/ Look at the item record and note the status is 'Available'.
5/ Apply patch
6/ Follow steps 2 - 4
7/ Note the status is now 'In transit to...'
8/ Signoff

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 8 Victor Grousset/tuxayo 2021-04-12 01:33:40 UTC
Created attachment 119441 [details] [review]
Bug 28064: (follow-up) Do not re-trigger transfer if it already exists

For some unknown reason the 'Print slip' button for when a transfer has
already been triggered also had the 'dotransfer' name attribute. It
won't have been getting passed to the form on submission due to the bug
with JS submissions and that this particular button was always a button
and not a submit in the past.  However, it does need removing to prevent
the re-triggering now.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2021-04-12 01:37:39 UTC
Here is another signoff in the hope of helping.
Comment 10 Victor Grousset/tuxayo 2021-04-12 01:43:14 UTC
Hum, actually I don't know how to test the second patch. «However, it does need removing to prevent the re-triggering now.»

Didn't found how to do a re-triggering when not having the second patch.
Comment 11 Nick Clemens 2021-04-14 15:04:23 UTC
Created attachment 119585 [details] [review]
Bug 28064: Add dotransfer field to slip print submission

By changing the code to only do a javascript triggered submission from
the from button we lose the original buttons name and value elements
upon submission.

This patch checks for those fields in the JS capture and triggers the
addition of a new hidden form field to contain the dotransfer data.

Test plan
1/ Set AutomaticItemReturn system preference is set to "Don't"
2/ Check in an item that belongs to another library, a dialog will ask
   you if you want to transfer.
3/ Click 'Yes, print slip'
4/ Look at the item record and note the status is 'Available'.
5/ Apply patch
6/ Follow steps 2 - 4
7/ Note the status is now 'In transit to...'
8/ Signoff

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2021-04-14 15:05:30 UTC
(In reply to Victor Grousset/tuxayo from comment #10)
> Hum, actually I don't know how to test the second patch. «However, it does
> need removing to prevent the re-triggering now.»
> 
> Didn't found how to do a re-triggering when not having the second patch.

I think Victor is right, the second patch touches code that isn't hit when we have an existing transfer - we hit the 'WrongTransfer' portion instead - dropping that and passing QA
Comment 13 Martin Renvoize 2021-04-14 15:12:59 UTC
Thanks Nick.. you guys were both right.. I hadn't taken a broad enough view of the code with that intuitive follow-up.. happy it's been dropped :)
Comment 14 Jonathan Druart 2021-04-16 10:01:32 UTC
The switch from input to button has been done on
  commit 10a9a1e147699bdb1fd2b58ab93fe87cb82074cf
  Bug 15821: Use Font Awesome icons in confirmation dialogs - Circulation

Does it mean it's broken for 5 years?

Additional question, shouldn't we remove the value="Yes" attribute from the buttons, to avoid confusion in the future?
Comment 15 Kyle M Hall 2021-04-20 13:27:00 UTC
*** Bug 28176 has been marked as a duplicate of this bug. ***
Comment 16 Jonathan Druart 2021-04-21 07:58:09 UTC
Waiting for answer on comment 14.
Comment 17 Martin Renvoize 2021-04-22 09:15:16 UTC
(In reply to Jonathan Druart from comment #14)
> The switch from input to button has been done on
>   commit 10a9a1e147699bdb1fd2b58ab93fe87cb82074cf
>   Bug 15821: Use Font Awesome icons in confirmation dialogs - Circulation
> 
> Does it mean it's broken for 5 years?
> 
> Additional question, shouldn't we remove the value="Yes" attribute from the
> buttons, to avoid confusion in the future?

A git bisect traced it back to bug 24085 for me... so not 5 years.. but certainly since 19.11.02.

Hmm, removing the 'value=Yes' is perhaps a good idea... I tried to be as noninvasive as I could as this template is plain confusing
Comment 18 Martin Renvoize 2021-04-22 10:46:02 UTC
Created attachment 119993 [details] [review]
Bug 28064: (QA follow-up) Remove value from button

To clarify that the value is not used on buttons that are not submit
type we remove the value entirely.
Comment 19 Jonathan Druart 2021-04-22 12:40:00 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 20 Fridolin Somers 2021-04-29 14:30:11 UTC
Pushed to 20.11.x for 20.11.06
Comment 21 Andrew Fuerste-Henry 2021-05-24 16:51:02 UTC
Pushed to 20.05.x for 20.05.12
Comment 22 Victor Grousset/tuxayo 2021-05-24 18:30:22 UTC
Backported: Pushed to 19.11.x branch for 19.11.18