Diversity survey 2017 #13884

breville · 2017-03-20T18:56:20Z

The diversity survey returns for 2017.

It can now be submitted without any answers.

tanyaparker

Touches the next section:

Prefer not to answer just scrolled me up instead of hiding.

I submitted and saw the button to google survey. On refresh I still see the diversity survey.

When I clicked through google survey it didn't have my ID?

tanyaparker · 2017-03-21T01:54:47Z

dashboard/app/views/home/_survey_diversity.haml

    }
    else
    {
      $("#survey")[0].reset();
-      $(".survey").hide();
+      $("#diversity-survey").hide();


Should this be #diversity_survey?

tanyaparker · 2017-03-21T01:56:38Z

dashboard/app/views/home/_survey_diversity.haml

      $(this).val($(this).val().replace(/[^0-9]/g, ''));
    });

    if (submittedValue)
    {
      $("#surveybody").hide();
-      $("#surveythanks").show();
+      $("#surveythanks").fadeIn();
    }
    else
    {
      $("#survey")[0].reset();


Does this reset both #surveythanks and #surveybody? Like where is the just #survey element?

This just resets the survey form. The form_for line specifies an id of survey for the form.

tanyaparker · 2017-03-21T01:58:07Z

dashboard/app/helpers/survey_results_helper.rb

+    false unless current_user
+    false unless language == "en"
+    false if current_user.under_13?
+    false if existing_diversity_survey_result?


The wording of this confused me at first because I thought it meant that if there was an existing one, even from 2016 that we wouldn't allow. Suggest something more like already_submitted_diversity_2017 ?

tanyaparker · 2017-03-21T02:06:35Z

dashboard/app/views/home/_survey_nps.html.haml

@@ -10,7 +10,7 @@
    cursor: auto;
  }

- if show_survey?
+- if show_nps_survey?


Do we need this check both here and in the home/index view? Can't it just be here?

ashercodeorg

Taking a step backward, I think there are some fundamental issues with this PR.

Given that SurveyResult.kind encapsulates when the survey was administered, I feel like including the year in the DIVERSITY_ATTRS is a mistake. Any reason we shouldn't not include the year, doing an appropriate data fix to remove the year from the 2016 diversity survey data? If so, I'd be happy to own the 2016 data fix and associated fallout. Without this, there is no programmatic way to access the 2016 constants.
Related, this seems like a lot more work than it should be. Assuming no changes to styling, wording, and such, I would expect this PR to create a new SurveyResult.kind, change the diversity survey to submit this new kind, and remove an early exit return false from the should-show-survey-method. Is there any reason we shouldn't make the appropriate changes so that disabling and reenabling is this simple moving forward?

ashercodeorg · 2017-03-21T12:36:12Z

dashboard/app/helpers/survey_results_helper.rb

+    false if current_user.under_13?
+    false if existing_diversity_survey_result?
+    false unless account_existed_14_days?
+    false unless teacher?


Personally, I find current_user.teacher? more readable, as it isn't obvious how or what teacher? is defined by in the helper.

In fact, I think it would be more readable (simpler) if all the one liners were not abstracted away into methods below (as all of them are super clear and readable).

ashercodeorg · 2017-03-21T12:37:41Z

dashboard/app/helpers/survey_results_helper.rb

-      return true
-    end
+  def existing_diversity_survey_result?
+    SurveyResult.where({user_id: current_user.id, kind: SurveyResult::DIVERSITY_2017}).exists?


nit: Though I'm totally fine with the braces, typically we omit hash braces on the last parameter, e.g., where(user_id: current_user.id, kind: SurveyResult::DIVERSITY_2017).

ashercodeorg · 2017-03-21T12:43:04Z

dashboard/test/helpers/survey_results_helper_test.rb

+    stubs(:current_user).returns(@teacher)
+
+    assert teacher?
+    assert_equal account_existed_14_days?, false


For assert_equal, the syntax is assert_equal expected, actual so that an appropriate error message is shown on failure. That said, here and below, I'd encourage using refute account_existed_14_days?.

ashercodeorg · 2017-03-21T12:45:00Z

dashboard/test/helpers/survey_results_helper_test.rb

@@ -0,0 +1,63 @@
+require 'test_helper'
+
+class SurveyResultsHelperTest < ActionView::TestCase


It would be great if the common setup was moved to the setup method. E.g.,

def setup @teacher = create(:teacher, created_at: 3.weeks.ago) sign_in @teacher end

Related, is there a reason to stub current_user rather than sign-in the teacher?

ashercodeorg · 2017-03-21T12:59:29Z

dashboard/test/helpers/survey_results_helper_test.rb

+    create(:follower, section: @section, student_user: @student)
+    stubs(:current_user).returns(@teacher)
+
+    assert has_any_students?


This could be shortened to (assuming the common setup)

follower = create :follower, user: @teacher assert has_any_students?

ashercodeorg · 2017-03-21T13:05:33Z

dashboard/test/helpers/survey_results_helper_test.rb

+    stubs(:current_user).returns(@teacher)
+
+    assert has_any_students?
+    assert_equal has_any_student_under_13?, false


Shorter (with common setup):

follower = create :follower, user: @teacher follower.student_user.update!(age: 10) assert has_any_students? refute has_any_student_under_13?

ashercodeorg · 2017-03-21T13:13:13Z

dashboard/app/views/home/_survey_diversity.haml

-      %a{href: survey_url, target: "_blank"}
-        %button.btn-primary
-          Go to survey
+- survey_url = "https://docs.google.com/a/code.org/forms/d/e/1FAIpQLSd7MppqXJi4FpURELDH3RyMG6PgevBVPaH373K0h-FSI_3FUg/viewform"


It seems odd (bad UX) that there is exactly one required question in this survey.

Wait, both of them should be optional.

This refers to the Google Forms survey?

breville · 2017-03-22T00:45:53Z

Thanks @ashercodeorg. On your first bullet, I had a similar concern and am glad you feel this way. I've removed the years from the attributes. Thanks for owning the data cleanup. On the second bullet, the intention going forward is certainly that we can easily adjust the criteria to show whichever survey comes next, and that it's easier to add more types in the future.

Conflicts: dashboard/app/models/survey_result.rb

ashercodeorg

Almost! Should I proceed with the data fix to remove the year from the diversity survey JSONs?

ashercodeorg · 2017-03-22T12:54:32Z

dashboard/app/helpers/survey_results_helper.rb

@@ -1,38 +1,45 @@
 module SurveyResultsHelper
-  def show_survey?
-    # rubocop:disable Lint/UnreachableCode
+  def show_diversity_survey?


Does this become more reusable (for future years) if this accepts a kind parameter, that in turn gets passed through via existing_diversity_survey(kind)?

Ideally, I'd like to see this helper not need any modification for future years (except perhaps adding or removing a return false at the top of this method).

Sure. Between this change and the earlier work in #12583 the goal has certainly been to make it trivial to turn these surveys on and off.

ashercodeorg · 2017-03-22T13:05:43Z

dashboard/test/helpers/survey_results_helper_test.rb

+  end
+
+  test 'teacher account newer than 14 days' do
+    @teacher.update(created_at: 1.week.ago)


nit: Use @teacher.update! to facilitate debugging if per chance the update fails.

ashercodeorg · 2017-03-22T13:07:49Z

dashboard/test/helpers/survey_results_helper_test.rb

+  end
+
+  test 'teacher account has no student under 13' do
+    create :follower, user: @teacher


Please don't rely implicitly on FactoryGirl giving you a follower with a young_student. Prefer something like create :follower, user: @teacher, student_user: (create :young_student).

ashercodeorg · 2017-03-22T13:08:39Z

dashboard/test/helpers/survey_results_helper_test.rb

+  end
+
+  test 'teacher account has student under 13' do
+    follower = create :follower, user: @teacher


Again, though it seems you may need to add old_student to FactoryGirl.

breville · 2017-03-22T17:12:56Z

@ashercodeorg Please go ahead with the data fix, though presumably it should be merged after this change goes to production.

ashercodeorg · 2017-03-22T17:17:04Z

LGTM (for ruby). And I'll send you a data fix PR in the upcoming future.

tanyaparker

Not blocking merging in this PR, but may be good to add an "x" to dismiss the second diversity section.

breville added 5 commits February 8, 2017 15:19

Diversity survey: initial work for 2017 edition

93a8397

Merge branch 'staging' into diversity-survey-2017

c433ba4

Diversity survey 2017: more work to match spec

9b5d806

Diversity survey 2017: extract styling from haml

1373fe5

Diversity survey 2017: unit test helper methods

e0d39a9

breville requested review from tanyaparker and ashercodeorg March 21, 2017 00:01

breville assigned Erin007 Mar 21, 2017

tanyaparker suggested changes Mar 21, 2017

View reviewed changes

ashercodeorg suggested changes Mar 21, 2017

View reviewed changes

breville added 2 commits March 21, 2017 17:17

Diversity survey 2017: code review feedback

3a45c1f

Diversity survey 2017: two small fixes

fc8a456

Merge branch 'staging' into diversity-survey-2017

f2d164f

Conflicts: dashboard/app/models/survey_result.rb

ashercodeorg reviewed Mar 22, 2017

View reviewed changes

Diversity survey 2017: more code review feedback

d5d10be

ashercodeorg approved these changes Mar 22, 2017

View reviewed changes

ashercodeorg mentioned this pull request Mar 22, 2017

Create script to remove survey2016_ prefix. #13939

Merged

tanyaparker approved these changes Mar 22, 2017

View reviewed changes

Diversity survey 2017: fix test

2098f2e

breville merged commit d0d050f into staging Mar 23, 2017

breville deleted the diversity-survey-2017 branch March 23, 2017 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diversity survey 2017 #13884

Diversity survey 2017 #13884

breville commented Mar 20, 2017 •

edited

tanyaparker left a comment

tanyaparker Mar 21, 2017

tanyaparker Mar 21, 2017

breville Mar 22, 2017

tanyaparker Mar 21, 2017

tanyaparker Mar 21, 2017

ashercodeorg left a comment

ashercodeorg Mar 21, 2017

ashercodeorg Mar 21, 2017

ashercodeorg Mar 21, 2017

ashercodeorg Mar 21, 2017 •

edited

ashercodeorg Mar 21, 2017

ashercodeorg Mar 21, 2017

ashercodeorg Mar 21, 2017

ashercodeorg Mar 21, 2017 •

edited

ashercodeorg Mar 21, 2017

tanyaparker Mar 21, 2017

breville Mar 22, 2017

breville commented Mar 22, 2017

ashercodeorg left a comment

ashercodeorg Mar 22, 2017

breville Mar 22, 2017

ashercodeorg Mar 22, 2017

ashercodeorg Mar 22, 2017

ashercodeorg Mar 22, 2017

breville commented Mar 22, 2017

ashercodeorg commented Mar 22, 2017

tanyaparker left a comment

		@@ -0,0 +1,63 @@
		require 'test_helper'

		class SurveyResultsHelperTest < ActionView::TestCase

Diversity survey 2017 #13884

Diversity survey 2017 #13884

Conversation

breville commented Mar 20, 2017 • edited

tanyaparker 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

ashercodeorg 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

ashercodeorg Mar 21, 2017 • 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

ashercodeorg Mar 21, 2017 • 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

breville commented Mar 22, 2017

ashercodeorg 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

breville commented Mar 22, 2017

ashercodeorg commented Mar 22, 2017

tanyaparker left a comment

Choose a reason for hiding this comment

breville commented Mar 20, 2017 •

edited

ashercodeorg Mar 21, 2017 •

edited

ashercodeorg Mar 21, 2017 •

edited