Moodle
  1. Moodle
  2. MDL-32249

Hard coded string "% required" in course completion

    Details

    • Testing Instructions:
      Hide

      To test:-
      Make sure course completion is enabled site-wide
      Create new course, enable completion
      Add Course Completion Status block to course
      Go to Completion settings for course, enable Course Grade criteria and enter a required grade with over 3 decimal places.
      Enrol student in course, and manually assign the student a grade via gradebook with a "long" grade e.g. 23.456743

      Old behaviour:
      Course Completion Status block, report doesn't round grades

      New behavour:
      Grades are rounded! (and whole numbers show no decimal places).

      Also, the "{$a} grade required" string in the block and report is no longer hardcoded in english.

      Show
      To test:- Make sure course completion is enabled site-wide Create new course, enable completion Add Course Completion Status block to course Go to Completion settings for course, enable Course Grade criteria and enter a required grade with over 3 decimal places. Enrol student in course, and manually assign the student a grade via gradebook with a "long" grade e.g. 23.456743 Old behaviour: Course Completion Status block, report doesn't round grades New behavour: Grades are rounded! (and whole numbers show no decimal places). Also, the "{$a} grade required" string in the block and report is no longer hardcoded in english.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
      git@github.com:srynot4sale/moodle.git
    • Pull Master Branch:
    • Rank:
      39031

      Description

      Thoughout course completion report the string "xx% required" is hard coded. It is possible to see the issue in action setting "grade" and a required grade value as course completion criteria, then switching to a language other than English.

      The string also report a "%" sign, but this will be fixed seperately as part of MDL-31635.

      The wording and code surrounding grades is also rather inconsistent, so I have renamed the criteria to be "Course Grade" and changed instances of "Passing grade" to "Required grade" as completing a course != passing it.

        Issue Links

          Activity

          Hide
          Aaron Barnes added a comment -

          Would be worth rounding grade % values at the same time also

          Show
          Aaron Barnes added a comment - Would be worth rounding grade % values at the same time also
          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that.

          Show
          Michael de Raadt added a comment - Thanks for spotting that.
          Hide
          Dan Poltawski added a comment -

          Looks good to me!

          Show
          Dan Poltawski added a comment - Looks good to me!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! (21, 22 & master)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 & master)
          Hide
          Ankit Agarwal added a comment -

          This needs testing instructions
          Thanks

          Show
          Ankit Agarwal added a comment - This needs testing instructions Thanks
          Hide
          Andrew Davis added a comment -

          tisk tisk, Aaron, Dan and Eloy.

          I attempted to test this even in the absence of testing instructions but was unable to make it work how I think it should. I added the new string to a non-English language pack, switched to that language but still always got the English string. I'm probably doing something wrong because of the lack of testing instructions

          Show
          Andrew Davis added a comment - tisk tisk, Aaron, Dan and Eloy. I attempted to test this even in the absence of testing instructions but was unable to make it work how I think it should. I added the new string to a non-English language pack, switched to that language but still always got the English string. I'm probably doing something wrong because of the lack of testing instructions
          Hide
          Aaron Barnes added a comment -

          Hi Andrew,

          My apologies!

          To test:-
          Make sure course completion is enabled site-wide
          Create new course, enable completion
          Add Course Completion Status block to course
          Go to Completion settings for course, enable Course Grade criteria and enter a pass grade with over 3 decimal places.
          Enrol student in course, and manually assign the student a grade via gradebook with a "long" grade e.g. 23.456743

          Old behaviour:
          Course Completion Status block doesn't round grades

          New behavour:
          Grades are rounded! (and whole numbers show no decimal places).

          Also, the "{$a} grade required" string in the block is no longer hardcoded in english.

          Show
          Aaron Barnes added a comment - Hi Andrew, My apologies! To test:- Make sure course completion is enabled site-wide Create new course, enable completion Add Course Completion Status block to course Go to Completion settings for course, enable Course Grade criteria and enter a pass grade with over 3 decimal places. Enrol student in course, and manually assign the student a grade via gradebook with a "long" grade e.g. 23.456743 Old behaviour: Course Completion Status block doesn't round grades New behavour: Grades are rounded! (and whole numbers show no decimal places). Also, the "{$a} grade required" string in the block is no longer hardcoded in english.
          Hide
          Andrew Davis added a comment - - edited

          Maybe Im doing this wrong but I'm still having trouble. The rounding doesn't seem to be occurring.

          Also, I'm attaching screenshots of the Course Completion Status block and the course completion report in Arabic. I added the following to /lang/ar/completion.php in the moodle data directory and purged the caches using the purge all caches link but it remains untranslated.

          $string['gradetopass']='{$a} to pass (in arabic)';
          Show
          Andrew Davis added a comment - - edited Maybe Im doing this wrong but I'm still having trouble. The rounding doesn't seem to be occurring. Also, I'm attaching screenshots of the Course Completion Status block and the course completion report in Arabic. I added the following to /lang/ar/completion.php in the moodle data directory and purged the caches using the purge all caches link but it remains untranslated. $string['gradetopass']='{$a} to pass (in arabic)';
          Hide
          Andrew Davis added a comment -

          Aaron doesn't seem to be online right now so calling this failed pending more information.

          Show
          Andrew Davis added a comment - Aaron doesn't seem to be online right now so calling this failed pending more information.
          Hide
          Andrew Davis added a comment -

          Testing against master by the way. Also, the following stuff in the testing instructions needs clarification.

          "Course Completion Status block doesn't round grades"
          The course completion status block seem to even show grades. Do you mean the block or the report?

          "Also, the "{$a} grade required" string in the block is no longer hardcoded in english."

          I assume you mean...

          $string['gradetopass']='{$a} to pass';
          Show
          Andrew Davis added a comment - Testing against master by the way. Also, the following stuff in the testing instructions needs clarification. "Course Completion Status block doesn't round grades" The course completion status block seem to even show grades. Do you mean the block or the report? "Also, the "{$a} grade required" string in the block is no longer hardcoded in english." I assume you mean... $string['gradetopass']='{$a} to pass';
          Show
          Dan Poltawski added a comment - Confirmed.. this line isn't updated: https://github.com/srynot4sale/moodle/blob/412cecc32bd33b108008791090f420b6892b4cac/lib/completion/completion_criteria_grade.php#L139
          Hide
          Eloy Lafuente (stronk7) added a comment -

          oops, sorry, I forgot to detect missing testing instructions. Thanks for spotting it Andrew.

          So... what if we fix that missing use and done? It seems that Aaron is not around here this week.

          Show
          Eloy Lafuente (stronk7) added a comment - oops, sorry, I forgot to detect missing testing instructions. Thanks for spotting it Andrew. So... what if we fix that missing use and done? It seems that Aaron is not around here this week.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm... finally, I'm reverting this, because while trying to fix the not-changed occurrence I discovered / was unsure about:

          • $gradetopass in get_staus() is undefined. Surely it is $gradepass but...
          • Can we safely assume that the currently untranslated use @ get_title_detailed() can be moved to the new string. Or should it be another one. Note that it always use one % applied to $this->gradepass. And in get_status() it is exactly the opposite, and $this->gradepass is used for the non-percentage part of the information.
          • It's not clear if we are talking about the block, the report or both.
          • There is also MDL-31635, that seems related to this, but I'm not sure about how/where.
          • Issue title/description (report) and test instructions (block) seem inconsistent. Need clarification.

          So, given those... reverting and reopening this... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm... finally, I'm reverting this, because while trying to fix the not-changed occurrence I discovered / was unsure about: $gradetopass in get_staus() is undefined. Surely it is $gradepass but... Can we safely assume that the currently untranslated use @ get_title_detailed() can be moved to the new string. Or should it be another one. Note that it always use one % applied to $this->gradepass. And in get_status() it is exactly the opposite, and $this->gradepass is used for the non-percentage part of the information. It's not clear if we are talking about the block, the report or both. There is also MDL-31635 , that seems related to this, but I'm not sure about how/where. Issue title/description (report) and test instructions (block) seem inconsistent. Need clarification. So, given those... reverting and reopening this... ciao
          Hide
          Aaron Barnes added a comment -

          New version of the patch added

          Show
          Aaron Barnes added a comment - New version of the patch added
          Hide
          Dan Poltawski added a comment -

          Looks good to me, thanks Aaron

          Show
          Dan Poltawski added a comment - Looks good to me, thanks Aaron
          Hide
          Dan Poltawski added a comment -

          Pushing integration button.

          Show
          Dan Poltawski added a comment - Pushing integration button.
          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
          Eloy Lafuente (stronk7) added a comment -

          Sorry, I'm reopening this because it's conflicting specially in the lang strings, where it seems that the same ones were modified by another issue by introducing {$a} variables.

          So sure it needs review and, ideally, provide patches for each branch needing fix.

          Apologizes for the delay, but until today, with all the 2.3 maremagnum, I have not realized this was awaiting since some weeks ago.

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, I'm reopening this because it's conflicting specially in the lang strings, where it seems that the same ones were modified by another issue by introducing {$a} variables. So sure it needs review and, ideally, provide patches for each branch needing fix. Apologizes for the delay, but until today, with all the 2.3 maremagnum, I have not realized this was awaiting since some weeks ago.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for the updated Aaron, re-looking...

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for the updated Aaron, re-looking...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (21, 22, 23 & master), thanks!

          Note 21_STABLE is being moved out from support along these days so future fixes won't apply there.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (21, 22, 23 & master), thanks! Note 21_STABLE is being moved out from support along these days so future fixes won't apply there. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tested on 22_STABLE and master. Everything works as defined. Patch is 100% the same for 21 and 23, so passing.

          Show
          Eloy Lafuente (stronk7) added a comment - Tested on 22_STABLE and master. Everything works as defined. Patch is 100% the same for 21 and 23, so passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing this as fixed, changes are now available upstream.

          Big thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Closing this as fixed, changes are now available upstream. Big thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: