to_not permit_actions behavior is misleading

[oneamtu]

Hi,

We just ran into a bit of unexpected behavior in our app when using .to_not permit_actions. The gist of it is that we had an action that was both permitted and not permitted in this setup

  it { is_expected.to permit_actions([:new, :create, :edit]) }
  it { is_expected.not_to permit_actions([:edit, :destroy]) }

The specs were passing even though nothing changed between the 2 examples. The problem is that the permit_actions matcher does the equivalent of a policy.new? && policy.create? && policy.edit?. to_not negates that into !(policy.new? && policy.create? && policy.edit?) which is equivalent to !policy.new? || !policy.create? || !policy.edit?, so it's enough for one action to not be permitted for the whole matcher to pass. That's counterintuitive, but it does look like the matcher implements a negative message. Using the forbid_actions matcher fixes it for our purposes.

I'm not sure how to fix the negative matcher issue. It doesn't look like rspec matchers has a special negative matcher that one can define, so I'm not sure how to disallow this behavior. Maybe updating the docs or changing the error message would be a good first step?

Thank you for making this gem!

-Octavian

[chrisalley]

Requiring the forbid_actions matcher would be ideal in this case (this case is exactly what it was designed for). Unfortunately I don't know of any programmatic way that we can prevent or warn programmers from using not_to and permit_actions together.

If the spec passes, you won't see an error message, so updating the error message wouldn't help in that case.

I would happy to receive a PR with a warning somewhere obvious (near the top of the readme, for example), recommending the forbid matchers and the reasoning for using these. If this applies to other matchers, those sections would need updating too.

[dewyze]

RSpec does support a negated matcher with separate logic: https://relishapp.com/rspec/rspec-expectations/v/3-9/docs/custom-matchers/define-a-custom-matcher#matcher-with-separate-logic-for-expect().to-and-expect().not-to

RSpec::Matchers.define :contain do |*expected|
  match do |actual|
    expected.all? { |e| actual.include?(e) }
  end

  match_when_negated do |actual|
    expected.none? { |e| actual.include?(e) }
  end
end

[dewyze]

I opened a pull request with an incredibly verbose warning, if you have an edits I'm happy to make them. This is pretty obnoxious. It is possible to use the negated matcher to force the forbid_actions behavior instead, but that would be a breaking change, right?

[chrisalley]

Thanks, I'll take a look your PR. I've set up a seperate issue for discussing a potential 2.0 release: #33

[dewyze]

@chrisalley If you want any help with cleanup for a 2.0, let me know. I could contribute a bit.

[oneamtu]

:+1:, thank you for the contribution! Super helpful.

On Wed, Apr 8, 2020, 18:36 John DeWyze [email protected] wrote:

@chrisalley https://github.com/chrisalley If you want any help with cleanup for a 2.0, let me know. I could contribute a bit.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/chrisalley/pundit-matchers/issues/30#issuecomment-611229612, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAG4JPIWR47W2CWDIOI3KDRLT36NANCNFSM4J7AWTEA .

[chrisalley]

I've merged your PR containing the warning, @dewyze; thanks for the contribution. I'll keep this issue open until a cleaner solution is implemented in Pundit Matchers 2.

[tagliala]

I understand that not_to *_actions can be automatically generated in some scenarios, but a simpler solution could be to disable negated matchers for *_actions and force to use the opposite expectation

Ref:

https://github.com/rspec/rspec-expectations/blob/8a468eea0a2ee456db4d6e2dccccd66ff269b862/lib/rspec/matchers/built_in/operators.rb#L58-L60

[chrisalley]

Yes, I think simplifying the implementation here and disabling the negated matchers would be sensible and clearer from both a security and maintenance standpoint.

It's already noted in the readme that it is "it is recommended to always use is_expected.to rather than is_expected.not_to when using Pundit Matchers" and that the current behaviour may change in a future version.

The next steps could be:

• Disable is_expected.not_to. For consistency, we could enforce the use of is_expected.to for all of the matchers.
• If is_expected.not_to is used, display a warning to use an appropriate forbid matcher instead.
• Since this a breaking change, release a new major version of the gem.

[chrisalley]

@tagliala is investigating a way to support negated matchers as part of some experimental work.

[tagliala]

Closed via #81

[tagliala]

@chrisalley this can be closed as addressed in 3.0.0.beta1