New school info interstitial #20390

islemaster · 2018-02-02T21:19:36Z

Replace the existing, HAML-and-jQuery School Info Interstitial with a React one that reuses the new SchoolInfoInputs component that has school autocomplete and other flow we want to replicate.

Guidance on this feature:

Spec for the old School Info Interstitial
Teacher should be able to save/dismiss the interstitial in any partially-complete state - no required fields.
Any previously entered info should be loaded the next time the teacher is shown the interstitial.
We should pop up this interstitial again every 7 days unless one of the following is true:
- The teacher already filled out all the fields
- They chose “Homeschool,” “After School”, "Organization" or “Other” for school type
- Teacher chose a country that’s not “United States”
Metrics:
- Interstitial seen
- Clicked 'save' with metadata about whether enough info was filled out.
  - “enough info” is the same logic as when we decide to show the interstitial again vs. not
- Clicked 'save' without "enough info"
- Save failed on first attempt.
- Save failed on second attempt.
- On all of above, mark users who have a 'last seen date' of February 21st.
  - "So basically we cleared out the interstitial for a ton of people b/c we were storing their data incorrectly, so we moved their last seen date to Feb 21. Hence us trying to get this UI changed before then so that teachers didn’t see a dialog they finished filling out like a week after they filled it out and ideally the next time they did see it, it was a better UI."

TODO:

Track down "US" vs "United States" weirdness
~~Test code that manages when we re-display the interstitial~~ Doing in a separate PR.
UI test for end-to-end coverage.
Metrics
Possibly deduplicate with work Drew's been doing elsewhere.
Check that partial input is also allowed on the sign-up page.
Localize text

New unit tests:

    SchoolInfoInterstitial
      ✔ renders an uncloseable dialog with school info inputs and a save button
      ✔ passes empty school info if created with no existing school info
      ✔ passes existing school info if it is provided
      initial NCES ID
        ✔ is the provided ID if it is provided
        ✔ is blank if country is not US
        ✔ is blank if school type is not public/private/charter
        ✔ is blank if none of school name/state/zip have been entered
        ✔ is "-1" if country is US and schoolType is public and school_name was provided
        ✔ is "-1" if country is US and schoolType is public and state was provided
        ✔ is "-1" if country is US and schoolType is public and zip was provided
        ✔ is "-1" if country is US and schoolType is private and school_name was provided
        ✔ is "-1" if country is US and schoolType is private and state was provided
        ✔ is "-1" if country is US and schoolType is private and zip was provided
        ✔ is "-1" if country is US and schoolType is charter and school_name was provided
        ✔ is "-1" if country is US and schoolType is charter and state was provided
        ✔ is "-1" if country is US and schoolType is charter and zip was provided
      form submission
        ✔ submits with no info
        ✔ submits with only a country
        ✔ submits with a country and a school type
        ✔ submits with a country, school type, and school id
        ✔ submits with a country, school type, and school name
        ✔ submits with a country, school type, name, state
        ✔ submits with a country, school type, name, state, zip
        ✔ closes the dialog on successful submission
        ✔ shows an error message on first failed submission
        ✔ closes the dialog on a second failed submission

ewjordan · 2018-02-08T02:37:08Z

apps/src/lib/ui/SchoolInfoInterstitial.jsx

+      >
+        <div style={styles.container}>
+          <div style={styles.heading}>
+            We want to bring Computer Science to every student - help us track our progress!


Does this need to be localized? (Sim. other text)

Yes, thanks for the reminder. Added to the TODO list in the PR description.

drewsamnick · 2018-02-08T13:52:08Z

apps/src/lib/ui/SchoolInfoInterstitial.jsx

+    const initialNcesSchoolId = existingSchoolInfo.school_id ?
+      existingSchoolInfo.school_id :
+      (
+        existingSchoolInfo.country === 'United States'


I'm pretty sure that you also need to check existingSchoolInfo.country === 'US'
We seem to be inconsistent about how we specify the country in school_info. Sometimes it is set to a two-character country code and sometimes to the country name. The validation logic doesn't check this and the column is longer than two characters.

OK, it looks like you actually map 'US' to 'United States' in _school_info_interstitial.html.haml.
Perhaps add a comment here to call that out. Also, if we are doing that mapping in the haml then why not also do the mapping of school_id to "-1" there?

That's a good point.

I added this code because I discovered a funny edge case for partial entry: We never send/save a school id of "-1" to the database, so there's nothing to differentiate a form submitted where there's been no interaction with the school autocomplete dropdown (school_id is null) and one where the user has checked "I can't find my school" but hasn't entered any details yet (school_id is "-1") - in essence, we don't serialize the state of that checkbox.

As much as possible, I'd like all the business logic around this component to live in one layer, and the checkbox state in particular seems like a UI concern so I think it should live with the React component. So I'm leaning toward moving the US/United States logic into the React component as well. Does that sound okay to you?

Probably easier to test that stuff in the React layer than in a HAML file, too.

drewsamnick · 2018-02-08T13:55:55Z

apps/src/lib/ui/SchoolInfoInterstitial.jsx

+        // TODO (bbuchanan): Gather metrics on this.
+        this.props.onClose();
+      }
+    });


Is this unknown error because we don't expect any validation to run on the submission?

This is a a fallback in case of an unexpected error response on submission - likely caused by the service being temporarily unavailable, so we ask the teacher to try again at least once. Based on this handler in the Census Teacher Banner:

code-dot-org/apps/src/templates/census2017/CensusTeacherBanner.jsx

Lines 194 to 199 in e98992f

updateSchoolInfoError= () => {

// It isn't clear what could cause an error here since none of the fields are required.

this.setState({

showSchoolInfoUnknownError: true,

});

};

drewsamnick · 2018-02-08T14:00:15Z

apps/src/lib/ui/SchoolInfoInterstitial.jsx

+  };
+
+  static defaultProps = {
+    onClose: function () {},


Does it actually make sense to use this component without providing a meaningful onClose?

Only in a storybook/test context. I'll clear this out.

drewsamnick · 2018-02-08T14:01:38Z

apps/src/lib/ui/SchoolInfoInterstitial.jsx

+              schoolZip={this.state.schoolZip}
+              schoolLocation={this.state.schoolLocation}
+              useGoogleLocationSearch={false}
+              showErrors={false}


Does the schoolLocation value even get used if useGoogleLocationSearch is false?

drewsamnick · 2018-02-08T14:03:02Z

apps/src/lib/ui/SchoolInfoInterstitial.jsx

+        "user[school_info_attributes][school_state]": this.state.schoolState,
+        "user[school_info_attributes][school_zip]": this.state.schoolZip,
+        "user[school_info_attributes][full_address]": this.state.schoolLocation,
+      };


It doesn't look like this.state.schoolLocation ever gets updated from the original value. Since you aren't using the google location search this should probably get set to null here.

I actually don't know whether we want the google location search or not - I'll check with Poorva. What's the potential harm in keeping this wired up, to minimize the work to enable it later?

In the case that the user entered new info, you will be keeping the old full_address resulting in a mashup of the old and new data. If they are submitting the form then the old data ought to be cleared out.

Oh - the guidance I got was that this dialog should be like the school info on the signup page as much as possible, and that one uses Google Location search. So instead of removing this I'm going to enable that feature.

Keep in mind the note here about including the google maps script.

islemaster force-pushed the school-info-interstitial branch from 3085ba9 to 49d35bd Compare February 7, 2018 20:43

islemaster added 9 commits February 7, 2018 17:57

Create SchoolInfoInterstitial and story

274fab7

Render new interstitial (not submittable)

cded009

Form makes processable request

9678303

Working submission

9348a61

Allow incomplete submission and close form correctly

563182c

Load previously entered school info

b01389b

Pass scriptData as prop and test initial state

60ab81c

More direct mapping

76e7a86

Test form submission

c287911

islemaster force-pushed the school-info-interstitial branch from d4a24f3 to c287911 Compare February 8, 2018 01:57

islemaster changed the title ~~[WIP] New school info interstitial~~ New school info interstitial Feb 8, 2018

islemaster requested a review from drewsamnick February 8, 2018 02:06

ewjordan reviewed Feb 8, 2018

View reviewed changes

drewsamnick reviewed Feb 8, 2018

View reviewed changes

islemaster added 6 commits February 8, 2018 14:21

Enable Google Location Search

54b2a35

Handle submission for non-NCES types correctly

a0b428d

Handle non-US submissions

0573997

Move US => United States mapping into React component

ab50af8

Remove defaultProps

146c58c

Localize strings

3fe1001

islemaster merged commit a387d55 into staging Feb 9, 2018

This was referenced Feb 9, 2018

Revert "New school info interstitial" #20538

Merged

New school info interstitial #20539

Merged

islemaster deleted the school-info-interstitial branch February 9, 2018 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New school info interstitial #20390

New school info interstitial #20390

islemaster commented Feb 2, 2018 •

edited

ewjordan Feb 8, 2018 •

edited

islemaster Feb 8, 2018

drewsamnick Feb 8, 2018 •

edited

drewsamnick Feb 8, 2018

islemaster Feb 8, 2018

islemaster Feb 8, 2018

drewsamnick Feb 8, 2018

islemaster Feb 9, 2018 •

edited

drewsamnick Feb 8, 2018

islemaster Feb 8, 2018

drewsamnick Feb 8, 2018

drewsamnick Feb 8, 2018

islemaster Feb 8, 2018

drewsamnick Feb 8, 2018

islemaster Feb 8, 2018

drewsamnick Feb 8, 2018

	updateSchoolInfoError= () => {
	// It isn't clear what could cause an error here since none of the fields are required.
	this.setState({
	showSchoolInfoUnknownError: true,
	});
	};

New school info interstitial #20390

New school info interstitial #20390

Conversation

islemaster commented Feb 2, 2018 • edited

ewjordan Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewsamnick Feb 8, 2018 • 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

islemaster Feb 9, 2018 • 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

islemaster commented Feb 2, 2018 •

edited

ewjordan Feb 8, 2018 •

edited

drewsamnick Feb 8, 2018 •

edited

islemaster Feb 9, 2018 •

edited