Allow non-authorized teachers to view CSF teacher markdown #13947
Conversation
CSF doesn't have cohorts, so we can't validate teacher accounts.
| else | ||
| false | ||
| end | ||
| end |
There was a problem hiding this comment.
nit:
return true if current_user.try(:authorized_teacher?)
return @script.k5_course? || @script.k5_draft_course? if current_user.try(:teacher?)
false
There was a problem hiding this comment.
Hmm, why? I think it's clearer as written.
There was a problem hiding this comment.
Keep as-is, if you prefer it as-is, my preference isn't that strong.
|
|
||
| stubs(:current_user).returns(@student) | ||
| refute can_view_teacher_markdown? | ||
| end |
There was a problem hiding this comment.
This is (EDIT: looks like) three different tests. Please make it three different tests.
There was a problem hiding this comment.
Note that the third test depends on @script.stubs(:k5_course?).returns(true) from line 1143. I can separate these into two tests if you prefer.
There was a problem hiding this comment.
I'm not sure how much value there is in a refute can_view_teacher_markdown? standalone test. The goal here is to verify initial assumptions for the rest of the test.
There was a problem hiding this comment.
That standalone test would be testing that users who aren't logged in can't see teacher markdown. If I make a change to can_view_teacher_markdown? that screws up the logic for logged-out users, but it still works correctly for non-authorized teachers, it would be nice if this test case kept passing but a separate logged out users can't see teacher markdown test failed.
|
LGTM |
|
PTAL |
CSF doesn't have cohorts, so we can't validate teacher accounts. Confirming with @ryansloan, is this what you expected?