Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-34707

JavaScript included twice in the backup UI

          Details

          • Type: Bug
          • Status: Closed
          • Priority: Minor
          • Resolution: Fixed
          • Affects Version/s: 2.2.4, 2.3.1
          • Fix Version/s: 2.2.5, 2.3.2
          • Component/s: Backup
          • Labels:
          • Testing Instructions:
            Hide

            1. Go to a course.

            2. Click Backup, then Next.

            3. Inspect the page using Firebug, and look in the last <script> tag in the HTML.

            4. Verify that there is only one call to M.core_backup.watch_cancel_buttons

            Show
            1. Go to a course. 2. Click Backup, then Next. 3. Inspect the page using Firebug, and look in the last <script> tag in the HTML. 4. Verify that there is only one call to M.core_backup.watch_cancel_buttons
          • Affected Branches:
            MOODLE_22_STABLE, MOODLE_23_STABLE
          • Fixed Branches:
            MOODLE_22_STABLE, MOODLE_23_STABLE
          • Pull from Repository:
          • Pull Master Branch:
          • Pull Master Diff URL:

            Description

            This is despite the fact that there is a require_definition_after_data method that tries to prevent this.

            The problems is that some parts of formslib (e.g. get_data) call definition_after_data directly.

              Gliffy Diagrams

                Attachments

                  Issue Links

                  will help resolve

                  Improvement - An enhancement to an existing Moodle feature. MDL-32705 No way to clear all checkboxes in backup schema settings

                  • Minor - Minor loss of function where workaround is possible
                  • Closed

                  Improvement - An enhancement to an existing Moodle feature. MDL-34727 Restore UI should use checkboxes for whether to restore userdata

                  • Minor - Minor loss of function where workaround is possible
                  • Closed

                    Activity

                    Ascending order - Click to sort in descending order
                    Hide
                    Permalink
                    timhunt Tim Hunt added a comment -

                    Oh, and not only that. We actually end up creating two different forms. For example when displaying the schema, we first create the backup_ui_stage_initial form, and get_data from it. Then we create the backup_ui_stage_schema form and display it.

                    Anyway, what this means is that definition_after_data is a bad place to initialise JavaScript.

                    Show
                    timhunt Tim Hunt added a comment - Oh, and not only that. We actually end up creating two different forms. For example when displaying the schema, we first create the backup_ui_stage_initial form, and get_data from it. Then we create the backup_ui_stage_schema form and display it. Anyway, what this means is that definition_after_data is a bad place to initialise JavaScript.
                    Hide
                    Permalink
                    timhunt Tim Hunt added a comment -

                    To INTEGRATORS MDL-32705 and MDL-34727 follow this on the same branch.

                    I will leave you the tricky decision how much of all this you put into each stable branch. On the one hand, there are clear UI improvement. On the other hand it is a UI change. I think the OU admins would all vote for this in 2.2.

                    Show
                    timhunt Tim Hunt added a comment - To INTEGRATORS MDL-32705 and MDL-34727 follow this on the same branch. I will leave you the tricky decision how much of all this you put into each stable branch. On the one hand, there are clear UI improvement. On the other hand it is a UI change. I think the OU admins would all vote for this in 2.2.
                    Hide
                    Permalink
                    poltawski Dan Poltawski added a comment -

                    Thanks Tim, i've integrated this to 22, 23 and master (I don't think there is anything about this fix which is controversial for stable branches).

                    Show
                    poltawski Dan Poltawski added a comment - Thanks Tim, i've integrated this to 22, 23 and master (I don't think there is anything about this fix which is controversial for stable branches).
                    Hide
                    Permalink
                    timhunt Tim Hunt added a comment -

                    Wait until you get the the next two commits on this branch!

                    Show
                    timhunt Tim Hunt added a comment - Wait until you get the the next two commits on this branch!
                    Hide
                    Permalink
                    poltawski Dan Poltawski added a comment -

                    Surely, there is only one commit on this branch

                    Show
                    poltawski Dan Poltawski added a comment - Surely, there is only one commit on this branch
                    Hide
                    Permalink
                    timhunt Tim Hunt added a comment -

                    There are two linked issues that are built on top of this commit - that may only be explained properly in those issues, not this one. Or I may completely have failed to explain it at all. I thought you integrators were telepathic anyway

                    Show
                    timhunt Tim Hunt added a comment - There are two linked issues that are built on top of this commit - that may only be explained properly in those issues, not this one. Or I may completely have failed to explain it at all. I thought you integrators were telepathic anyway
                    Hide
                    Permalink
                    poltawski Dan Poltawski added a comment -

                    I understood it, though actually I think pointing out the forward dependencies might make it harder to understand/easier to confuse!

                    Show
                    poltawski Dan Poltawski added a comment - I understood it, though actually I think pointing out the forward dependencies might make it harder to understand/easier to confuse!
                    Hide
                    Permalink
                    stronk7 Eloy Lafuente (stronk7) added a comment -

                    This has been integrated to 22, 23 and master.

                    Show
                    stronk7 Eloy Lafuente (stronk7) added a comment - This has been integrated to 22, 23 and master.
                    Hide
                    Permalink
                    salvetore Michael de Raadt added a comment -

                    Test result: Success!

                    Tested in 2.2, 2.3 and master.

                    Show
                    salvetore Michael de Raadt added a comment - Test result: Success! Tested in 2.2, 2.3 and master.
                    Hide
                    Permalink
                    stronk7 Eloy Lafuente (stronk7) added a comment -

                    Fixed STOP Closed STOP Thanks STOP

                    Yay, imagination! Ciao

                    Show
                    stronk7 Eloy Lafuente (stronk7) added a comment - Fixed STOP Closed STOP Thanks STOP Yay, imagination! Ciao

                      People

                      • Votes:
                        0 Vote for this issue
                        Watchers:
                        3 Start watching this issue

                        Dates

                        • Created:
                          Updated:
                          Resolved:
                          Fix Release Date:
                          10/Sep/12