Moodle
  1. Moodle
  2. MDL-34907

JavaScript error in assignment upgrade tool

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      43433

      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
      

        Issue Links

          Activity

          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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 Wiese added a comment -

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

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

          This looks OK for integration.

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

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

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

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

          Show
          Damyon Wiese added a comment - He has re-pushed without the white space change - ready to go.
          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
          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
          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
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - Thanks guys! integrated into master and backported to 2.3 cleanly.
          Hide
          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
          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
          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
          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
          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
          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
          David Monllaó added a comment -

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

          Show
          David Monllaó added a comment - Sorry Raymond, I would mean "when I click 'Upgrade all assignments'"
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          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: