Moodle
  1. Moodle
  2. MDL-28195

confirm_action should allow the button labels to be customised

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1.1
    • Component/s: Libraries
    • Labels:
      None
    • Testing Instructions:
      Hide

      1. Check a place that currently uses confirm_action. For example clearing all assessments in a workshop. Make sure it sill works.

      2. Once MDL-26165 is integrated, check that the quiz 'are you sure you want to start/submit this attempt' dialogues have customised button labels.

      Show
      1. Check a place that currently uses confirm_action. For example clearing all assessments in a workshop. Make sure it sill works. 2. Once MDL-26165 is integrated, check that the quiz 'are you sure you want to start/submit this attempt' dialogues have customised button labels.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      17911

      Description

      The confirm_action lib/outputactions.php is hard-coded to use button captions Cancel and Yes. This is not always appropriate. The API should be extended to allow custom labels. (For example, MDL-26165 is hard to fix at the moment.)

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Sam, please could you take a look at this and let me know if you think it should go into 2.1 and/or master.

          If it is conceptually OK, is there anything in the code you would like changed before this is submitted for integration?

          Show
          Tim Hunt added a comment - Sam, please could you take a look at this and let me know if you think it should go into 2.1 and/or master. If it is conceptually OK, is there anything in the code you would like changed before this is submitted for integration?
          Hide
          Sam Hemelryk added a comment -

          Hi Tim,

          You code changes look good

          There was one thing I was looking at though, the way in which the cancel and yes strings are made available.
          The strings are only there because other code has requested them. After a bit of debugging I found that they will be available on all pages because the filepicker module is included for ALL pages (which includes all of its sub modules and strings etc).

          Certainly I think there are two bugs here although unrelated to your changes:
          1. Filepicker module should be included only where it is needed - not on every page. This presently leads to loading and piles of strings, yui3 modules, and yui2 modules for every page.
          2. confirm_action should be requiring its own strings in JS or alternatively sending them after making a get_string call.

          And in fact there would be two more tasks to create issues for:
          3. Uses of M.str.

          {component}

          .

          {identifier}

          should be reviewed and converted to M.util.get_string where possible to avoid needless JS errors from missing strings.
          4. Find a way to require the dock module ONLY when needed, this would be if $THEME->enable_dock (of whatever it is) is true. This may require smarts as I think JS requirement is initialised before theme.

          I think 1, 3, and 4 should most definitely be separate issues.
          As for point 2 I will leave that up for you. You code at the moment is perfect to the current situation so feel free to put it up for integration as it is. Or feel free to add in point 2 (if you agree with my thinking on that subject of course) and then put it up for integration

          As for the questions about backporting this to 2.1 as well, it is a small improvement and the code is very safe so I think yes it should be backported to 2.1 as well.
          We can of course double check with Eloy on that point as well as he certainly has a better understanding or our protocols than I do, however I could just msg him on integration of the issue.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, You code changes look good There was one thing I was looking at though, the way in which the cancel and yes strings are made available. The strings are only there because other code has requested them. After a bit of debugging I found that they will be available on all pages because the filepicker module is included for ALL pages (which includes all of its sub modules and strings etc). Certainly I think there are two bugs here although unrelated to your changes: 1. Filepicker module should be included only where it is needed - not on every page. This presently leads to loading and piles of strings, yui3 modules, and yui2 modules for every page. 2. confirm_action should be requiring its own strings in JS or alternatively sending them after making a get_string call. And in fact there would be two more tasks to create issues for: 3. Uses of M.str. {component} . {identifier} should be reviewed and converted to M.util.get_string where possible to avoid needless JS errors from missing strings. 4. Find a way to require the dock module ONLY when needed, this would be if $THEME->enable_dock (of whatever it is) is true. This may require smarts as I think JS requirement is initialised before theme. I think 1, 3, and 4 should most definitely be separate issues. As for point 2 I will leave that up for you. You code at the moment is perfect to the current situation so feel free to put it up for integration as it is. Or feel free to add in point 2 (if you agree with my thinking on that subject of course) and then put it up for integration As for the questions about backporting this to 2.1 as well, it is a small improvement and the code is very safe so I think yes it should be backported to 2.1 as well. We can of course double check with Eloy on that point as well as he certainly has a better understanding or our protocols than I do, however I could just msg him on integration of the issue. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Clicking looks good to me, up to you Tim whether you put it up for integration now or fix point 2 and then put it up

          Show
          Sam Hemelryk added a comment - Clicking looks good to me, up to you Tim whether you put it up for integration now or fix point 2 and then put it up
          Hide
          Tim Hunt added a comment -

          Eloy has been backporting other stuff of mine to 2.1, so I am sure he would say yes to this.

          I will do and ammended commit with 2. I think it would be neater to do the null -> default in the PHP code, and always pass the button labels from PHP to JS.

          Show
          Tim Hunt added a comment - Eloy has been backporting other stuff of mine to 2.1, so I am sure he would say yes to this. I will do and ammended commit with 2. I think it would be neater to do the null -> default in the PHP code, and always pass the button labels from PHP to JS.
          Hide
          Tim Hunt added a comment -

          Hmm. I cannot move the null -> default strings into PHP because M.util.show_confirm_dialog is used from a whole bunch of random places, so I cannot suddenly make args.cancellabel and args.continuelabel required.

          Also, this mainly gets added using page->requires->event_handler, which does not seem to take a $jsmodule argument - so, no easy way to make strings available here.

          So, while I completely agree with you that all this code needs a bit clean-up, I am afraid I cannot do it now. I will leave it to you to create the necessary MDL issues for the cleanup that is required in future. In the mean time, my changes do not make the situation any worse.

          I did change the JS code to use an explicit if statement, because even sam (marshall) thought that args.cancellabel || M.str.moodle.cancel was evil, and sam likes the ? : operator.

          I was also able to eliminate one direct reference to M.util.show_confirm_dialog.

          Show
          Tim Hunt added a comment - Hmm. I cannot move the null -> default strings into PHP because M.util.show_confirm_dialog is used from a whole bunch of random places, so I cannot suddenly make args.cancellabel and args.continuelabel required. Also, this mainly gets added using page->requires->event_handler, which does not seem to take a $jsmodule argument - so, no easy way to make strings available here. So, while I completely agree with you that all this code needs a bit clean-up, I am afraid I cannot do it now. I will leave it to you to create the necessary MDL issues for the cleanup that is required in future. In the mean time, my changes do not make the situation any worse. I did change the JS code to use an explicit if statement, because even sam (marshall) thought that args.cancellabel || M.str.moodle.cancel was evil, and sam likes the ? : operator. I was also able to eliminate one direct reference to M.util.show_confirm_dialog.
          Hide
          Tim Hunt added a comment -

          Please put on both 2.1 and master branches.

          Show
          Tim Hunt added a comment - Please put on both 2.1 and master branches.
          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 -

          Thanks Tim, this has been integrated now. I've merged to master and cherry-picked to MOODLE_21_STABLE as well.
          I'll create issues for the points I noted now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Tim, this has been integrated now. I've merged to master and cherry-picked to MOODLE_21_STABLE as well. I'll create issues for the points I noted now. Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          tested and it works great.

          Show
          Rossiani Wijaya added a comment - tested and it works great.
          Hide
          Sam Hemelryk added a comment -

          Congratulations - this fix has just been released in the weeklies.

          Show
          Sam Hemelryk added a comment - Congratulations - this fix has just been released in the weeklies.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: