Fallback to `select` if array of `checkboxes` fail #34

seanpdoyle · 2014-09-12T14:56:28Z

Allows fields that are arrays of strings to be passed to <select multiple>
inputs.

seanpdoyle · 2014-09-12T14:57:15Z

lib/formulaic/inputs/array_input.rb

@@ -3,6 +3,8 @@ module Inputs
    class ArrayInput < Input
      def fill
        value.each { |checkbox| check checkbox }
+      rescue
+        value.each { |option| select option, from: label }


@calebthompson this is obviously a lazy and naive implementation. I was hoping you'd be able to point me to a more elegant approach if one exists.

Haha, a little maybe. Check out StringInput for how I've done this before.

seanpdoyle · 2014-09-15T21:26:36Z

lib/formulaic/inputs/array_input.rb

+      private
+
+      def has_select?
+        page.has_select?(label)


Looking at the docs, it doesn't look like we can filter for <select multiple="true">

Would we really need to check for this? Wouldn't a select's single/multiple selection come out when trying to fill it in and assert on the side effects of submitting the form?

Also, wouldn't the onus be on the caller to pass in the correct label?

They might, but if we only check for the select, we just change the
selection for each array element then submit - no errors.

I'm a big fan of the Unix philosophy of failing early and loudly.

On Mon, Sep 15, 2014 at 02:26:37PM -0700, Sean Doyle wrote:

@@ -2,6 +2,26 @@ module Formulaic
module Inputs
class ArrayInput < Input
def fill

if has_select?

select_options

else

check_boxes

end

rescue Capybara::ElementNotFound

raise InputNotFound.new(%[Unable to find input "#{label}".])

end

private

def has_select?

page.has_select?(label)

Looking at the docs, it doesn't look like we can filter for multiple selects.

Would we really need to check for this? Wouldn't a select's single/multiple selection come out when trying to fill it in and assert on the side effects of submitting the form?

Wouldn't the onus be on the caller to pass in the correct label?

Reply to this email directly or view it on GitHub:
https://github.com/thoughtbot/formulaic/pull/34/files#r17569921

has_selector? should take attribute selectors: has_selector? "select[multiple]"

When trying to call has_selector?("select[multiple]", label: label), the failure reads

invalid keys :label, should be one of :count, :minimum, :maximum, :between, :text, :visible, :exact, :match, :wait

I don't think any of those accepted keys apply to an input's label

I'm not sure how to find the select with both multiple and the corresponding label

I agree, if the select isn't multiple, we'd just select the last option provided.

Is correcting this a formulaic concern? Is it a form-filling 'failure'?

I don't think 'silently' failing will be an issue, since controllers expecting an array will be requiring form[field][] while controllers expecting a single option will require form[field].

Generally speaking, it's a test's job to show mistakes. The more verbose we can be about that, the more useful Formulaic is.

Consider:

Formulaic::InputNotFound No select or checkbox has all options in [x, y, z].

versus:

ActionController::UnpermittedParameters: found unpermitted parameters: letters

I agree, the former is much more helpful.

However, I don't know how to move forward with this

calebhearth · 2014-09-18T14:54:08Z

Pushed up ugly but working code. It could benefit from some extract method.

calebhearth · 2014-09-18T14:55:30Z

Your failing test is in a branch at sd-select-array-failing

seanpdoyle · 2014-09-18T15:01:08Z

Nice work, I'll take a look tomorrow

seanpdoyle · 2014-09-19T14:51:31Z

lib/formulaic/inputs/array_input.rb

+      end
+    end
+
+    class SelectInput < ArrayInput


@calebthompson I know that private classes are more or less a no-no, but I wanted to get your feedback on the extraction before I went any further.

I'm fine with them.

Might make more sense to have them just be PORO helper classes though.

to do that, we'd have to expose label, value and somehow share contains_all_options?

label & value can still be passed in. contains_all_options? could be moved down into those, but then it'd be duplicated.

Oh right, I forgot we're passing in label and value.

My first instinct was to use SimpleDelegator, and I passed in self instead of label, value. I got to the point where I made those fields public, but then had to duplicate contains_all_options?, so I fell back to inheritance.

seanpdoyle · 2014-09-19T15:18:51Z

Right, I plan on squashing and authoring a good commit message before this gets merged.

seanpdoyle · 2014-09-19T18:24:17Z

@calebthompson ok squashed my commit and split out the files.

What else is blocking this?

seanpdoyle · 2014-09-19T18:25:09Z

lib/formulaic/inputs/array_input.rb

      end
    end
  end
 end
+
+require 'formulaic/inputs/checkbox_input'
+require 'formulaic/inputs/select_input'


These need to be at the bottom of the file, since they both depend on ArrayInput

calebhearth · 2014-09-19T20:25:59Z

spec/spec_helper.rb

@@ -26,6 +26,8 @@ def load_translations
              awesome: Are you awesome?
              bio: Biography
              date_of_birth: Date of birth
+              likes: Likes
+              dislikes: Dislikes


I have / had a passing interest in not having these just be humanized versions of the translations, but that doesn't really matter anymore since we have a lot of those.

calebhearth · 2014-09-19T21:04:17Z

LGTM.

seanpdoyle · 2014-09-19T21:10:05Z

Should these commits get squashed to 1 or are you ok with them as-is?

calebhearth · 2014-09-19T21:11:09Z

Not much of a story being told, I think. Go ahead and squash them.

* Allows fields that are arrays of strings to be passed to `<select multiple>` inputs * Update README.md * Add coverage for Array of checkboxes * Rescue `Capybara` errors and rethrow as Formulaic * Check for presence of all options in ArrayInput * Raise Formulaic::InputNotFound with useful message if not all options are present in either a select[multiple]'s options or as checkboxes. * Ensure checkboxes exist with `:has` http://dev.w3.org/csswg/selectors4/#relational * Extract `SelectInput` and `CheckboxInput` * Both are subclasses of `ArrayInput`. Now, `ArrayInput#fill` chains together successive calls to `SelectInput#fill` and `CheckboxInput#fill`, finally failing with an informative exception if neither inputs exist on the page. * Enure that I18n's aren't humanized

Formulaic now knows how to `select` from a `select[multiple]` if it can't `check` array options as `input[type=checkbox]`. It will also refuse to fill an `Array` value if all elements can't be selected or checked (and they must all be the same action). #34

seanpdoyle reviewed Sep 12, 2014
View reviewed changes

seanpdoyle force-pushed the sd-select-array branch from ac85b2d to 149f4eb Compare September 12, 2014 19:57

seanpdoyle reviewed Sep 15, 2014
View reviewed changes

calebhearth force-pushed the sd-select-array branch from 2e3467d to 3333da1 Compare September 18, 2014 14:53

seanpdoyle reviewed Sep 19, 2014
View reviewed changes

seanpdoyle force-pushed the sd-select-array branch 3 times, most recently from 5d91b18 to 7b3d1cb Compare September 19, 2014 18:23

seanpdoyle reviewed Sep 19, 2014
View reviewed changes

seanpdoyle force-pushed the sd-select-array branch from 7b3d1cb to 4d96326 Compare September 19, 2014 19:57

calebhearth reviewed Sep 19, 2014
View reviewed changes

seanpdoyle force-pushed the sd-select-array branch from 420be84 to 4258e08 Compare September 19, 2014 21:19

seanpdoyle merged commit 4258e08 into master Sep 19, 2014

seanpdoyle deleted the sd-select-array branch September 19, 2014 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback to `select` if array of `checkboxes` fail #34

Fallback to `select` if array of `checkboxes` fail #34

seanpdoyle commented Sep 12, 2014

seanpdoyle Sep 12, 2014

calebhearth Sep 12, 2014

seanpdoyle Sep 15, 2014

calebhearth Sep 15, 2014

calebhearth Sep 15, 2014

seanpdoyle Sep 15, 2014

seanpdoyle Sep 15, 2014

calebhearth Sep 17, 2014

seanpdoyle Sep 17, 2014

calebhearth commented Sep 18, 2014

calebhearth commented Sep 18, 2014

seanpdoyle commented Sep 18, 2014

seanpdoyle Sep 19, 2014

calebhearth Sep 19, 2014

calebhearth Sep 19, 2014

seanpdoyle Sep 19, 2014

calebhearth Sep 19, 2014

seanpdoyle Sep 19, 2014

seanpdoyle commented Sep 19, 2014

seanpdoyle commented Sep 19, 2014

seanpdoyle Sep 19, 2014

calebhearth Sep 19, 2014

calebhearth commented Sep 19, 2014

seanpdoyle commented Sep 19, 2014

calebhearth commented Sep 19, 2014

Fallback to select if array of checkboxes fail #34

Fallback to select if array of checkboxes fail #34

Conversation

seanpdoyle commented Sep 12, 2014

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

calebhearth commented Sep 18, 2014

calebhearth commented Sep 18, 2014

seanpdoyle commented Sep 18, 2014

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

seanpdoyle commented Sep 19, 2014

seanpdoyle commented Sep 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebhearth commented Sep 19, 2014

seanpdoyle commented Sep 19, 2014

calebhearth commented Sep 19, 2014

Fallback to `select` if array of `checkboxes` fail #34

Fallback to `select` if array of `checkboxes` fail #34