Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30179

User report and Grader report are inconsistent for the grade that is hidden. Allow teacher to toggle to/from "user view"

    Details

    • Testing Instructions:
      Hide

      Run behat tests (@gradereport_user)

      1. Create a course, enroll student and a teacher
      2. In that course, create several grade items and categories, hide various ones. We want at least one hidden category, and one hidden item in a visible category.
      3. Give the student a grade in each grade item
      4. In two separate browsers, login as the student and the teacher
      5. As the teacher go to the Course grade settings
      6. Make sure Hide totals if they contain hidden items under User report (very bottom) is set to Hide
      7. Navigate to the user grade report for the course in both courses
      8. In the teacher browser, set the report user to the student
      9. Set View report as to Myself
      10. Confirm all items and totals are visible
      11. Set View report as to User
      12. Compare the student and teacher windows of the user report
      13. Confirm that the same items are visible and not visible between the two, and that the same totals are shown.
      14. Repeat steps 4-13, but with the settings Show totals including hidden and Show totals excluding hidden

      Student permission

      1. Login as an admin
      2. Change the student role to allow the moodle/grade:viewhidden permission
      3. Repeat steps 4-14 of the section above

      Confirm sticky setting

      1. Login as a teacher
      2. Go to a course, then to the user report
      3. Set View user as to Myself
      4. Log out
      5. Log back in as the teacher
      6. Go to a course, then to the user report
      7. Confirm View user as is set to Myself
      Show
      Run behat tests (@gradereport_user) Create a course, enroll student and a teacher In that course, create several grade items and categories, hide various ones. We want at least one hidden category, and one hidden item in a visible category. Give the student a grade in each grade item In two separate browsers, login as the student and the teacher As the teacher go to the Course grade settings Make sure Hide totals if they contain hidden items under User report (very bottom) is set to Hide Navigate to the user grade report for the course in both courses In the teacher browser, set the report user to the student Set View report as to Myself Confirm all items and totals are visible Set View report as to User Compare the student and teacher windows of the user report Confirm that the same items are visible and not visible between the two, and that the same totals are shown. Repeat steps 4-13, but with the settings Show totals including hidden and Show totals excluding hidden Student permission Login as an admin Change the student role to allow the moodle/grade:viewhidden permission Repeat steps 4-14 of the section above Confirm sticky setting Login as a teacher Go to a course, then to the user report Set View user as to Myself Log out Log back in as the teacher Go to a course, then to the user report Confirm View user as is set to Myself
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_27_STABLE, MOODLE_28_STABLE, MOODLE_31_STABLE
    • Fixed Branches:
      MOODLE_32_STABLE
    • Epic Link:
    • Pull Master Branch:
      MDL-30179-master

      Description

      If I am a grader and I decided to hide the grade for one student for an assignment, I can still see it in the Grader report (but the font is gray).
      If I (again as a grader) go to the user report for this user I:

      • CAN see the assignment
      • CAN NOT see any grade, not even gray, in the columns 'Grade', 'Percentage', 'Rank'
      • CAN see the grade in the column 'Letter grade' (I enabled it in settings)
        As a student I don't see this assignment in the user report at all.

      There should be a clear policy, what User report shows if it is viewed by a grader:
      1) it shows the same as Grader report but for a particular user
      or
      2) it shows the same as what this user would see in his User report

      Depending on this policy the hidden grades should be shown or the whole line with module should not be shown at all.


      Regarding 'Rank': there should be also a policy whether to include the hidden grades in rank or not. At the moment they are included, so for other students the rank is calculated as if hidden grades were normal grades. But for student whose grade is hidden, rank is not displayed (although exists)

      The column 'Average' displays the average only for grades that are not hidden. So for other users it looks strange: "Rank 2/4, Average 30.00 (3)"

      So I would suggest that Rank should not include hidden grades

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dougiamas Martin Dougiamas added a comment -

              Adam and Robert: should we change the default assignee for newly reported gradebook bugs (like this one) to be one of you guys? If not, let's work out what to do.

              Show
              dougiamas Martin Dougiamas added a comment - Adam and Robert: should we change the default assignee for newly reported gradebook bugs (like this one) to be one of you guys? If not, let's work out what to do.
              Hide
              marina Marina Glancy added a comment -

              Just noticed:

              in Grader report the 'Average' for the module DOES include hidden grades. The same field in User report DOES NOT include them. One more inconsistent thing

              Show
              marina Marina Glancy added a comment - Just noticed: in Grader report the 'Average' for the module DOES include hidden grades. The same field in User report DOES NOT include them. One more inconsistent thing
              Hide
              marina Marina Glancy added a comment -

              Sorry, can't keep commenting:

              There is actually a setting for Grader report "Grades selected for column averages" (grade_report_meanselection), but it does not say how it considers hidden grades

              There is a setting for user report grade_report_user_showhiddenitems. If set the student can see module on which his grade was hidden, and luckily no grades there, even 'Letter grade' is empty.

              And there is another setting for User report grade_report_user_showtotalsifcontainhidden, which can be set to 'Show totals including hidden items' which is absolutely crazy. In this case student can not see the hidden grade, he sees this module as if it has not been graded, but his total looks completely random for him (because it includes grades he can not see).

              Show
              marina Marina Glancy added a comment - Sorry, can't keep commenting: There is actually a setting for Grader report "Grades selected for column averages" ( grade_report_meanselection ), but it does not say how it considers hidden grades There is a setting for user report grade_report_user_showhiddenitems . If set the student can see module on which his grade was hidden, and luckily no grades there, even 'Letter grade' is empty. And there is another setting for User report grade_report_user_showtotalsifcontainhidden, which can be set to 'Show totals including hidden items' which is absolutely crazy. In this case student can not see the hidden grade, he sees this module as if it has not been graded, but his total looks completely random for him (because it includes grades he can not see).
              Hide
              rrusso Robert Russo added a comment -

              Please assign Adam Zapletal (azaple1), Robert Russo (rrusso), and Philip Cali (pcali1) to Moodle 2 Gradebook Bugs.

              Show
              rrusso Robert Russo added a comment - Please assign Adam Zapletal (azaple1), Robert Russo (rrusso), and Philip Cali (pcali1) to Moodle 2 Gradebook Bugs.
              Hide
              jackdaniels JD added a comment -

              And there is another setting for User report grade_report_user_showtotalsifcontainhidden, which can be set to 'Show totals including hidden items' which is absolutely crazy. In this case student can not see the hidden grade, he sees this module as if it has not been graded, but his total looks completely random for him (because it includes grades he can not see).

              We have exactly this behavior using latest version 2.4.3. It would be nice to clean this eventually.

              Thanks.

              Show
              jackdaniels JD added a comment - And there is another setting for User report grade_report_user_showtotalsifcontainhidden, which can be set to 'Show totals including hidden items' which is absolutely crazy. In this case student can not see the hidden grade, he sees this module as if it has not been graded, but his total looks completely random for him (because it includes grades he can not see). We have exactly this behavior using latest version 2.4.3. It would be nice to clean this eventually. Thanks.
              Hide
              andyjdavis Andrew Davis added a comment -

              This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

              For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

              Show
              andyjdavis Andrew Davis added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
              Hide
              damyon Damyon Wiese added a comment -

              This was raised again during the gradebook push for 2.8. It was decided a teacher should be able to toggle their view to show/hide the hidden items (if hidden the view should match the student view).

              Show
              damyon Damyon Wiese added a comment - This was raised again during the gradebook push for 2.8. It was decided a teacher should be able to toggle their view to show/hide the hidden items (if hidden the view should match the student view).
              Hide
              mcka0074 Mark McKay added a comment -

              Damyon, is there a separate issue for the toggle feature?

              Show
              mcka0074 Mark McKay added a comment - Damyon, is there a separate issue for the toggle feature?
              Hide
              emerrill Eric Merrill added a comment -

              The Gradebook Working Group took this up. I will be working on specs for this.

              Show
              emerrill Eric Merrill added a comment - The Gradebook Working Group took this up. I will be working on specs for this.
              Hide
              jlsimms Jason Simms added a comment -

              This fix would reduce the issue where a student contacts faculty asking why their grade view shows something, and then the faculty has no way to view this in the absence of physically being with the student; that is, faculty have no way to see what a particular student sees. So then faculty have to contact a support admin to log in as a given student, take a screenshot, etc. in order to move forward and resolve questions or concerns.

              Show
              jlsimms Jason Simms added a comment - This fix would reduce the issue where a student contacts faculty asking why their grade view shows something, and then the faculty has no way to view this in the absence of physically being with the student; that is, faculty have no way to see what a particular student sees. So then faculty have to contact a support admin to log in as a given student, take a screenshot, etc. in order to move forward and resolve questions or concerns.
              Hide
              emerrill Eric Merrill added a comment -

              I'm looking to get feedback so we can take this the "right" way.

              I made a UI mockup. It's functional, but I don't really love it. Also, each menu auto-submits, so you can't change both on the same page load. Thoughts? Ideas?
              Unable to render embedded object: File (MDL-30179mockup1.png) not found.
              One change that was already suggested is that the "View as" item shouldn't appear when no selection has been made from the menu.

              For the following, I'm going to be using Teacher to mean the person logged in, and Student to mean the person they are viewing, but roles and permissions may vary.

              Here are general considerations that have to be decided:
              1 - Should the toggle be displayed even if the Teacher and Student have the same relevant permissions?
              1A - Yes. Provides consistency in the interface, and is less ambiguous, particularly on the 'all users' display
              1B - No. We should not how and interface that will provide no visible change

              My opinion 1A. I think it would be complicated (pragmatically) and confusing (from a user standpoint) to have it there sometimes and not others.

              2 - How should this setting be "remembered":
              2A - The setting should return to its default every time the teacher comes to the report page
              2B - The setting should be remembered for the duration of the teacher's session
              2C - The setting should always remembered the last used setting

              My opinion 2B. I think this is the right balance between usability when clicking around and sticking in a permanent state.

              3 - Should the default be configurable:
              3A - No, it should always default to "Student" view
              3B - No, it should always default to "Teacher" view (current behavior)
              3C - The default should be site configurable
              3D - The default should be configurable as a grade setting (Site setting -> course setting)
              3E - The default should be a user preference

              My opinion 3A. They are all valid - the order of complexity to implement: 3A/3B, 3C, 3D, 3E. 3E gets particularly complicated.

              4 - Verbiage (these are examples of a style, and could be cleaned up/made more clear. Open to suggestions):
              4A - View as [user | myself]
              4B - View as [student | instructor]
              4C - View as [Student Name | myself]
              4D - Some graphical toggle/representation (what?)
              ?

              My opinion 4A. Because of how permissions and roles may be assigned, student to student and instructor to instructor may have different views of the report - we don't have a accurate way to describe that.

              Show
              emerrill Eric Merrill added a comment - I'm looking to get feedback so we can take this the "right" way. I made a UI mockup. It's functional, but I don't really love it. Also, each menu auto-submits, so you can't change both on the same page load. Thoughts? Ideas? Unable to render embedded object: File ( MDL-30179 mockup1.png) not found. One change that was already suggested is that the "View as" item shouldn't appear when no selection has been made from the menu. For the following, I'm going to be using Teacher to mean the person logged in, and Student to mean the person they are viewing, but roles and permissions may vary. Here are general considerations that have to be decided: 1 - Should the toggle be displayed even if the Teacher and Student have the same relevant permissions? 1A - Yes. Provides consistency in the interface, and is less ambiguous, particularly on the 'all users' display 1B - No. We should not how and interface that will provide no visible change My opinion 1A . I think it would be complicated (pragmatically) and confusing (from a user standpoint) to have it there sometimes and not others. 2 - How should this setting be "remembered": 2A - The setting should return to its default every time the teacher comes to the report page 2B - The setting should be remembered for the duration of the teacher's session 2C - The setting should always remembered the last used setting My opinion 2B . I think this is the right balance between usability when clicking around and sticking in a permanent state. 3 - Should the default be configurable: 3A - No, it should always default to "Student" view 3B - No, it should always default to "Teacher" view (current behavior) 3C - The default should be site configurable 3D - The default should be configurable as a grade setting (Site setting -> course setting) 3E - The default should be a user preference My opinion 3A . They are all valid - the order of complexity to implement: 3A/3B, 3C, 3D, 3E. 3E gets particularly complicated. 4 - Verbiage (these are examples of a style, and could be cleaned up/made more clear. Open to suggestions): 4A - View as [user | myself] 4B - View as [student | instructor] 4C - View as [Student Name | myself] 4D - Some graphical toggle/representation (what?) ? My opinion 4A . Because of how permissions and roles may be assigned, student to student and instructor to instructor may have different views of the report - we don't have a accurate way to describe that.
              Hide
              damyon Damyon Wiese added a comment -

              I understand that "view as student" makes sense for the common use case - but there are other use cases to consider as well - e.g. Moodle for workplace. Conceptually we do not have any such thing as "student" and "teacher" any user can have any number of roles at any context.

              What this is really doing - is toggling the whether the view includes hidden grades or not. I would think that if the user has the capability to see hidden grades, they should have a checkbox option to either show or hide hidden grades from the view and that achieves the same end result.

              Show
              damyon Damyon Wiese added a comment - I understand that "view as student" makes sense for the common use case - but there are other use cases to consider as well - e.g. Moodle for workplace. Conceptually we do not have any such thing as "student" and "teacher" any user can have any number of roles at any context. What this is really doing - is toggling the whether the view includes hidden grades or not. I would think that if the user has the capability to see hidden grades, they should have a checkbox option to either show or hide hidden grades from the view and that achieves the same end result.
              Hide
              emerrill Eric Merrill added a comment -

              That is why of you look, I don't recommend as student/teacher (option 4), I was just using that as nomenclature for the discussion.

              Also, hide/show hidden does not work, as the "student" may have view hidden permissions, and the aggregate can have various hidden settings, and the course may be set to show hidden items (or even just some hidden items).

              IMO, the report has to have 2 displays modes:

              1. Show the "teacher" report, like it does now with the viewing user's permissions
              2. Show the report as the other user ("student") will see it, based on their specific permissions and the course settings.

              So it becomes a matter of how do we convey that concept to the user using the report.

              Show
              emerrill Eric Merrill added a comment - That is why of you look, I don't recommend as student/teacher (option 4), I was just using that as nomenclature for the discussion. Also, hide/show hidden does not work, as the "student" may have view hidden permissions, and the aggregate can have various hidden settings, and the course may be set to show hidden items (or even just some hidden items). IMO, the report has to have 2 displays modes: Show the "teacher" report, like it does now with the viewing user's permissions Show the report as the other user ("student") will see it, based on their specific permissions and the course settings. So it becomes a matter of how do we convey that concept to the user using the report.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Hi, my opinion about the numbered aspects:

              1A - Agree, yes, the select must be there always irrespectively of permissions/results.

              2B - Agree, has sense to keep the last used "mode" for the session.

              3 - Disagree. Here I'm undecided. It clearly depends of which is the most important reason teachers do visit the user report. Is it used to view the information isolated for a student (3B) or is it used to verify which information the student will see (3A). Personally I feel myself inclined to use it for isolation/better reading, not for verifying how the student will see the information (I assume it will be ok always). But I'm not strong about this, maybe real teachers do use it in another way.

              4 - This is the more complex one, indeed. For sure, one of the options must be the "teacher view". I don't mind if it's called "default view" (note I did pick 3A above), "my view", "teacher view" or the translation of the "teacher" role for the course. Then, it's the other element, the tricky one. Right now I don't understand how all the settings/permissions are going to be "simulated", in fact I'm not 100% sure if it's achievable at all, is it?

              And that immediately leads me to the question... wouldn't be easier to, instead of that... simply show the "Change to role xxxxx" there and done? That way everything is evaluated in "real" mode with the correct permissions and it already exists and works. Do we really need to create another "switcher" feature when we do have one working for any page?

              Note that, if the "change to role xxxx" feature is decided to be used, that renders some/all of the questions above to fall out of context. That feature already has its way to work.

              If the "change to role xxxx" is not accepted as a viable solution, then I feel myself inclined to show there a list of different roles (different from the default/my one shown as default) having access to the report and then, go and simulate the view for each one with its permissions (again, if that is possible, I don't think so).

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Hi, my opinion about the numbered aspects: 1A - Agree, yes, the select must be there always irrespectively of permissions/results. 2B - Agree, has sense to keep the last used "mode" for the session. 3 - Disagree. Here I'm undecided. It clearly depends of which is the most important reason teachers do visit the user report . Is it used to view the information isolated for a student (3B) or is it used to verify which information the student will see (3A). Personally I feel myself inclined to use it for isolation/better reading, not for verifying how the student will see the information (I assume it will be ok always). But I'm not strong about this, maybe real teachers do use it in another way. 4 - This is the more complex one, indeed. For sure, one of the options must be the "teacher view". I don't mind if it's called "default view" (note I did pick 3A above), "my view", "teacher view" or the translation of the "teacher" role for the course. Then, it's the other element, the tricky one. Right now I don't understand how all the settings/permissions are going to be "simulated", in fact I'm not 100% sure if it's achievable at all, is it? And that immediately leads me to the question... wouldn't be easier to, instead of that... simply show the "Change to role xxxxx" there and done? That way everything is evaluated in "real" mode with the correct permissions and it already exists and works. Do we really need to create another "switcher" feature when we do have one working for any page? Note that, if the "change to role xxxx" feature is decided to be used, that renders some/all of the questions above to fall out of context. That feature already has its way to work. If the "change to role xxxx" is not accepted as a viable solution, then I feel myself inclined to show there a list of different roles (different from the default/my one shown as default) having access to the report and then, go and simulate the view for each one with its permissions (again, if that is possible, I don't think so). Ciao
              Hide
              emerrill Eric Merrill added a comment -

              Eloy Lafuente (stronk7) - You are making this much harder both conceptually and for the end user than it needs to be.

              Imagine you have 2 students, A is a Learner, and B is an Advanced Learner - who has different hidden grade permissions. Why should the teacher need to remember which role each student is? And why would the teacher every want to view student A as an Advanced Learner, when that is not what they are?

              Or what if you have supplemental role that only has grades:viewhidden and nothing else - with the intent of it being applied over an existing student enrolment. You would never be able to replicate the student view with "switch to role"

              We know the user we are showing the report for, and the context we are in, so it's trivial to show the report as the user will see it. Even if we are in the "show all users" mode, we can show each report exactly as each user will see it, regardless of their specific permissions.

              Also note that "teacher view" isn't a thing either - you can have teachers that do not have permissions to see hidden items. It's kinda stupid, but still possible.

              Show
              emerrill Eric Merrill added a comment - Eloy Lafuente (stronk7) - You are making this much harder both conceptually and for the end user than it needs to be. Imagine you have 2 students, A is a Learner, and B is an Advanced Learner - who has different hidden grade permissions. Why should the teacher need to remember which role each student is? And why would the teacher every want to view student A as an Advanced Learner, when that is not what they are? Or what if you have supplemental role that only has grades:viewhidden and nothing else - with the intent of it being applied over an existing student enrolment. You would never be able to replicate the student view with "switch to role" We know the user we are showing the report for, and the context we are in, so it's trivial to show the report as the user will see it. Even if we are in the "show all users" mode, we can show each report exactly as each user will see it, regardless of their specific permissions. Also note that "teacher view" isn't a thing either - you can have teachers that do not have permissions to see hidden items. It's kinda stupid, but still possible.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              Hi Eric,

              cannot but agree the switch role is not a good solution, let's forget it. it just came to my mind as an existing solution to "view information as", but, yes, it's tricky (and harder) to move to roles. So the duple must be, "current user" and "graded user", agree 100%.

              I think my problem with that "graded user" view is that I'm not 100% sure that "...so it's trivial to show the report as the user will see it." I mean, I was looking overall to code and, while preferences shouldn't be a problem, I saw a number of calls to various functions that are using $USER unconditionally. To be honest I did no go deeper in report/gradebook internals but saw every one of those uses like a potential problem against being able to show information as "another user". And from that I came to the "switch roles (idiot) proposal".

              In any case, maybe it's better to try it and implementation will tell us if it was trivial or we did end modifying too much. So, go for it. About 4 I think that viewing report as "graded user or myself" (very similar to your orginal 4A propsal) does kill any reference to a possible role and clearly defines how information is being shown, so my vote goes for it.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited Hi Eric, cannot but agree the switch role is not a good solution, let's forget it. it just came to my mind as an existing solution to "view information as", but, yes, it's tricky (and harder) to move to roles. So the duple must be, "current user" and "graded user", agree 100%. I think my problem with that "graded user" view is that I'm not 100% sure that " ...so it's trivial to show the report as the user will see it. " I mean, I was looking overall to code and, while preferences shouldn't be a problem, I saw a number of calls to various functions that are using $USER unconditionally. To be honest I did no go deeper in report/gradebook internals but saw every one of those uses like a potential problem against being able to show information as "another user". And from that I came to the "switch roles (idiot) proposal". In any case, maybe it's better to try it and implementation will tell us if it was trivial or we did end modifying too much. So, go for it. About 4 I think that viewing report as "graded user or myself" (very similar to your orginal 4A propsal) does kill any reference to a possible role and clearly defines how information is being shown, so my vote goes for it. Ciao
              Hide
              dougiamas Martin Dougiamas added a comment -

              +1 for a clear button/toggle to "Show exactly what Johnny sees"

              Show
              dougiamas Martin Dougiamas added a comment - +1 for a clear button/toggle to "Show exactly what Johnny sees"
              Hide
              urpokarhu Jari Vilkman added a comment -

              Hello,

              Any progress on this?

              Show
              urpokarhu Jari Vilkman added a comment - Hello, Any progress on this?
              Hide
              emerrill Eric Merrill added a comment -

              Sorry for the delay on this one. I'm going to get working on this again over the next couple weeks.

              Show
              emerrill Eric Merrill added a comment - Sorry for the delay on this one. I'm going to get working on this again over the next couple weeks.
              Hide
              emerrill Eric Merrill added a comment -

              Luckily, the core logic was written in MDL-43197 in 2.6, we just need to connect it up to the UI.

              I went with a simple 'View report as' (User|Myself) single select under the user list. I think that we can't wait for a perfect UI solution here - this or something similar should be acceptable, and is better than the status quo.

              It defaults to User, and remembers the selecting as a user preference (so it always remembers your last selection, like assign with the submissions list filter settings)

              I added some behat tests to cover the new and existing views

              Show
              emerrill Eric Merrill added a comment - Luckily, the core logic was written in MDL-43197 in 2.6, we just need to connect it up to the UI. I went with a simple 'View report as' (User|Myself) single select under the user list. I think that we can't wait for a perfect UI solution here - this or something similar should be acceptable, and is better than the status quo. It defaults to User, and remembers the selecting as a user preference (so it always remembers your last selection, like assign with the submissions list filter settings) I added some behat tests to cover the new and existing views
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-30179 using repository: git://github.com/merrill-oakland/moodle.git

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-30179 using repository: git://github.com/merrill-oakland/moodle.git master (4 errors / 0 warnings) [branch: MDL-30179-master | CI Job ] phplint (0/0) , phpcs (2/0) , js (0/0) , css (1/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-30179 using repository: git://github.com/merrill-oakland/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-30179 using repository: git://github.com/merrill-oakland/moodle.git master (0 errors / 0 warnings) [branch: MDL-30179-master | CI Job ] More information about this report
              Hide
              abgreeve Adrian Greeve added a comment -

              Hello Eric,

              Thank you for the patch. The code looks fine. I just found a couple of behat tests that also need to be updated to continue passing:

              • grade/tests/behat/grade_aggregation.feature
              • grade/tests/behat/grade_hidden_items.feature

              These reports need to change the "View report as" setting to "Myself". It should be ready for integration after that is fixed up.

              Kind regards,

              Adrian

              Show
              abgreeve Adrian Greeve added a comment - Hello Eric, Thank you for the patch. The code looks fine. I just found a couple of behat tests that also need to be updated to continue passing: grade/tests/behat/grade_aggregation.feature grade/tests/behat/grade_hidden_items.feature These reports need to change the "View report as" setting to "Myself". It should be ready for integration after that is fixed up. Kind regards, Adrian
              Hide
              emerrill Eric Merrill added a comment -

              Good catch, I should have caught that!

              Submitting for integration.

              Also a note, IMO, we should consider this for backporting after the waiting period, at least to 3.1, since it is LTS, and this has been a pretty major oversight/error for quite some time. I'm posting it here so you/integrators can register their opinions about that possibility.

              Show
              emerrill Eric Merrill added a comment - Good catch, I should have caught that! Submitting for integration. Also a note, IMO, we should consider this for backporting after the waiting period, at least to 3.1, since it is LTS, and this has been a pretty major oversight/error for quite some time. I'm posting it here so you/integrators can register their opinions about that possibility.
              Hide
              stronk7 Eloy Lafuente (stronk7) 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
              stronk7 Eloy Lafuente (stronk7) 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
              jprovasnik John Provasnik added a comment -

              I am out of the office Thursday, Oct 6 through Friday Oct 7. If this is
              an urgent matter, please email isd@21cccs.org.

              John Provasnik

              Show
              jprovasnik John Provasnik added a comment - I am out of the office Thursday, Oct 6 through Friday Oct 7. If this is an urgent matter, please email isd@21cccs.org. John Provasnik
              Hide
              cibot CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-30179 using repository: git://github.com/merrill-oakland/moodle.git

              • master [branch: MDL-30179-master | CI Job]
                • Error: The MDL-30179-master branch at git://github.com/merrill-oakland/moodle.git does not apply clean to origin/master

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-30179 using repository: git://github.com/merrill-oakland/moodle.git master [branch: MDL-30179-master | CI Job ] Error: The MDL-30179 -master branch at git://github.com/merrill-oakland/moodle.git does not apply clean to origin/master Should these errors be fixed?
              Hide
              dmonllao David Monllaó added a comment -

              Thanks for working on this Eric.

              It seems that this issue evolved from a bug to an improvement. The bugs described in this issue (the description has not been updated since 2011) are not applicable to master any more. I haven't spent hours looking at this, but the only bug I've noticed is that average column does not seem affected by "Course grade settings" -> "User report" settings, like "Hide totals if they contain hidden items" for example. This is a bug and should be fixed in stables, if we find other inconsistencies between the user report and the grader report they can be considered a bug as well and backport fixes to stables.

              The proposed patch is an improvement though, the Allow teacher to toggle to/from "user view" part of the issue's title. I think it is important that we separate both things because we don't backport improvements and the issue's title is misleading.

              Show
              dmonllao David Monllaó added a comment - Thanks for working on this Eric. It seems that this issue evolved from a bug to an improvement. The bugs described in this issue (the description has not been updated since 2011) are not applicable to master any more. I haven't spent hours looking at this, but the only bug I've noticed is that average column does not seem affected by "Course grade settings" -> "User report" settings, like "Hide totals if they contain hidden items" for example. This is a bug and should be fixed in stables, if we find other inconsistencies between the user report and the grader report they can be considered a bug as well and backport fixes to stables. The proposed patch is an improvement though, the Allow teacher to toggle to/from "user view" part of the issue's title. I think it is important that we separate both things because we don't backport improvements and the issue's title is misleading.
              Hide
              dmonllao David Monllaó added a comment -

              I am keen to integrate this solution as there is already agreement and many votes (from 2015 probably from the gradebook working group) but I wouldn't like that we just forget about the original reported problem.

              Show
              dmonllao David Monllaó added a comment - I am keen to integrate this solution as there is already agreement and many votes (from 2015 probably from the gradebook working group) but I wouldn't like that we just forget about the original reported problem.
              Hide
              dmonllao David Monllaó added a comment - - edited

              Some minor comments about the patch:

              1. $PAGE->url is an object, it is safer to clone the object if we want to modify it
              2. Please, don't use ids for styling
              3. There are conflicts, please rebase on top of master.
              Show
              dmonllao David Monllaó added a comment - - edited Some minor comments about the patch: $PAGE->url is an object, it is safer to clone the object if we want to modify it Please, don't use ids for styling There are conflicts, please rebase on top of master.
              Hide
              emerrill Eric Merrill added a comment -

              To me, this ticket is now what the title says - which is perfectly descriptive, but the narrative is outdated. The problem is that this is the ticket that all the duplicates were collapsed into, but it seems like they shouldn't have been.

              I would agree that the average and rank issues can be split off into new tickets - although the solution here makes them a bit less problematic, since the tearch will see exactly as the student sees again.

              1. Is is already a clone
              2. Done
              3. Done
              Show
              emerrill Eric Merrill added a comment - To me, this ticket is now what the title says - which is perfectly descriptive, but the narrative is outdated. The problem is that this is the ticket that all the duplicates were collapsed into, but it seems like they shouldn't have been. I would agree that the average and rank issues can be split off into new tickets - although the solution here makes them a bit less problematic, since the tearch will see exactly as the student sees again. Is is already a clone Done Done
              Hide
              dmonllao David Monllaó added a comment -

              Sure, np, it is fine, but please create issues for the bugs, some people voted for them to be fixed would be a pity to miss them.

              Integrated to master.

              Show
              dmonllao David Monllaó added a comment - Sure, np, it is fine, but please create issues for the bugs, some people voted for them to be fixed would be a pity to miss them. Integrated to master.
              Hide
              jpataleta Jun Pataleta added a comment -

              Thanks for working on this Eric. Works as described and as expected. Behat tests are all passed.

              Tested on master. Passing!

              Show
              jpataleta Jun Pataleta added a comment - Thanks for working on this Eric. Works as described and as expected. Behat tests are all passed. Tested on master. Passing!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Changes have been rolled upstream and are available in main moodle.git repo and official clones. Many thanks for your hard work and awesome solutions. Closing this as fixed!

              I have some ideas on how to fix that.
              They're not very good ideas, but
              at least they're ideas!
              — Adam Savage

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Changes have been rolled upstream and are available in main moodle.git repo and official clones. Many thanks for your hard work and awesome solutions. Closing this as fixed! I have some ideas on how to fix that. They're not very good ideas, but at least they're ideas! — Adam Savage
              Hide
              emerrill Eric Merrill added a comment -

              Backport request created MDL-56592

              Also, per David Monllaó's comments, a followup for Rank/Average columns was opened MDL-56593

              Show
              emerrill Eric Merrill added a comment - Backport request created MDL-56592 Also, per David Monllaó 's comments, a followup for Rank/Average columns was opened MDL-56593

                People

                • Votes:
                  24 Vote for this issue
                  Watchers:
                  32 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    5/Dec/16