Merge pull request #24684 from code-dot-org/overview-unhide-assigned-…

…unit

unhide script and warn when assigning it from the script overview page

apps/i18n/common/en_us.json

@@ -70,6 +70,7 @@
   "assignACourse": "Assign a course to your classroom or start your own course.",
   "assignCourse": "Assign Course",
   "assignConfirm": "Are you sure you want to assign \"{assignmentName}\" to \"{sectionName}\"?",
+  "assignHiddenUnitConfirm": "It looks like you previously hid \"{assignmentName}\" from section \"{sectionName}\". Do you still want to assign this unit and make it visible?",
   "assignedTo": "Assigned to",
   "assignUnit": "Assign Unit",
   "assignmentSelectorCourse": "Select course",
@@ -1285,6 +1286,8 @@
   "unattachedBlockTipBody": "Blocks that are not attached will not do anything. If you want these blocks in your program, try connecting them to other blocks.",
   "unexpectedError": "An unexpected error occurred, please try again. If this keeps happening, try reloading the page.",
   "unfeatured": "Unfeatured",
+  "unhideAndAssignHeader": "This unit is currently hidden from the section",
+  "unhideUnitAndAssign": "Unhide unit and assign",
   "unnamedFunction": "You have a variable or function that does not have a name. Don't forget to give everything a descriptive name.",
   "unplugged": "Unplugged",
   "unpluggedActivity": "Unplugged Activity",

apps/src/code-studio/hiddenStageRedux.js

@@ -80,7 +80,7 @@ export default function reducer(state = new HiddenState(), action) {
 
   if (action.type === UPDATE_HIDDEN_SCRIPT) {
     const { sectionId, scriptId, hidden } = action;
-    const nextState = state.setIn(['scriptsBySection', sectionId, scriptId.toString()], hidden);
+    const nextState = state.setIn(['scriptsBySection', sectionId.toString(), scriptId.toString()], hidden);
     validateSectionIds(nextState);
     return nextState;
   }
@@ -262,5 +262,5 @@ function isHiddenForSection(state, sectionId, itemId, bySectionKey) {
     sectionId = STUDENT_SECTION_ID;
   }
   const bySection = state.get(bySectionKey);
-  return !!bySection.getIn([sectionId, itemId.toString()]);
+  return !!bySection.getIn([sectionId.toString(), itemId.toString()]);
 }

apps/src/code-studio/progress.js

@@ -13,7 +13,7 @@ import DisabledBubblesAlert from './DisabledBubblesAlert';
 import { getStore } from './redux';
 import { authorizeLockable } from './stageLockRedux';
 import { setViewType, ViewType } from './viewAsRedux';
-import { getHiddenStages } from './hiddenStageRedux';
+import { getHiddenStages, initializeHiddenScripts } from './hiddenStageRedux';
 import { TestResults } from '@cdo/apps/constants';
 import {
   initProgress,
@@ -135,6 +135,8 @@ progress.renderCourseProgress = function (scriptData) {
   const teacherResources = (scriptData.teacher_resources || []).map(
     ([type, link]) => ({type, link}));
 
+  store.dispatch(initializeHiddenScripts(scriptData.section_hidden_unit_info));
+
   const mountPoint = document.createElement('div');
   $('.user-stats-block').prepend(mountPoint);
   ReactDOM.render(

apps/src/templates/courseOverview/AssignToSection.js

@@ -1,12 +1,14 @@
 import React, { Component, PropTypes } from 'react';
 import Radium from 'radium';
+import { connect } from 'react-redux';
 import $ from 'jquery';
 import queryString from 'query-string';
 import i18n from '@cdo/locale';
 import Button from '@cdo/apps/templates/Button';
 import DropdownButton from '@cdo/apps/templates/DropdownButton';
 import BaseDialog from '@cdo/apps/templates/BaseDialog';
 import ConfirmAssignment from './ConfirmAssignment';
+import { isScriptHiddenForSection, updateHiddenScript } from '@cdo/apps/code-studio/hiddenStageRedux';
 
 const styles = {
   main: {
@@ -28,6 +30,8 @@ class AssignToSection extends Component {
       id: PropTypes.number.isRequired,
       name: PropTypes.string.isRequired,
     })).isRequired,
+    hiddenStageState: PropTypes.object,
+    updateHiddenScript: PropTypes.func.isRequired,
   };
 
   state = {
@@ -47,7 +51,8 @@ class AssignToSection extends Component {
   };
 
   updateCourse = () => {
-    const section = this.props.sectionsInfo[this.state.sectionIndexToAssign];
+    const { sectionsInfo, updateHiddenScript, scriptId } = this.props;
+    const section = sectionsInfo[this.state.sectionIndexToAssign];
     $.ajax({
       url: `/dashboardapi/sections/${section.id}`,
       method: 'PATCH',
@@ -57,6 +62,7 @@ class AssignToSection extends Component {
         script_id: this.props.scriptId,
       }),
     }).done(result => {
+      updateHiddenScript(section.id, scriptId, false);
       this.setState({
         sectionIndexToAssign: null
       });
@@ -74,10 +80,12 @@ class AssignToSection extends Component {
   };
 
   render() {
-    const { courseId, scriptId, assignmentName, sectionsInfo } = this.props;
+    const { courseId, scriptId, assignmentName, sectionsInfo, hiddenStageState } = this.props;
     const { sectionIndexToAssign, errorString } = this.state;
     const section = sectionsInfo[sectionIndexToAssign];
     const queryParams = queryString.stringify({courseId, scriptId});
+    const isHiddenFromSection = section && hiddenStageState &&
+      isScriptHiddenForSection(hiddenStageState, section.id, scriptId);
 
     return (
       <div style={styles.main}>
@@ -108,6 +116,7 @@ class AssignToSection extends Component {
             assignmentName={assignmentName}
             onClose={this.onCloseDialog}
             onConfirm={this.updateCourse}
+            isHiddenFromSection={isHiddenFromSection}
           />
         )}
         {errorString && (
@@ -123,4 +132,10 @@ class AssignToSection extends Component {
   }
 }
 
-export default Radium(AssignToSection);
+export const UnconnectedAssignToSection = Radium(AssignToSection);
+
+export default connect(state => ({
+  hiddenStageState: state.hiddenStage,
+}), ({
+  updateHiddenScript,
+}))(UnconnectedAssignToSection);

apps/src/templates/courseOverview/AssignToSection.story.jsx

@@ -1,5 +1,5 @@
 import React from 'react';
-import AssignToSection from './AssignToSection';
+import {UnconnectedAssignToSection as AssignToSection} from './AssignToSection';
 
 const defaultProps = {
   courseId: 30,
@@ -21,7 +21,8 @@ const defaultProps = {
       id: 338,
       name: "section_with_course"
     }
-  ]
+  ],
+  updateHiddenScript: () => {},
 };
 
 export default storybook => {

apps/src/templates/courseOverview/ConfirmAssignment.js

@@ -34,18 +34,24 @@ export default class ConfirmAssignment extends Component {
     assignmentName: PropTypes.string.isRequired,
     onClose: PropTypes.func.isRequired,
     onConfirm: PropTypes.func.isRequired,
+    isHiddenFromSection: PropTypes.bool,
   };
 
   render() {
-    const { sectionName, assignmentName, onClose, onConfirm } = this.props;
+    const { sectionName, assignmentName, onClose, onConfirm, isHiddenFromSection } = this.props;
+    const header = isHiddenFromSection ? i18n.unhideAndAssignHeader() : i18n.assignCourse();
+    const content = isHiddenFromSection ?
+      i18n.assignHiddenUnitConfirm({assignmentName, sectionName}) :
+      i18n.assignConfirm({assignmentName, sectionName});
+    const buttonText = isHiddenFromSection ? i18n.unhideUnitAndAssign() : i18n.assign();
 
     return (
       <BaseDialog isOpen={true} handleClose={onClose}>
         <div style={styles.header}>
-          {i18n.assignCourse()}
+          {header}
         </div>
         <div style={styles.content}>
-          {i18n.assignConfirm({assignmentName, sectionName})}
+          {content}
         </div>
         <div style={{textAlign: 'right'}}>
           <Button
@@ -54,7 +60,7 @@ export default class ConfirmAssignment extends Component {
             color={Button.ButtonColor.gray}
           />
           <Button
-            text={i18n.assign()}
+            text={buttonText}
             style={{marginLeft: 5}}
             onClick={onConfirm}
             color={Button.ButtonColor.orange}

apps/test/unit/templates/courseOverview/AssignToSectionTest.js

@@ -1,7 +1,8 @@
 import { assert } from '../../../util/configuredChai';
 import React from 'react';
 import { shallow, mount } from 'enzyme';
-import AssignToSection from '@cdo/apps/templates/courseOverview/AssignToSection';
+import {UnconnectedAssignToSection as AssignToSection} from '@cdo/apps/templates/courseOverview/AssignToSection';
+import Immutable from 'immutable';
 
 const defaultProps = {
   courseId: 30,
@@ -23,7 +24,8 @@ const defaultProps = {
       id: 338,
       name: "section_with_course"
     }
-  ]
+  ],
+  updateHiddenScript: () => {},
 };
 
 describe('AssignToSection', () => {
@@ -81,6 +83,39 @@ describe('AssignToSection', () => {
     const confirm = wrapper.find('ConfirmAssignment');
     assert.equal(confirm.props().assignmentName, 'Computer Science Principles');
     assert.equal(confirm.props().sectionName, 'brent_section');
+    assert.equal(confirm.find('Button').at(1).text(), 'Assign');
+  });
+
+  it('shows a warning when clicking a hidden section', () => {
+    const scriptId = 99;
+    const sectionId = defaultProps.sectionsInfo[0].id;
+    const hiddenStageState = Immutable.fromJS({
+      scriptsBySection: { [sectionId]: {[scriptId]: true} }
+    });
+
+    const props = {
+      ...defaultProps,
+      courseId: undefined,
+      scriptId,
+      hiddenStageState,
+    };
+    const wrapper = mount(
+      <AssignToSection {...props}/>
+    );
+    wrapper.find('Button').simulate('click');
+    const firstSection = wrapper.find('a').at(1);
+    assert.equal(firstSection.props()['data-section-index'], 0);
+    // Enzyme simulate doesn't set target for us automatically, so we fake one.
+    const target = {
+      getAttribute: () => 0
+    };
+    firstSection.simulate('click', {target});
+
+    assert.strictEqual(wrapper.state().sectionIndexToAssign, 0);
+    const confirm = wrapper.find('ConfirmAssignment');
+    assert.equal(confirm.props().assignmentName, 'Computer Science Principles');
+    assert.equal(confirm.props().sectionName, 'brent_section');
+    assert.equal(confirm.find('Button').at(1).text(), 'Unhide unit and assign');
   });
 
   it('shows an error dialog when we have an error', () => {

apps/test/unit/templates/courseOverview/CourseOverviewTopRowTest.js

@@ -27,7 +27,7 @@ describe('CourseOverviewTopRow', () => {
         {...defaultProps}
       />
     );
-    assert.equal(wrapper.find('AssignToSection').length, 1);
+    assert.equal(wrapper.find('Connect(AssignToSection)').length, 1);
   });
 
   it('has a button for each resource', () => {

dashboard/app/controllers/api/v1/sections_controller.rb

@@ -77,10 +77,13 @@ def update
 
     if script_id
       script = Script.get_from_cache(script_id)
+      return head :bad_request if script.nil?
       # If given a course and script, make sure the script is in that course
       return head :bad_request if course_id && course_id != script.course.try(:id)
       # If script has a course and no course_id was provided, use default course
       course_id ||= script.course.try(:id)
+      # Unhide script for this section before assigning
+      section.toggle_hidden_script script, false
     end
 
     # TODO: (madelynkasula) refactor to use strong params

dashboard/app/models/script.rb

@@ -1100,7 +1100,8 @@ def summarize(include_stages = true, user = nil)
       show_course_unit_version_warning: !course&.has_dismissed_version_warning?(user) && has_older_course_progress,
       show_script_version_warning: !user_script&.version_warning_dismissed && !has_older_course_progress && has_older_script_progress,
       versions: summarize_versions,
-      supported_locales: supported_locales
+      supported_locales: supported_locales,
+      section_hidden_unit_info: section_hidden_unit_info(user),
     }
 
     summary[:stages] = stages.map(&:summarize) if include_stages
@@ -1111,6 +1112,18 @@ def summarize(include_stages = true, user = nil)
     summary
   end
 
+  # @return {Hash<string,number[]>}
+  #   For teachers, this will be a hash mapping from section id to a list of hidden
+  #   script ids for that section, filtered so that the only script id which appears
+  #   is the current script id. This mirrors the output format of
+  #   User#get_hidden_script_ids, and satisfies the input format of
+  #   initializeHiddenScripts in hiddenStageRedux.js.
+  def section_hidden_unit_info(user)
+    return {} unless user&.teacher?
+    hidden_section_ids = SectionHiddenScript.where(script_id: id, section: user.sections).pluck(:section_id)
+    hidden_section_ids.map {|section_id| [section_id, [id]]}.to_h
+  end
+
   # Similar to summarize, but returns an even more narrow set of fields, restricted
   # to those needed in header.html.haml
   def summarize_header

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

@@ -635,6 +635,17 @@ class Api::V1::SectionsControllerTest < ActionController::TestCase
     assert_nil section.script_id
   end
 
+  test "update: hidden script is unhidden when assigned" do
+    sign_in @teacher
+    @section.toggle_hidden_script @csp_script, true
+    refute_nil SectionHiddenScript.find_by(script: @csp_script, section: @section)
+    post :update, params: {
+      id: @section.id,
+      script_id: @csp_script.id,
+    }
+    assert_nil SectionHiddenScript.find_by(script: @csp_script, section: @section)
+  end
+
   test "update: cannot update section you dont own" do
     other_teacher = create(:teacher)
     sign_in other_teacher

dashboard/test/models/script_test.rb

@@ -1268,6 +1268,38 @@ def populate_cache_and_disconnect_db
     assert_equal ['English', 'fr-fr'], script.supported_locale_names
   end
 
+  test 'section_hidden_unit_info' do
+    teacher = create :teacher
+    section1 = create :section, user: teacher
+    assert_equal({}, @script_in_course.section_hidden_unit_info(teacher))
+
+    create :section_hidden_script, section: section1, script: @script_in_course
+    assert_equal({section1.id => [@script_in_course.id]}, @script_in_course.section_hidden_unit_info(teacher))
+
+    # other script has no effect
+    other_script = create :script
+    create :section_hidden_script, section: section1, script: other_script
+    assert_equal({section1.id => [@script_in_course.id]}, @script_in_course.section_hidden_unit_info(teacher))
+
+    # other teacher's sections have no effect
+    other_teacher = create :teacher
+    other_teacher_section = create :section, user: other_teacher
+    create :section_hidden_script, section: other_teacher_section, script: @script_in_course
+    assert_equal({section1.id => [@script_in_course.id]}, @script_in_course.section_hidden_unit_info(teacher))
+
+    # other section for same teacher hidden for same script appears in list
+    section2 = create :section, user: teacher
+    assert_equal({section1.id => [@script_in_course.id]}, @script_in_course.section_hidden_unit_info(teacher))
+    create :section_hidden_script, section: section2, script: @script_in_course
+    assert_equal(
+      {
+        section1.id => [@script_in_course.id],
+        section2.id => [@script_in_course.id]
+      },
+      @script_in_course.section_hidden_unit_info(teacher)
+    )
+  end
+
   private
 
   def has_hidden_script?(scripts)