|
Description
Alex Arnaud
2018-05-18 12:37:25 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 Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr> 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> I never noticed that point before :) 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. (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 (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. Caused by commit 666a1b276097785c2a7c33e718f26f5512b3d972 Bug 19777: Move template JavaScript to the footer: Serials, part 3 TT var used in .js 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. 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> 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> Pushed to master for 18.11, awesome work all! This patch has been pushed to 18.05.x and will be in 18.05.01 Depends on Bug 19777 not in 17.11.x |