Moodle
  1. Moodle
  2. MDL-6424

Confirmation message when deleting blocks

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.4, 1.6.1, 2.3, 2.4
    • Fix Version/s: 2.3.2
    • Component/s: Blocks
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Add a block on any page.
      2. Delete the block.

      [Test] Check that When you delete the block that it redirects to a page asking you if you really want to delete the block. Cancel / No should return with out deleting the block. While 'yes' should remove the block and return you back to the page you were on.

      Show
      Add a block on any page. Delete the block. [Test] Check that When you delete the block that it redirects to a page asking you if you really want to delete the block. Cancel / No should return with out deleting the block. While 'yes' should remove the block and return you back to the page you were on.
    • Affected Branches:
      MOODLE_15_STABLE, MOODLE_16_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-6424-master
    • Rank:
      1664

      Description

      When deleting html blocks (click "X"/delete in html block), there expected to be a warning "Are you absolutely sure you want to delete this block?" It's same thing when deleting Side bar blocks.

      Now it just delete block without any asking.

      It could be also a good idea to change place of delete icon, now it's too close to edit icon.

        Issue Links

          Activity

          Hide
          Damaris Revell added a comment -

          Two of us have accidentally deleted html blocks within an hour of each other. It could definitely do with having the warning return! Thank you.

          Show
          Damaris Revell added a comment - Two of us have accidentally deleted html blocks within an hour of each other. It could definitely do with having the warning return! Thank you.
          Hide
          Donnie Hedin added a comment -

          Has a confirm box now, as well as moved the delete button to the far right.

          Show
          Donnie Hedin added a comment - Has a confirm box now, as well as moved the delete button to the far right.
          Hide
          Helen Foster added a comment -

          Although this issue may have been fixed at some point, it currently affects Moodle 2.3, as reported by Fernanda in http://moodle.org/mod/forum/discuss.php?d=206151

          Show
          Helen Foster added a comment - Although this issue may have been fixed at some point, it currently affects Moodle 2.3, as reported by Fernanda in http://moodle.org/mod/forum/discuss.php?d=206151
          Hide
          Adrian Greeve added a comment -

          Issue size: M

          Show
          Adrian Greeve added a comment - Issue size: M
          Hide
          David Monllaó added a comment -

          Hi Andrian, all seems ok to me

          Show
          David Monllaó added a comment - Hi Andrian, all seems ok to me
          Hide
          Adrian Greeve added a comment -

          Thanks David for having a look at this for me.
          Submitting for peer review.

          Show
          Adrian Greeve added a comment - Thanks David for having a look at this for me. Submitting for peer review.
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Sorry Adrian,

          I'm sending this back this week. You should not/can not use globals within a the block actions.
          Have a look at what process_url_edit is doing which is also producing Output.
          You need to produce a new moodle_page and use it to generate an output renderer that is related to that page.
          The problem stems from that fact that you are within core_renderer::header when those block actions are being processed and are already deep within the output API and settings should have been finalised.

          A couple of things to consider as well:

          1. You should be using $block->get_title() rather than $block->title.
          2. I think it would be worth testing this the multilang filter enabled and with an HTML block with a multilang title. That is the most complex situation a block title can get itself into.
          3. Using confirm is not a great option. Block editing actions are designed to work on any page that has blocks. That page may have a param confirm already that would cause a conflict here. Best to stick with the bui_ prefix I think (that is there to help try keep it unique).
          4. Don't forget to strip the confirm from the url when you are done with the action: $this->page->ensure_param_not_in_url

          By the way and just out of curiosity did you find whether we originally had this functionality and if so when it was lost?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Sorry Adrian, I'm sending this back this week. You should not/can not use globals within a the block actions. Have a look at what process_url_edit is doing which is also producing Output. You need to produce a new moodle_page and use it to generate an output renderer that is related to that page. The problem stems from that fact that you are within core_renderer::header when those block actions are being processed and are already deep within the output API and settings should have been finalised. A couple of things to consider as well: You should be using $block->get_title() rather than $block->title. I think it would be worth testing this the multilang filter enabled and with an HTML block with a multilang title. That is the most complex situation a block title can get itself into. Using confirm is not a great option. Block editing actions are designed to work on any page that has blocks. That page may have a param confirm already that would cause a conflict here. Best to stick with the bui_ prefix I think (that is there to help try keep it unique). Don't forget to strip the confirm from the url when you are done with the action: $this->page->ensure_param_not_in_url By the way and just out of curiosity did you find whether we originally had this functionality and if so when it was lost? Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Oh never mind about the last curiosity, I just looked at the age of this issue, I thought we already supported this in 1.9 but clearly I was wrong.

          Show
          Sam Hemelryk added a comment - Oh never mind about the last curiosity, I just looked at the age of this issue, I thought we already supported this in 1.9 but clearly I was wrong.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Adrian Greeve added a comment -

          I've made the changes that Sam has suggested.
          I'm not exactly sure about my placement of
          $this->page->ensure_param_not_in_url (delete_form.php line: 80) So if that could get some extra attention I would be very happy.

          Thanks.

          Show
          Adrian Greeve added a comment - I've made the changes that Sam has suggested. I'm not exactly sure about my placement of $this->page->ensure_param_not_in_url (delete_form.php line: 80) So if that could get some extra attention I would be very happy. Thanks.
          Hide
          David Monllaó added a comment -

          Hi Adrian,

          About ensure_param_not_in_url(), as you said the call only seems to be necessary when the param exists, after the delete confirmation and the blocks_delete_instance() execution.

          I'm not sure if it's necessary to create a form to display the confirm box because there are no form elements to display, maybe the OUTPUT->confirm() could be included in a renderer or in process_url_delete(), but following the process_url_edit() way to do it, to overwrite $PAGE and $OUTPUT as Sam proposes.

          About the form, with this patch block plugins has the option to overwrite the default blocks/block_delete_form creating a new block_BLOCKNAME_delete_form; just asking, this kind of additions should be documented in MDocs? Adding to this, it could be useful to have more info in block_delete_form->__construct, maybe type hintings and/or method description

          Show
          David Monllaó added a comment - Hi Adrian, About ensure_param_not_in_url(), as you said the call only seems to be necessary when the param exists, after the delete confirmation and the blocks_delete_instance() execution. I'm not sure if it's necessary to create a form to display the confirm box because there are no form elements to display, maybe the OUTPUT->confirm() could be included in a renderer or in process_url_delete(), but following the process_url_edit() way to do it, to overwrite $PAGE and $OUTPUT as Sam proposes. About the form, with this patch block plugins has the option to overwrite the default blocks/block_delete_form creating a new block_BLOCKNAME_delete_form; just asking, this kind of additions should be documented in MDocs? Adding to this, it could be useful to have more info in block_delete_form->__construct, maybe type hintings and/or method description
          Hide
          Adrian Greeve added a comment -

          Thanks for the comments David.

          • I've moved the ensure_param_not_in_url() function to its appropriate spot.
          • I had a talk to Rajesh and he doesn't see any problem with using an mform to create a yes no confirmation page.
          • I changed a couple of the parameters being passed to the delete_form so as to not pass unused information.

          Submitting for integration review.

          Show
          Adrian Greeve added a comment - Thanks for the comments David. I've moved the ensure_param_not_in_url() function to its appropriate spot. I had a talk to Rajesh and he doesn't see any problem with using an mform to create a yes no confirmation page. I changed a couple of the parameters being passed to the delete_form so as to not pass unused information. Submitting for integration review.
          Hide
          Sam Hemelryk added a comment -

          Hi Adrian,

          I've just been looking at this and I've decided to send it back once more sorry.

          There are a couple of things that I don't like about this solution when I sit down and look at it.

          1. block_delete_form is inheriting from moodleform but is not acting like a form.
          2. blocks don't need to be able to override the delete form like they do for the action form.

          I think I get where you were heading with this but I think you've approached it incorrectly.
          In concept the moodleform library is not supposed to have anything at all to do with page output. It should only be concerning itself with the form it is representing. That is how that library (quickforms+moodleform) is designed and is how it should be used.
          The point of allowing a form to be overridden is so that its specification can alter the definition of the form, implement validation etc. Under your present use it is just a wrapper to output.
          Its also worth noting that confirming the deletion of a block should never need to change, simply confirm yes or no. I can't think of a use case in which a blocks would need to override form and I really don't think it is worth doing.

          What I think needs to happen here is pretty simple.

          1. Copy the code from block_delete_form::definition process_url_delete and do away with delete_form.php.
          2. Clean up that process_url_delete method within that !$confirmdelete condition.
          3. Make sure after any $output->footer() call within that function that exit is called to make sure nothing more can occur.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Adrian, I've just been looking at this and I've decided to send it back once more sorry. There are a couple of things that I don't like about this solution when I sit down and look at it. block_delete_form is inheriting from moodleform but is not acting like a form. blocks don't need to be able to override the delete form like they do for the action form. I think I get where you were heading with this but I think you've approached it incorrectly. In concept the moodleform library is not supposed to have anything at all to do with page output. It should only be concerning itself with the form it is representing. That is how that library (quickforms+moodleform) is designed and is how it should be used. The point of allowing a form to be overridden is so that its specification can alter the definition of the form, implement validation etc. Under your present use it is just a wrapper to output. Its also worth noting that confirming the deletion of a block should never need to change, simply confirm yes or no. I can't think of a use case in which a blocks would need to override form and I really don't think it is worth doing. What I think needs to happen here is pretty simple. Copy the code from block_delete_form::definition process_url_delete and do away with delete_form.php. Clean up that process_url_delete method within that !$confirmdelete condition. Make sure after any $output->footer() call within that function that exit is called to make sure nothing more can occur. Cheers Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Adrian Greeve added a comment -

          Okay,
          What I've done this time around.

          1. Removed the form for deleting the block.
          2. Taken the code from that page and included it into the function in the blocklib.
          3. Tidied up the !$confirmdelete condition.
          4. Added an exit after $OUTPUT->footer() call so that nothing else happens after the form is displayed.

          Heading to peer review.

          Show
          Adrian Greeve added a comment - Okay, What I've done this time around. Removed the form for deleting the block. Taken the code from that page and included it into the function in the blocklib. Tidied up the !$confirmdelete condition. Added an exit after $OUTPUT->footer() call so that nothing else happens after the form is displayed. Heading to peer review.
          Hide
          David Monllaó added a comment -

          Hi Adrian,

          All looks ok for me

          Show
          David Monllaó added a comment - Hi Adrian, All looks ok for me
          Hide
          Adrian Greeve added a comment -

          Thanks David for your time and patience with going through this code and finding things that I missed.

          Third time lucky for integration perhaps.

          Show
          Adrian Greeve added a comment - Thanks David for your time and patience with going through this code and finding things that I missed. Third time lucky for integration perhaps.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame some-body/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame some-body/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Adrian,

          I am happy with the way this looks now thanks.
          Could you please produce branches for the versions you want this to land on and then I'll integrate.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Adrian, I am happy with the way this looks now thanks. Could you please produce branches for the versions you want this to land on and then I'll integrate. Many thanks Sam
          Hide
          Adrian Greeve added a comment -

          Hi Sam,

          I tend to think that it's more of an improvement than a bug fix, so I'm inclined to just leave it in master, but I'm quite willing to create other branches for it if you think it should be back ported.

          Show
          Adrian Greeve added a comment - Hi Sam, I tend to think that it's more of an improvement than a bug fix, so I'm inclined to just leave it in master, but I'm quite willing to create other branches for it if you think it should be back ported.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23 & master), thanks!

          Notes:

          • The $PAGE = ... trick is really, really awful, but.
          • Just guessing if, instead of defaulting to "admin" layout, we couldn't continue using the same, so blocks continue being shown.
          • I've added one extra commit about $PAGE and $OUTPUT missing global declaration.
          • Finally I've backported it to 2.3, because it seems to be a nice safety feature and we are still with .1). I was no intrepid enough to backport it to 22_STABLE.
          • ui_change and docs_required labels added.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23 & master), thanks! Notes: The $PAGE = ... trick is really, really awful, but. Just guessing if, instead of defaulting to "admin" layout, we couldn't continue using the same, so blocks continue being shown. I've added one extra commit about $PAGE and $OUTPUT missing global declaration. Finally I've backported it to 2.3, because it seems to be a nice safety feature and we are still with .1). I was no intrepid enough to backport it to 22_STABLE. ui_change and docs_required labels added. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tested under 23 and master, with teacher and admin, both "yes" and "no". Everything seems to work ok. Passing.

          Show
          Eloy Lafuente (stronk7) added a comment - Tested under 23 and master, with teacher and admin, both "yes" and "no". Everything seems to work ok. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Created MDL-35294 about to bring ajax facilities to the block buttons.

          Show
          Eloy Lafuente (stronk7) added a comment - Created MDL-35294 about to bring ajax facilities to the block buttons.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work.

          These changes have been spread upstream and are already available in the git and cvs repositories.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao
          Hide
          Michael de Raadt added a comment -

          Hurray!

          Show
          Michael de Raadt added a comment - Hurray!
          Hide
          Helen Foster added a comment -

          I'm removing the docs_required label, even though I haven't actually written anything in the docs about this issue, since I don't think it needs documenting. If anyone thinks differently, and can suggest where in the docs some info should be added, please shout!

          Show
          Helen Foster added a comment - I'm removing the docs_required label, even though I haven't actually written anything in the docs about this issue, since I don't think it needs documenting. If anyone thinks differently, and can suggest where in the docs some info should be added, please shout!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Perfect Helen,

          I think it is better to add labels by excess than missing them, and then let the specialists to decide if it's worth the documentation.

          In this case I was thinking that, perhaps, there is some place where the deletion of blocks is documented (with some warning about the deletion happening without confirmation or so) and it now has the new confirmation step. That's the reason I added the ui_change and also the docs_required labels.

          In fact, I'm changing the title of this issue as far as it's global and not only html-block related.

          Of course, if you think there is another better strategy to decide the labeling, let me know, 100%. Else, I'll continue current way.

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Perfect Helen, I think it is better to add labels by excess than missing them, and then let the specialists to decide if it's worth the documentation. In this case I was thinking that, perhaps, there is some place where the deletion of blocks is documented (with some warning about the deletion happening without confirmation or so) and it now has the new confirmation step. That's the reason I added the ui_change and also the docs_required labels. In fact, I'm changing the title of this issue as far as it's global and not only html-block related. Of course, if you think there is another better strategy to decide the labeling, let me know, 100%. Else, I'll continue current way. Thanks and ciao
          Hide
          Mary Cooch added a comment -

          Just joining in here to say with regards to documentation I Very Much agree that "it's better to add labels by excess than missing them"

          Show
          Mary Cooch added a comment - Just joining in here to say with regards to documentation I Very Much agree that "it's better to add labels by excess than missing them"
          Hide
          Helen Foster added a comment -

          I agree too, that it's better to have ui_change and docs_required labels in too many places than not enough.

          Eloy, thanks a lot for adding them everywhere. Please keep it up!

          Show
          Helen Foster added a comment - I agree too, that it's better to have ui_change and docs_required labels in too many places than not enough. Eloy, thanks a lot for adding them everywhere. Please keep it up!

            People

            • Votes:
              16 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: