[BUG]: Inconsistent Behavior in Requests

[elasticspoon]

Is there an existing issue for this?

• [X] I have searched the existing issues

Current Behavior

When submitting an individuals request with an blank input, an error is returned. When submitting a quantity request with a blank input the input is ignored and the rest of the form submits.

Expected Behavior

I am not sure which is correct but it should not be inconsistent.

Steps To Reproduce

in spec/system/partners/managing_requests_system_spec.rb:61 change:

        before do
          fill_in 'Comments', with: Faker::Lorem.paragraph

          # Select items
          item_details.each_with_index do |item, idx|
            if idx != 0
              click_link 'Add Another Item'
            end

            last_row = find_all('tr').last
            last_row.find('option', text: item[:name], exact_text: true).select_option
            last_row.find_all('.form-control').last.fill_in(with: item[:person_count])
          end

          # delete an item
          find_all('td').last.click
+
+        click_link 'Add Another Item'
        end

the tests will now fail.

Environment

No response

Criteria for Completion

No response

Anything else?

The relevant code is in family_request_create_service.rb:

    def valid?
      if family_requests_attributes.blank?
        errors.add(:base, 'family_requests_attributes cannot be empty')
      end

      if item_requests_attributes.any? { |attr| included_items_by_id[attr[:item_id].to_i].nil? }
        errors.add(:base, 'detected a unknown item_id')
      end

      errors.none?
    end

the validation in this class prevents blank attributes whereas in request_create_service.rb:

    def populate_item_request(partner_request)
      # Exclude any line item that is completely empty
      formatted_line_items = item_requests_attributes.reject do |attrs|
        attrs['item_id'].blank? && attrs['quantity'].blank?
      end

blank values are filtered out.

I am planning on working the bug. Not sure which behavior is correct currently.

Code of Conduct

• [X] I've read the Code of Conduct and understand my responsibilities as a member of the Ruby for Good community

[cielf]

@elasticspoon Looking at the analogous behaviour for distributions, purchases, and donations, the behaviour shown in the quantity request is consistent with that.
I'd like to check in with the core team, though, as to whether we think it is correct. I can see arguments both ways.

[cielf]

Core team meets Sundays, so don't expect a full answer before then.

[cielf]

Notes another inconsistency -- the item field on the quantity request shows "Select an item", whereas the individual is just blank. On the bank side we show "Choose an item"

[cielf]

Ok... The answer is that we should be giving an error in these cases. Thank you for your patience.

[elasticspoon]

@cielf just so that you have another data point. not sure if this was taken into account. but the behavior of allowing empty items was specifically added for UX reasons. https://github.com/rubyforgood/human-essentials/issues/2489.

Does that change the thinking about this in any way?

[cielf]

The case we're talking about is that they have an item chosen, but no quantity. Pretty sure #2489 is covering cases where no item is chosen.

[elasticspoon]

@cielf Sorry, I think I confused myself a bit by bringing up that issue. Could you make sure I am understanding this correctly.

Current behavior:

Quantity:

Comment	Item	Number	Result
Valid	Valid	Valid	Valid
Valid	Blank	Blank	Valid
Valid	1 row Blank	1 row Blank	Valid (removes blank row)
Valid	Blank	Valid	Invalid
Valid	Valid	Blank	Invalid

Individual:

Comment	Item	Number	Result
Valid	Valid	Valid	Valid
Valid	Blank	Blank	Invalid
Valid	1 row Blank	1 row Blank	Invalid
Valid	Blank	Valid	Invalid
Valid	Valid	Blank	Invalid

Target Behavior

the goal is to make them both operate like quantity requests?

Quantity:

Comment	Item	Number	Result
Valid	Valid	Valid	Valid
Valid	Blank	Blank	Valid
Valid	1 row Blank	1 row Blank	Valid (removes blank row)
Valid	Blank	Valid	Invalid
Valid	Valid	Blank	Invalid

Individual:

Comment	Item	Number	Result
Valid	Valid	Valid	Valid
Valid	Blank	Blank	Valid
Valid	1 row Blank	1 row Blank	Valid (removes blank row)
Valid	Blank	Valid	Invalid
Valid	Valid	Blank	Invalid

[cielf]

Ah, I somehow thought you were saying that Item filled in but number not filled in on quantity was removing the row silently, which is currently happening on donations (making a note to myself to write that up).

It should be valid in either case to have a comment but no items. This covers the weird case where there is a special request that the bank has some one-off items that they aren't going to bother putting in the system, but they've let the partners know are available.

In short, yes, please make the individual behave like quantity in this case.