Add spec for missing #replace_fields scenario

[gnfisher]

• Test scenarios where replace_<type>? is not defined for related records, not when user is authorized and unauthorized to update? source record.

As requested by @valscion at the end of issue #51's comments.

[valscion]

Thanks for starting this PR!

When you're ready with this PR and would want a review, you can add the Status: Ready for review and we'll be able to more easily pick this up when we've got time ☺️

[valscion]

These specs seem to cover a little less ground than what relationship record specs do. For example, this is done immediately inside #replace_to_one_relationship specs:

context 'where replace_<type>? not defined' do
  # CommentPolicy does not define #replace_article?, so #update? should determine authorization
  let(:source_record) { comments(:comment_1) }
  let(:related_records) { Article.new }
  subject(:method_call) do
    -> { authorizer.replace_to_one_relationship(source_record, related_record, :article) }
  end

  context 'authorized for update? on record' do
    before { allow_action(source_record, 'update?') }
    it { is_expected.not_to raise_error }
  end

  context 'unauthorized for update? on record' do
    before { disallow_action(source_record, 'update?') }
    it { is_expected.to raise_error(::Pundit::NotAuthorizedError) }
  end
end

Note that this isn't inside any

context 'unauthorized for update? on source record' do

blocks, so there is no extraneous stubbing going on before the specs themselves.

Do you think these specs could be made to cover a bit more ground, taking example from the relationship tests?

[valscion]

Here's a bit of a typo, it probably should be "where" instead of "were"

[valscion]

Here's a bit of a typo, too — it probably should be "where" instead of "were"

[valscion]

Looking very good! Just a few nitpicks left inline. Almost ready to go! 🎉 💞

[valscion]

When not expecting an error, we should not expect a specific error class, as literally any other error would cause the expectation to pass. RSpec spams the test logs with this warning for these cases:

WARNING: Using expect { }.not_to raise_error(SpecificErrorClass) risks false positives, since literally any other error would cause the expectation to pass, including those raised by Ruby (e.g. NoMethodError, NameError and ArgumentError), meaning the code you are intending to test may not even get reached. Instead consider using expect {}.not_to raise_error or expect { }.to raise_error(DifferentSpecificErrorClass).

This is the same case as in #60 review notes

[valscion]

It has unfortunately passed review before, a bit incorrectly though, as the original code also contained this mistake. Now's a good chance to fix it, then 😄

[valscion]

This should also be changed to be just

it { is_expected.not_to raise_error }

[gnfisher]

Two issues fixed.

[valscion]

Thank you! I fixed the last nitpick and this is good to merge now 🎉