Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Critical
-
Resolution: Fixed
-
Affects Version/s: 2.1.1
-
Component/s: Gradebook
-
Testing Instructions:
-
Workaround:
-
Affected Branches:MOODLE_21_STABLE
-
Fixed Branches:MOODLE_21_STABLE, MOODLE_22_STABLE
-
Pull from Repository:
-
Pull Master Branch:
MDL-29173_calc -
Pull Master Diff URL:
-
Rank:18721
Description
When attempting to use the round function in the gradebook, If you set up a custom calculation along the lines of '=round([[ec]],2)' it will return the previous error.
When you track it this is being thrown in 'moodle/lib/evalmath/evalmath.class.php' line 256.
It looks like 'round' was added to the '$fb' array in moodle2+, even though it was no in moodle1.9
This makes it match on the first if statement, which only allows for one argument or throws an exception. In previous versions it was not in the '$fb' array only the '$fc' array (which it still is in moodle2+). My guess is removing it from the '$fb' array will fix the problem, but I am not sure if that will break anything else.
$fnn = $matches[1]; // get the function name $arg_count = $stack->pop(); // see how many arguments there were (cleverly stored on the stack, thank you) $fn = $stack->pop(); $output[] = array('fn'=>$fn, 'fnn'=>$fnn, 'argcount'=>$arg_count); // send function to output if (in_array($fnn, $this->fb)) { // check the argument count if($arg_count > 1) { $a= new stdClass(); $a->expected = 1; $a->given = $arg_count; return $this->trigger(get_string('wrongnumberofarguments', 'mathslib', $a)); } } elseif (array_key_exists($fnn, $this->fc)) { $counts = $this->fc[$fnn]; if (in_array(-1, $counts) and $arg_count > 0) {} elseif (!in_array($arg_count, $counts)) { $a= new stdClass(); $a->expected = implode('/',$this->fc[$fnn]); $a->given = $arg_count; return $this->trigger(get_string('wrongnumberofarguments', 'mathslib', $a)); } }
Replication instructions:
- Add a new grade item.
- Add a custom calculation.
- Use the 'round' function in that calculation.
- Will receive 'invalid formula' error (in non-development mode)
Issue Links
Activity
This issue has been duplicated, but I think the priority currently appropriate.
Due to this students are not able to see the points and grades in all courses where part of the grade was calculated using the round function.
This is not just a problem when you create a new course and new grade item based on formula with round. It is also the case for all old courses and we noticed it right after migration from Moodle 1.9.x
For us it was critical and we had to put the patch, althuough we are not sure if the patch is safe and does not have side-effects.
Just stumbled upon this in Moodle 2.2.2...
Thank you very much for the workaround!
Our university is planning to upgrade to Moodle 2.x from Moodle 1.9 this summer and we have used alot grade calculations in our courses.
I think, its a very critical issue and should be fixed ASAP.
Adding a branch containing a possible fix and some testing instructions.
I've put this up for peer review. I've also raised MDL-33112. I noticed some other functions that I suspect can simply be removed but I'll need to do more research.
Thanks Andrew,
Patch looks good to me. IMO you should backported this on 22 and 21 branches.
Adding branches for 2.2 and 2.1. Submitting for integration.
Hi Andrew,
Do you know if what you are changing is changes in the third party library or changes to our changes to that library?
I think that we might need to make a comment on this in the readme_moodle.txt (ideally we try to avoid any changes to third party libraries because it makes it difficult to track and also for upstreams (e.g. debian/ubuntu) to strip out our bundled libraries).
Adding Petr here.
Petr - could you give input on this?
Hi. These are changes to our changes. The addition of the round() function is already in the readme_moodle.txt file.
The more functions the better, if unsure how it should work look into Excel and do the same.
Thanks Andrew for the clarification.
I've integrated this now.
I'm failing this as it has broken unit tests
Looks like the function was designed to always round to 0 decimal places by the unit tests:
Unexpected PHP error [wrong number of arguments (1 given, 2 expected)] severity [512] in [/lib/evalmath/evalmath.class.php line 440]
1) mathsslib_testcase::test_rounding_function
Failed asserting that 3 matches expected false.
/Users/danp/git/integration/lib/tests/mathslib_test.php:146
/Users/danp/git/integration/lib/phpunit/classes/basic_testcase.php:64
Reverted this change, since it looks like it breaks backwards compatibility and fails unit tests on all branches.
For anyone who is interested, here is the Excel doco for round() http://office.microsoft.com/en-us/excel-help/round-HP005209239.aspx?CTT=5&origin=HP003056144
I've altered this code so that it will work with two arguments as Excel does. It will also work with only 1 argument in which case 0 decimal places is assumed.
That makes the unit tests work. I've added some new ones that test the 2 argument behaviour. The tests are the same in 2.1, 2.2 and in master although they are in different files due to changes in the Moodle unit test framework.
Hi Andrew,
(Not stricly doing a peer review)
I have just noticed a few lines above, round is already defined in array $fc, so actually I think we need to remove round completely there?
Good spotting. Fixed.
Thanks Andrew,
Looks Good, it might be nice to request tester, to run unit test.
Updated testing instructions. Submitting for integration.
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 
Thanks Andrew,
i've integrated that now
I was able to save =round([[x]]) as the formula (x being the id)
Grade of 1.2345 is converted to 1.00 in that case
it does generate error if I use something like =round([[x]],1,2)
Failing this, sorry
My fault. I forgot to update the testing instructions (which I have done now). That behaviour is correct. Both =round([[x]]) and =round([[x]], 1) are valid. If you only supply one parameter it assumes you want it rounded to 0 decimal places.
I dont seem to be able to reset this issue.
I am getting following warnings on 22, when entering an invalid formula
Warning: wrong number of arguments (3 given, 1/2 expected) in /var/www/int/22/moodle/lib/evalmath/evalmath.class.php on line 440
if you enter something like =round([[y]],1,1)
and
if you enter something like =round([[x]],1,1) (x is an invalid id) I get the following
Warning: illegal character '[' in /var/www/int/22/moodle/lib/evalmath/evalmath.class.php on line 440
Please advise
Thanks
That looks correct.
Hi Andrew,
These warnings do not appear on master. And if I was not clear earlier these are PHP(not moodle form) warnings displayed on top of the page, should I ignore them?
Thanks
ah. Ill take another look.
Yeah, I think we can ignore them. It looks like that at some point or other the eval class has had error suppression turned on in master. Its not directly related to this issue.
In lib/mathslib.php at line 53 master has
$this->_em->suppress_errors = true; // no PHP errors!
while 2.2 stable has
$this->_em->suppress_errors = !debugging('', DEBUG_DEVELOPER);
Okay Andrew,
I entered 1.23456 as the grade and it was displayed as below:-
1.23 1.24 for x and =round([[x]],3)
1.23 1.23 for x and =round([[x]],2)
Seems we round the result to two digits after decimal point to display.
(1.23456 -> 1.235 ->1.24)
(1.23456 -> 1.2346 -> 1.23)
Seems fine to me, after you confirm I can pass the test.
Thanks
Theres a course setting that can also reduce decimal places which you can see /grade/edit/settings/index.php?id=2 <= course id.
imo, can pass this.
Thanks for the clarification.
Passing!
Thankyou for your patience
We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.
Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.
Many thanks for your collaboration!
Ciao
I removed "round" from "fb" in our setup and that fixed the problem, but I don't know if this change has some other impact.