PD: international opt-in form #23235

breville · 2018-06-21T10:59:55Z

This implements a form for teachers to record their attendance at a non-U.S. workshop, at https://studio.code.org/pd/international_workshop.

Existing logged-out page (now loc'ed)

Existing not-teacher page (now loc'ed)

New teacher-with-no-email page (loc'ed)

Thanks (loc'ed)

breville · 2018-06-26T07:03:54Z

dashboard/test/controllers/api/v1/pd/international_optins_controller_test.rb

+  }
+
+  test 'create creates a new international optin' do
+    assert_creates Pd::InternationalOptin do


Not sure why this fails...

Perhaps the API call is failing for some reason? I'd suggest moving the assert_response inside the assert_creates so we first assert the successful response and then assert that it created a new model instance.

Also does assert_response :created (201) work? I think we return 200 :success. In any case, I suspect we're failing to create and returning a 400 or other error code. See https://www.rubydoc.info/gems/rack/Rack/Utils for the list of codes.

Thanks for the suggestion, it helped narrow down the problems (which were a failure to name the class properly and a failure to underscore the parameters).

breville · 2018-06-26T07:06:09Z

dashboard/app/models/pd/international_optin.rb

+class Pd::InternationalOptin < ApplicationRecord
+  include Pd::Form
+
+  belongs_to :user


Not seeing an associated user_id in the database after submitting this form, even when signed in. Is there something else needed to make the association work?

The association is fine, but its value is not being set. It should be set on initialization, here.

Is user_id required? If so we should also probably update the db column to not null, add a required validation, and controller auth.

breville · 2018-06-26T07:13:15Z

apps/src/code-studio/pd/international_optin/InternationalOptin.jsx

+          </Row>
+        </FormGroup>
+        {
+          this.buildSelectFieldGroupFromOptions({


Even though these dropdowns are shown with a default selection, pressing Submit causes them to be highlighted as required, indicating that there's no known selection at this point. Is there a preferred solution?

There is no default value in React component props, and it will render as empty string initially. I think at that point, the browser shows the first element selected but it doesn't trigger a React event so doesn't set the value. Either initialize formData in the controller to have these initial values, or add a placeholder here

Conflicts: dashboard/db/schema.rb

aoby · 2018-06-26T20:32:51Z

apps/Gruntfile.js

@@ -513,6 +513,8 @@ describe('entry tests', () => {
    'pd/professional_learning_landing/index': './src/sites/studio/pages/pd/professional_learning_landing/index.js',
    'pd/regional_partner_contact/new': './src/sites/studio/pages/pd/regional_partner_contact/new.js',

+    'pd/international_optin/new': './src/sites/studio/pages/pd/international_optin/new.js',


nit: s/optin/opt_in

And elsewhere. optin isn't a word, nor a common abbreviation AFAIK, so I'd go with opt_in and OptIn over optin and Optin

aoby · 2018-06-26T20:34:24Z

apps/src/code-studio/pd/international_optin/InternationalOptin.jsx

+    //Object.assign(formData['form_data'], this.getDistrictData());
+
+    return formData;
+  }


What is the point of this override if it just calls super and returns the result?

aoby · 2018-06-26T20:40:38Z

apps/src/code-studio/pd/international_optin/InternationalOptin.jsx

+          </Row>
+        </FormGroup>
+        {
+          this.buildSelectFieldGroupFromOptions({


There is no default value in React component props, and it will render as empty string initially. I think at that point, the browser shows the first element selected but it doesn't trigger a React event so doesn't set the value. Either initialize formData in the controller to have these initial values, or add a placeholder here

aoby · 2018-06-26T20:44:27Z

dashboard/app/models/pd/international_optin.rb

+class Pd::InternationalOptin < ApplicationRecord
+  include Pd::Form
+
+  belongs_to :user


The association is fine, but its value is not being set. It should be set on initialization, here.

aoby · 2018-06-26T20:45:33Z

dashboard/app/models/pd/international_optin.rb

+class Pd::InternationalOptin < ApplicationRecord
+  include Pd::Form
+
+  belongs_to :user


Is user_id required? If so we should also probably update the db column to not null, add a required validation, and controller auth.

aoby · 2018-06-26T20:48:27Z

dashboard/db/migrate/20180620110434_create_pd_international_optins.rb

+  def change
+    create_table :pd_international_optins do |t|
+      t.references :user, index: true
+      t.text :form_data


Is user required?
Presumably form_data is required, and should be not null.

Also please move the migration out into its own PR (per convention, in case we have to roll back).

aoby · 2018-06-26T20:53:44Z

dashboard/test/controllers/api/v1/pd/international_optins_controller_test.rb

+  }
+
+  test 'create creates a new international optin' do
+    assert_creates Pd::InternationalOptin do


Perhaps the API call is failing for some reason? I'd suggest moving the assert_response inside the assert_creates so we first assert the successful response and then assert that it created a new model instance.

Also does assert_response :created (201) work? I think we return 200 :success. In any case, I suspect we're failing to create and returning a 400 or other error code. See https://www.rubydoc.info/gems/rack/Rack/Utils for the list of codes.

aoby · 2018-06-26T20:54:40Z

dashboard/test/controllers/api/v1/pd/international_optins_controller_test.rb

+      }
+    end
+
+    assert_response :bad_request


Same as above, move this inside the assert_creates.

I think it's a clearer test (and easier to decipher results) to first check the return code, then check the side effects.

aoby · 2018-06-26T20:56:38Z

dashboard/test/models/pd/international_optin_test.rb

+    optin = build :pd_international_optin, form_data: {}.to_json
+    refute optin.valid?
+
+    assert build(:pd_international_optin, form_data: FORM_DATA.to_json).valid?


nit: why 2 lines for refute and one line for assert?

The optin variable from line 25 doesn't appear to be used anywhere else (after line 26, which can be combined).

Good catch; will simplify.

No particular reason, was just basing it off of these tests.

aoby · 2018-06-26T21:00:08Z

lib/international_optin_people.rb

@@ -0,0 +1,19 @@
+INTERNATIONAL_OPTIN_FACILITATORS = [


This should be contained in a named module, which can then be included where needed.

Then, since dashboard/lib is autoloaded we won't need to explicitly require this file, and these constants and methods will be contained in the module and applied to the imported class. Better containment, better style :)

mehalshah · 2018-06-26T22:04:21Z

apps/src/code-studio/pd/international_optin/InternationalOptin.jsx

+  FormGroup
+} from 'react-bootstrap';
+
+export default class InternationalOptin extends FormController {


Suggest renaming to InternationalOptIn

mehalshah · 2018-06-26T22:04:46Z

apps/src/code-studio/pd/international_optin/InternationalOptin.jsx

+
+class InternationalOptinComponent extends FormComponent {
+  static propTypes = {
+    accountEmail: PropTypes.string.isRequired


Are we going to pre-fill the name as well?

Good idea. Is teacher name always available as separate first & last name fields on User? Is that what ops_first_name and ops_last_name are?

No, User.name is whatever the user provides for Display Name in account settings.

ops_first_name and ops_last_name were manually supplied by our internal "ops" team via the old / deprecated ops workshop dashboard. These are not reliable and not up to date.

Good to know. We won't pre-fill the name then, since we're not sure how to parse the Display Name afaik.

mehalshah · 2018-06-26T22:19:15Z

apps/src/code-studio/pd/international_optin/InternationalOptin.jsx

+            name: 'email',
+            label: labels.email,
+            type: 'text',
+            value: this.props.accountEmail,


Thinking about this a bit - there are some users that would fill out this form that do not have an email (they mistakenly created a student account way back when). Maybe make this readOnly only if we have an email?

Good point. We will actually route them to a separate page directing them to edit their profile and add an email, similar to what we do for non-teachers and signed-out users. So anybody who actually gets to this form will have an email.

mehalshah · 2018-06-26T22:19:36Z

dashboard/app/controllers/api/v1/pd/international_optins_controller.rb

@@ -0,0 +1,17 @@
+class Api::V1::Pd::InternationalOptinsController < Api::V1::Pd::FormsController
+  def new_form
+    @contact_form = ::Pd::InternationalOptin.new


Do we let anyone use this form, or only teachers?

Only teachers with email addresses.

tanyaparker · 2018-06-28T22:45:53Z

cc @bencodeorg

Checking "Other:" for one of the three choices that supports it now satisfies the requirement (for two of them) that an option is checked. This is written to the server something like this: {"form_data":{"robotics":["Other:","Wonder Workshop"],"robotics_other":"My custom information"}} Some extra work is done in the textFieldMap setup to handle the "Other:" string being localized.

Also loc'ed the three standalone HAML pages.

breville · 2018-07-02T03:21:02Z

apps/src/code-studio/pd/international_opt_in/InternationalOptIn.jsx

+
+    let textFieldMapSubjects = {};
+    const lastSubjectsKey = this.props.options.subjects.slice(-1)[0];
+    textFieldMapSubjects[lastSubjectsKey] = "other";


I didn't want to change the underlying form system, but it seems this would be more robust if we identified the fields based on an identifier rather than by a user-facing string, especially if we're loc'ing like here.

Can you elaborate? I'm not sure I understand. Do you mean here the key (option text, which does appear to be localized since it's pulled from the dynamic last value of props.options.subjects), or the value (which will become the key suffix for the additional text field)? If the latter, can you explain why localizing the key would be helpful? If the former, what is missing? If you want it to be a constant rather than the last in options, then by all means create a new object mapping field name to the "Other" text fields and pass it through from the server in script data.

Oh, it's the key. It just seemed a bit delicate to have a key in textFieldMap being a human-readable string (e.g. "Other:", especially since it will vary with language. As far as I can tell, this is implemented in FormComponent and I didn't want to change that code. Hopefully not a big deal, unless there's something unexpected in any translations...

The human-readable string is the option (out of options passed through from the model) that gets an additional text box. It's just used for matching purposes at render time, not stored. The right-side, the value in this mapping, becomes the suffix on the new key in the resulting form_data for the new field. Like other field names, this key is not (and should not be) localized.

Hopefully this explanation helps (from just above your link in FormComponent):

code-dot-org/apps/src/code-studio/pd/form_components/FormComponent.jsx

Lines 234 to 236 in f0d1496

* @param {Object} textFieldMap - map specifying which answers should be followed by a text field

* Each key is an answer text from options.

* Each value is the suffix (appended to `${name}_`) which will become the name of the new text field

If not, feel free to modify / rewrite to make it more clear.

In our case here with localized options the key in this mapping, the option to match, must also be localized.

Backing up, another design choice originally would have been to have richer objects for options that specified aspects of the UI including whether that particular option had an additional text box. However we added this feature after options was already just a simple array, so we chose this approach instead - to specify additional text fields as an add-on, with a mapping from the options that already existed.

Anyway, if both options and the source for the textFieldMap both come from the model and are localized, then what is delicate? It's only really different from the above approach in that it's distributed over 2 lists.

We discussed offline. My main concern was having JS hash key names be user-facing strings that are localised, meaning they can contain special & international characters. However, we believe that all of those are valid, so this should be fine.

aoby · 2018-07-05T23:59:34Z

apps/src/code-studio/pd/international_opt_in/InternationalOptIn.jsx

+
+    let textFieldMapRobotics = {};
+    const lastRoboticsKey = this.props.options.robotics.slice(-1)[0];
+    textFieldMapRobotics[lastRoboticsKey] = "other";


nit: this can be const since its not reassigned (let vs const is about the reference. You can still modify the object contents in an object referred to by a const variable)`

Also the declaration and assignment can be combined if you want:

const textFieldMapRobotics = {[lastRoboticsKey]: "other"};

See Computed Property Names here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer

aoby · 2018-07-06T00:08:14Z

lib/international_opt_in_people.rb

@@ -0,0 +1,13 @@
+module InternationalOptInPeople


Is this only used in Pd::InternationalOptIn? Why does it need to be its own module? And why in the root lib directory? Do we expect to share with Pegasus?

As suggested earlier, if we put it instead in dashboard/lib we won't need this require.

bencodeorg · 2018-07-06T19:16:41Z

Sorry @breville I forgot about this PR and just checked it out, two questions:

can only teacher accounts fill out this form?
do you have to be logged in to fill it out?

Asking b/c I believe this form is the way we'll establish teacher "cohorts" for international partners, so any work we can do up front to avoid problems later are appreciated.

aoby · 2018-07-06T19:23:21Z

@bencodeorg Yes and yes, because of the ability granted only to teachers, and authorized in the controller.

@breville since this is a requirement, it's probably worth adding tests for the InternationalOptInsController to make sure that signed in teachers can see the page, but students and sign outs can't. It's probably also worth adding an authorize to the API controller (& tests) for the same.

bencodeorg · 2018-07-06T20:06:34Z

Helpful, thanks @aoby!

breville · 2018-07-10T04:25:54Z

@aoby I added tests that the correct page is rendered in #23603, though they are for InternationalOptInController rather than InternationalOptInsController. Is that right?

Api::V1::Pd::InternationalOptInsController already has an authorize_resource. Should Pd::InternationalOptInController get one too?

Sorry if I've got these back-to-front.

aoby · 2018-07-10T04:41:49Z

Heh, the names are confusing. How about this:

The front end controller (Pd::InternationalOptInController). Now that I see the tests in PD: international opt-in additional work #23603 it's clear this should not have an authorize because it renders various templates for users who are not authorized to see the form. They should not be forbidden. 👍
The API controller (Api::V1::Pd::InternationalOptInController), which does use authorize_resource still should have a unit test confirming that authorization IMO - that only (logged in) teachers can create one, while students cannot.

breville added 6 commits June 21, 2018 20:20

PD: international opt-in form

74817fb

PD: International opt-in work

12feb46

PD: international opt-in form gets loc'ed labels

ded35b6

PD: international opt-in gets loc'ed options

1359b0a

PD: international opt-in, some validation & testing

caaa78e

PD: international opt-in gets date picker

ce89358

breville requested review from aoby and tanyaparker June 26, 2018 07:02

breville commented Jun 26, 2018

View reviewed changes

Merge branch 'staging' into international-optin-pd-form

bcc3ecc

Conflicts: dashboard/db/schema.rb

mehalshah self-requested a review June 26, 2018 19:16

aoby reviewed Jun 26, 2018

View reviewed changes

mehalshah reviewed Jun 26, 2018

View reviewed changes

PD: some international opt-in code review feedback

a93d3ec

breville added 4 commits June 29, 2018 23:42

PD: international opt-in naming: Optin->OptIn, optin -> opt_in

fbcc1e4

Also loc'ed the three standalone HAML pages.

PD: international opt-in gets more strings & loc

bcadab5

PD: international opt-in URL now /pd/international_workshop

48f93f1

breville commented Jul 2, 2018

View reviewed changes

PD: international opt-in gets email & legal opt-ins

f44b731

aoby approved these changes Jul 6, 2018

View reviewed changes

breville added 2 commits July 5, 2018 20:11

PD: international opt-in code review feedback

469c531

PD: international opt-in gets comment about people lists

a8e947e

breville merged commit b30a86f into staging Jul 6, 2018

breville deleted the international-optin-pd-form branch July 6, 2018 18:02

breville mentioned this pull request Jul 10, 2018

PD: international opt-in additional work #23603

Merged

	* @param {Object} textFieldMap - map specifying which answers should be followed by a text field
	* Each key is an answer text from options.
	* Each value is the suffix (appended to `${name}_`) which will become the name of the new text field

PD: international opt-in form #23235

PD: international opt-in form #23235

Conversation

breville commented Jun 21, 2018 • edited

Existing logged-out page (now loc'ed)

Existing not-teacher page (now loc'ed)

New teacher-with-no-email page (loc'ed)

Thanks (loc'ed)

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

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 Jun 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanyaparker commented Jun 28, 2018

breville Jul 2, 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

bencodeorg commented Jul 6, 2018

aoby commented Jul 6, 2018

bencodeorg commented Jul 6, 2018

breville commented Jul 10, 2018

aoby commented Jul 10, 2018

breville commented Jun 21, 2018 •

edited

breville Jun 27, 2018 •

edited

breville Jul 2, 2018 •

edited