Moodle
  1. Moodle
  2. MDL-31403

Add a dialogue message warning that the importation, backup and restore process of a course can be long and prevent clicking twice on the import

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.1, 2.2, 2.5
    • Fix Version/s: None
    • Component/s: AJAX and JavaScript, Backup
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Log in as a teacher in a course
      2. Begin the import steps
      3. At the last step, that of confirmation, click on the button "Perform import"
      4. A confirm dialogue should appear
      5. If the "Cancel" or the "Close" button is clicked, the confirm dialogue is closed (this should also work with the Escape and Return keys)
      6. If the "Perform backup" is clicked, the confirm dialogue is closed and a progress bar dialogue is displayed with this properties:
        1. No "Close" button, so impossible to close it.
        2. The animated gif of the progress bar should be animated on all browser
        3. The progress bar dialogue should be always centered on the screen when we scroll or resize it.
      7. Repeat the steps for backup and restore step. The only differences should be on the backup, restore and import strings.
      8. Test this on all supported browsers.
      Show
      Log in as a teacher in a course Begin the import steps At the last step, that of confirmation, click on the button "Perform import" A confirm dialogue should appear If the "Cancel" or the "Close" button is clicked, the confirm dialogue is closed (this should also work with the Escape and Return keys) If the "Perform backup" is clicked, the confirm dialogue is closed and a progress bar dialogue is displayed with this properties: No "Close" button, so impossible to close it. The animated gif of the progress bar should be animated on all browser The progress bar dialogue should be always centered on the screen when we scroll or resize it. Repeat the steps for backup and restore step. The only differences should be on the backup, restore and import strings. Test this on all supported browsers.
    • Workaround:
      Hide

      Tell users to not click many times on the button

      Show
      Tell users to not click many times on the button
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31403-master
    • Rank:
      37925

      Description

      Currently, if we click twice on the button that performs the import, the modules can import as many times as there are clicks.

      In addition, there is no mention that indicates the time that this process can take. Indeed, when importing a large course, the process can take several minutes.

      It should therefore either add a message with a flag indicating that the loading process can be long while disabling the import button after clicking either implemented the progress bar used for example when importing language packs.

      Please note that the same kind of improvement should be made during the backup.

      To Reproduce:

      1. Log in as a teacher in a course
      2. Begin the import steps
      3. At the last stage, that of confirmation, click several times on the button "Perform import"
      4. The course modules to be imported will be included several times

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Hi Gilles-Philippe,

          I'll try and peer review this patch this evening.

          As a first comment, this would be a great candidate for the new Shifter implementation that we integrated last week. I wrote some documentation on using Shifter last night, and it would be great if you are able to try it out and provide some feedback on that documentation. It's located on the developer wiki at http://docs.moodle.org/dev/Javascript/Shifter.

          I do have a few other comments, but I'll put them all together in a single Peer Review later on this evening.

          Thanks,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Gilles-Philippe, I'll try and peer review this patch this evening. As a first comment, this would be a great candidate for the new Shifter implementation that we integrated last week. I wrote some documentation on using Shifter last night, and it would be great if you are able to try it out and provide some feedback on that documentation. It's located on the developer wiki at http://docs.moodle.org/dev/Javascript/Shifter . I do have a few other comments, but I'll put them all together in a single Peer Review later on this evening. Thanks, Andrew
          Hide
          Andrew Nicols added a comment -

          Hi Gilles-Philippe,

          Thank you for working on this - I love the concept and the code is mostly there.

          There are a few changes which need to be made. I realise that the below looks like quite a tome, but they're mostly just small things which will really help readability, maintainability, and performance of the code.

          I also mentioned about shifter in my previous comments. If you need any help with Shifter, please let me know, and it would be great to have feedback on the instructions - thanks

          [N] Syntax
          [Y] Output
          [N] Whitespace
          [N] Language
          [-] Databases
          [N] Testing
          [Y] Security
          [N] Documentation
          [Y] Git
          [Y] Sanity check

          Syntax

          It would be great if you could pass this through jshint (See http://docs.moodle.org/dev/YUI#JSHint for details) and tidy up the warnings. There are only a few but the advice it offers is generally good.

          I have a few other comments too:

          • rather than wrapping in a Y.all, it would be clearer and more efficient if you could use Y.delegate to attach the click event handler to the proceed button and have it call a single function. As it standsm, an anonymous function will be created for each instance of the .proceedbutton.
          • rather than detaching the existing events on this._confirmationListener, you should be able to call event.preventDefault(); and, if necessary event.stopPropagation(); Additionally, there shoudln't be a need in this case to store the event in (e.g. in this._confirmationListener)
          • I've been trying to move all seletors and css styles to a set of
          • variables at the top of the module (see https://github.com/moodle/moodle/blob/master/lib/yui/popuphelp/popuphelp.js for an example). This means that if we need to change the classes at a later date, the resultant diff should be smaller and retain more history, and the change much easier to apply.
          • you should use M.util.image_url() to get the correct link to the progressbar image. This ensures that it will use the correct image loaders (which add appropriate headers for browser caching)
          • The dialogue width should be set in CSS. I'm trying to gradually remove all instances of explicit style in JavaScript as it's much harder for a theme developer to style such a dialogue (the JS set methods apply style directly to the node which takes precedence over CSS without !important which becomes harder for subsequent themes to override)
          • I notice that you're passing the config variable straight into the constructor for M.core.confirm - this feels wrong - if it's right, could you add an appropriate comment to explain what's going on
          • Equally, could you add a comment as to why you've removed the closeButton on the dialogue. I can see exactly why you've done it, but I had to think about what the code was doing and why you would be doing so
          • the image manipulation should be chainable which will make it easier to read
            image = Y.Node.create('<img>')
                    .setStyle('display','block')
                    .setStyle('margin','0 auto')
                    .set('src', M.cfg.wwwroot + '/pix/i/progressbar.gif');
            
          • ideally you should be able to target the image using CSS to set the display and margin (out of interest, is there a reason you need to set display to block? CSS isn't my strong-point and it would be interesting to know)
          • you shouldn't need to wrap centerDialogue in a function. Although it works because of the closure, I guess you're doing it to work around context issues and I don't think it's entirely necessary. If you need to achieve this, then you can write it as:
            Y.one(Y.config.doc).on('scroll', this.centerDialogue, dialogue);
            Y.one(Y.config.win).on('resize', this.centerDialogue, dialogue);
            
          • Also, rather than using 'doc' and window, use Y.config.doc and Y.config.win which are set up by YUI and attached to the correct window (which could be an iFrame for example one day) - I admit I only learned about these last week
          • Rather than appending to the boundingbox, you can also build the content and set the bodyNode value:
            bodyNode = Node.create('<div>')
                .set('content', M.util.get_string(type + 'confirmperforminprogress', 'backup')
                .appendChild(image);
            dialogue.set('bodyContent', bodyNode);
            
          • Rather than creating the progress dialogue each time, it would be better if you create it in the complete-yes event. If a user clicks no, there's no point in creating the dialogue as it's never disposed of.
          • you should be able to reduce the dependency set. You aren't using base or overlay, and I don't think that you're using node-event-simulate

          Whitespace

          Nothing major, I just noticed that your editor is set to remove trailing whitespace at the end of the file.
          There's also some trailing whitespace in the docblock at the top of the JS

          Language

          Strings for JavaScript modules should be included using the page requirements manager rather than as arguments to yui_module.

          $PAGE->requires->strings_for_js(array(
                  'list',
                  'of',
                  'identifiers',
                  ), 'backup');
          

          and

          M.util.get_string('identifier' 'backup');
          

          Testing

          If you could provide some test instructions for the testing team to test the working solution that would be great.

          Documentation

          This is mostly fine - just those things I mentioned above.
          The only other thing worth mentioning is that there's little point to the docblock at the top of the JS file. Since it doesn't contain the relevant yuidoc identifiers it won't actually be turned into any documentation, and since it's not a reusable component (like the chooser dialogue for example), I don't see a huge amount of value in adding further docblock tags to fulfil the full requirements for JS documentation.

          As I say, it's great to have your involvement here and the code is very nearly there. Hopefully we'll be able to get this into 2.5 before the freeze in a few weeks time.

          Thanks again for your time and contributions,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Gilles-Philippe, Thank you for working on this - I love the concept and the code is mostly there. There are a few changes which need to be made. I realise that the below looks like quite a tome, but they're mostly just small things which will really help readability, maintainability, and performance of the code. I also mentioned about shifter in my previous comments. If you need any help with Shifter, please let me know, and it would be great to have feedback on the instructions - thanks [N] Syntax [Y] Output [N] Whitespace [N] Language [-] Databases [N] Testing [Y] Security [N] Documentation [Y] Git [Y] Sanity check Syntax It would be great if you could pass this through jshint (See http://docs.moodle.org/dev/YUI#JSHint for details) and tidy up the warnings. There are only a few but the advice it offers is generally good. I have a few other comments too: rather than wrapping in a Y.all, it would be clearer and more efficient if you could use Y.delegate to attach the click event handler to the proceed button and have it call a single function. As it standsm, an anonymous function will be created for each instance of the .proceedbutton. rather than detaching the existing events on this._confirmationListener, you should be able to call event.preventDefault(); and, if necessary event.stopPropagation(); Additionally, there shoudln't be a need in this case to store the event in (e.g. in this._confirmationListener) I've been trying to move all seletors and css styles to a set of variables at the top of the module (see https://github.com/moodle/moodle/blob/master/lib/yui/popuphelp/popuphelp.js for an example). This means that if we need to change the classes at a later date, the resultant diff should be smaller and retain more history, and the change much easier to apply. you should use M.util.image_url() to get the correct link to the progressbar image. This ensures that it will use the correct image loaders (which add appropriate headers for browser caching) The dialogue width should be set in CSS. I'm trying to gradually remove all instances of explicit style in JavaScript as it's much harder for a theme developer to style such a dialogue (the JS set methods apply style directly to the node which takes precedence over CSS without !important which becomes harder for subsequent themes to override) I notice that you're passing the config variable straight into the constructor for M.core.confirm - this feels wrong - if it's right, could you add an appropriate comment to explain what's going on Equally, could you add a comment as to why you've removed the closeButton on the dialogue. I can see exactly why you've done it, but I had to think about what the code was doing and why you would be doing so the image manipulation should be chainable which will make it easier to read image = Y.Node.create('<img>') .setStyle('display','block') .setStyle('margin','0 auto') .set('src', M.cfg.wwwroot + '/pix/i/progressbar.gif'); ideally you should be able to target the image using CSS to set the display and margin (out of interest, is there a reason you need to set display to block? CSS isn't my strong-point and it would be interesting to know) you shouldn't need to wrap centerDialogue in a function. Although it works because of the closure, I guess you're doing it to work around context issues and I don't think it's entirely necessary. If you need to achieve this, then you can write it as: Y.one(Y.config.doc).on('scroll', this .centerDialogue, dialogue); Y.one(Y.config.win).on('resize', this .centerDialogue, dialogue); Also, rather than using 'doc' and window, use Y.config.doc and Y.config.win which are set up by YUI and attached to the correct window (which could be an iFrame for example one day) - I admit I only learned about these last week Rather than appending to the boundingbox, you can also build the content and set the bodyNode value: bodyNode = Node.create('<div>') .set('content', M.util.get_string(type + 'confirmperforminprogress', 'backup') .appendChild(image); dialogue.set('bodyContent', bodyNode); Rather than creating the progress dialogue each time, it would be better if you create it in the complete-yes event. If a user clicks no, there's no point in creating the dialogue as it's never disposed of. you should be able to reduce the dependency set. You aren't using base or overlay, and I don't think that you're using node-event-simulate Whitespace Nothing major, I just noticed that your editor is set to remove trailing whitespace at the end of the file. There's also some trailing whitespace in the docblock at the top of the JS Language Strings for JavaScript modules should be included using the page requirements manager rather than as arguments to yui_module. $PAGE->requires->strings_for_js(array( 'list', 'of', 'identifiers', ), 'backup'); and M.util.get_string('identifier' 'backup'); Testing If you could provide some test instructions for the testing team to test the working solution that would be great. Documentation This is mostly fine - just those things I mentioned above. The only other thing worth mentioning is that there's little point to the docblock at the top of the JS file. Since it doesn't contain the relevant yuidoc identifiers it won't actually be turned into any documentation, and since it's not a reusable component (like the chooser dialogue for example), I don't see a huge amount of value in adding further docblock tags to fulfil the full requirements for JS documentation. As I say, it's great to have your involvement here and the code is very nearly there. Hopefully we'll be able to get this into 2.5 before the freeze in a few weeks time. Thanks again for your time and contributions, Andrew
          Hide
          Gilles-Philippe Leblanc added a comment - - edited

          As a first comment, this would be a great candidate for the new Shifter implementation that we integrated last week.

          Thats a good idea. I want to learn that but unfortunately, I don't have to much time left on this task. It is absolutely required ?

          Syntax

          It would be great if you could pass this through jshint

          Done but with the notepad++ jshint plugin. the nodejs jshint installation do not work for me.
          Its maybe related to an incompatibility between jshint and with the latest node js version.

          rather than wrapping in a Y.all, it would be clearer and more efficient if you could use Y.delegate to attach the click event handler to the proceed button and have it call a single function.
          rather than detaching the existing events on this._confirmationListener, you should be able to call event.preventDefault(); and, if necessary event.stopPropagation(); Additionally, there shouldn't be a need in this case to store the event in (e.g. in this._confirmationListener)

          I tough you were right on this one but after implemented your way with the Y.delegate I remembered why I did it like I did... The import php code extend the backup code so the module is called twice... the only way is to only keep one. All the preventDefault() or stopPropagation() cannot change this. By the way the Cancel Popup: "backup\util\ui\yui\confirmcancel\confirmcancel.js" have the same behavior.

          I've been trying to move all selectors and css styles to a set of
          variables at the top of the module (see https://github.com/moodle/moodle/blob/master/lib/yui/popuphelp/popuphelp.js for an example). This means that if we need to change the classes at a later date, the resultant diff should be smaller and retain more history, and the change much easier to apply.

          Done.

          you should use M.util.image_url() to get the correct link to the progressbar image. This ensures that it will use the correct image loaders (which add appropriate headers for browser caching)

          Done.

          The dialogue width should be set in CSS. I'm trying to gradually remove all instances of explicit style in JavaScript as it's much harder for a theme developer to style such a dialogue (the JS set methods apply style directly to the node which takes precedence over CSS without !important which becomes harder for subsequent themes to override)

          I understand very well that. I just hardly wondered where put the CSS. I finally putted in in themes/base/core.css.

          I notice that you're passing the config variable straight into the constructor for M.core.confirm - this feels wrong - if it's right, could you add an appropriate comment to explain what's going on

          For all the structure, I have only copied the one used in backup\util\ui\yui\confirmcancel\confirmcancel.js

          Equally, could you add a comment as to why you've removed the closeButton on the dialogue. I can see exactly why you've done it, but I had to think about what the code was doing and why you would be doing so

          Done.

          the image manipulation should be chainable which will make it easier to read

          Done.

          (out of interest, is there a reason you need to set display to block? CSS isn't my strong-point and it would be interesting to know)

          "display: block;" combined with: "margin: 0 auto;" is a good way to center the img tag. this also fix the height of the image. If you want to see the difference, use firebug or your favorite DOM debugger and replace "display: block" with "display:inline" and you'll see. I could also let the display and change the line-height of his parent and set "text-align: center".

          you shouldn't need to wrap centerDialogue in a function

          Done.

          Also, rather than using 'doc' and window, use Y.config.doc and Y.config.win which are set up by YUI and attached to the correct window (which could be an iFrame for example one day) - I admit I only learned about these last week

          Done.

          Rather than appending to the boundingbox, you can also build the content and set the bodyNode value:

          Done, well the principle of. I though it was not possible so easily because the use of the dialogue base of the moodle-core-dialog. You give me the solution. Thanks!

          Rather than creating the progress dialogue each time, it would be better if you create it in the complete-yes event. If a user clicks no, there's no point in creating the dialogue as it's never disposed of.

          I totally agree with you with that but I created it here to avoid to see the loading of the picture. (No-picture and then, the picture appeared).

          Many way to fix it:

          1. Create the progress bar dialogue before the complete-yes event like it was.
          2. Create the progress bar dialogue inside the onload event of the image (so a delay before showing the dialog)
          3. Put CSS default height to the img tag (So we will still see the image appeared, but without the dialog height change)
            For now, I choose to put the height to the image tag. Its ugly for a second. It will be maybe correct on faster machine... I still believe that the first choice was better. Do you know if there is a image preloader on YUI? It should be the best.

          you should be able to reduce the dependency set. You aren't using base or overlay, and I don't think that you're using node-event-simulate

          That was junk from an old version (and again from confirmcancel). Done. But I always though that the base was required everywhere.

          Whitespace

          Nothing major, I just noticed that your editor is set to remove trailing whitespace at the end of the file.
          There's also some trailing whitespace in the docblock at the top of the JS

          Done, I think.

          Language

          Strings for JavaScript modules should be included using the page requirements manager rather than as arguments to yui_module.

          I agree but again, just copied the code from: backup\util\ui\yui\confirmcancel\confirmcancel.js

          Testing

          If you could provide some test instructions for the testing team to test the working solution that would be great.

          Done.

          Documentation

          The only other thing worth mentioning is that there's little point to the docblock at the top of the JS file. Since it doesn't contain the relevant yuidoc identifiers it won't actually be turned into any documentation, and since it's not a reusable component (like the chooser dialogue for example), I don't see a huge amount of value in adding further docblock tags to fulfill the full requirements for JS documentation.

          I'm not sure to understand well why and if you talk about to remove the comments but I think it is still important to put a module description even if the description is not official or the module, reusable.
          Again, the yuidoc for nodejs plugin don't want to install on my computer. If you want to help me with that, your welcome!

          Show
          Gilles-Philippe Leblanc added a comment - - edited As a first comment, this would be a great candidate for the new Shifter implementation that we integrated last week. Thats a good idea. I want to learn that but unfortunately, I don't have to much time left on this task. It is absolutely required ? Syntax It would be great if you could pass this through jshint Done but with the notepad++ jshint plugin. the nodejs jshint installation do not work for me. Its maybe related to an incompatibility between jshint and with the latest node js version. rather than wrapping in a Y.all, it would be clearer and more efficient if you could use Y.delegate to attach the click event handler to the proceed button and have it call a single function. rather than detaching the existing events on this._confirmationListener, you should be able to call event.preventDefault(); and, if necessary event.stopPropagation(); Additionally, there shouldn't be a need in this case to store the event in (e.g. in this._confirmationListener) I tough you were right on this one but after implemented your way with the Y.delegate I remembered why I did it like I did... The import php code extend the backup code so the module is called twice... the only way is to only keep one. All the preventDefault() or stopPropagation() cannot change this. By the way the Cancel Popup: "backup\util\ui\yui\confirmcancel\confirmcancel.js" have the same behavior. I've been trying to move all selectors and css styles to a set of variables at the top of the module (see https://github.com/moodle/moodle/blob/master/lib/yui/popuphelp/popuphelp.js for an example). This means that if we need to change the classes at a later date, the resultant diff should be smaller and retain more history, and the change much easier to apply. Done. you should use M.util.image_url() to get the correct link to the progressbar image. This ensures that it will use the correct image loaders (which add appropriate headers for browser caching) Done. The dialogue width should be set in CSS. I'm trying to gradually remove all instances of explicit style in JavaScript as it's much harder for a theme developer to style such a dialogue (the JS set methods apply style directly to the node which takes precedence over CSS without !important which becomes harder for subsequent themes to override) I understand very well that. I just hardly wondered where put the CSS. I finally putted in in themes/base/core.css. I notice that you're passing the config variable straight into the constructor for M.core.confirm - this feels wrong - if it's right, could you add an appropriate comment to explain what's going on For all the structure, I have only copied the one used in backup\util\ui\yui\confirmcancel\confirmcancel.js Equally, could you add a comment as to why you've removed the closeButton on the dialogue. I can see exactly why you've done it, but I had to think about what the code was doing and why you would be doing so Done. the image manipulation should be chainable which will make it easier to read Done. (out of interest, is there a reason you need to set display to block? CSS isn't my strong-point and it would be interesting to know) "display: block;" combined with: "margin: 0 auto;" is a good way to center the img tag. this also fix the height of the image. If you want to see the difference, use firebug or your favorite DOM debugger and replace "display: block" with "display:inline" and you'll see. I could also let the display and change the line-height of his parent and set "text-align: center". you shouldn't need to wrap centerDialogue in a function Done. Also, rather than using 'doc' and window, use Y.config.doc and Y.config.win which are set up by YUI and attached to the correct window (which could be an iFrame for example one day) - I admit I only learned about these last week Done. Rather than appending to the boundingbox, you can also build the content and set the bodyNode value: Done, well the principle of. I though it was not possible so easily because the use of the dialogue base of the moodle-core-dialog. You give me the solution. Thanks! Rather than creating the progress dialogue each time, it would be better if you create it in the complete-yes event. If a user clicks no, there's no point in creating the dialogue as it's never disposed of. I totally agree with you with that but I created it here to avoid to see the loading of the picture. (No-picture and then, the picture appeared). Many way to fix it: Create the progress bar dialogue before the complete-yes event like it was. Create the progress bar dialogue inside the onload event of the image (so a delay before showing the dialog) Put CSS default height to the img tag (So we will still see the image appeared, but without the dialog height change) For now, I choose to put the height to the image tag. Its ugly for a second. It will be maybe correct on faster machine... I still believe that the first choice was better. Do you know if there is a image preloader on YUI? It should be the best. you should be able to reduce the dependency set. You aren't using base or overlay, and I don't think that you're using node-event-simulate That was junk from an old version (and again from confirmcancel). Done. But I always though that the base was required everywhere. Whitespace Nothing major, I just noticed that your editor is set to remove trailing whitespace at the end of the file. There's also some trailing whitespace in the docblock at the top of the JS Done, I think. Language Strings for JavaScript modules should be included using the page requirements manager rather than as arguments to yui_module. I agree but again, just copied the code from: backup\util\ui\yui\confirmcancel\confirmcancel.js Testing If you could provide some test instructions for the testing team to test the working solution that would be great. Done. Documentation The only other thing worth mentioning is that there's little point to the docblock at the top of the JS file. Since it doesn't contain the relevant yuidoc identifiers it won't actually be turned into any documentation, and since it's not a reusable component (like the chooser dialogue for example), I don't see a huge amount of value in adding further docblock tags to fulfill the full requirements for JS documentation. I'm not sure to understand well why and if you talk about to remove the comments but I think it is still important to put a module description even if the description is not official or the module, reusable. Again, the yuidoc for nodejs plugin don't want to install on my computer. If you want to help me with that, your welcome!
          Hide
          Andrew Nicols added a comment -

          Hi again Gilles-Philippe, I'll try and review this again this evening.

          Don't worry about shifter, but if you have some spare time though, I'd really like to get to the bottom of why you can't get Shifter, and JShint working.

          Show
          Andrew Nicols added a comment - Hi again Gilles-Philippe, I'll try and review this again this evening. Don't worry about shifter, but if you have some spare time though, I'd really like to get to the bottom of why you can't get Shifter, and JShint working.
          Hide
          Andrew Nicols added a comment -

          Hi Gilles-Philippe,

          Thanks for the hard work on this - it's really much appreciated. Everything looks much better now - thanks. Pretty much everything is there - just a few minor points. If you don't have time to address them, I can fix up the final few bits.

          Regarding the delegation - that's an interesting one. I'll be taking a look at that backup code for Moodle 2.6 so I'll keep that in mind when I do so. Thanks for the heads up.

          With the CSS, I think that you're right - that's probably the best place to place it.

          With the dependency on Y.Base: Y.Base provides the base class style that we use in places like moodle-core-notification. We extend Y.Base and benefit from attributes, and a complete initalizer and destructor set chained through the load. It's generally only required where you're creating multiple copies or something likethat. In this case, we're just using existing items in a function so it's not required. Ultimately thoughh, half of Moodle JS modules require it, but there is still a minor performance hit for loading it into the module sandbox.

          For the progressbar image, I've just found that you can preload it with something like:

              var progressbar = M.util.image_url('i/progressbar', 'core');
              // Preload the progressbar image.
              Y.Node.create('<image />')
                  .setAttribute('src', progressbar)
                  .setStyles({
                      width: 0,
                      height: 0
                  });
          

          You should be able to put this in the confirmationlistener function so that when the initial confirmation dialogue is shown, the progress bar is pre-loaded. That way we're not adding additional HTTP transactions on page load, and they're only loaded when they're used.

          If you have a chance, then there are still a few minor things to address:

          • According to the Moodle style guide, comments should end with appropriate punctuation (. ! or ?).
            This is relatively new so there will be plenty of places in Moodle which don't yet have this right.
          • Indenting from line 45 to 65 is 4 spaces too far.

          One that is slightly less minor is that it would be great if the functions that you add to the listeners were attached to a varible rather than being anonymous. Same goes for the Y.all function. It would make it a little easier to read (consistent indent style) and would mean that we're not creating functions in functions in functions on a loop. If you don't have a chance to do this, then I'll be happy to do so though.

          Otherwise this looks like a great addition. When you're ready, I'll submit this for integration - just in time for freeze

          Thanks again,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Gilles-Philippe, Thanks for the hard work on this - it's really much appreciated. Everything looks much better now - thanks. Pretty much everything is there - just a few minor points. If you don't have time to address them, I can fix up the final few bits. Regarding the delegation - that's an interesting one. I'll be taking a look at that backup code for Moodle 2.6 so I'll keep that in mind when I do so. Thanks for the heads up. With the CSS, I think that you're right - that's probably the best place to place it. With the dependency on Y.Base: Y.Base provides the base class style that we use in places like moodle-core-notification. We extend Y.Base and benefit from attributes, and a complete initalizer and destructor set chained through the load. It's generally only required where you're creating multiple copies or something likethat. In this case, we're just using existing items in a function so it's not required. Ultimately thoughh, half of Moodle JS modules require it, but there is still a minor performance hit for loading it into the module sandbox. For the progressbar image, I've just found that you can preload it with something like: var progressbar = M.util.image_url('i/progressbar', 'core'); // Preload the progressbar image. Y.Node.create('<image />') .setAttribute('src', progressbar) .setStyles({ width: 0, height: 0 }); You should be able to put this in the confirmationlistener function so that when the initial confirmation dialogue is shown, the progress bar is pre-loaded. That way we're not adding additional HTTP transactions on page load, and they're only loaded when they're used. If you have a chance, then there are still a few minor things to address: According to the Moodle style guide, comments should end with appropriate punctuation (. ! or ?). This is relatively new so there will be plenty of places in Moodle which don't yet have this right. Indenting from line 45 to 65 is 4 spaces too far. One that is slightly less minor is that it would be great if the functions that you add to the listeners were attached to a varible rather than being anonymous. Same goes for the Y.all function. It would make it a little easier to read (consistent indent style) and would mean that we're not creating functions in functions in functions on a loop. If you don't have a chance to do this, then I'll be happy to do so though. Otherwise this looks like a great addition. When you're ready, I'll submit this for integration - just in time for freeze Thanks again, Andrew
          Hide
          Tim Hunt added a comment -

          I am sorry, but doing a backup already involves too many mouse clicks, and you have adding another one. That is a mistake.

          If you want to avoid problems with people clicking the button twice, then disable the button when it is clicked (or replace it with a progress indicator).

          If you want users to read some extra message, but in directly on the page above the submit button, don't hide it in a pop-up.

          -1 from me for this change.

          Show
          Tim Hunt added a comment - I am sorry, but doing a backup already involves too many mouse clicks, and you have adding another one. That is a mistake. If you want to avoid problems with people clicking the button twice, then disable the button when it is clicked (or replace it with a progress indicator). If you want users to read some extra message, but in directly on the page above the submit button, don't hide it in a pop-up. -1 from me for this change.
          Hide
          Gilles-Philippe Leblanc added a comment - - edited

          @Tim Hunt
          I agree that the additional click its not the best but I think its important to tells the user that this step may be long.
          We use this dialog since 2.1.2 and no user complain about it. More, other University want the confirm dialog. My comprehension is that there so many click on the backup, restore and import process that one more or less...

          Concerning the progress bar dialog, I think it just need to be here to respect the usability.

          So the only question is: Do we put a confirm dialog or we put a sentence at the bottom of the proceed button ?

          We could then rename the task related to this decision.

          Show
          Gilles-Philippe Leblanc added a comment - - edited @ Tim Hunt I agree that the additional click its not the best but I think its important to tells the user that this step may be long. We use this dialog since 2.1.2 and no user complain about it. More, other University want the confirm dialog. My comprehension is that there so many click on the backup, restore and import process that one more or less... Concerning the progress bar dialog, I think it just need to be here to respect the usability. So the only question is: Do we put a confirm dialog or we put a sentence at the bottom of the proceed button ? We could then rename the task related to this decision.
          Hide
          Gilles-Philippe Leblanc added a comment -

          All requested changes were made. There would be, at least, I think, have to decide whether or not to display a confirmation dialog box and put in place a sentence stating that the process can be long.

          Show
          Gilles-Philippe Leblanc added a comment - All requested changes were made. There would be, at least, I think, have to decide whether or not to display a confirmation dialog box and put in place a sentence stating that the process can be long.
          Hide
          Andrew Nicols added a comment -

          I'm going to temporarily park this a little longer. I know that Sam Marshall has been working on a variety of improvements to the backup/restore process and he may be better to peer review this or have an alterative solution.

          Show
          Andrew Nicols added a comment - I'm going to temporarily park this a little longer. I know that Sam Marshall has been working on a variety of improvements to the backup/restore process and he may be better to peer review this or have an alterative solution.
          Hide
          Sam Marshall added a comment -

          Thanks for mention Andrew. For info I think this will be addressed by MDL-38190 except that I haven't implemented that for import! I forgot about import. Assuming there aren't any problems with doing so, I'll modify that commit now as it hasn't been reviewed yet.

          That change makes backup and restore (and import once I code it) display a progress bar during the import stage. As a result, instead of taking e.g. 2 minutes between when you click the button and when you see something, it should take only a couple of seconds before the new page starts to load and shows a progress bar.

          Once this behaviour works, it probably isn't necessary to add an annoying confirm prompt.

          Show
          Sam Marshall added a comment - Thanks for mention Andrew. For info I think this will be addressed by MDL-38190 except that I haven't implemented that for import! I forgot about import. Assuming there aren't any problems with doing so, I'll modify that commit now as it hasn't been reviewed yet. That change makes backup and restore (and import once I code it) display a progress bar during the import stage. As a result, instead of taking e.g. 2 minutes between when you click the button and when you see something, it should take only a couple of seconds before the new page starts to load and shows a progress bar. Once this behaviour works, it probably isn't necessary to add an annoying confirm prompt.
          Hide
          Andrew Nicols added a comment -

          Agreed Sam Marshall - I think that avoiding an additional confirmation (on what is already a 7? step process) is probably best.

          Cheers - I guess best close this once MDL-38190 is integrated in case integrators differ in opinion.

          Show
          Andrew Nicols added a comment - Agreed Sam Marshall - I think that avoiding an additional confirmation (on what is already a 7? step process) is probably best. Cheers - I guess best close this once MDL-38190 is integrated in case integrators differ in opinion.
          Hide
          Gilles-Philippe Leblanc added a comment -

          I agree that we should close this one if MDL-38190 include the import.

          Show
          Gilles-Philippe Leblanc added a comment - I agree that we should close this one if MDL-38190 include the import.
          Hide
          Andrew Nicols added a comment -

          As discussed, this issue will be resolved in 2.6 with a progress bar rather than a blocking dialogue. This should give a better user experience.

          Show
          Andrew Nicols added a comment - As discussed, this issue will be resolved in 2.6 with a progress bar rather than a blocking dialogue. This should give a better user experience.

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: