Bug 20778 - Unable to delete a subscription
Summary: Unable to delete a subscription
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 19777
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-18 12:37 UTC by Alex Arnaud
Modified: 2019-10-14 19:56 UTC (History)
8 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:


Attachments
Bug 20778: Fix subscription deletion. Also remove the first confirmation pop because we have another one. (1.35 KB, patch)
2018-05-18 12:47 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20778: Fix subscription deletion. Also remove the first confirmation pop because we have another one. (1.55 KB, patch)
2018-05-23 14:45 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 20778: (bug 19777 follow-up) Fix subscription deletion (1.33 KB, patch)
2018-06-04 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20778: (bug 19777 follow-up) Fix subscription deletion (1.37 KB, patch)
2018-06-04 15:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20778: (bug 19777 follow-up) Fix subscription deletion (1.44 KB, patch)
2018-06-04 20:05 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2018-05-18 12:37:25 UTC
To reproduce:

  - go to a subscription detail page,
  - click on edit -> delete subscription,
  - confirm the pop,
  => subscription is still here
Comment 1 Alex Arnaud 2018-05-18 12:47:55 UTC
Created attachment 75429 [details] [review]
Bug 20778: Fix subscription deletion. Also remove the first confirmation pop because we have another one.

Test plan:
  - go to a subscription detail page,
  - click on edit -> delete subscription,
  - confirm deletion,
  - check that the subscription is no longer here
Comment 2 Biblibre Sandboxes 2018-05-23 14:45:28 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 3 Biblibre Sandboxes 2018-05-23 14:45:49 UTC
Created attachment 75505 [details] [review]
Bug 20778: Fix subscription deletion. Also remove the first confirmation pop because we have another one.

Test plan:
  - go to a subscription detail page,
  - click on edit -> delete subscription,
  - confirm deletion,
  - check that the subscription is no longer here

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 4 Séverine Queune 2018-05-23 14:47:33 UTC
I never noticed that point before :)
Comment 5 Marcel de Rooy 2018-05-25 09:22:45 UTC
I think we are not adding consistency in this script by removing a confirm from one of the js functions in a row of similar ones.
Should you remove the other confirmation perhaps ?

If you have confirm_deletion as name, I would expect a confirm there too.
Comment 6 Alex Arnaud 2018-06-01 08:59:10 UTC
(In reply to Marcel de Rooy from comment #5)
> I think we are not adding consistency in this script by removing a confirm
> from one of the js functions in a row of similar ones.
> Should you remove the other confirmation perhaps ?
> 
> If you have confirm_deletion as name, I would expect a confirm there too.

Hello Marcel,

Not sure to understand what you are expecting. If i remove the 2 others JS confirmation (for closing and reopening), we have no more confirmation at all. I did it for deleting because there is an other one after. So the JS one is useless.

Other possibility is to remove the one after JS for subscription deletion.

I agree that this BZ is for fixing subscription deleting. Not for removing a useless confirmation. So i can just do what it is expected for
Comment 7 Marcel de Rooy 2018-06-01 09:24:47 UTC
(In reply to Alex Arnaud from comment #6)
> Not sure to understand what you are expecting. If i remove the 2 others JS
> confirmation (for closing and reopening), we have no more confirmation at
> all. I did it for deleting because there is an other one after. So the JS
> one is useless.
> 
> Other possibility is to remove the one after JS for subscription deletion.
> 
> I agree that this BZ is for fixing subscription deleting. Not for removing a
> useless confirmation. So i can just do what it is expected for

Yeah. Consider removing the one after the JS confirm.
Comment 8 Jonathan Druart 2018-06-04 15:13:01 UTC
Caused by
  commit 666a1b276097785c2a7c33e718f26f5512b3d972
  Bug 19777: Move template JavaScript to the footer: Serials, part 3


TT var used in .js
Comment 9 Jonathan Druart 2018-06-04 15:16:10 UTC
Created attachment 75777 [details] [review]
Bug 20778: (bug 19777 follow-up) Fix subscription deletion

Caused by
  commit 666a1b276097785c2a7c33e718f26f5512b3d972
  Bug 19777: Move template JavaScript to the footer: Serials, part 3

TT vars should not be used in .js files

Test plan:
Delete a subscription, confirm you see the confirmation popup and that
the it deletes the subscription correctly after it has been confirmed.
Comment 10 Owen Leonard 2018-06-04 15:22:38 UTC
Created attachment 75778 [details] [review]
Bug 20778: (bug 19777 follow-up) Fix subscription deletion

Caused by
  commit 666a1b276097785c2a7c33e718f26f5512b3d972
  Bug 19777: Move template JavaScript to the footer: Serials, part 3

TT vars should not be used in .js files

Test plan:
Delete a subscription, confirm you see the confirmation popup and that
the it deletes the subscription correctly after it has been confirmed.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Katrin Fischer 2018-06-04 20:05:02 UTC
Created attachment 75793 [details] [review]
Bug 20778: (bug 19777 follow-up) Fix subscription deletion

Caused by
  commit 666a1b276097785c2a7c33e718f26f5512b3d972
  Bug 19777: Move template JavaScript to the footer: Serials, part 3

TT vars should not be used in .js files

Test plan:
Delete a subscription, confirm you see the confirmation popup and that
the it deletes the subscription correctly after it has been confirmed.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Nick Clemens 2018-06-08 13:56:23 UTC
Pushed to master for 18.11, awesome work all!
Comment 13 Martin Renvoize 2018-06-08 14:41:31 UTC
This patch has been pushed to 18.05.x and will be in 18.05.01
Comment 14 Fridolin Somers 2018-06-14 13:22:49 UTC
Depends on Bug 19777 not in 17.11.x