[95896] Fix AccreditedOrganization Model flaky spec

[jvcAdHoc]

Summary

• This sets the poa_code on model validation specs. The factory's random poa code can sometimes only contain numeric characters which breaks the case sensitivity check in the shoulda matcher uniqueness assertion.

Related issue(s)

• 95896

Testing done

• New code is covered by unit tests

Acceptance criteria

• I fixed|updated|added unit tests and integration tests for each feature (if applicable).
• No error nor warning in the console.
• Events are being sent to the appropriate logging solution
• Documentation has been updated (link to documentation)
• No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
• Feature/bug has a monitor built into Datadog (if applicable)
• If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
• I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

[bosawt]

I just randomly saw this, but I've seen this fail before, one quick question. Is there a reason you're not updating the Factory itself in this case? The factory shouldn't be coming up with poa_code values that are not valid in the first place. So if the requirement is a mix of alpha and numeric that should probably be reflected in the factory itself.

In fact, Faker::Alphanumeric.alphanumeric can do that automatically for you min_alpha, and min_numeric, as described here: https://github.com/faker-ruby/faker/blob/main/doc/default/alphanumeric.md

[jvcAdHoc]

I just randomly saw this, but I've seen this fail before, one quick question. Is there a reason you're not updating the Factory itself in this case? The factory shouldn't be coming up with poa_code values that are not valid in the first place. So if the requirement is a mix of alpha and numeric that should probably be reflected in the factory itself.

In fact, Faker::Alphanumeric.alphanumeric can do that automatically for you min_alpha, and min_numeric, as described here: https://github.com/faker-ruby/faker/blob/main/doc/default/alphanumeric.md

POA codes can be any mix of 3 alphanumeric characters (074 is equally as valid and real as HTC), so the factory is generating legitimate values. The uniqueness matcher in the model spec cares about case sensitivity and would fail the 074 case for example.

We could change the uniqueness constraint to be case insensitive.

[bosawt]

I just randomly saw this, but I've seen this fail before, one quick question. Is there a reason you're not updating the Factory itself in this case? The factory shouldn't be coming up with poa_code values that are not valid in the first place. So if the requirement is a mix of alpha and numeric that should probably be reflected in the factory itself.
In fact, Faker::Alphanumeric.alphanumeric can do that automatically for you min_alpha, and min_numeric, as described here: https://github.com/faker-ruby/faker/blob/main/doc/default/alphanumeric.md

POA codes can be any mix of 3 alphanumeric characters (074 is equally as valid and real as HTC), so the factory is generating legitimate values. The uniqueness matcher in the model spec cares about case sensitivity and would fail the 074 case for example.

We could change the uniqueness constraint to be case insensitive.

I just randomly saw this, but I've seen this fail before, one quick question. Is there a reason you're not updating the Factory itself in this case? The factory shouldn't be coming up with poa_code values that are not valid in the first place. So if the requirement is a mix of alpha and numeric that should probably be reflected in the factory itself.
In fact, Faker::Alphanumeric.alphanumeric can do that automatically for you min_alpha, and min_numeric, as described here: https://github.com/faker-ruby/faker/blob/main/doc/default/alphanumeric.md

POA codes can be any mix of 3 alphanumeric characters (074 is equally as valid and real as HTC), so the factory is generating legitimate values. The uniqueness matcher in the model spec cares about case sensitivity and would fail the 074 case for example.

We could change the uniqueness constraint to be case insensitive.

Wouldn't that be the correct thing to do then? As you described it, right now certain valid POA codes will fail validation, which seems like you actually just described a bug that currently exists in the validation logic

[jvcAdHoc]

I just randomly saw this, but I've seen this fail before, one quick question. Is there a reason you're not updating the Factory itself in this case? The factory shouldn't be coming up with poa_code values that are not valid in the first place. So if the requirement is a mix of alpha and numeric that should probably be reflected in the factory itself.
In fact, Faker::Alphanumeric.alphanumeric can do that automatically for you min_alpha, and min_numeric, as described here: https://github.com/faker-ruby/faker/blob/main/doc/default/alphanumeric.md

POA codes can be any mix of 3 alphanumeric characters (074 is equally as valid and real as HTC), so the factory is generating legitimate values. The uniqueness matcher in the model spec cares about case sensitivity and would fail the 074 case for example.
We could change the uniqueness constraint to be case insensitive.

I just randomly saw this, but I've seen this fail before, one quick question. Is there a reason you're not updating the Factory itself in this case? The factory shouldn't be coming up with poa_code values that are not valid in the first place. So if the requirement is a mix of alpha and numeric that should probably be reflected in the factory itself.
In fact, Faker::Alphanumeric.alphanumeric can do that automatically for you min_alpha, and min_numeric, as described here: https://github.com/faker-ruby/faker/blob/main/doc/default/alphanumeric.md

POA codes can be any mix of 3 alphanumeric characters (074 is equally as valid and real as HTC), so the factory is generating legitimate values. The uniqueness matcher in the model spec cares about case sensitivity and would fail the 074 case for example.
We could change the uniqueness constraint to be case insensitive.

Wouldn't that be the correct thing to do then? As you described it, right now certain valid POA codes will fail validation, which seems like you actually just described a bug that currently exists in the validation logic

I started on this change right after I sent the message

[jvcAdHoc]

Maybe not. I found an old thread for shoulda_matchers that covers this exact scenario and the recommendation is what I did here.

[bosawt]

Maybe not. I found an old thread for shoulda_matchers that covers this exact scenario and the recommendation is what I did here.

Whoa, so they're saying that the validation works but the shoulda_matcher fails? That's so weird, haha

[bosawt]

Maybe not. I found an old thread for shoulda_matchers that covers this exact scenario and the recommendation is what I did here.

Whoa, so they're saying that the validation works but the shoulda_matcher fails? That's so weird, haha

I ran the spec and it actually seems to give what's probably the right answer, adding case_sensitive to the validation itself, right?

 Shoulda::Matchers::ActiveRecord::ValidateUniquenessOfMatcher::NonCaseSwappableValueError:
   Your AccreditedOrganization model has a uniqueness validation on
   :poa_code which is declared to be case-sensitive, but the value the
   uniqueness matcher used, "123", doesn't contain any alpha characters, so
   using it to test the case-sensitivity part of the validation is
   ineffective. There are two possible solutions for this depending on what
   you're trying to do here:

   a) If you meant for the validation to be case-sensitive, then you need
      to give the uniqueness matcher a saved instance of
      AccreditedOrganization with a value for :poa_code that contains alpha
      characters.

   b) If you meant for the validation to be case-insensitive, then you need
      to add `case_sensitive: false` to the validation and add
      `case_insensitive` to the matcher.

[jvcAdHoc]

I'd say our situation falls under a) here where we want to keep case sensitivity in the uniqueness constraint and need to "trick" the matcher with a poa code that contains at least 1 alpha character.

Whoa, so they're saying that the validation works but the shoulda_matcher fails? That's so weird, haha

Yeah it seems like a weird way to handle this edge case of swapcase not having a meaningful change on a string

[bosawt]

I'd say our situation falls under a) here where we want to keep case sensitivity in the uniqueness constraint and need to "trick" the matcher with a poa code that contains at least 1 alpha character.

Whoa, so they're saying that the validation works but the shoulda_matcher fails? That's so weird, haha

Yeah it seems like a weird way to handle this edge case of swapcase not having a meaningful change on a string

Just to follow up, I think the change here makes sense then. My only other comment would be that it seems weird that the POA numbers are case sensitive (like if a12 and A12 were completely different POA numbers, seems weird that the POA source of truth would allow that), but I suppose that's a comment about the POA schema construction itself and not anything about this service

[rmtolmach]

Thanks for the review and the back-and-forth, yall!

[github-actions]

Backend-review-group approval confirmed.