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

Thanks for spotting that.

Looks good to me!

Integrated, thanks! (21, 22 & master)

This needs testing instructions
Thanks

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

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.

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)';

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

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';

Confirmed.. this line isn't updated:

https://github.com/srynot4sale/moodle/blob/412cecc32bd33b108008791090f420b6892b4cac/lib/completion/completion_criteria_grade.php#L139

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.

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

New version of the patch added

Looks good to me, thanks Aaron

Pushing integration button.

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

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.

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

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

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

Closing this as fixed, changes are now available upstream.

Big thanks!