zooniverse / talk-api

Apache License 2.0
6 stars 0 forks source link

Rails 5 fix user conversation and blocked user tests #327

Closed Tooyosi closed 2 months ago

Tooyosi commented 2 months ago

This has to do with validation messages. With the upgrade the validation message for associations are now 'must exist' as opposed to 'can't be blank'. Added the text on the spec_helper so i can factor in tests on the previous rails version aswell (good idea??)

yuenmichelle1 commented 2 months ago

I wonder if there's a way we can customize the error message on the model instead so that we can avoid doing Rails version checks. In that way we have more control over what we're testing (as opposed to being at the mercy of Rails when they decide to change up their error messages).

I have a couple opinions on this:

I think one thing we can do is either: A) customize the error message and therefore we have more control over it and can be more descriptive if we want to OR B) (My preference*) Remove the second argument of fail_validation (i.e. remove user: "can't be blank" on specs). since I feel like it is enough to say that the record we are attempting to create/update failed validation.

Eg:

it 'should require a user' do
      without_user = build :blocked_user, user: nil
      expect(without_user).to fail_validation 
end

See: https://github.com/zooniverse/talk-api/blob/master/spec/support/validation_matcher.rb for where fail_validation is defined.

SideNote One thing we do have to make note of if you do decide to just remove args of fail_validation and just have fail_validation is that when we get to Rails 5, the error message will change. Hopefully none of our FE/clients are expecting a specific error message (eg. "User can't be blank"). I sincerely hope not. 🤞

Tooyosi commented 2 months ago

expect(without_user).to fail_validation

Totally agree, i prefer the second aswell, given the FE doesn't expect that specific message