Allow sections to disable code review #42082

bethanyaconnor · 2021-08-17T19:41:48Z

This PR adds a setting on a section that can disable code review.

When code review is always disabled for a section:

A teacher can still comment on the code of one of their students
A student can still comment on their own code
A student cannot view or comment on another student's project
A student cannot mark their project as ready for review

When code review is disabled after code comments have been created:

A teacher can still comment on the code of one of their students
A student can still comment on their own code
A student cannot view or comment on another student's project
A student cannot change the review-status of their project
Any comments that already existed stick around. Teachers can still delete those comments

Links

https://codedotorg.atlassian.net/browse/CSA-693

Testing story

Added quite a few unit tests, plus some manual testing. If reviewers would like to test this locally, please have at it!

Follow-up work

The way we check whether to show this setting on the section dialog is not future proof. We will need a new solution before CSA is fully launched.

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

…A pilot sections

bethanyaconnor · 2021-08-17T21:40:18Z

apps/src/templates/teacherDashboard/EditSectionForm.jsx

@@ -24,31 +24,7 @@ import ConfirmHiddenAssignment from '../courseOverview/ConfirmHiddenAssignment';
 import {SectionLoginType} from '@cdo/apps/util/sharedConstants';
 import firehoseClient from '@cdo/apps/lib/util/firehose';

-const style = {


I think because this is named style and not styles it wasn't moved down automatically, so I'm moving it down while I'm here.

bethanyaconnor · 2021-08-17T21:41:03Z

apps/src/templates/teacherDashboard/EditSectionForm.jsx

@@ -205,6 +181,11 @@ class EditSectionForm extends Component {
    const showLoginTypeField =
      !isNewSection && changeableLoginTypes.includes(section.loginType);

+    // These are server-side experiments, which are passed down to the client
+    const showCodeReviewEnabledCheckbox =


This should work for now but is not future-proof. I had trouble coming up with a good future-proof way of only showing this setting for CSA courses and couldn't come up with anything.

I'm guessing we don't have access to the course here?

Hmm we might actually. I'll look. Though I think we'll still have to hardcode the courses here but that is a bit better than the pilots.

Are there course settings similar to the level settings? i.e. I think certain courses you can only view if you're over 13 and some are hidden by default, etc.

I think the list of courses is populated by a server call. I just got distracted but I was trying to pull through the course name and check against that (we do have the course id here but I don't want to hard code in an id that will be environment-dependent).

+1 if it's possible to use the course...are our CSA pilot scripts part of some family (not sure if this is the right word in cplat any more) that will be reused across each year's version of the course?

So thinking a bit more on this I realized that this dialog is also already kind of broken for pilots and it doesn't pre-fill the course if assigning a pilot course. Which means that accessing the course info seems like a pretty fragile approach. I'm going to keep this as-is for now but continue to think on this and I can always update later. Thanks for the ideas all!

Oh sorry, replied before I saw Ben's comment!

re our CSA pilot scripts part of some family (not sure if this is the right word in cplat any more) that will be reused across each year's version of the course?

Yes! Currently one is in the csa family name (which I don't see changing) and the other in csa-pilot-facilitator (which likely won't be reused). I'm going to stick with this for now like I said but this thread has given me a lot of ideas that I can hopefully follow-up on this week.

molly-moen

Looks good! Thanks for adding so many tests

jmkulwik · 2021-08-19T23:51:38Z

apps/src/templates/teacherDashboard/EditSectionForm.jsx

+    padding: '0.3em'
+  },
+  sectionNameInput: {
+    // Full-width, large happy text, lots of space.


Haha I know you didn't add this, but this makes me smile :)

bencodeorg · 2021-08-20T16:07:37Z

apps/src/templates/instructions/ReviewTab.jsx

              (viewAsCodeReviewer
                ? this.renderBackToMyProject(this.onClickBackToProject)
                : this.renderPeerDropdown(reviewablePeers, this.onSelectPeer))}
-            {this.renderReadyForReviewCheckbox()}
+            {codeReviewEnabled && this.renderReadyForReviewCheckbox()}


Just confirming the desired behavior is that if you have any code review comments on your project, you still see them even if code review is turned off your your section?

Good question! I made that assumption as teachers can still delete comments but I'd love @jmkulwik to weigh in here!

CodeReviewEnabled should act the same way as the ReadyForReview checkbox for these cases. So generally, yes, you should still be able to see the comments.

bencodeorg · 2021-08-20T16:08:49Z

apps/i18n/common/en_us.json

@@ -772,6 +772,8 @@
  "enableMakerDialogDescription": "Maker Toolkit is a feature used in the Computer Science Discoveries curriculum. See the setup page for more details:",
  "enableMakerDialogSetupPageLinkText": "Maker Toolkit Setup",
  "enablePairProgramming": "Allow students to Pair Program?",
+  "enablePeerFeedback": "Enable Peer Feedback?",


Is this the spec'd text? I have been waffling between the language of "peer feedback" and "code review", not sure which phrases we use in which contexts. @moneppo any thoughts?

haha and I now see you had a similar question here :)

yeah I'd also defer to @moneppo but one thought here is that "Peer Feedback" seems to describe the ability for students to comment on each others' projects (which is what is being enabled/disabled here) vs "Code Review" describes the whole review tab, including teacher comments?

Also realizing I'm saying conflicting things now :) I misunderstood on the other PR and thought the flag was enabling/disabling the Code Review feature entirely (rather than just commenting on peers' projects) but seeing how it's used here, "Peer Feedback" seemed to make more sense. So that being said, since the migration already uses "Code Review" i think sticking to "code review" for all the flags/parameters while using "Peer Feedback" in the user-facing component is okay?

Let's use the spec'd text for the UI components for now, but try to keep the code consistent. I've put a task on the product sprint to take a final pass of all the copy for the whole Code Review feature to ensure we're using consistent language.

Task: https://codedotorg.atlassian.net/browse/CSA-710

Sorry for not bringing up the question more broadly 😞 I tried to keep the code consistent and just this copy uses "peer feedback".

FWIW, I could definitely change all the variables to be peer_feedback_enabled (or the like) but I'm not sure how easy the column would be to change.

Personally, having i18n keys similar to their values makes things simpler for me when I'm reading code. The key references a string in i18n whereas variables and columns reference features. Neither of those are strong opinions.

Agreed, plus having the kets match the values makes searching for a string much simpler. I tried to keep the string and key as the only thing that used the phrase "peer feedback", at least for now.

bencodeorg · 2021-08-20T16:14:57Z

apps/src/templates/teacherDashboard/EditSectionForm.jsx

@@ -205,6 +181,11 @@ class EditSectionForm extends Component {
    const showLoginTypeField =
      !isNewSection && changeableLoginTypes.includes(section.loginType);

+    // These are server-side experiments, which are passed down to the client
+    const showCodeReviewEnabledCheckbox =


+1 if it's possible to use the course...are our CSA pilot scripts part of some family (not sure if this is the right word in cplat any more) that will be reused across each year's version of the course?

bencodeorg · 2021-08-20T16:26:46Z

apps/test/unit/templates/teacherDashboard/EditSectionFormTest.js

@@ -310,4 +312,57 @@ describe('EditSectionForm', () => {
    const loginTypeField = wrapper.find('LoginTypeField');
    assert.equal(loginTypeField.length, 0);
  });
+
+  it('renders CodeReviewField for csa-pilot teacher', () => {


Maybe a negative test that it doesn't show up if not in either pilot?

bencodeorg · 2021-08-20T16:36:50Z

dashboard/app/controllers/script_levels_controller.rb

@@ -114,6 +114,8 @@ def show
    raise ActiveRecord::RecordNotFound unless @script_level
    authorize! :read, @script_level, params.slice(:login_required)

+    @code_review_enabled = @script_level.level.is_a?(Javalab) && (current_user.teacher? || current_user&.sections_as_student&.all?(&:code_review_enabled?))


So if a student is in a section that doesn't have code review enabled they wouldn't be able to use code review? Seems ok since the migration set code review enabled to true for all sections, but if I'm reading this right we'll start setting it to false to newly created non-CSA sections?

Yes that's the idea. But we're going to default to having code review enabled for now (even for new sections). There's a broader conversation to be had about how to communicate to teachers about how to turn this feature on and off.

Cool, I like the decision to make that default consistent between migrated sections and new sections (both CSA and non-CSA).

bencodeorg · 2021-08-20T17:06:26Z

dashboard/test/controllers/api/v1/sections_controller_test.rb

@@ -391,6 +391,16 @@ class Api::V1::SectionsControllerTest < ActionController::TestCase
    assert_equal false, returned_section.lesson_extras
  end

+  test 'default code_review_enabled value is FALSE' do


Maybe I missed this (I think you mentioned during standup), but confused why we set code review enabled to true for every section in migration, yet want to set to false going forward? Feel like this could create weirdness down the line...

sanchitmalhotra126

Couple of small nits but LGTM!

sanchitmalhotra126 · 2021-08-20T18:24:10Z

apps/i18n/common/en_us.json

@@ -772,6 +772,8 @@
  "enableMakerDialogDescription": "Maker Toolkit is a feature used in the Computer Science Discoveries curriculum. See the setup page for more details:",
  "enableMakerDialogSetupPageLinkText": "Maker Toolkit Setup",
  "enablePairProgramming": "Allow students to Pair Program?",
+  "enablePeerFeedback": "Enable Peer Feedback?",


yeah I'd also defer to @moneppo but one thought here is that "Peer Feedback" seems to describe the ability for students to comment on each others' projects (which is what is being enabled/disabled here) vs "Code Review" describes the whole review tab, including teacher comments?

sanchitmalhotra126 · 2021-08-20T18:27:00Z

apps/src/templates/instructions/ReviewTab.jsx

            {viewAs !== ViewType.Teacher &&
              !errorLoadingReviewblePeers &&
+              codeReviewEnabled &&


nit: as this set of conditions is growing, could we pull this out into a local canViewPeerProject or something?

I pulled these checks out into the functions that already render them. Let me know if you think that duplicates too much code

sanchitmalhotra126 · 2021-08-20T18:32:30Z

apps/src/templates/instructions/ReviewTab.jsx

              (viewAsCodeReviewer
                ? this.renderBackToMyProject(this.onClickBackToProject)
                : this.renderPeerDropdown(reviewablePeers, this.onSelectPeer))}
-            {this.renderReadyForReviewCheckbox()}
+            {codeReviewEnabled && this.renderReadyForReviewCheckbox()}


nit: the logic for showing/hiding the checkbox all resides in renderReadyForReviewCheckbox so could we read the codeReviewEnabled flag there? FWIW this component could definitely use some logic cleanup down the road, so I imagine this will hopefully get simplified soon :)

bethanyaconnor · 2021-08-24T15:08:53Z

Thanks for all the feedback! To summarize:

In a follow-up, I'll look for a way to move away from using the pilot experiment to show this setting
I'll change everything, except the column name, to "peer feedback" (maybe? it doesn't sound like we have as much consensus here)

I hopefully have addressed all the other feedback, including switching the default to be code review enabled for a section. Please let me know if I missed anything!

bencodeorg

Nice work!

bencodeorg · 2021-08-24T17:07:38Z

apps/src/templates/instructions/ReviewTab.jsx

@@ -459,6 +480,7 @@ class ReviewTab extends Component {

 export const UnconnectedReviewTab = ReviewTab;
 export default connect(state => ({
+  codeReviewEnabled: state.sectionData.section.codeReviewEnabled,


Nit (we can fix this another time, I feel like we're going to need to get back in here to simplify some of our wild if statements): I feel like this should be named something that indicates that it relates to the section (eg, sectionCodeReviewEnabled) since we do a lot of additional logic checks to see whether to actually enable code review.

Do you mean just in the apps code or everywhere?

bencodeorg · 2021-08-24T17:23:58Z

dashboard/test/models/code_review_comment_test.rb

@@ -20,4 +20,62 @@ class CodeReviewCommentTest < ActiveSupport::TestCase
    code_review_comment = create :code_review_comment, commenter: teacher
    assert_equal true, code_review_comment.is_from_teacher?
  end
+
+  test 'can review own project' do


Thank you for adding these tests!

bencodeorg · 2021-08-24T17:36:41Z

apps/src/redux/sectionDataRedux.js

@@ -45,6 +46,7 @@ export const setSection = section => {
    id: section.id,
    script: section.script,
    students: sortedStudents,
+    codeReviewEnabled: section.code_review_enabled,


For my understanding, can you explain how the section's code_review_enabled status gets used (if at all)? It seems like we mostly rely on the @code_review_enabled variable set in script_levels_controller in deciding what to show, is that right?

The @code_review_enabled variable that gets sent down to the apps code is how we set this variable in redux (in show.js below). @code_review_enabled is decided based on the section(s) the student is in. Not sure if that answered your question?

sanchitmalhotra126

LGTM!

bethanyaconnor added 5 commits August 17, 2021 12:41

Update section dialog to include a field to enable code review for CS…

21a1a70

…A pilot sections

Add a function to disable code review for a section

84c5926

Fix viewing reviewable project when code reviews are disabled

36de32e

Fixes and tests to apps changes

615fa32

Add tests and fixups on server-side

e5071f4

bethanyaconnor commented Aug 17, 2021

View reviewed changes

bethanyaconnor added 2 commits August 17, 2021 14:58

Add a description for the peer feedback field

d21f9ea

Fix tests

4af6d7f

bethanyaconnor changed the title ~~Disable code review by section~~ Allow sections to disable code review Aug 19, 2021

bethanyaconnor added 2 commits August 19, 2021 08:32

Fix code_review_comments_controller_test

56bba80

Fix sections_controller_test.rb

e25f0d8

bethanyaconnor marked this pull request as ready for review August 19, 2021 18:33

bethanyaconnor requested a review from a team as a code owner August 19, 2021 18:33

bethanyaconnor requested a review from a team August 19, 2021 18:34

molly-moen approved these changes Aug 19, 2021

View reviewed changes

jmkulwik reviewed Aug 19, 2021

View reviewed changes

bencodeorg reviewed Aug 20, 2021

View reviewed changes

sanchitmalhotra126 approved these changes Aug 20, 2021

View reviewed changes

bethanyaconnor mentioned this pull request Aug 23, 2021

Set code_review_enabled default to true #42140

Merged

8 tasks

jmkulwik approved these changes Aug 23, 2021

View reviewed changes

bethanyaconnor added 2 commits August 23, 2021 14:42

Merge branch 'staging' into disable-code-review-by-section

b965329

Change default of code_review_enabled to true, address other PR feedback

1ef4246

bethanyaconnor requested review from bencodeorg and sanchitmalhotra126 August 24, 2021 15:09

bencodeorg approved these changes Aug 24, 2021

View reviewed changes

sanchitmalhotra126 approved these changes Aug 24, 2021

View reviewed changes

bethanyaconnor merged commit 9ad3bf1 into staging Aug 25, 2021

bethanyaconnor deleted the disable-code-review-by-section branch August 25, 2021 14:44

Allow sections to disable code review #42082

Allow sections to disable code review #42082

Conversation

bethanyaconnor commented Aug 17, 2021 • edited

Links

Testing story

Follow-up work

PR Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molly-moen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmkulwik Aug 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanchitmalhotra126 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bethanyaconnor commented Aug 24, 2021

bencodeorg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanchitmalhotra126 left a comment

Choose a reason for hiding this comment

bethanyaconnor commented Aug 17, 2021 •

edited

jmkulwik Aug 20, 2021 •

edited