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

Uses of M.str.{component}.{identifier} should be reviewed and converted to M.util.get_string where possible to avoid needless JS errors from missing strings

    Details

    • Type: Task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.9
    • Component/s: JavaScript
    • Labels:
    • Testing Instructions:
      Hide

      In reality, this is pretty irritating to fully test. Behat will cover some of it, but many of the changes are to error strings or exception strings that you will struggle to force through the UI.

      Running behat will be of use, but will not cover everything. Most are trivial changes, but the following areas could do with general testing for string issues:

      1. Availability builder
      2. File picker
      3. File manager
      4. Enrolment

      The things you are looking for are JS errors in the console; and missing strings.

      I also recommend carefully reading each of the changes in the diff. For the most part they consist of replacing:

      M.str.component.identifier
      

      with

      M.util.get_string('identifier', 'component')
      

      Show
      In reality, this is pretty irritating to fully test. Behat will cover some of it, but many of the changes are to error strings or exception strings that you will struggle to force through the UI. Running behat will be of use, but will not cover everything. Most are trivial changes, but the following areas could do with general testing for string issues: Availability builder File picker File manager Enrolment The things you are looking for are JS errors in the console; and missing strings. I also recommend carefully reading each of the changes in the diff. For the most part they consist of replacing: M.str.component.identifier with M.util.get_string('identifier', 'component')
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_29_STABLE
    • Pull Master Branch:
      MDL-28286-master
    • Sprint:
      FRONTEND Sprint 15
    • Sprint:
      FRONTEND Sprint 15

      Description

      Just an obvious win - we now have M.util.get_string in javascript-static - it would be nice to use it to avoid any missing string errors.

      Moodle admins - it may be nice to have a couple of new components `JavaScript` and `Page and Output`

      Cheers
      Sam

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              samhemelryk Sam Hemelryk added a comment -

              Linking to the issue where this issue was first noticed

              Show
              samhemelryk Sam Hemelryk added a comment - Linking to the issue where this issue was first noticed
              Hide
              dobedobedoh Andrew Nicols added a comment -

              +1 for this - I came across some in a peer review earlier too. It may be time to start writing JS guidelines which cover these kinds of things, and can be referenced by reviewers.

              Show
              dobedobedoh Andrew Nicols added a comment - +1 for this - I came across some in a peer review earlier too. It may be time to start writing JS guidelines which cover these kinds of things, and can be referenced by reviewers.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              I'd say it's probably worth only applying this to dev, not stable though.

              Show
              dobedobedoh Andrew Nicols added a comment - I'd say it's probably worth only applying this to dev, not stable though.
              Hide
              dobedobedoh Andrew Nicols added a comment - - edited

              We now have guidelines! Huzzah.
              It would be great to do this, but I suspect best to wait until after on sync. There are only about 30 files where we do this, and I favour a big bang approach. Testing should be safe to be a careful read of the diff, and allow behat. This will make things more reliable (no js errors, just console warnings) when strings are forgotten.

              Show
              dobedobedoh Andrew Nicols added a comment - - edited We now have guidelines! Huzzah. It would be great to do this, but I suspect best to wait until after on sync. There are only about 30 files where we do this, and I favour a big bang approach. Testing should be safe to be a careful read of the diff, and allow behat. This will make things more reliable (no js errors, just console warnings) when strings are forgotten.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              We are out of the on-sync period.
              I think it would be great to hit this in one foul swoop and eliminate all uses of M.str now.
              I only found one particularly nasty use of it, and that was in lib/yui/src/dock/js/dock.js and was a nasty fix for IE6/7. No point entirely removing the hack, but I have corrected it to not be a hack

              Show
              dobedobedoh Andrew Nicols added a comment - We are out of the on-sync period. I think it would be great to hit this in one foul swoop and eliminate all uses of M.str now. I only found one particularly nasty use of it, and that was in lib/yui/src/dock/js/dock.js and was a nasty fix for IE6/7. No point entirely removing the hack, but I have corrected it to not be a hack
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-28286 using repository: git://github.com/andrewnicols/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28286 using repository: git://github.com/andrewnicols/moodle.git master (branch: MDL-28286-master | CI Job ) Coding style problems found More information about this report
              Hide
              zac Zachary Durber added a comment -

              More verbose testing instructions would help here. Or at least an example or two.

              Show
              zac Zachary Durber added a comment - More verbose testing instructions would help here. Or at least an example or two.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              As I say in my testing instructions, this is really hard to test. I've added a bit more, but there really isn't much more to add here that's of any help.

              Show
              dobedobedoh Andrew Nicols added a comment - As I say in my testing instructions, this is really hard to test. I've added a bit more, but there really isn't much more to add here that's of any help.
              Hide
              fred Frédéric Massart added a comment -

              Thanks Andrew,

              that was the most interesting review of all time . I think that it is a good idea to do that now, this early in the cycle, because it will give us time to fix potential issues with this change, though I cannot see any.

              There are maybe 1 or 2 remaining strings that could have been missed, I did a grep for "M.str[" (I did not grep M.str.), could you please fix those?

              Also, I was wondering if we could add a rule to jslint to warn when we are using M.str[\[.], that would be handy, or maybe that should be part of the codechecker, I don't know, just throwing the idea out there.

              Don't you need to compile the shifter modules that have been changed?

              Cheers!
              Fred

              Show
              fred Frédéric Massart added a comment - Thanks Andrew, that was the most interesting review of all time . I think that it is a good idea to do that now, this early in the cycle, because it will give us time to fix potential issues with this change, though I cannot see any. There are maybe 1 or 2 remaining strings that could have been missed, I did a grep for "M.str[" (I did not grep M.str.), could you please fix those? Also, I was wondering if we could add a rule to jslint to warn when we are using M.str [\[.] , that would be handy, or maybe that should be part of the codechecker, I don't know, just throwing the idea out there. Don't you need to compile the shifter modules that have been changed? Cheers! Fred
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Fred, Pushed the shifted version. I'd forgotten to push (grr).
              I think I'd just missed one case of M.str in the availability API which I've just pushed now.

              I've raised MDLSITE-3646 too - thanks for the prompt. I'd planned to raise and suggest this already, but there's no time like the present!

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Fred, Pushed the shifted version. I'd forgotten to push (grr). I think I'd just missed one case of M.str in the availability API which I've just pushed now. I've raised MDLSITE-3646 too - thanks for the prompt. I'd planned to raise and suggest this already, but there's no time like the present!
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Submitting for integration.

              FTR, the grep I used was:

              git grep M\.str
              

              This picks up all uses of M.str. There are two exceptions we allow:

              1. lib/outputrequirements.php - to insert the strings
              2. lib/javascript-static.js in M.util.get_string()
              Show
              dobedobedoh Andrew Nicols added a comment - Submitting for integration. FTR, the grep I used was: git grep M\.str This picks up all uses of M.str. There are two exceptions we allow: lib/outputrequirements.php - to insert the strings lib/javascript-static.js in M.util.get_string()
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi Andrew,

              Sending it back this week sorry Andrew - there are a few problems here.

              1. availability/yui/src/form/js/form.js:594: Wrong function name => M.util.getstring

              A full behat run turned up several failures - Looks like they are all related to that one bug but I think it'd probably be best to fix up the issue above and then run the full behat suite to check for any further remaining issues (I couldn't spot any others).

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi Andrew, Sending it back this week sorry Andrew - there are a few problems here. availability/yui/src/form/js/form.js:594: Wrong function name => M.util. getstring A full behat run turned up several failures - Looks like they are all related to that one bug but I think it'd probably be best to fix up the issue above and then run the full behat suite to check for any further remaining issues (I couldn't spot any others). Cheers Sam
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Sam,

              Buggered I missed that one. I've got it running behat now, but that should be the only issue.

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Sam, Buggered I missed that one. I've got it running behat now, but that should be the only issue. Andrew
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Sam,

              Submitting for integration again. I'm running behat now against master with that patch. Not expecting any related failures.

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Sam, Submitting for integration again. I'm running behat now against master with that patch. Not expecting any related failures. Andrew
              Hide
              poltawski Dan Poltawski 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
              poltawski Dan Poltawski 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
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Andrew this has been integrated now. Just noting that I cherry-picked the relevant commit as there were three commits, for three issues on your branch. One that has already landed and one that is held presently.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Andrew this has been integrated now. Just noting that I cherry-picked the relevant commit as there were three commits, for three issues on your branch. One that has already landed and one that is held presently.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              I randomly tested a few things, in the mentioned area. All looks good.

              This can be passed once, CI confirms behats are passing.

              Cheers

              Show
              ankit_frenz Ankit Agarwal added a comment - I randomly tested a few things, in the mentioned area. All looks good. This can be passed once, CI confirms behats are passing. Cheers
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              This issue is not causing behat failure, hence passing this on behalf-of Ankit.

              Thanks everyone.

              Show
              rajeshtaneja Rajesh Taneja added a comment - This issue is not causing behat failure, hence passing this on behalf-of Ankit. Thanks everyone.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This code is now part of Moodle, the open source learning beauty.

              Experiences all over the world will be a bit better with this fixed and you did it!

              (`’·.¸ (`’·.¸*¤* ¸.·’´)¸.·’)
              ♥ ** Many, many thanks! ** ♥
              (¸.·’´(¸.·’´* :* `’·.¸)`’·.)
              

              Closing, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This code is now part of Moodle, the open source learning beauty. Experiences all over the world will be a bit better with this fixed and you did it! (`’·.¸ (`’·.¸*¤* ¸.·’´)¸.·’) ♥ ** Many, many thanks! ** ♥ (¸.·’´(¸.·’´* :* `’·.¸)`’·.) Closing, ciao
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Removing dev_docs_required - this is long-since documented.

              Show
              dobedobedoh Andrew Nicols added a comment - Removing dev_docs_required - this is long-since documented.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/May/15