Contrib: Gradebook2
  1. Contrib: Gradebook2
  2. GRBK-1144

User with gradebook.gradeSection cannot upload correctly formatted csv files

    Details

    • Target Version:
    • QA Status:
      PASS
    • Previous Issue Keys:

      Description

      Here's a description from our academic support person:

      Generally speaking, when we've got the permissions set so that gsi's can only grade their own students, they cannot import grades via .csv. The Import function appears to require gradebook.gradeAll

      For instance, instructor1 is associated only with the group called Section 1, which includes student1.

      In the GB2 interface, instructor1 can enter grades for student1. But if instructor1 tries to import a correctly formatted csv with student1 data, GB2 returns "You are not authorized to upload grades." Note that this error message appears *after* GB2 shows the Preview of Imported Grades.

      As soon as you uncheck gradebook.gradeSection and check gradebook.gradeAll in Realms, instructor1 is able to successfully upload the same csv. Note, you have to make this change both for the site realm and also the group realm. And once you do that, then instructor1 can view and grade all students, not just his own.
      1. GRBK-1144.patch
        0.8 kB
        Jim Eng
      1. gb2CantUpload.jpg
        111 kB

        Issue Links

          Activity

          Hide
          Jim Eng added a comment -
          It looks like org.sakaiproject.gradebook.gwt.sakai.Gradebook2ComponentServiceImpl.upload() checks the "gradebook.gradeAll" permission. Would it hurt anything if that method checked for "gradebook.gradeAll" OR "gradebook.gradeSection"?

          If not, it looks like it could be accomplished by calling org.sakaiproject.gradebook.gwt.sakai.Gradebook2Authz.isUserAbleToGrade() instead of org.sakaiproject.gradebook.gwt.sakai.Gradebook2Authz.isUserAbleToGradeAll().
          Show
          Jim Eng added a comment - It looks like org.sakaiproject.gradebook.gwt.sakai.Gradebook2ComponentServiceImpl.upload() checks the "gradebook.gradeAll" permission. Would it hurt anything if that method checked for "gradebook.gradeAll" OR "gradebook.gradeSection"? If not, it looks like it could be accomplished by calling org.sakaiproject.gradebook.gwt.sakai.Gradebook2Authz.isUserAbleToGrade() instead of org.sakaiproject.gradebook.gwt.sakai.Gradebook2Authz.isUserAbleToGradeAll().
          Hide
          Jim Eng added a comment -
          As Jon G pointed out, if taking the approach described above, it may be necessary to check each name in the file to make sure user with "gradebook.gradeSection" has permission to grade that user (i.e. is that user in a section for which current user has "gradebook.gradeSection" permission?).
          Show
          Jim Eng added a comment - As Jon G pointed out, if taking the approach described above, it may be necessary to check each name in the file to make sure user with "gradebook.gradeSection" has permission to grade that user (i.e. is that user in a section for which current user has "gradebook.gradeSection" permission?).
          Hide
          Beth Kirschner added a comment -
          Let's work on a patch for the simplest solution -- just allowing uploads with the gradeSection permission. If there are further problems with specific users, that can be written up as a separate JIRA.
          Show
          Beth Kirschner added a comment - Let's work on a patch for the simplest solution -- just allowing uploads with the gradeSection permission. If there are further problems with specific users, that can be written up as a separate JIRA.
          Hide
          Jim Eng added a comment -
          We will need test cases to document behavior of the patch. Here are some possible cases:

          1) User with "gradebook.gradeSection" permission (and not "gradebook.gradeAll" permission) uploads file containing records only for students user has permission to grade.
          2) User with "gradebook.gradeSection" permission (and not "gradebook.gradeAll" permission) uploads file containing records for students user has no permission to grade (as well as records for students user has permission to grade).
          3) User with "gradebook.gradeSection" permission (and not "gradebook.gradeAll" permission) uploads file containing records only for students user has no permission to grade.

          We need to document the cases in which the upload succeeds or fails and the way in which failure is reported to the user. For example, is an entire file rejected if it results in one permission failure? If upload of a file fails to assign scores for some students because of permission checks, does it fail silently? Does it identify incorrect records so they can be fixed?
           
          Show
          Jim Eng added a comment - We will need test cases to document behavior of the patch. Here are some possible cases: 1) User with "gradebook.gradeSection" permission (and not "gradebook.gradeAll" permission) uploads file containing records only for students user has permission to grade. 2) User with "gradebook.gradeSection" permission (and not "gradebook.gradeAll" permission) uploads file containing records for students user has no permission to grade (as well as records for students user has permission to grade). 3) User with "gradebook.gradeSection" permission (and not "gradebook.gradeAll" permission) uploads file containing records only for students user has no permission to grade. We need to document the cases in which the upload succeeds or fails and the way in which failure is reported to the user. For example, is an entire file rejected if it results in one permission failure? If upload of a file fails to assign scores for some students because of permission checks, does it fail silently? Does it identify incorrect records so they can be fixed?  
          Hide
          Jim Eng added a comment -
          Attaching patch with "simple" fix for GRBK-1144.

          Simple fix handles the basic cases very well. Instructor (or TA?) with "gradebook.gradeSection" permission but not "gradebook.gradeAll" permission is able to upload a file containing scores on an assignment for students in that section. We then added another assignment and added one student who is in a different section to the file, assigning scores for all students, including the student this instructor should not be able to grade. In that case, when the file was uploaded, the score for that one student was highlighted in the preview. After confirming the upload, all scores were successfully entered except the score for the student who was not in the section.
          Show
          Jim Eng added a comment - Attaching patch with "simple" fix for GRBK-1144 . Simple fix handles the basic cases very well. Instructor (or TA?) with "gradebook.gradeSection" permission but not "gradebook.gradeAll" permission is able to upload a file containing scores on an assignment for students in that section. We then added another assignment and added one student who is in a different section to the file, assigning scores for all students, including the student this instructor should not be able to grade. In that case, when the file was uploaded, the score for that one student was highlighted in the preview. After confirming the upload, all scores were successfully entered except the score for the student who was not in the section.
          Hide
          Jim Eng added a comment -
          We next created a file containing all 127 of the students in the class, only about one-fifth of whom the current instructor has permission to grade. We entered scores for all 127 students and uploaded the file. In the preview screen, all students who were not in the instructor's section were highlighted. After the upload was completed, the scores for the students in that section were changed, and no scores for other students were affected.

          This seems like a good solution to this problem.
          Show
          Jim Eng added a comment - We next created a file containing all 127 of the students in the class, only about one-fifth of whom the current instructor has permission to grade. We entered scores for all 127 students and uploaded the file. In the preview screen, all students who were not in the instructor's section were highlighted. After the upload was completed, the scores for the students in that section were changed, and no scores for other students were affected. This seems like a good solution to this problem.
          Hide
          Jim Eng added a comment -
          Also, a separate message is posted in the logs for each score that is rejected during the import. Here's a sample of the message, with identifying info about the students obscured:

          2011-10-07 14:18:37,158 WARN TP-Processor10 org.sakaiproject.gradebook.gwt.sakai.Gradebook2ComponentServiceImpl - Invalid Input: Failed to score numeric item for Some Student and item 23 to 10.0 : You are not authorized to grade this student for this item.
          Show
          Jim Eng added a comment - Also, a separate message is posted in the logs for each score that is rejected during the import. Here's a sample of the message, with identifying info about the students obscured: 2011-10-07 14:18:37,158 WARN TP-Processor10 org.sakaiproject.gradebook.gwt.sakai.Gradebook2ComponentServiceImpl - Invalid Input: Failed to score numeric item for Some Student and item 23 to 10.0 : You are not authorized to grade this student for this item.
          Hide
          Thomas Amsler added a comment -
          Do you guys want this patch also applied to the 1.5.1 branch?
          Show
          Thomas Amsler added a comment - Do you guys want this patch also applied to the 1.5.1 branch?
          Hide
          Jim Eng added a comment -
          We would like to have that patch applied to trunk and the 1.5.1 branch if you agree that it solves the problem.

          As noted above, it looks to me like the existing code checks each entry in the file to ensure the permissions are correct to allow the person uploading the file to grade each student. If the file includes any entries for other students, they are rejected, and the user uploading the file is given feedback.

          The only improvement I could see would be to show that user an explicit message saying the highlighted scores are being rejected because the instructor lacks permission to grade those students. But I think that will be clear to most users.
          Show
          Jim Eng added a comment - We would like to have that patch applied to trunk and the 1.5.1 branch if you agree that it solves the problem. As noted above, it looks to me like the existing code checks each entry in the file to ensure the permissions are correct to allow the person uploading the file to grade each student. If the file includes any entries for other students, they are rejected, and the user uploading the file is given feedback. The only improvement I could see would be to show that user an explicit message saying the highlighted scores are being rejected because the instructor lacks permission to grade those students. But I think that will be clear to most users.
          Hide
          Thomas Amsler added a comment -
          Merged patch into 1.5.x-test branch and created 1.5.1-b3 tag for UMICH:
          https://source.sakaiproject.org/contrib/gradebook2/tags/1.5.1-b3/
          Show
          Thomas Amsler added a comment - Merged patch into 1.5.x-test branch and created 1.5.1-b3 tag for UMICH: https://source.sakaiproject.org/contrib/gradebook2/tags/1.5.1-b3/
          Hide
          Thomas Amsler added a comment -
          Applied UMICH provided patch to trunk @ r76878
          Show
          Thomas Amsler added a comment - Applied UMICH provided patch to trunk @ r76878
          Hide
          David L. Woods added a comment -
          QA Testing

          There doesn't seem to be any affect from the patch in our environment with standard permission settings.

          When I modify a site to remove "gradebook.gradeAll" permission from the instructor role (leaving the "gradebook.gradeSection" permission), instructors lose the ability to see the students (they can't be assigned to a section using the Sections tool, so they don't see any sections' students) or the gradebook structure in the gradebook == so they can't grade anyone. They do have the Import choice available on the Tools menu, but, although the import process starts, they get a message saying they aren't authorized to grade the students before the import completes.

          When I go in as a TA (the role by definition has only the "gradebook.gradeSection" permission and not the "gradebook.gradeAll" permission), they can see and grade students if they have been assigned to a section in the Sections tool (or put in a group with students in manually created Group), but they don't have the Import option available on the Tools menu (oddly, they do have the export option). So they can't import grades for their sections' students.

          I found that to get this to work, I also needed to adjust the section.role..... permissions for each role, as well as adjusting the gradebook.grade.... permissions.

          "section.role.instructor" permission, when turned on, allows user to assign, as TAs to sections in the Sections tool, any role that has the "section.role.ta" permission turned on. It also allows the gradebook import function to be visible.

          "section.role.ta" permission allows the user to be assigned to a section in the Sections tool.

          "gradebook.gradeAll" permission allows the user to see all students in all sections; see the structure of the gradebook; export all students or selected sections from the gradebook; import spreadsheet of grades for all or a subset of students to the gradebook.

          "gradebook.gradeSection" permission allows the user, when assigned to a section in the Sections tool, to see the gradebook structure, see the students in their assigned section(s), export a gradebook spreadsheet with their section(s)' students.

          Since I can now adjust the permissions for to allow export and import of only individual section grades, which is what the patch was attempting to accomplish,

          QA Pass
          Show
          David L. Woods added a comment - QA Testing There doesn't seem to be any affect from the patch in our environment with standard permission settings. When I modify a site to remove "gradebook.gradeAll" permission from the instructor role (leaving the "gradebook.gradeSection" permission), instructors lose the ability to see the students (they can't be assigned to a section using the Sections tool, so they don't see any sections' students) or the gradebook structure in the gradebook == so they can't grade anyone. They do have the Import choice available on the Tools menu, but, although the import process starts, they get a message saying they aren't authorized to grade the students before the import completes. When I go in as a TA (the role by definition has only the "gradebook.gradeSection" permission and not the "gradebook.gradeAll" permission), they can see and grade students if they have been assigned to a section in the Sections tool (or put in a group with students in manually created Group), but they don't have the Import option available on the Tools menu (oddly, they do have the export option). So they can't import grades for their sections' students. I found that to get this to work, I also needed to adjust the section.role..... permissions for each role, as well as adjusting the gradebook.grade.... permissions. "section.role.instructor" permission, when turned on, allows user to assign, as TAs to sections in the Sections tool, any role that has the "section.role.ta" permission turned on. It also allows the gradebook import function to be visible. "section.role.ta" permission allows the user to be assigned to a section in the Sections tool. "gradebook.gradeAll" permission allows the user to see all students in all sections; see the structure of the gradebook; export all students or selected sections from the gradebook; import spreadsheet of grades for all or a subset of students to the gradebook. "gradebook.gradeSection" permission allows the user, when assigned to a section in the Sections tool, to see the gradebook structure, see the students in their assigned section(s), export a gradebook spreadsheet with their section(s)' students. Since I can now adjust the permissions for to allow export and import of only individual section grades, which is what the patch was attempting to accomplish, QA Pass

            People

            • Assignee:
              Jim Eng
              Reporter:
              Jim Eng
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: