add a new user permission to determine whether a user is an authorize… #17199
Conversation
| @@ -41,7 +41,9 @@ class UserPermission < ActiveRecord::Base | |||
| # professional development workshop attendance. | |||
| WORKSHOP_ORGANIZER = 'workshop_organizer'.freeze, | |||
| # Grants ability to conduct peer reviews for professional learning courses | |||
| PLC_REVIEWER = 'plc_reviewer'.freeze | |||
| PLC_REVIEWER = 'plc_reviewer'.freeze, | |||
| # Makes the account satisfy authorized_teacher? | |||
There was a problem hiding this comment.
This could probably have a more useful description, but I'm not sure what that would be
There was a problem hiding this comment.
Something that matches the pattern of the other commends would be ideal.
It looks like authorized teachers are special in that they can't have their accounts locked and are able to view teacher markdown and level examples; a brief summary of that would probably be just fine.
There was a problem hiding this comment.
Something like "An Authorized Teacher account belongs to someone who is verified as a teacher. They can see answer keys, teacher-facing markdown, and other locked-down teacher-only content" would work
dashboard/app/models/user.rb
Outdated
| @@ -858,8 +858,8 @@ def teacher? | |||
| end | |||
|
|
|||
| def authorized_teacher? | |||
| # you are "really" a teacher if you are a teacher in any cohort for an ops workshop or in a plc course | |||
| admin? || (teacher? && (cohorts.present? || plc_enrollments.present?)) || | |||
| # you are "really" a teacher if you have the AUTHORIZED_TEACHER user permissions, or are a teacher in any cohort for an ops workshop or in a plc course | |||
There was a problem hiding this comment.
It looks like this comment is slightly stale, mind updating it? Perhaps:
# You are an authorized teacher if you are an admin, have the AUTHORIZED_TEACHER or the
# LEVELBUILDER permission, or are a teacher in a cohort or PLC course.
dashboard/app/models/user.rb
Outdated
| # you are "really" a teacher if you are a teacher in any cohort for an ops workshop or in a plc course | ||
| admin? || (teacher? && (cohorts.present? || plc_enrollments.present?)) || | ||
| # you are "really" a teacher if you have the AUTHORIZED_TEACHER user permissions, or are a teacher in any cohort for an ops workshop or in a plc course | ||
| permission?(UserPermission::AUTHORIZED_TEACHER) || admin? || (teacher? && (cohorts.present? || plc_enrollments.present?)) || | ||
| permission?(UserPermission::LEVELBUILDER) |
There was a problem hiding this comment.
super nit: Though I'm probably to blame for the existing code, I think it is more readable as:
return false unless teacher?
return true if admin?
if permission?(UserPermission::AUTHORIZED_TEACHER) || permission?(UserPermission::LEVELBUILDER)
return true
end
return true if cohorts.present? || plc_enrollments.present?
false
There was a problem hiding this comment.
Feel welcome to disagree and keep as-is, if you feel the existing is more readable.
| permission?(UserPermission::LEVELBUILDER) | ||
| # You are an authorized teacher if you are an admin, have the AUTHORIZED_TEACHER or the | ||
| # LEVELBUILDER permission, or are a teacher in a cohort or PLC course. | ||
| return true if admin? |
There was a problem hiding this comment.
Admins should not be authorized teachers
There was a problem hiding this comment.
Nor should LEVELBUILDERs, IMO. Feel welcome to remove them (ideally in another PR) if you want to deal with the associated fallout.
There was a problem hiding this comment.
Levelbuilders definitely should be so that they can see the teacher markdown that they are editing. Anyone have context for why admins previously were?
There was a problem hiding this comment.
Possibly because we used to let admins do everything but now they can't see Scripts, so there's no point to giving them this permission
There was a problem hiding this comment.
My guess is that admin? predates the UserPermission model (circa a year ago?).
https://trello.com/c/wraJEFSl/129-create-authorized-teacher-property-on-user-and-simple-management-page
And the description of the feature: https://docs.google.com/document/d/15Rk04e0BlGiDZpo82YcQ7qDpRgS5aGPH93IF85hVpR4/edit#
Per Elijah's recommendation, I created a user permission so that this can be managed on the existing permissions management page. This PR does not include the data migration to set the new user permission for preexisting authorized teachers.