Code review v2 clean-up #46549
Code review v2 clean-up #46549
Conversation
maureensturgeon
changed the title
| @@ -24,7 +24,7 @@ def project_commits | |||
| user_storage_id, project_id = storage_decrypt_channel_id(params[:channel_id]) | |||
| project_owner = User.find_by(id: user_id_for_storage_id(user_storage_id)) | |||
| return render :not_acceptable unless project_owner | |||
| return render :forbidden unless can?(:project_commits, ProjectVersion.new, project_owner, project_id) | |||
| return render :forbidden unless can?(:project_commits, ProjectVersion.new, project_owner) | |||
There was a problem hiding this comment.
(minor) This seems a little strange to create a ProjectVersion object and then not use it at all. Maybe define an action on Project or User instead? Also ok to just leave as-is.
Also, should the create endpoint above check some kind of permissions? (ok to fix later).
There was a problem hiding this comment.
I'm not going to fix create endpoint right now
dashboard/app/models/ability.rb
Outdated
| @@ -201,6 +202,11 @@ def initialize(user) | |||
| ) | |||
| can_view_as_user_for_code_review = true | |||
| end | |||
|
|
|||
| # Code review V2 | |||
| if user != user_to_assume && !user_to_assume.student_of?(user) && can?(:code_review, user_to_assume) | |||
There was a problem hiding this comment.
What case does !user_to_assume.student_of?(user) cover?
There was a problem hiding this comment.
I copied this from v1, I guess the teacher isn't viewing as a code reviewer like the student is, the teacher is viewing through the teacher view? We should probably run through all of these scenarios in a bug bash and make sure it all works as expected
There was a problem hiding this comment.
Hmmm, we don't have to hold up this PR, but we should clarify with mike what this pane looks like for the teacher of a student. Does it looks just like when a (student) peer views it? If not, what's different?
What happens if a teacher creates a code review?
Also, I wonder if teachers practice this in PL and what view (student or teacher) they expect to see.
There was a problem hiding this comment.
Catching up after a week out -- did y'all get clarity on this? I had to some work in the spring to get V1 to work for teacher accounts acting as participants (ie, the PL scenario), and my understanding at that point was that we wanted code review to work for a teacher account enrolled as in a section as it would for a student account:
There was a problem hiding this comment.
That seems right, that we'd want the teacher to be able to participate as a student. We'll want to make sure that scenario works. During the bug bash last week it was decided that the teacher should be able to comment on student's code reviews and delete comments, but there wasn't more clarity given on specific teacher scenario teachers/experience.
Links
Testing story
Tested locally
PR Checklist: