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

JavaScript error in assignment upgrade tool

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.4
    • Fix Version/s: 2.3.2
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide
      1. goto /admin/tool/assignmentupgrade/listnotupgraded.php
      2. click on the button "upgrade all assignments"
      3. There should be no javascript errors in the console and you should be taken to a confirm page.
      4. goto /admin/tool/assignmentupgrade/listnotupgraded.php
      5. click on the button "upgrade selected assignments" without selecting any assignments
      6. There should be an alert: "No assignments selected" and you should stay on the same page.
      Show
      goto /admin/tool/assignmentupgrade/listnotupgraded.php click on the button "upgrade all assignments" There should be no javascript errors in the console and you should be taken to a confirm page. goto /admin/tool/assignmentupgrade/listnotupgraded.php click on the button "upgrade selected assignments" without selecting any assignments There should be an alert: "No assignments selected" and you should stay on the same page.
    • Workaround:
      Hide

      None required.

      Show
      None required.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:

      Description

      Replication steps:

      1. goto /admin/tool/assignmentupgrade/listnotupgraded.php
      2. click on the button "upgrade all assignments"

      check console for an error something like :-
      Timestamp: 15/08/12 14:16:51
      Error: TypeError: M.str.assign is undefined
      Source File: http://127.0.0.1/int/master/moodle/lib/javascript.php?rev=1345010974&jsfile=%2Fadmin%2Ftool%2Fassignmentupgrade%2Fmodule.js
      Line: 1

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            oh, it seems the string is not there at all, solution should be

            alert(M.util.get_string('noassignmentsselected', 'tool_assignmentupgrade'));
             
            $this->page->requires->string_for_js('noassignmentsselected', 'tool_assignmentupgrade');
            $this->page->requires->js_init_call('M.tool_assignmentupgrade.init_upgrade_table', array());

            adding the new lang string

            Show
            skodak Petr Skoda added a comment - oh, it seems the string is not there at all, solution should be alert(M.util.get_string('noassignmentsselected', 'tool_assignmentupgrade'));   $this->page->requires->string_for_js('noassignmentsselected', 'tool_assignmentupgrade'); $this->page->requires->js_init_call('M.tool_assignmentupgrade.init_upgrade_table', array()); adding the new lang string
            Hide
            corleone Raymond Antonio added a comment - - edited

            Hi Damyon and Petr,

            The missing lang string has been added and this is a proposed patch for this tracker and it sits on my github repo : MDL-34907
            https://github.com/raymondAntonio/moodle/tree/MDL-34907

            and here is the diff:

            https://github.com/raymondAntonio/moodle/commit/335347179b34c75c7ed99fbacd295b7b7919b24c

            Ps: a newdiff above as the white space has been deleted

            Cheers

            Show
            corleone Raymond Antonio added a comment - - edited Hi Damyon and Petr, The missing lang string has been added and this is a proposed patch for this tracker and it sits on my github repo : MDL-34907 https://github.com/raymondAntonio/moodle/tree/MDL-34907 and here is the diff: https://github.com/raymondAntonio/moodle/commit/335347179b34c75c7ed99fbacd295b7b7919b24c Ps: a newdiff above as the white space has been deleted Cheers
            Hide
            damyon Damyon Wiese added a comment -

            There is an extra white space change here - but it is OK.

            Show
            damyon Damyon Wiese added a comment - There is an extra white space change here - but it is OK.
            Hide
            damyon Damyon Wiese added a comment -

            This looks OK for integration.

            Show
            damyon Damyon Wiese added a comment - This looks OK for integration.
            Hide
            damyon Damyon Wiese added a comment -

            Raymond is going to re-push this without the whitespace change.

            Show
            damyon Damyon Wiese added a comment - Raymond is going to re-push this without the whitespace change.
            Hide
            damyon Damyon Wiese added a comment -

            He has re-pushed without the white space change - ready to go.

            Show
            damyon Damyon Wiese added a comment - He has re-pushed without the white space change - ready to go.
            Hide
            stronk7 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
            stronk7 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
            corleone Raymond Antonio added a comment - - edited

            Hi Eloy,

            Thanks for your suggestion and I just did the rebase my PULL branches to make integrator's life less tough and here is the new diff:

            https://github.com/raymondAntonio/moodle/commit/77b3e35105ac24139353c25ebda50595b7a7e35f

            Cheers

            Show
            corleone Raymond Antonio added a comment - - edited Hi Eloy, Thanks for your suggestion and I just did the rebase my PULL branches to make integrator's life less tough and here is the new diff: https://github.com/raymondAntonio/moodle/commit/77b3e35105ac24139353c25ebda50595b7a7e35f Cheers
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks guys!
            integrated into master and backported to 2.3 cleanly.

            Show
            nebgor Aparup Banerjee added a comment - Thanks guys! integrated into master and backported to 2.3 cleanly.
            Hide
            dmonllao David Monllaó added a comment -

            Sorry it fails, I don't receive any JS error, but when I click 'Upgrade selected assignments' I receive an alert with 'No assignments selected' text.

            Show
            dmonllao David Monllaó added a comment - Sorry it fails, I don't receive any JS error, but when I click 'Upgrade selected assignments' I receive an alert with 'No assignments selected' text.
            Hide
            dmonllao David Monllaó added a comment -

            I It doesn't seems that is caused by this issue patch, but probably this wrong behaviour wasn't discovered earlier because of the JS crash

            Show
            dmonllao David Monllaó added a comment - I It doesn't seems that is caused by this issue patch, but probably this wrong behaviour wasn't discovered earlier because of the JS crash
            Hide
            corleone Raymond Antonio added a comment -

            Hi David,

            I am thinking.. Maybe..maybe it doesn't fail because it just alerts you 'No assignments selected' as Moodle was confused and simply didn't know which assignments to upgrade ( as the result of No assignments selected) when you clicked 'Upgrade SELECTED/ALL assignments' button? so hence it alerted you 'No assignment selected'. That's just my 2 cents.

            Best regards,

            Ray

            Show
            corleone Raymond Antonio added a comment - Hi David, I am thinking.. Maybe..maybe it doesn't fail because it just alerts you 'No assignments selected' as Moodle was confused and simply didn't know which assignments to upgrade ( as the result of No assignments selected) when you clicked 'Upgrade SELECTED/ALL assignments' button? so hence it alerted you 'No assignment selected'. That's just my 2 cents. Best regards, Ray
            Hide
            dmonllao David Monllaó added a comment -

            Sorry Raymond, I would mean "when I click 'Upgrade all assignments'"

            Show
            dmonllao David Monllaó added a comment - Sorry Raymond, I would mean "when I click 'Upgrade all assignments'"
            Hide
            corleone Raymond Antonio added a comment -

            Hi David,
            It could be a bug (as the button says 'Upgrade all assignments' so it implies that there is no need to tick any of selected box.. just hit the button )or it might be a kinda grey area because it is expected behavior as you are expected to tick 'select all' box on the top left of the table containing the list of all assignments before hitting the 'upgrade ALL' button? otherwise you will be 'alerted'.

            cheers

            Show
            corleone Raymond Antonio added a comment - Hi David, It could be a bug (as the button says 'Upgrade all assignments' so it implies that there is no need to tick any of selected box.. just hit the button )or it might be a kinda grey area because it is expected behavior as you are expected to tick 'select all' box on the top left of the table containing the list of all assignments before hitting the 'upgrade ALL' button? otherwise you will be 'alerted'. cheers
            Hide
            nebgor Aparup Banerjee added a comment - - edited

            ah yep, i'm getting that too.. thank for picking that up David , can we provide a quick fix for this here? or another issue?
            (truthfully this reported issue is fixed, but it would be great to have this totally working here)

            edit: ok maybe create a separate issue then? i would expect that hitting the button 'upgrade all' with nothing ticked will simply help me do that (perhaps help me tick all boxes + 'are you sure?' )

            Show
            nebgor Aparup Banerjee added a comment - - edited ah yep, i'm getting that too.. thank for picking that up David , can we provide a quick fix for this here? or another issue? (truthfully this reported issue is fixed, but it would be great to have this totally working here) edit: ok maybe create a separate issue then? i would expect that hitting the button 'upgrade all' with nothing ticked will simply help me do that (perhaps help me tick all boxes + 'are you sure?' )
            Hide
            dmonllao David Monllaó added a comment -

            Hi Raymond, thanks for your comments; my point on considering it a bug: If you have 200 assignments with 'Assignments per page' set to 100, the "Upgrade all assignments" button, as far as I understand, should upgrade all the assignments, not only the 100 you are able to select clicking the "select all" checkbox; I'm not familiarized with the assignment upgrade tool, but with a quick view at the code I see that the 'upgradeall' param is treated as "select all the BD 2.2 assignments" not as a "select all the selected assignments of this page", this last feature is also covered by "Upgrade selected assignments"

            Show
            dmonllao David Monllaó added a comment - Hi Raymond, thanks for your comments; my point on considering it a bug: If you have 200 assignments with 'Assignments per page' set to 100, the "Upgrade all assignments" button, as far as I understand, should upgrade all the assignments, not only the 100 you are able to select clicking the "select all" checkbox; I'm not familiarized with the assignment upgrade tool, but with a quick view at the code I see that the 'upgradeall' param is treated as "select all the BD 2.2 assignments" not as a "select all the selected assignments of this page", this last feature is also covered by "Upgrade selected assignments"
            Hide
            corleone Raymond Antonio added a comment -

            Hi Aparup and David,
            Yes IMO we should create another separate tracker (but linking it with this tracker) for this. (I can fix it and update the tracker).

            cheers

            Show
            corleone Raymond Antonio added a comment - Hi Aparup and David, Yes IMO we should create another separate tracker (but linking it with this tracker) for this. (I can fix it and update the tracker). cheers
            Hide
            nebgor Aparup Banerjee added a comment -

            just noting that : (3) There should be no javascript errors in the console and you should be taken to a confirm page. - its really simply no js error here now. there was no confirm page - will be fixed in linked issue.

            Show
            nebgor Aparup Banerjee added a comment - just noting that : (3) There should be no javascript errors in the console and you should be taken to a confirm page. - its really simply no js error here now. there was no confirm page - will be fixed in linked issue.
            Hide
            dmonllao David Monllaó added a comment -

            It passes, I'm creating a new issue with patch to solve the related problem. Thanks for the quick responses Raymond

            Show
            dmonllao David Monllaó added a comment - It passes, I'm creating a new issue with patch to solve the related problem. Thanks for the quick responses Raymond
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I'm so proud...of you, many thanks!

            http://youtu.be/n64CdfDRnZY

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao

              People

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

                Dates

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