Moodle
  1. Moodle
  2. MDL-20617

More controls for the User report (show/hide feedback, averages, range)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.4, 1.9.5, 1.9.6
    • Fix Version/s: 2.0.2
    • Component/s: Gradebook
    • Labels:
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      5684

      Description

      I have seen a lot of similar requests on forum, tracker and from our local users. So I believe there is a need to develop more gradebook settings for the User report.

      • Show/hide Averages
      • Show/hide Feedback
      • Show/hide Range

      This was already done for Percentages: MDL-15887, so it is just another improvement.

      As usual, those can be forced (hidden from instructors) on the admin level, to make the gradebook Settings page less clutter if needed.
      Overall I believe that we need to have Force and Advanced checkboxes for almost all settings done by the admins, as it makes system a lot more flexible.

      Related tickets: MDL-18722, MDL-16809;
      posts :
      http://moodle.org/mod/forum/discuss.php?d=135843;
      http://moodle.org/mod/forum/discuss.php?d=119925;
      http://moodle.org/mod/forum/discuss.php?d=135357;
      http://moodle.org/mod/forum/discuss.php?d=120598

      1. 20101207user_report20.patch
        25 kB
        Andrew Davis
      2. 20101213user_report20.patch
        26 kB
        Andrew Davis
      3. user_report19.patch
        27 kB
        Robert Puffer
      4. user_report19.patch
        28 kB
        Robert Puffer
      5. user_report20.patch
        27 kB
        Robert Puffer

        Issue Links

          Activity

          Hide
          Susan Mangan added a comment -

          yes! yes! yes! yes! please add this loooooong feedback really clutters up the students' gradebook view.

          Show
          Susan Mangan added a comment - yes! yes! yes! yes! please add this loooooong feedback really clutters up the students' gradebook view.
          Hide
          Susan Mangan added a comment -

          Possible solution for displaying feedback? I wanted to hide feedback in the user report as some user reports are unweildy and cluttered when there is a lot of feedback.... and because the user can access the feedback via the activity itself I thought that hiding the feedback column wouldn't be an issue. However, when I did this I realized that another problem may present if the teacher creates a manual graded item and adds feedback here. Currently the only place for a user to view this feedback is via the user report. conundrum What would be IDEAL (I think) is if a feedback link could be inserted into that column where the user can opt to open a new window to view his or her feedback if they wish while still maintaining a neat and tidy gradebook view.

          Show
          Susan Mangan added a comment - Possible solution for displaying feedback? I wanted to hide feedback in the user report as some user reports are unweildy and cluttered when there is a lot of feedback.... and because the user can access the feedback via the activity itself I thought that hiding the feedback column wouldn't be an issue. However, when I did this I realized that another problem may present if the teacher creates a manual graded item and adds feedback here. Currently the only place for a user to view this feedback is via the user report. conundrum What would be IDEAL (I think) is if a feedback link could be inserted into that column where the user can opt to open a new window to view his or her feedback if they wish while still maintaining a neat and tidy gradebook view.
          Hide
          Kevin Wiliarty added a comment -

          It would be great to be able to choose among possible columns for the user report. Admins should be able to set defaults. Teachers should be able to override them.

          Show
          Kevin Wiliarty added a comment - It would be great to be able to choose among possible columns for the user report. Admins should be able to set defaults. Teachers should be able to override them.
          Hide
          Elena Ivanova added a comment -

          knock, knock
          Any chance this can be moved in priority? Seems to be relatively easy to implement, and such feature is being asked for a lot.
          http://moodle.org/mod/forum/discuss.php?d=154530#p710181

          Show
          Elena Ivanova added a comment - knock, knock Any chance this can be moved in priority? Seems to be relatively easy to implement, and such feature is being asked for a lot. http://moodle.org/mod/forum/discuss.php?d=154530#p710181
          Hide
          Robert Puffer added a comment -

          I'm quite familiar with the code on this (we implemented it on the laeuser report) so would be glad to pick it up for 1.9x and 2.0 unless there's some disagreement.

          Show
          Robert Puffer added a comment - I'm quite familiar with the code on this (we implemented it on the laeuser report) so would be glad to pick it up for 1.9x and 2.0 unless there's some disagreement.
          Hide
          Andrew Davis added a comment -

          I should be able to get to this shortly. Hopefully. As ever, other things frequently come up. Robert, if you are able to start before me by all means go ahead

          Show
          Andrew Davis added a comment - I should be able to get to this shortly. Hopefully. As ever, other things frequently come up. Robert, if you are able to start before me by all means go ahead
          Hide
          Robert Puffer added a comment -

          I've put this together but with one hanging question... calculating averages is processor-intensive and the existing code would need to iterate through that for each User report on-screen. Do the interested parties believe as I do that average could be included only when one user report is being requested?

          Show
          Robert Puffer added a comment - I've put this together but with one hanging question... calculating averages is processor-intensive and the existing code would need to iterate through that for each User report on-screen. Do the interested parties believe as I do that average could be included only when one user report is being requested?
          Hide
          Elena Ivanova added a comment -

          I would agree

          p.s. I have noticed that in 2.0 User report the Range column is no longer shown (with no way to allow it back). I guess something changed in the code. Just want to note that it is a useful column for students to see.

          p.p.s. I really like Susan's suggestion for displaying feedback in a pop-up.

          Show
          Elena Ivanova added a comment - I would agree p.s. I have noticed that in 2.0 User report the Range column is no longer shown (with no way to allow it back). I guess something changed in the code. Just want to note that it is a useful column for students to see. p.p.s. I really like Susan's suggestion for displaying feedback in a pop-up.
          Hide
          Robert Puffer added a comment -

          PATCH ATTACHED
          Here's the patch and a zip of the files changed for this issue for 1.9x (built on 1.9.10 20101025 but the patch applies cleanly and workably to all the other versions of 1.9 I have in-house):

          • grade/report/user/lib.php
          • grade/report/user/settings.php
          • grade/report/user/styles.php
          • lang/en_utf8/grades.php
          • grade/report/user/index.php

          NOTES:

          1. This makes eight columns configurable for the user report:
            1. Weight
            2. Points
            3. Range
            4. Percentage
            5. Letter grade
            6. Rank
            7. Average
            8. Feedback
          2. styles.php has been changed to allow for feedback formatting and centering of cell contents (can be edited to admin's desires)
          3. have forced range display to points (real) – doesn't make any sense to me any other way
          4. when you display the "real" value this way it sure makes me want to deal with accurate point totals and ranges – but, alas, I've left that off (we've been using accurate point totals and ranges for a year now)
          5. averages: show up on individual user reports only not when all the users' reports are paginated; Also, doesn't make much sense to me that average decimal display should be a preference but I left it at that... just couldn't bring myself to leave the ranges relying on a preference for decimals so that's configurable


          Will get to work on 2.0

          Show
          Robert Puffer added a comment - PATCH ATTACHED Here's the patch and a zip of the files changed for this issue for 1.9x (built on 1.9.10 20101025 but the patch applies cleanly and workably to all the other versions of 1.9 I have in-house): grade/report/user/lib.php grade/report/user/settings.php grade/report/user/styles.php lang/en_utf8/grades.php grade/report/user/index.php NOTES: This makes eight columns configurable for the user report: Weight Points Range Percentage Letter grade Rank Average Feedback styles.php has been changed to allow for feedback formatting and centering of cell contents (can be edited to admin's desires) have forced range display to points (real) – doesn't make any sense to me any other way when you display the "real" value this way it sure makes me want to deal with accurate point totals and ranges – but, alas, I've left that off (we've been using accurate point totals and ranges for a year now) averages: show up on individual user reports only not when all the users' reports are paginated; Also, doesn't make much sense to me that average decimal display should be a preference but I left it at that... just couldn't bring myself to leave the ranges relying on a preference for decimals so that's configurable Will get to work on 2.0
          Hide
          Robert Puffer added a comment -

          Removed a couple notices I'd not previously detected. 2.0 is right behind.

          Show
          Robert Puffer added a comment - Removed a couple notices I'd not previously detected. 2.0 is right behind.
          Hide
          Robert Puffer added a comment -

          PATCH ATTACHED
          Based on 2.0 RC2
          Same specs as 1.9 above but slight changes in averaging code as currently does some funky things when you "login as" a student (wants to to re-enroll in the course and actually creates another role_assignment record). Otherwise, very little change from 1.9 to 2.0.

          Show
          Robert Puffer added a comment - PATCH ATTACHED Based on 2.0 RC2 Same specs as 1.9 above but slight changes in averaging code as currently does some funky things when you "login as" a student (wants to to re-enroll in the course and actually creates another role_assignment record). Otherwise, very little change from 1.9 to 2.0.
          Hide
          Robert Puffer added a comment -

          Hi Andrew... wondering if you'd be so kind as to review the code submitted for this issue in the hopes it might get in the next release? Cheers.

          Show
          Robert Puffer added a comment - Hi Andrew... wondering if you'd be so kind as to review the code submitted for this issue in the hopes it might get in the next release? Cheers.
          Hide
          Andrew Davis added a comment -

          Hi Robert. I'm halfway done reviewing this. I'll finish up tomorrow. Thanks for the work

          Show
          Andrew Davis added a comment - Hi Robert. I'm halfway done reviewing this. I'll finish up tomorrow. Thanks for the work
          Hide
          Andrew Davis added a comment - - edited

          Note: This is all about your 2.0 patch. Haven't yet looked at the 1.9 version.

          Attaching my version of your patch. In the interests of speed I've made changes myself rather than us going back and forth a bunch of times. Let me know if I've broken anything or if I've misunderstood your intent anywhere.

          When you introduce new settings you need to put up the version number. That will cause the admin to be prompted about the new settings and defaults to be inserted. Otherwise you get a lot of php warnings about $CFG->grade_report_user_showpoints, for example, not existing.

          I've altered the new settings (and one of the old ones) so that the defaults mirror 1.9. Having all the columns hidden by default will lead to confusion.

          Re grade_report_user_showaverage_ur. Read aloud that's "grade report user show average user report". Not sure its necessary to repeat the words user and report. Also, in Moodle code, try and stay away from acronyms. They tend to be a bit hard to decipher especially for programmers for whom English isn't their first language.

          Points? You're American aren't you? Throughout the Moodle code we use the terminology grade item and grade. For consistency within the code I've substituted the word grade for points so it matches the rest of the code.

          I've shortened the setting descriptions and removed repetitions of words from the strings. Strings like that are of course very subjective however I generally believe that the less text on screen the more likely it is that a user will actually read it.

          I've also removed all the code you commented out.

          I believe you were accessing the wrong preference in grade_report_user::user_avg(). If you look in grade_report::get_pref() the preference name has 'grade_report_' added to the beginning. If you just supply 'showaverages' the preference you actually get is 'grade_report_showaverages', the preference for the grader report, and not 'grade_report_user_showaverage', the preference for the user report. Supplying 'user_showaverage' makes it work. Can you please check the other instances of calls to $this->get_pref() and make sure that they are getting the preference you really want?

          Also, within grade_report_user::user_avg() I've shifted everything to be within the if block. Trying to do as little work as necessary if the average isn't going to be displayed anyway.

          That's it

          Show
          Andrew Davis added a comment - - edited Note: This is all about your 2.0 patch. Haven't yet looked at the 1.9 version. Attaching my version of your patch. In the interests of speed I've made changes myself rather than us going back and forth a bunch of times. Let me know if I've broken anything or if I've misunderstood your intent anywhere. When you introduce new settings you need to put up the version number. That will cause the admin to be prompted about the new settings and defaults to be inserted. Otherwise you get a lot of php warnings about $CFG->grade_report_user_showpoints, for example, not existing. I've altered the new settings (and one of the old ones) so that the defaults mirror 1.9. Having all the columns hidden by default will lead to confusion. Re grade_report_user_showaverage_ur. Read aloud that's "grade report user show average user report". Not sure its necessary to repeat the words user and report. Also, in Moodle code, try and stay away from acronyms. They tend to be a bit hard to decipher especially for programmers for whom English isn't their first language. Points? You're American aren't you? Throughout the Moodle code we use the terminology grade item and grade. For consistency within the code I've substituted the word grade for points so it matches the rest of the code. I've shortened the setting descriptions and removed repetitions of words from the strings. Strings like that are of course very subjective however I generally believe that the less text on screen the more likely it is that a user will actually read it. I've also removed all the code you commented out. I believe you were accessing the wrong preference in grade_report_user::user_avg(). If you look in grade_report::get_pref() the preference name has 'grade_report_' added to the beginning. If you just supply 'showaverages' the preference you actually get is 'grade_report_showaverages', the preference for the grader report, and not 'grade_report_user_showaverage', the preference for the user report. Supplying 'user_showaverage' makes it work. Can you please check the other instances of calls to $this->get_pref() and make sure that they are getting the preference you really want? Also, within grade_report_user::user_avg() I've shifted everything to be within the if block. Trying to do as little work as necessary if the average isn't going to be displayed anyway. That's it
          Hide
          Robert Puffer added a comment -

          Andrew, your 2.0 patch appears right and the patch installs with error except for the styles.css file. Thanks for the updates and corrections.

          Show
          Robert Puffer added a comment - Andrew, your 2.0 patch appears right and the patch installs with error except for the styles.css file. Thanks for the updates and corrections.
          Hide
          Robert Puffer added a comment -

          sorry... meant to say "without error except for styles.css"

          Show
          Robert Puffer added a comment - sorry... meant to say "without error except for styles.css"
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Robert thanks for the work so far .

          Andrew cheers for tiding it up for 2.0, I've just been reviewing the patch and in general everything looks good, however there are a couple of things I spotted within grade_report_user that should be cleaned up before committing.

          1. There are unused globals that should be removed.
          2. Change {$CFG->prefix}tablename to {tablename}
          3. line 550: should probably be using a recordset for both performance and because $sums is temporary.
          4. Line 573: It looks like $ungradedcounts is an unused var~query.
          5. It's probably a good idea to get Helen's ideas about the strings used for the new settings, particually the descriptions.
          6. PHP Notice: Undefined property: grade_item::$avg in moodle/grade/report/user/lib.php on line 391, referer: moodle/grade/report/user/index.php?id=7

          Once those have been fixed up it has my +1, although we should probably just get Martins +1 for the new settings.

          The only other thing worth mentioning is that when viewing the user report for all users it doesn't show the averages, I've seen the comment that it is done that way for performance reasons however I think we should either remove the column completely or turn it back on, otherwise it looks as though there are no averages.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Robert thanks for the work so far . Andrew cheers for tiding it up for 2.0, I've just been reviewing the patch and in general everything looks good, however there are a couple of things I spotted within grade_report_user that should be cleaned up before committing. There are unused globals that should be removed. Change {$CFG->prefix}tablename to {tablename} line 550: should probably be using a recordset for both performance and because $sums is temporary. Line 573: It looks like $ungradedcounts is an unused var~query. It's probably a good idea to get Helen's ideas about the strings used for the new settings, particually the descriptions. PHP Notice: Undefined property: grade_item::$avg in moodle/grade/report/user/lib.php on line 391, referer: moodle/grade/report/user/index.php?id=7 Once those have been fixed up it has my +1, although we should probably just get Martins +1 for the new settings. The only other thing worth mentioning is that when viewing the user report for all users it doesn't show the averages, I've seen the comment that it is done that way for performance reasons however I think we should either remove the column completely or turn it back on, otherwise it looks as though there are no averages. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Sorry I should clarify, are you intending to fix both 1.9 and 2.0 or just 2.0?

          Show
          Sam Hemelryk added a comment - Sorry I should clarify, are you intending to fix both 1.9 and 2.0 or just 2.0?
          Hide
          Andrew Davis added a comment -

          I was thinking about 2.0 only but http://moodle.org/mod/forum/discuss.php?d=162754#p718956 it looks like 1.9 is actually broken right now so will have to backport I guess.

          Show
          Andrew Davis added a comment - I was thinking about 2.0 only but http://moodle.org/mod/forum/discuss.php?d=162754#p718956 it looks like 1.9 is actually broken right now so will have to backport I guess.
          Hide
          Andrew Davis added a comment -

          I've fixed all of those items except for this one. "Line 573: It looks like $ungradedcounts is an unused var~query." It is actually used its just that it was putting the result of the query in $ungradedcounts then referencing $ungraded_counts. Unfortunately correcting that has broken the calculation of averages Figuring that out now.

          I'm also trying to come up with something to avoid recalculating the averages for each grade item for each user when you're showing all users.

          Show
          Andrew Davis added a comment - I've fixed all of those items except for this one. "Line 573: It looks like $ungradedcounts is an unused var~query." It is actually used its just that it was putting the result of the query in $ungradedcounts then referencing $ungraded_counts. Unfortunately correcting that has broken the calculation of averages Figuring that out now. I'm also trying to come up with something to avoid recalculating the averages for each grade item for each user when you're showing all users.
          Hide
          Andrew Davis added a comment -

          New patch attached. I believe all the issues mentioned above are resolved.

          I gave up on caching the averages when multiple user reports are requested at once as the average take into account groups so they may vary from student to student. A cache of averages would have to be quite clever (and would possibly even want to store stuff in the database)

          Show
          Andrew Davis added a comment - New patch attached. I believe all the issues mentioned above are resolved. I gave up on caching the averages when multiple user reports are requested at once as the average take into account groups so they may vary from student to student. A cache of averages would have to be quite clever (and would possibly even want to store stuff in the database)
          Hide
          Andrew Davis added a comment -

          Note that as we speak we are switching from CVS to Git. This code will have to be reviewed and dealt with through the new git-centric process. I'll update once I have a better understanding of exactly what that is.

          Show
          Andrew Davis added a comment - Note that as we speak we are switching from CVS to Git. This code will have to be reviewed and dealt with through the new git-centric process. I'll update once I have a better understanding of exactly what that is.
          Hide
          Andrew Davis added a comment -

          Code is available from git://github.com/andyjdavis/moodle.git branch is MDL-20617 Its for 2.0. After we're happy with this in 2.0 I'll backport it to 1.9.

          Show
          Andrew Davis added a comment - Code is available from git://github.com/andyjdavis/moodle.git branch is MDL-20617 Its for 2.0. After we're happy with this in 2.0 I'll backport it to 1.9.
          Hide
          Andrew Davis added a comment -

          will do the 1.9 version before this is formally reviewed.

          Show
          Andrew Davis added a comment - will do the 1.9 version before this is formally reviewed.
          Hide
          Petr Škoda added a comment -

          I do not like the huge joins there at all, it should definitely use the new enrolment table to restrict the scope of the join...

          Show
          Petr Škoda added a comment - I do not like the huge joins there at all, it should definitely use the new enrolment table to restrict the scope of the join...
          Hide
          Petr Škoda added a comment -

          The SQL in 2.0 is going to be very different from 1.9, I would personally vote for working on problems from 2.0 only because you might hit nasty performance issues or regression in 1:9.

          Show
          Petr Škoda added a comment - The SQL in 2.0 is going to be very different from 1.9, I would personally vote for working on problems from 2.0 only because you might hit nasty performance issues or regression in 1:9.
          Hide
          Andrew Davis added a comment -

          I've committed a really really simple fix to a 1.9 branch that just makes the range column re-appear. Diff is at https://github.com/andyjdavis/moodle/compare/MOODLE_19_STABLE...MDL-20617_1-9#

          Will fix up 2.0 version now.

          Show
          Andrew Davis added a comment - I've committed a really really simple fix to a 1.9 branch that just makes the range column re-appear. Diff is at https://github.com/andyjdavis/moodle/compare/MOODLE_19_STABLE...MDL-20617_1-9# Will fix up 2.0 version now.
          Hide
          Andrew Davis added a comment -

          Having trouble switching to the new enorolment table. The original query looks like this

          SELECT gi.id, COUNT(u.id) AS count
          FROM mdl_grade_items gi
          CROSS JOIN mdl_user u
          JOIN mdl_role_assignments ra ON ra.userid = u.id
          LEFT OUTER JOIN mdl_grade_grades g ON (g.itemid = gi.id AND g.userid = u.id AND g.finalgrade IS NOT NULL)

          WHERE gi.courseid = 2
          AND ra.roleid in (5)
          AND ra.contextid IN (11,3,1)
          AND g.id IS NULL
          GROUP BY gi.id

          (role.id==5 is student)

          Replacement query looks like
          SELECT gi.id, COUNT(u.id) AS count
          FROM mdl_grade_items gi
          CROSS JOIN mdl_user u
          JOIN mdl_user_enrolments ue ON ue.userid = u.id
          JOIN mdl_enrol e ON e.id=ue.enrolid
          LEFT OUTER JOIN mdl_grade_grades g ON (g.itemid = gi.id AND g.userid = u.id AND g.finalgrade IS NOT NULL)

          WHERE gi.courseid = 2
          AND e.courseid = 2
          AND ue.status = 0
          AND g.id IS NULL
          GROUP BY gi.id

          However for me this is always out by one. I believe its including the teacher. Is there anyway, based on the enrollment tables, to tell teachers from students or do I need to pull in role assignments anyway?

          Show
          Andrew Davis added a comment - Having trouble switching to the new enorolment table. The original query looks like this SELECT gi.id, COUNT(u.id) AS count FROM mdl_grade_items gi CROSS JOIN mdl_user u JOIN mdl_role_assignments ra ON ra.userid = u.id LEFT OUTER JOIN mdl_grade_grades g ON (g.itemid = gi.id AND g.userid = u.id AND g.finalgrade IS NOT NULL) WHERE gi.courseid = 2 AND ra.roleid in (5) AND ra.contextid IN (11,3,1) AND g.id IS NULL GROUP BY gi.id (role.id==5 is student) Replacement query looks like SELECT gi.id, COUNT(u.id) AS count FROM mdl_grade_items gi CROSS JOIN mdl_user u JOIN mdl_user_enrolments ue ON ue.userid = u.id JOIN mdl_enrol e ON e.id=ue.enrolid LEFT OUTER JOIN mdl_grade_grades g ON (g.itemid = gi.id AND g.userid = u.id AND g.finalgrade IS NOT NULL) WHERE gi.courseid = 2 AND e.courseid = 2 AND ue.status = 0 AND g.id IS NULL GROUP BY gi.id However for me this is always out by one. I believe its including the teacher. Is there anyway, based on the enrollment tables, to tell teachers from students or do I need to pull in role assignments anyway?
          Hide
          Petr Škoda added a comment -

          1/ only enrolled users are supposed to have grades, all grades for not-enrolled should be dropped at some time; when unenrolling users we now delete the grades; in any case you should not add grades for users that are not enrolled

          2/ there is a function in enrollib.php that gives you the necessary SQL code and params

          3/ gradebook uses nasty roles hack "graded roles" at the moment, it should really be graded capability now; in any case this is for displaying reports and exports only - teachers do have grades in gradebook because they are enrolled

          Show
          Petr Škoda added a comment - 1/ only enrolled users are supposed to have grades, all grades for not-enrolled should be dropped at some time; when unenrolling users we now delete the grades; in any case you should not add grades for users that are not enrolled 2/ there is a function in enrollib.php that gives you the necessary SQL code and params 3/ gradebook uses nasty roles hack "graded roles" at the moment, it should really be graded capability now; in any case this is for displaying reports and exports only - teachers do have grades in gradebook because they are enrolled
          Hide
          Andrew Davis added a comment -

          Im sorry Petr but Im not able to find the function youre referring to in enrollib.php. What is it called?

          Also, when I use the enrollment tables they seem to report everyone as student. For example this query says everyone is a student.
          select ue.userid, r.name from mdl_user_enrolments ue
          join mdl_enrol e on ue.enrolid=e.id
          join mdl_role r on e.roleid=r.id
          where e.courseid=2

          "gradebook uses nasty roles hack "graded roles" at the moment, it should really be graded capability now"

          Do you have an example of where this is done? I can see example of the use of "graded roles" then building a query accessing the role assignment table but not this graded capability you mention.

          Show
          Andrew Davis added a comment - Im sorry Petr but Im not able to find the function youre referring to in enrollib.php. What is it called? Also, when I use the enrollment tables they seem to report everyone as student. For example this query says everyone is a student. select ue.userid, r.name from mdl_user_enrolments ue join mdl_enrol e on ue.enrolid=e.id join mdl_role r on e.roleid=r.id where e.courseid=2 "gradebook uses nasty roles hack "graded roles" at the moment, it should really be graded capability now" Do you have an example of where this is done? I can see example of the use of "graded roles" then building a query accessing the role assignment table but not this graded capability you mention.
          Hide
          Andrew Davis added a comment -

          I've committed a version that uses the enrollment tables to git://github.com/andyjdavis/moodle.git branch MDL-20617
          diff: https://github.com/andyjdavis/moodle/compare/master...MDL-20617

          Doesn't seem quite right and the results are slightly different to those generated by using role assignments.

          Show
          Andrew Davis added a comment - I've committed a version that uses the enrollment tables to git://github.com/andyjdavis/moodle.git branch MDL-20617 diff: https://github.com/andyjdavis/moodle/compare/master...MDL-20617 Doesn't seem quite right and the results are slightly different to those generated by using role assignments.
          Hide
          Andrew Davis added a comment -

          After some conversation in dev chat I think I know how to rewrite the queries.

          re this "when unenrolling users we now delete the grades" That seems to be a really bad idea. See MDL-25718

          Show
          Andrew Davis added a comment - After some conversation in dev chat I think I know how to rewrite the queries. re this "when unenrolling users we now delete the grades" That seems to be a really bad idea. See MDL-25718
          Hide
          Andrew Davis added a comment -

          ok. ready for peer review again I believe. See github links above.

          Show
          Andrew Davis added a comment - ok. ready for peer review again I believe. See github links above.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,
          I've run out of time to actually test this in a site, I'll do that Monday, Ihave had a quick look at the code and spotted the following minor points:

          lib.php
          line 104~111: White space alignment
          line 161: Comments need to be updated to reflect columns again.
          line 561~570: The joins in that query look real unconventional, do they make sense?

          line 209~211:
          line 302~319:
          line 333~335:
          line 339~408: Nesting issue, not sure if this is Netbeans munting the code or not... worth checking.

          Have a good weekend.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, I've run out of time to actually test this in a site, I'll do that Monday, Ihave had a quick look at the code and spotted the following minor points: lib.php line 104~111: White space alignment line 161: Comments need to be updated to reflect columns again. line 561~570: The joins in that query look real unconventional, do they make sense? line 209~211: line 302~319: line 333~335: line 339~408: Nesting issue, not sure if this is Netbeans munting the code or not... worth checking. Have a good weekend. Cheers Sam
          Hide
          Andrew Davis added a comment - - edited

          Ive pushed fixes for all of those things except the query at line 561. Note sure whats up with all the code formatting/alignment issues. Trying to figure out a less unconventional query that gets the same data.

          update: Query reworked to hopefully be a little clearer. Added some description of how the query works. Also, it now uses null final grade instead of grade_grades::id to find ungraded grade items to catch both grade items with no grade_grades entry and ones that have an entry but with no final grade.

          Show
          Andrew Davis added a comment - - edited Ive pushed fixes for all of those things except the query at line 561. Note sure whats up with all the code formatting/alignment issues. Trying to figure out a less unconventional query that gets the same data. update: Query reworked to hopefully be a little clearer. Added some description of how the query works. Also, it now uses null final grade instead of grade_grades::id to find ungraded grade items to catch both grade items with no grade_grades entry and ones that have an entry but with no final grade.
          Hide
          Sam Hemelryk added a comment -

          Cool changes look good thanks Andrew, hehe one more thing sorry should $SQL be $sql?
          I'll leave that up to you, peer review == passed
          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Cool changes look good thanks Andrew, hehe one more thing sorry should $SQL be $sql? I'll leave that up to you, peer review == passed Cheers Sam
          Hide
          Andrew Davis added a comment - - edited

          Made it $sql. Resolving.

          PULL-30

          Show
          Andrew Davis added a comment - - edited Made it $sql. Resolving. PULL-30
          Hide
          Andrew Davis added a comment -

          1.9 fix in PULL-32 diff URL is https://github.com/andyjdavis/moodle/compare/MOODLE_19_STABLE...MDL-20617_1-9

          Note that the 1.9 fix just readds the range column. The controlling of the showing and hiding of columns has not been backported to 1.9

          Show
          Andrew Davis added a comment - 1.9 fix in PULL-32 diff URL is https://github.com/andyjdavis/moodle/compare/MOODLE_19_STABLE...MDL-20617_1-9 Note that the 1.9 fix just readds the range column. The controlling of the showing and hiding of columns has not been backported to 1.9
          Hide
          Andrew Davis added a comment -

          Failed integration testing. Comments below are from Petr:

          1/ missing $CFG global in fill_table_recursive() in lib.php
          2/ hardcoded English string: "'OVERRIDDEN: "
          3/ missing spaces after comma file settings.php and lib.php
          4/ non-standard indentation in SQL selects
          5/ potential problem with averages is that it is sensitive information and students should not be allowed to see it by default - if you periodically get the average you may reconstruct grades of all other users even before they are allowed to know their own grade
          6/ the average is calculated from all enrolled users which includes teachers too, there is missing join into the role assignments for the graded roles (please note the grader report should be fixed too because it uses only the role assignments workaround from 1.9)
          7/ you are calculating avgs even when setting is disabled (why not do it simply in constructor based on setting and preference), also you might not calculate the avgs when columns for it are printed
          8/ where can the student set the showaverages preference? they can not access the grader report prefs

          This code requires version bump for initialisation of new settings, it should be noted in the pull request.

          Rejecting, sorry, there seem to be too many problems, the biggest is the SQL graded users query which should be imo standardised throughout the whole gradebook (either use the legacy way or new way).

          Show
          Andrew Davis added a comment - Failed integration testing. Comments below are from Petr: 1/ missing $CFG global in fill_table_recursive() in lib.php 2/ hardcoded English string: "'OVERRIDDEN: " 3/ missing spaces after comma file settings.php and lib.php 4/ non-standard indentation in SQL selects 5/ potential problem with averages is that it is sensitive information and students should not be allowed to see it by default - if you periodically get the average you may reconstruct grades of all other users even before they are allowed to know their own grade 6/ the average is calculated from all enrolled users which includes teachers too, there is missing join into the role assignments for the graded roles (please note the grader report should be fixed too because it uses only the role assignments workaround from 1.9) 7/ you are calculating avgs even when setting is disabled (why not do it simply in constructor based on setting and preference), also you might not calculate the avgs when columns for it are printed 8/ where can the student set the showaverages preference? they can not access the grader report prefs This code requires version bump for initialisation of new settings, it should be noted in the pull request. Rejecting, sorry, there seem to be too many problems, the biggest is the SQL graded users query which should be imo standardised throughout the whole gradebook (either use the legacy way or new way).
          Hide
          Andrew Davis added a comment -

          Think thats all dealt with apart from the missing spaces after commas as I cant see where you're talking about. You're going to have to give me a clue.

          Diff is https://github.com/andyjdavis/moodle/compare/master...MDL-20617
          git://github.com/andyjdavis/moodle.git
          branch: MDL-20617

          1) fixed

          2) fixed

          3) I can't see where you're talking about. Line number?

          4) I'm not sure what this standard SQL indentation is that you mentioned. Do you have a link to a document describing it? I looked through the code and our SQL formatting seems to be highly irregular (to put it politely). I have however attempted to reformat it according to what I suspect you mean.

          5) The potential for students to estimate grades based on the average of submitted grades is a potential issue. However, averages are off by default. Teachers turn them on at their own risk. I'm not sure this is really a terribly significant issue. We could add some sort of limit like averages are only displayed after at least 10% of the class have submitted the grade item. Its another db query.

          6) Ive added the join for role_assignment. These queries were using role_assignment originally. What exactly is the point of using the enrollment tables if you are going to have to use role_assignments anyhow?
          "(please note the grader report should be fixed too because it uses only the role assignments workaround from 1.9)" Please dont piggyback new bug fixes on existing ones just because this one reminded you. I've raised MDL-25769 to fix the grader report.

          7) I have moved the call to the function into the constructor. I have also renamed user_avg() to calculate_averages(). The average is only calculated when the average column is going to be displayed. See the first few lines of the function now known as calculate_averages().

          8) Students don't currently have any capacity to choose what columns are shown in the grade reports. The teacher sets up the reports. The students just view them. You can put in a feature request at http://tracker.moodle.org

          "This code requires version bump for initialisation of new settings, it should be noted in the pull request."

          Will do in future.

          Show
          Andrew Davis added a comment - Think thats all dealt with apart from the missing spaces after commas as I cant see where you're talking about. You're going to have to give me a clue. Diff is https://github.com/andyjdavis/moodle/compare/master...MDL-20617 git://github.com/andyjdavis/moodle.git branch: MDL-20617 1) fixed 2) fixed 3) I can't see where you're talking about. Line number? 4) I'm not sure what this standard SQL indentation is that you mentioned. Do you have a link to a document describing it? I looked through the code and our SQL formatting seems to be highly irregular (to put it politely). I have however attempted to reformat it according to what I suspect you mean. 5) The potential for students to estimate grades based on the average of submitted grades is a potential issue. However, averages are off by default. Teachers turn them on at their own risk. I'm not sure this is really a terribly significant issue. We could add some sort of limit like averages are only displayed after at least 10% of the class have submitted the grade item. Its another db query. 6) Ive added the join for role_assignment. These queries were using role_assignment originally. What exactly is the point of using the enrollment tables if you are going to have to use role_assignments anyhow? "(please note the grader report should be fixed too because it uses only the role assignments workaround from 1.9)" Please dont piggyback new bug fixes on existing ones just because this one reminded you. I've raised MDL-25769 to fix the grader report. 7) I have moved the call to the function into the constructor. I have also renamed user_avg() to calculate_averages(). The average is only calculated when the average column is going to be displayed. See the first few lines of the function now known as calculate_averages(). 8) Students don't currently have any capacity to choose what columns are shown in the grade reports. The teacher sets up the reports. The students just view them. You can put in a feature request at http://tracker.moodle.org "This code requires version bump for initialisation of new settings, it should be noted in the pull request." Will do in future.
          Hide
          Petr Škoda added a comment -

          4/ congratulations, you have created yet another SQL coding style, please have a look at any new code from me or DML conversion code ( I have spent a lot of time trying to normalize the SQL coding style all over the place during the DML conversion) - for example accesslib.php, moodlelib.php, DML, DDL, DTM, most of enrol... If you want some guidelines see http://docs.moodle.org/en/Development:DB_layer_2.0_examples

          6/ you should update all reports to work consistently

          7/ I expected you would check the setting and preference in one place and made "if and" condition and based on the result did the average calculation and also number of columns.

          Looking at the patch I can see many spaces with inconsistent whitespace - in ifs, comments, etc.; it makes me happy when you do it consistently in new code - it is not critical of course

          Show
          Petr Škoda added a comment - 4/ congratulations, you have created yet another SQL coding style, please have a look at any new code from me or DML conversion code ( I have spent a lot of time trying to normalize the SQL coding style all over the place during the DML conversion) - for example accesslib.php, moodlelib.php, DML, DDL, DTM, most of enrol... If you want some guidelines see http://docs.moodle.org/en/Development:DB_layer_2.0_examples 6/ you should update all reports to work consistently 7/ I expected you would check the setting and preference in one place and made "if and" condition and based on the result did the average calculation and also number of columns. Looking at the patch I can see many spaces with inconsistent whitespace - in ifs, comments, etc.; it makes me happy when you do it consistently in new code - it is not critical of course
          Hide
          Robert Puffer added a comment -

          I love watching this "back-and-forth" except my fear is that most of what's being rejected is carryover from the original 1.95 gradebook and were someone to submit the whole gradebook (even as it appears in 1.9.10) it would NEVER get through integration testing. Just wait until you try to fix it though... at that point I might humbly suggest revisiting my early remarks about the need to simplify the gradebook.

          Show
          Robert Puffer added a comment - I love watching this "back-and-forth" except my fear is that most of what's being rejected is carryover from the original 1.95 gradebook and were someone to submit the whole gradebook (even as it appears in 1.9.10) it would NEVER get through integration testing. Just wait until you try to fix it though... at that point I might humbly suggest revisiting my early remarks about the need to simplify the gradebook.
          Hide
          Andrew Davis added a comment -

          4) I have altered the formatting (replaced tabs with 2 spaces and indented where conditions after the first one to line up with the first one) as appears to be the way its done based on http://docs.moodle.org/en/Development:DB_layer_2.0_examples If this isn't satisfactory you may have to resort to actually explaining the correct way to do this.

          6) Ok, Ive closed MDL-25769 and have just fixed the grader report as part of this item.

          7) Not entirely sure what you mean here. There is a setting. There is no user preference involved that I'm aware of. $this->showaverages is set in the report constructor so using that.

          Show
          Andrew Davis added a comment - 4) I have altered the formatting (replaced tabs with 2 spaces and indented where conditions after the first one to line up with the first one) as appears to be the way its done based on http://docs.moodle.org/en/Development:DB_layer_2.0_examples If this isn't satisfactory you may have to resort to actually explaining the correct way to do this. 6) Ok, Ive closed MDL-25769 and have just fixed the grader report as part of this item. 7) Not entirely sure what you mean here. There is a setting. There is no user preference involved that I'm aware of. $this->showaverages is set in the report constructor so using that.
          Hide
          Petr Škoda added a comment -

          7/ ahh, sorry - it was already fixed

          thanks

          Show
          Petr Škoda added a comment - 7/ ahh, sorry - it was already fixed thanks
          Hide
          Andrew Davis added a comment -

          Once more unto the breach

          PULL-53

          Show
          Andrew Davis added a comment - Once more unto the breach PULL-53
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Andrew (et all),

          surely I'm wrong but I've been re-reading the history of this issue and the proposed pull requests along the last weeks... and I really think that we should try to split this, in order to be able to "accept" some parts to go upstream.

          I mean, if this issue is about the user report why the proposed patch includes changes to the grader report and (partially/wrongly...?) change the way to get the "gradeable" users?

          Why not, simply, provide one pull request covering exactlty this issue (user report) and delegate the rest to other issues, that will have their own review/test cycle? If we continue adding and adding bits here all the time, doing it to grow and grow with IMO, unrelated issues.

          Just a side comment from an integrator POV, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Andrew (et all), surely I'm wrong but I've been re-reading the history of this issue and the proposed pull requests along the last weeks... and I really think that we should try to split this, in order to be able to "accept" some parts to go upstream. I mean, if this issue is about the user report why the proposed patch includes changes to the grader report and (partially/wrongly...?) change the way to get the "gradeable" users? Why not, simply, provide one pull request covering exactlty this issue (user report) and delegate the rest to other issues, that will have their own review/test cycle? If we continue adding and adding bits here all the time, doing it to grow and grow with IMO, unrelated issues. Just a side comment from an integrator POV, ciao
          Hide
          Petr Škoda added a comment -

          I agree Eloy, two issues would be better:
          1/ fixing all "graded users" places
          2/ user report improvements

          Show
          Petr Škoda added a comment - I agree Eloy, two issues would be better: 1/ fixing all "graded users" places 2/ user report improvements
          Hide
          Petr Škoda added a comment -

          the SQL coding style is still different, all the SELECT, WHERE, GROUP BY are to the left, the rest is to the right of an imaginary vertical line.

          Show
          Petr Škoda added a comment - the SQL coding style is still different, all the SELECT, WHERE, GROUP BY are to the left, the rest is to the right of an imaginary vertical line.
          Hide
          Andrew Davis added a comment - - edited

          PULL-53 rejected with this comment from Petr:

          Did you grep for the "gradebookroles" string? There are several more places that should be updated at the same time imho. Sorry, rejecting again.

          Show
          Andrew Davis added a comment - - edited PULL-53 rejected with this comment from Petr: Did you grep for the "gradebookroles" string? There are several more places that should be updated at the same time imho. Sorry, rejecting again.
          Hide
          Andrew Davis added a comment - - edited

          "Why not, simply, provide one pull request covering exactlty this issue (user report) and delegate the rest to other issues"

          I entirely agree. I did in fact split out one of these other tasks however Petr insisted they be done right now. I can't raise new issues and add them to the current sprint therefore have no choice but to do them as part of this issue as long as Petr continues to insist that I fix unrelated parts of gradebook immediately.

          Show
          Andrew Davis added a comment - - edited "Why not, simply, provide one pull request covering exactlty this issue (user report) and delegate the rest to other issues" I entirely agree. I did in fact split out one of these other tasks however Petr insisted they be done right now. I can't raise new issues and add them to the current sprint therefore have no choice but to do them as part of this issue as long as Petr continues to insist that I fix unrelated parts of gradebook immediately.
          Hide
          Robert Puffer added a comment -

          Amidst the onlooking eyes of a community that greatly respects Moodle, with high aspirations for its future, the handling of this issue bodes poorly for the "new, more receptive" system of code review (imho).

          Show
          Robert Puffer added a comment - Amidst the onlooking eyes of a community that greatly respects Moodle, with high aspirations for its future, the handling of this issue bodes poorly for the "new, more receptive" system of code review (imho).
          Hide
          Andrew Davis added a comment -

          re this "Did you grep for the "gradebookroles" string? There are several more places that should be updated at the same time imho. Sorry, rejecting again."

          I can either do this as part of this issue or open another issue which will be scheduled for a future sprint? What is it to be Petr?

          Show
          Andrew Davis added a comment - re this "Did you grep for the "gradebookroles" string? There are several more places that should be updated at the same time imho. Sorry, rejecting again." I can either do this as part of this issue or open another issue which will be scheduled for a future sprint? What is it to be Petr?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Petr, Andrew,

          what is really the problem with this issue, I think you both are agreeing (above) about get this issue (exclusively the user report improvements) here and create new issue for the "graded users" fixes, isn't it?

          Or does the former require the later or something like this? If nobody object, plz fix the report here, and create exclusive PULL for that and then fix the "graded users" thing (note I really don't know what the hell that is, lol) and create another PULL for that. Sounds simple, though surely I'm missing something. Please comment if there is any problem about that (the split).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Petr, Andrew, what is really the problem with this issue, I think you both are agreeing (above) about get this issue (exclusively the user report improvements) here and create new issue for the "graded users" fixes, isn't it? Or does the former require the later or something like this? If nobody object, plz fix the report here, and create exclusive PULL for that and then fix the "graded users" thing (note I really don't know what the hell that is, lol) and create another PULL for that. Sounds simple, though surely I'm missing something. Please comment if there is any problem about that (the split). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Robert,

          right now we are "learning as we do" with this new way of handling core stuff. Can you expose why it "bodes poorly" or why it isn't "receptive" in your opinion?

          Note I'm not arguing at all, just asking, the most opinions/comments/feelings we have the better we'll be able to handle this in the future, just that. So any alternative way, objection, whatever is really welcome, IMO.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Robert, right now we are "learning as we do" with this new way of handling core stuff. Can you expose why it "bodes poorly" or why it isn't "receptive" in your opinion? Note I'm not arguing at all , just asking, the most opinions/comments/feelings we have the better we'll be able to handle this in the future, just that. So any alternative way, objection, whatever is really welcome, IMO. TIA and ciao
          Hide
          Andrew Davis added a comment - - edited

          Also re "the SQL coding style is still different, all the SELECT, WHERE, GROUP BY are to the left, the rest is to the right of an imaginary vertical line."

          Ive formatted the SQL the same as in http://docs.moodle.org/en/Development:DB_layer_2.0_examples, the document you linked to as an example. A query from that document:

           
              $sql = "SELECT f.course, COUNT(*) count
                        FROM {forum} f
                        JOIN {forum_discussions} d ON d.forum = f.id
                       WHERE f.type = :forum_type
                             AND f.course $in_sql
                    GROUP BY f.course";
          

          Note, everything but the select, where and group by are indented. One of the queries in the submitted code for comparison:

                      $sql = "SELECT g.itemid, SUM(g.finalgrade) AS sum
                                FROM {grade_items} gi
                                JOIN {grade_grades} g ON g.itemid = gi.id
                                JOIN ($enrolledsql) je ON je.id = g.userid
                                JOIN {role_assignments} ra ON ra.userid = g.userid
                                $groupsql
                              WHERE gi.courseid = $this->courseid
                                    AND ra.roleid $gradebookrolessql
                                    AND g.finalgrade IS NOT NULL
                                    $groupwheresql
                              GROUP BY g.itemid";
          
          Show
          Andrew Davis added a comment - - edited Also re "the SQL coding style is still different, all the SELECT, WHERE, GROUP BY are to the left, the rest is to the right of an imaginary vertical line." Ive formatted the SQL the same as in http://docs.moodle.org/en/Development:DB_layer_2.0_examples , the document you linked to as an example. A query from that document: $sql = "SELECT f.course, COUNT(*) count FROM {forum} f JOIN {forum_discussions} d ON d.forum = f.id WHERE f.type = :forum_type AND f.course $in_sql GROUP BY f.course"; Note, everything but the select, where and group by are indented. One of the queries in the submitted code for comparison: $sql = "SELECT g.itemid, SUM(g.finalgrade) AS sum FROM {grade_items} gi JOIN {grade_grades} g ON g.itemid = gi.id JOIN ($enrolledsql) je ON je.id = g.userid JOIN {role_assignments} ra ON ra.userid = g.userid $groupsql WHERE gi.courseid = $this->courseid AND ra.roleid $gradebookrolessql AND g.finalgrade IS NOT NULL $groupwheresql GROUP BY g.itemid";
          Hide
          Robert Puffer added a comment -

          Eloy,
          A read of the lonnnnggggg discussion here and the many forum discussions from which this issue germinated would lead even a casual observer to conclude that getting needed (and long overdue) functionality out to the community is of minor importance. Even though it is well voted-up, the issue is over a year old; the forum discussions are much older than that; code was provided for both 1.9x and 2.0 about three months ago and still we've been unable to see the issue resolved because a code reviewer has decided that other erroneous code in the gradebook, unrelated to the original issue must be fixed prior to it being approved. We are ALL just learning, but I must say that the learning I'm getting from this particular issue is to not count on important items like this being fixed in core in any usable timeframe. I'm not trying to argue but I'm serious when I say that in 2004 I got better response from Blackboard then I usually get with gradebook issues on Moodle – and Andrew is not at fault.

          Show
          Robert Puffer added a comment - Eloy, A read of the lonnnnggggg discussion here and the many forum discussions from which this issue germinated would lead even a casual observer to conclude that getting needed (and long overdue) functionality out to the community is of minor importance. Even though it is well voted-up, the issue is over a year old; the forum discussions are much older than that; code was provided for both 1.9x and 2.0 about three months ago and still we've been unable to see the issue resolved because a code reviewer has decided that other erroneous code in the gradebook, unrelated to the original issue must be fixed prior to it being approved. We are ALL just learning, but I must say that the learning I'm getting from this particular issue is to not count on important items like this being fixed in core in any usable timeframe. I'm not trying to argue but I'm serious when I say that in 2004 I got better response from Blackboard then I usually get with gradebook issues on Moodle – and Andrew is not at fault.
          Hide
          Martin Dougiamas added a comment -

          Robert, the code was submitted only a month ago (late November) if you factor in the Xmas break etc (not to mention the complete change in our processes and tools happening in there at the same time), and it is being dealt with. As detailed above, Petr's review has found a long list of small problems that would not have been caught before and would just have led to more subtle issues later on.

          These seem to be dealt with so I hope to see these settings integrated in the next cycle.

          Show
          Martin Dougiamas added a comment - Robert, the code was submitted only a month ago (late November) if you factor in the Xmas break etc (not to mention the complete change in our processes and tools happening in there at the same time), and it is being dealt with. As detailed above, Petr's review has found a long list of small problems that would not have been caught before and would just have led to more subtle issues later on. These seem to be dealt with so I hope to see these settings integrated in the next cycle.
          Hide
          Robert Puffer added a comment -

          I'm sorry, Martin – thanks for catching my exaggeration. The problem that Andrew alludes to and to which I expand upon is that a complete review of the gradebook code will invariably uncover immense numbers of problems for which waiting would postpone any real improvement indefinitely. I am in favor of a complete review of code in the gradebook and have faith in Petr's ability to uncover problems. However, I'm currently sitting amidst a group of twenty schools at a Hack-doc who are chartered with determining migration dates for 2.0 for a larger group of colleges in North America and we had one vote for Summer, 2011 – the rest are Summer, 2012. That leaves us hanging onto 1.9 for which it (feels like to me) fixes and enhancements are now receiving very low priority. I'm wondering if part of the learning process could be ever-increasingly stringent review of code so we don't suddenly go from where we were to where we want to be overnight and nobody gets any code changes approved that really matter to production sites?

          Show
          Robert Puffer added a comment - I'm sorry, Martin – thanks for catching my exaggeration. The problem that Andrew alludes to and to which I expand upon is that a complete review of the gradebook code will invariably uncover immense numbers of problems for which waiting would postpone any real improvement indefinitely. I am in favor of a complete review of code in the gradebook and have faith in Petr's ability to uncover problems. However, I'm currently sitting amidst a group of twenty schools at a Hack-doc who are chartered with determining migration dates for 2.0 for a larger group of colleges in North America and we had one vote for Summer, 2011 – the rest are Summer, 2012. That leaves us hanging onto 1.9 for which it (feels like to me) fixes and enhancements are now receiving very low priority. I'm wondering if part of the learning process could be ever-increasingly stringent review of code so we don't suddenly go from where we were to where we want to be overnight and nobody gets any code changes approved that really matter to production sites?
          Hide
          Martin Dougiamas added a comment -

          I don't really see the logic in "some colleges don't feel like migrating to 2.0 yet so we should spend more time on 1.9" ... surely it means we need to spend even more time on 2.x to encourage them to move up faster? 1.x and 2.x code are quite different and spreading resources across them just slows progress in both.

          WRT the gradebook I would love to see a complete review of the code but that of course can't happen for 1.9.x or 2.0 ... changes of that magnitude need to be in 2.1 or 2.2. Unfortunately at this moment we don't quite have the resources (Andrew is part-time on gradebook issues) though I'm working on that. I've had some people sort-of volunteer to take over this task (most recently SFSU) but none have progressed past the "we're interested" stage yet.

          I'll start some discussions in the gradebook forum later in January when I'm back at work.

          Show
          Martin Dougiamas added a comment - I don't really see the logic in "some colleges don't feel like migrating to 2.0 yet so we should spend more time on 1.9" ... surely it means we need to spend even more time on 2.x to encourage them to move up faster? 1.x and 2.x code are quite different and spreading resources across them just slows progress in both. WRT the gradebook I would love to see a complete review of the code but that of course can't happen for 1.9.x or 2.0 ... changes of that magnitude need to be in 2.1 or 2.2. Unfortunately at this moment we don't quite have the resources (Andrew is part-time on gradebook issues) though I'm working on that. I've had some people sort-of volunteer to take over this task (most recently SFSU) but none have progressed past the "we're interested" stage yet. I'll start some discussions in the gradebook forum later in January when I'm back at work.
          Hide
          Andrew Davis added a comment -

          Ive split out the fix to the grader report and the other, not yet completed, gradebookroles related fixes. They will be done as part of MDL-25769.

          Show
          Andrew Davis added a comment - Ive split out the fix to the grader report and the other, not yet completed, gradebookroles related fixes. They will be done as part of MDL-25769 .
          Hide
          Andrew Davis added a comment -

          Petr, Ill need some sort of feedback from you about the SQL formatting. Not going to submit this for review again just to have it rejected.

          Show
          Andrew Davis added a comment - Petr, Ill need some sort of feedback from you about the SQL formatting. Not going to submit this for review again just to have it rejected.
          Hide
          Robert Puffer added a comment -

          Martin, your points are very logical but I still struggle with the reality that seems to be upon us – i.e.:

          1. I wouldn't DREAM of offering to help recode the gradebook amidst watching the struggles Andrew's going through just getting something simple through the "gauntlet"
          2. I'm becoming sheepish about even making strong observations anymore after the rebuff of MDL-25718 where my issue was "renamed" to make it look more appealing when, in fact my position of its seriousness is widely supported.

          These things together are what cause me to question if, indeed the prevailing attitude will be more receptive. I certainly don't envy all those on the opposite side of the "new workflow" fence since it sure appears like there's a lot more work for the reviewers and core developers to do and a lot less code that will ultimately be submitted by non-core developers.

          Show
          Robert Puffer added a comment - Martin, your points are very logical but I still struggle with the reality that seems to be upon us – i.e.: I wouldn't DREAM of offering to help recode the gradebook amidst watching the struggles Andrew's going through just getting something simple through the "gauntlet" I'm becoming sheepish about even making strong observations anymore after the rebuff of MDL-25718 where my issue was "renamed" to make it look more appealing when, in fact my position of its seriousness is widely supported. These things together are what cause me to question if, indeed the prevailing attitude will be more receptive. I certainly don't envy all those on the opposite side of the "new workflow" fence since it sure appears like there's a lot more work for the reviewers and core developers to do and a lot less code that will ultimately be submitted by non-core developers.
          Hide
          Martin Dougiamas added a comment -

          Andrew, please just make a new PULL request with the last version. Sam can integrate today.

          Show
          Martin Dougiamas added a comment - Andrew, please just make a new PULL request with the last version. Sam can integrate today.
          Hide
          Andrew Davis added a comment -

          PULL-134

          Show
          Andrew Davis added a comment - PULL-134
          Hide
          Martin Dougiamas added a comment -

          After some conversations here about the very complex stuff going on in the gradebook, there seems to be two problems not really discussed on this bug yet:

          1) revealing the class average to students can allow students (in some cases) to work out what grades were gained by other students. eg if only two students completed an assignment, and the average is 50, and you know you got 70, then you know the other student got 30. This is a potential privacy issue.

          2) if any grades in the gradebook have been hidden by the teacher then the class average for an item in the gradebook may be inaccurate. The question is whether to include these in the average calculations or not. It's further complicated by calculated columns that may aggregate hidden and non-hidden items in inconsistent ways. A completely accurate result would actually involve a complete recalculation of the entire gradebook every time a user report is shown. (Please note that the quiz module will automatically hide/unhide items so this scenario is not as uncommon as it may seem)

          Now, these don't affect all courses. And I know a lot of people would like to see these columns shown in the user reports so...

          My proposal is to keep the patch as it is but to add help buttons to the "show/hide switch" (admin and course level) and also to the user report column to explain these two problems. So if you turn the average column on then you should at least be aware of these caveats.

          The problem of the possible inaccuracy of the numbers is a separate bug, much bigger than this one and requiring some rewriting, which will probably have to be done in 2.1 or so.

          How does this sound to everyone? Does someone want to suggest some help text?

          Show
          Martin Dougiamas added a comment - After some conversations here about the very complex stuff going on in the gradebook, there seems to be two problems not really discussed on this bug yet: 1) revealing the class average to students can allow students (in some cases) to work out what grades were gained by other students. eg if only two students completed an assignment, and the average is 50, and you know you got 70, then you know the other student got 30. This is a potential privacy issue. 2) if any grades in the gradebook have been hidden by the teacher then the class average for an item in the gradebook may be inaccurate. The question is whether to include these in the average calculations or not. It's further complicated by calculated columns that may aggregate hidden and non-hidden items in inconsistent ways. A completely accurate result would actually involve a complete recalculation of the entire gradebook every time a user report is shown. (Please note that the quiz module will automatically hide/unhide items so this scenario is not as uncommon as it may seem) Now, these don't affect all courses. And I know a lot of people would like to see these columns shown in the user reports so... My proposal is to keep the patch as it is but to add help buttons to the "show/hide switch" (admin and course level) and also to the user report column to explain these two problems. So if you turn the average column on then you should at least be aware of these caveats. The problem of the possible inaccuracy of the numbers is a separate bug, much bigger than this one and requiring some rewriting, which will probably have to be done in 2.1 or so. How does this sound to everyone? Does someone want to suggest some help text?
          Hide
          Petr Škoda added a comment -

          3) If you see the real class average (including hidden grades) and know the number of students in the course you can calculate your grade from activity such as quiz with delayed release of grades. This defeats the purpose of hidden grades from activities completely (you just have to make sure that only you submitted the quiz during the last few seconds before/after refresh of user report).

          Show
          Petr Škoda added a comment - 3) If you see the real class average (including hidden grades) and know the number of students in the course you can calculate your grade from activity such as quiz with delayed release of grades. This defeats the purpose of hidden grades from activities completely (you just have to make sure that only you submitted the quiz during the last few seconds before/after refresh of user report).
          Hide
          Andrew Davis added a comment -

          Ive added help items describing the potential data leakage issue and the potential inaccuracy of the average if hidden grades are involved.

          Hidden grades are now excluded from the average and rank calculation on the user report. More specifically, in the average, hidden grade items are counted as the grade items minimum grade. Its still possible for hidden grades to make the average inaccurate if the grade item is a calculated item which is dependent on a hidden grade item further down the line so I have retained the caveat in the help item.

          Show
          Andrew Davis added a comment - Ive added help items describing the potential data leakage issue and the potential inaccuracy of the average if hidden grades are involved. Hidden grades are now excluded from the average and rank calculation on the user report. More specifically, in the average, hidden grade items are counted as the grade items minimum grade. Its still possible for hidden grades to make the average inaccurate if the grade item is a calculated item which is dependent on a hidden grade item further down the line so I have retained the caveat in the help item.
          Hide
          Andrew Davis added a comment -

          With some git instruction from Sam I've squashed all the commits for this into one.

          repo: git://github.com/andyjdavis/moodle.git
          branch: MDL-20617_squashed
          diff: https://github.com/andyjdavis/moodle/compare/master...MDL-20617_squashed

          Show
          Andrew Davis added a comment - With some git instruction from Sam I've squashed all the commits for this into one. repo: git://github.com/andyjdavis/moodle.git branch: MDL-20617 _squashed diff: https://github.com/andyjdavis/moodle/compare/master...MDL-20617_squashed
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, it has been integrated now
          Providing it passes the QA testing it will be in stable tomorrow.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Andrew, it has been integrated now Providing it passes the QA testing it will be in stable tomorrow. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Cool. Fingers crossed for testing. Thankyou everyone for your contribution. Especially Robert Puffer for the initial patch

          Show
          Andrew Davis added a comment - Cool. Fingers crossed for testing. Thankyou everyone for your contribution. Especially Robert Puffer for the initial patch
          Hide
          Petr Škoda added a comment -

          committed, thanks

          Show
          Petr Škoda added a comment - committed, thanks
          Hide
          Robert Puffer added a comment -

          WRT the complete review of the gradebook, I'd suggest complete separation of calculations from display making it easier to write custom reports knowing the calculations are just waiting to be "popped in".

          Show
          Robert Puffer added a comment - WRT the complete review of the gradebook, I'd suggest complete separation of calculations from display making it easier to write custom reports knowing the calculations are just waiting to be "popped in".

            People

            • Votes:
              15 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: