First vendor of the page has a confirmation alert, but the answer is ignored! I guess it's coming from
Can we confirm if this is still the case?
Yes, still valid.
Created attachment 168668 [details] [review] Bug 31921: Add confirmation modal when deleting a vendor This patch adds a new modal for confirming vendor deletion. It fixes the previous functionality, showing a confirmation modal for each vendor and appropriately handling the response. To test: 1) Visit Acquisitions home. Under where it says 'Manage orders', click the search bar to bring up the list of vendors. 2) Add a few vendors by clicking 'New' and 'Vendor'. After this, go back to the vendor search so that all the vendors are showing. 3) For each vendor, click on 'Delete vendor' and ensure the modal shows properly. 4) Attempt to delete one of the vendors. Go back to the vendor search and ensure they are deleted correctly. 5) Click on a vendor's name to go their supplier page. Make sure modal works properly and attempt to delete a vendor from here.
Created attachment 168674 [details] [review] Bug 31921: Add confirmation modal when deleting a vendor This patch adds a new modal for confirming vendor deletion. It fixes the previous functionality, showing a confirmation modal for each vendor and appropriately handling the response. To test: 1) Visit Acquisitions home. Under where it says 'Manage orders', click the search bar to bring up the list of vendors. 2) Add a few vendors by clicking 'New' and 'Vendor'. After this, go back to the vendor search so that all the vendors are showing. 3) For each vendor, click on 'Delete vendor' and ensure the modal shows properly. 4) Attempt to delete one of the vendors. Go back to the vendor search and ensure they are deleted correctly. 5) Click on a vendor's name to go their supplier page. Make sure modal works properly and attempt to delete a vendor from here. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
It will only works from supplier.pl and booksellers.pl For instance it does not work from aqcontract.pl I think you should include the modal code from acquisitions-toolbar.inc to avoid this kind of mistakes.
Maybe this should be a new bug since there wasn't already a delete button there? If you disagree I could just add it here
Created attachment 168749 [details] [review] Bug 31921: Add confirmation modal to contracts and uncertain prices To test: 1) Have 2 deletable vendors 2) Visit the first vendor's "Contracts" tab 3) Attempt to delete from here, modal shows and deltion is successful 4) Visit the second vendor's "Uncertain prices" tab. 5) Attempt to delete from here, modal shows and deletion is successful
(In reply to Jonathan Druart from comment #5) > I think you should include the modal code from acquisitions-toolbar.inc to > avoid this kind of mistakes. Did you try this?
(In reply to Jonathan Druart from comment #8) > (In reply to Jonathan Druart from comment #5) > > I think you should include the modal code from acquisitions-toolbar.inc to > > avoid this kind of mistakes. > > Did you try this? There is no modal code in acquisitions-toolbar.inc so I'm not sure what you mean. Unless you are saying to include the new 'modals/delete_vendor.inc' inside of the acquisitions toolbar? In which case, no I did not.
Created attachment 168832 [details] [review] Bug 31921: Include the modal code from acquisitions-toolbar.inc
This patch is what I meant. It works on all pages where the modal exists and avoid the code duplication.
(In reply to Jonathan Druart from comment #11) > This patch is what I meant. It works on all pages where the modal exists and > avoid the code duplication. Got it, that makes sense. Thanks for the help
The patches still apply. I am getting this error when I go to delete a vendor: 1. Go to Acquisitions > Manage orders > Search (/cgi-bin/koha/acqui/booksellers.pl?supplier=) 2. Add some vendors (+New vendor) 3. Click on "Delete vendor" for any vendor where this option appears. 4. From the pop-up window ("Are you sure you want to delete this vendor"), click "Yes". 5. Error message: Can't call method "baskets" on an undefined value at /kohadevbox/koha/acqui/supplier.pl line 94.... I get the same error from any of the pages that the delete vendor action is available.
Created attachment 170004 [details] [review] Bug 31921: Add confirmation modal when deleting a vendor This patch adds a new modal for confirming vendor deletion. It fixes the previous functionality, showing a confirmation modal for each vendor and appropriately handling the response. To test: 1) Visit Acquisitions home. Under where it says 'Manage orders', click the search bar to bring up the list of vendors. 2) Add a few vendors by clicking 'New' and 'Vendor'. After this, go back to the vendor search so that all the vendors are showing. 3) For each vendor, click on 'Delete vendor' and ensure the modal shows properly. 4) Attempt to delete one of the vendors. Go back to the vendor search and ensure they are deleted correctly. 5) Click on a vendor's name to go their supplier page. Make sure modal works properly and attempt to delete a vendor from here. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 170005 [details] [review] Bug 31921: Include the modal code from acquisitions-toolbar.inc Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to David Nind from comment #13) > The patches still apply. > > I am getting this error when I go to delete a vendor: > > 1. Go to Acquisitions > Manage orders > Search > (/cgi-bin/koha/acqui/booksellers.pl?supplier=) > 2. Add some vendors (+New vendor) > 3. Click on "Delete vendor" for any vendor where this option appears. > 4. From the pop-up window ("Are you sure you want to delete this vendor"), > click "Yes". > 5. Error message: > Can't call method "baskets" on an undefined value at > /kohadevbox/koha/acqui/supplier.pl line 94.... > > I get the same error from any of the pages that the delete vendor action is > available. I can't recreate this David, added my SO line
Created attachment 170538 [details] [review] Bug 31921: Add confirmation modal when deleting a vendor This patch adds a new modal for confirming vendor deletion. It fixes the previous functionality, showing a confirmation modal for each vendor and appropriately handling the response. To test: 1) Visit Acquisitions home. Under where it says 'Manage orders', click the search bar to bring up the list of vendors. 2) Add a few vendors by clicking 'New' and 'Vendor'. After this, go back to the vendor search so that all the vendors are showing. 3) For each vendor, click on 'Delete vendor' and ensure the modal shows properly. 4) Attempt to delete one of the vendors. Go back to the vendor search and ensure they are deleted correctly. 5) Click on a vendor's name to go their supplier page. Make sure modal works properly and attempt to delete a vendor from here. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 170539 [details] [review] Bug 31921: Include the modal code from acquisitions-toolbar.inc Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice consistency improvement, thanks. All works as expected Passing QA
Pushed for 24.11! Well done everyone, thank you!
(In reply to Nick Clemens (kidclamp) from comment #16) > (In reply to David Nind from comment #13) ... > > I can't recreate this David, added my SO line I couldn't recreate the error either (after checking out main before the Bootstrap 5 update). Apologies taking so long to respond to your comment! However, after the Bootstrap 5 update, you can no longer delete vendors. I've created bug 37739 for this - I'm assuming it is related to the Bootstrap 5 update, rather than anything else.
Pushed at almost the same time of the bootstrap 5 update, but should have been pushed after with the code adjusted. See bug 37739.
Backported to 24.05.x for upcoming 24.05.04
Not easy to backport on 23.11.x that has not "cud-" I'd say skip for oldstable