Add spec for missing #replace_fields scenario#59
Add spec for missing #replace_fields scenario#59valscion merged 5 commits intovenuu:masterfrom gnfisher:replace-fields-spec
Conversation
`replace_fields`
replace_fields spec|
Thanks for starting this PR! When you're ready with this PR and would want a review, you can add the |
|
These specs seem to cover a little less ground than what relationship record specs do. For example, this is done immediately inside 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
endNote that this isn't inside any context 'unauthorized for update? on source record' doblocks, 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? |
| it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } | ||
| end | ||
|
|
||
| context 'were replace_<type>? is undefined' do |
There was a problem hiding this comment.
Here's a bit of a typo, it probably should be "where" instead of "were"
| it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } | ||
| end | ||
|
|
||
| context 'were replace_<type>? is undefined' do |
There was a problem hiding this comment.
Here's a bit of a typo, too — it probably should be "where" instead of "were"
valscion
left a comment
There was a problem hiding this comment.
Looking very good! Just a few nitpicks left inline. Almost ready to go! 🎉 💞
| end | ||
| context 'authorized for replace_comments? and authorized for update? on source record' do | ||
| before { stub_policy_actions(source_record, replace_comments?: true, update?: true) } | ||
| it { is_expected.not_to raise_error(::Pundit::NotAuthorizedError) } |
There was a problem hiding this comment.
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 usingexpect {}.not_to raise_errororexpect { }.to raise_error(DifferentSpecificErrorClass).
This is the same case as in #60 review notes
There was a problem hiding this comment.
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 😄
| context 'where replace_<type>? is undefined' do | ||
| context 'authorized for update? on source record' do | ||
| before { stub_policy_actions(source_record, update?: true) } | ||
| it { is_expected.not_to raise_error(::Pundit::NotAuthorizedError) } |
There was a problem hiding this comment.
This should also be changed to be just
it { is_expected.not_to raise_error }|
Two issues fixed. |
valscion
left a comment
There was a problem hiding this comment.
Thank you! I fixed the last nitpick and this is good to merge now 🎉
replace_<type>?is not defined for related records, not when user is authorized and unauthorized toupdate?source record.As requested by @valscion at the end of issue #51's comments.