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

          Attachments

            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