'.authorize' and '#authorize' return record even with passed record with namespace array #626

QWYNG · 2019-11-10T12:14:15Z

Hi, Thank you for the great Gem!
This is a proposal and pull request at once.
'.authorize' and '#authorize' return passed record now.

def show
  @post  = authorize Post.find(params[:id])
end

But When passing record with a namespace. return array

def show
  @post  = authorize [:admin, Post.find(params[:id])]
end

# @post is  [:admin, Post.find(params[:id])]

These methods are expected to return record not namespase array

When authorize override the helpers in AdminController to automatically apply the namespacing, This change will be very useful.

class AdminController < ApplicationController
  def authorize(record, query = nil)
    super([:admin, record], query)
  end
end

class Admin::PostController < AdminController
  def show
    post = authorize Post.find(params[:id])
    # We don't need `authorize(post)` line!
  end
end

If there is a place to fix, please let me know.
best regards.

dgmstuart

I'm basically in favour of this change - I have a few questions about the tests and some edge cases.

spec/pundit_spec.rb

lib/pundit.rb

dgmstuart · 2019-11-11T12:26:19Z

While I think it's unlikely that anyone is relying on getting the namespace array back from the authorize method, technically this is a breaking change: I don't think there's a clear expectation set that we always return the model instance, since the README does say "authorize returns the object passed to it", which I'd argue could mean either "the instance" or "the array".

Not sure how seriously we need to take that 🤷‍♂

QWYNG · 2019-11-11T12:55:51Z

@dgmstuart
Thank you for your comments!
I'll fix spec!
I think that this is a breaking change too.
But I think that most users would expect the only record itself to return in this case.

…reaking any existing behaviour

QWYNG · 2019-11-11T14:33:36Z

@dgmstuart
Thank you for your review.
I fixed spec.
I'm really grateful as a pundit user for your interaction even with this PR doesn't marge.

dgmstuart

Good job with the test coverage 👍

we maybe need to tweak a couple of the tests, but this feels close.

dgmstuart · 2019-11-11T14:48:34Z

spec/pundit_spec.rb

+    end
+
+    it "returns the class when passed record not a particular instance" do
+      expect(Pundit.authorize(user, Post, :show?)).to be(Post)


be feels unusual to me - would eq be more expressive/conventional?
The same for "comment" above

dgmstuart · 2019-11-11T14:52:10Z

spec/pundit_spec.rb

+    end
+
+    it "can use without a particular instance" do
+      expect(Pundit.authorize(user, Post, :show?)).to be_truthy


This feels like quite a weak assertion - is truthiness a property we care about?

Would it be better to make this an assertion about the return value ("returns the... ") and to expect the specific value we get in this case? (is it Post?)

But we have not 'authorize can use without a particular instance' spec and 'authorize can use headless policy' spec other.
May I add it at this time?

The thing for me is that I'm not sure what "can use" (or "can be used") is intending to test.

For me there are usually two types of test, which correspond to the two kinds of property which we care about our code having:

What's the return value (if we care)

What side effects happen (if any)

If we have one of these tests for a particular scenario/context then we're also implicitly testing that we "can use" the method in that context, since we're using the code. If we don't care about either 1 or 2, then probably the code doesn't need to exist 😉

Am I making sense, or am I missing the point?

Thanks! It became clear.
Certainly, 1 is testing that we "can use" the method.

Cool, in that case I suggest it should be the same shape as the other specs which test a return value:
title: it "returns..." do
expectation: expect(Pundit.authorize(...).to eq(<specific value>)

Yes
I left only test a return value.

Linuus · 2019-11-11T15:41:22Z

I would definitely say this is a bug 🐛

QWYNG · 2019-11-11T15:44:32Z

@dgmstuart
Thank you for your review!
fixed again

dgmstuart · 2019-11-12T08:55:32Z

spec/pundit_spec.rb

    it "returns the policy name symbol when passed record with headless policy" do
      expect(Pundit.authorize(user, :publication, :create?)).to eq(:publication)
    end

-    it "can use without a particular instance" do
-      expect(Pundit.authorize(user, Post, :show?)).to be_truthy
-    end


Ah I see - I hadn't read this correctly before. Yes, it's good to delete these specs, since they're entirely covered by the it "returns..." specs.

dgmstuart

Great work - let's get this merged in 👍

QWYNG · 2019-11-12T09:07:41Z

@dgmstuart @Linuus
I really appreciate both!
Thank you for the useful gem again!

update README for #626

Friday 13th-release! Careful! The bugfix below (#626) could break existing code. If you rely on the return value for `authorize` and namespaced policies you might need to do some changes.

authorize return record even with passed record with namespace arry

3de189c

QWYNG force-pushed the authorize_return_record_with_namespase_arry branch from 62cfec6 to 3de189c Compare November 10, 2019 12:15

QWYNG changed the title ~~'.authorize' and '#authorize' return record even with passed record with namespace arry~~ '.authorize' and '#authorize' return record even with passed record with namespace array Nov 10, 2019

dgmstuart requested changes Nov 11, 2019

View reviewed changes

spec/pundit_spec.rb Show resolved Hide resolved

spec/pundit_spec.rb Show resolved Hide resolved

spec/pundit_spec.rb Show resolved Hide resolved

lib/pundit.rb Show resolved Hide resolved

fix spec for make sure that .authorize and #authorize change is not b…

49a34d2

…reaking any existing behaviour

QWYNG requested a review from dgmstuart November 11, 2019 14:37

dgmstuart requested changes Nov 11, 2019

View reviewed changes

tweak spec be -> eq

5fdec1c

rm weak assertion

1b46006

QWYNG requested a review from dgmstuart November 11, 2019 15:43

dgmstuart reviewed Nov 12, 2019

View reviewed changes

dgmstuart approved these changes Nov 12, 2019

View reviewed changes

dgmstuart merged commit 772fcac into varvet:master Nov 12, 2019

QWYNG deleted the authorize_return_record_with_namespase_arry branch November 12, 2019 09:07

QWYNG added a commit to QWYNG/pundit that referenced this pull request Nov 12, 2019

update README for varvet#626

332e09e

QWYNG mentioned this pull request Nov 12, 2019

update README for #626 #628

Merged

dgmstuart added a commit that referenced this pull request Dec 6, 2019

Merge pull request #628 from QWYNG/update_readme_#626

d89a572

update README for #626

dgmstuart mentioned this pull request Jun 1, 2021

Pundit::NotAuthorizedError default error message for namespaced policies #670

Closed

Burgestrand added a commit that referenced this pull request Aug 13, 2021

Release 2.1.1

0b5c082

Friday 13th-release! Careful! The bugfix below (#626) could break existing code. If you rely on the return value for `authorize` and namespaced policies you might need to do some changes.

Burgestrand mentioned this pull request Aug 13, 2021

Release 2.1.1 #686

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

'.authorize' and '#authorize' return record even with passed record with namespace array #626

'.authorize' and '#authorize' return record even with passed record with namespace array #626

QWYNG commented Nov 10, 2019

dgmstuart left a comment

dgmstuart commented Nov 11, 2019

QWYNG commented Nov 11, 2019

QWYNG commented Nov 11, 2019

dgmstuart left a comment

dgmstuart Nov 11, 2019 •

edited

Loading

QWYNG Nov 11, 2019

dgmstuart Nov 11, 2019

QWYNG Nov 11, 2019

QWYNG Nov 11, 2019 •

edited

Loading

dgmstuart Nov 11, 2019

QWYNG Nov 11, 2019

dgmstuart Nov 11, 2019

QWYNG Nov 11, 2019

Linuus commented Nov 11, 2019

QWYNG commented Nov 11, 2019

dgmstuart Nov 12, 2019

dgmstuart left a comment

QWYNG commented Nov 12, 2019

'.authorize' and '#authorize' return record even with passed record with namespace array #626

'.authorize' and '#authorize' return record even with passed record with namespace array #626

Conversation

QWYNG commented Nov 10, 2019

dgmstuart left a comment

Choose a reason for hiding this comment

dgmstuart commented Nov 11, 2019

QWYNG commented Nov 11, 2019

QWYNG commented Nov 11, 2019

dgmstuart left a comment

Choose a reason for hiding this comment

dgmstuart Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QWYNG Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Linuus commented Nov 11, 2019

QWYNG commented Nov 11, 2019

Choose a reason for hiding this comment

dgmstuart left a comment

Choose a reason for hiding this comment

QWYNG commented Nov 12, 2019

dgmstuart Nov 11, 2019 •

edited

Loading

QWYNG Nov 11, 2019 •

edited

Loading