LG-4398: Support ActiveModel::Errors instance for FormResponse

[aduth]

Why: An ActiveModel::Errors instance can represent both the "message" and "type" of an error, where the message is the translated, human-readable text shown to the user, and the type is typically the locale-independent, machine-readable unique identifier of a particular error. By supporting an ActiveModel::Errors instance in FormResponse, we can retain current behaviors of showing and logging a human-readable error message, while also allowing us to identify and group instances of an error across locales in a CloudWatch query.

With few exceptions, this also aligns to how we most often use FormResponse, where it's initialized using errors from an instance of ActiveModel::Errors. The difference is a simplification to pass the errors instance itself, rather than the result of its messages method (i.e. errors: errors instead of errors: errors.messages).

It may be possible to consolidate to avoid overloading and instead supporting only the ActiveModel::Errors instance in this class, but (a) there are a handful of existing cases where we create hashes ad-hoc for logging and (b) in those cases, the ActiveModel::Errors equivalent implementation is relatively clunky.

Publishing this as a draft, since there are a number of specs which need to be updated, and I'd not want to start down that path if there's questions about the viability of this approach. For ease of review, 445c4ad contains only the relevant changes for FormResponse.

[zachmargolis]

1. what if we made error_details an attribute, and only put it in the hash at the end? then we could simplify merge because we wouldn't need to special-case the error_details key and could just append the two arrays in that method?
2. Do we need to a check that errors has #details before calling this? Because plain hashes will not have #details

[aduth]

1. what if we made error_details an attribute, and only put it in the hash at the end? then we could simplify merge because we wouldn't need to special-case the error_details key and could just append the two arrays in that method?

The original motivation for this approach was to avoid making those details part of the public interface, where nobody should ever need to explicitly pass error_details, instead relying on it to be handled internally when given an ActiveModel::Errors instance. Though I suppose if this were attr_accessor, we wouldn't necessarily need to make this part of the initialize signature, but could still handle it in the merge? I'll poke at it a bit more.

2. Do we need to a check that errors has #details before calling this? Because plain hashes will not have #details

The condition on the next line should avoid any issues here?

identity-idp/app/services/form_response.rb

Line 8 in 445c4ad

) if errors.is_a?(ActiveModel::Errors) && errors.details.present?

Maybe there's some weird cases in merge if someone manually defines an error_details property of a hash, but the problem might go away depending on changes regarding your first point.

[zachmargolis]

The condition on the next line should avoid any issues here?

🤦 somehow didn't see that line ya that works

[zachmargolis]

The original motivation for this approach was to avoid making those details part of the public interface, where nobody should ever need to explicitly pass error_details, instead relying on it to be handled internally when given an ActiveModel::Errors instance. Though I suppose if this were attr_accessor, we wouldn't necessarily need to make this part of the initialize signature, but could still handle it in the merge? I'll poke at it a bit more.

We could make it an attr_reader so it's a read-only property from outside the class?

[aduth]

The original motivation for this approach was to avoid making those details part of the public interface, where nobody should ever need to explicitly pass error_details, instead relying on it to be handled internally when given an ActiveModel::Errors instance. Though I suppose if this were attr_accessor, we wouldn't necessarily need to make this part of the initialize signature, but could still handle it in the merge? I'll poke at it a bit more.

We could make it an attr_reader so it's a read-only property from outside the class?

We'd still need to set it in the new instance created in merge. There's a few ways we could do this:

• Add error_details as a property argument to initialize
• Make error_details an accessor and set on the new instance before returning in merge
• Create an ActiveModel::Errors from the merged errors & details to be passed as the errors of the new instance.

The first is what I'd mentioned as wanting to avoid because of the weird redundancy in the interface. The third is maybe "cleanest", though more complicated logic in determining how to merge what are potentially two objects of mixed types.

[zachmargolis]

Maybe some instance_eval to get around the property access? It's ugly but gets the job done... 😬

class A
  attr_accessor :a

  def initialize(a)
    @a = a
    @b = a + 1
  end

  def merge(other)
    self.class.new(self.a + other.a).tap do |merged|
      outer_b = b
      merged.instance_eval { @b = outer_b + other.instance_eval { b } }
    end
  end

  private

  attr_accessor :b
end

puts A.new(1).merge(A.new(2))

[aduth]

That could work 😄 947e9e6

[zachmargolis]

LGTM

[aduth]

FYI I'd expect this test case to pass on main as well, but it doesn't:

Failures:

  1) FormResponse#merge merges multiple errors for key
     Failure/Error: expect(combined_response.errors).to eq(front: ['front-error-1', 'front-error-2'])
     
       expected: {:front=>["front-error-1", "front-error-2"]}
            got: {:front=>["front-error-2"]}
     
       (compared using ==)
     
       Diff:
       @@ -1 +1 @@
       -:front => ["front-error-1", "front-error-2"],
       +:front => ["front-error-2"],
       
     # ./spec/services/form_response_spec.rb:56:in `block (3 levels) in <top (required)>'

So we incidentally fix an issue where we may be dropping some errors in merging two responses.