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

    • 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

      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

          Issue Links

            Activity

            Hide
            Sam Hemelryk added a comment -

            Linking to the issue where this issue was first noticed

            Show
            Sam Hemelryk added a comment - Linking to the issue where this issue was first noticed
            Hide
            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
            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
            Andrew Nicols added a comment -

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

            Show
            Andrew Nicols added a comment - I'd say it's probably worth only applying this to dev, not stable though.
            Hide
            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
            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
            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
            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 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 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
            Zachary Durber added a comment -

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

            Show
            Zachary Durber added a comment - More verbose testing instructions would help here. Or at least an example or two.
            Hide
            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
            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
            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
            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
            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
            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
            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
            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 added a comment -

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

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            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
            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 added a comment -

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

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            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
            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
            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
            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
            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
            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 added a comment -

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

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            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
            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 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 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
            Rajesh Taneja added a comment -

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

            Thanks everyone.

            Show
            Rajesh Taneja added a comment - This issue is not causing behat failure, hence passing this on behalf-of Ankit. Thanks everyone.
            Hide
            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
            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

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile