Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `ActiveRecord::Base.suppress` to allow save events to be suppressed within a specified block. #18910

Merged
merged 1 commit into from Feb 19, 2015

Conversation

@perceptec
Copy link
Contributor

@perceptec perceptec commented Feb 12, 2015

Fixes #18847.

@georgeclaghorn
georgeclaghorn reviewed Feb 12, 2015
View changes
activerecord/test/cases/suppressor_test.rb Outdated
def test_suppresses_creation_of_related_record_on_after_create_callback
count = Notification.count
Notification.suppress { UserWithNotification.create! }
assert_equal count, Notification.count

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 12, 2015
Member

Should assert that other models used in the suppress block -- in this case, User -- are unaffected:

assert_difference -> { User.count } do
assert_no_difference -> { Notification.count } do
  Notification.suppress { UserWithNotification.create! }
end
end
@georgeclaghorn
georgeclaghorn reviewed Feb 12, 2015
View changes
activerecord/CHANGELOG.md Outdated

Closes #18847.

*Michael Ryan*

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 12, 2015
Member

New changelog entries are added to the top of the file.

@georgeclaghorn
georgeclaghorn reviewed Feb 12, 2015
View changes
activerecord/lib/active_record/suppressor.rb Outdated
extend ActiveSupport::Concern

module ClassMethods
# Suppresses creation of events for the duration of the block.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 12, 2015
Member

The docs could use an example demonstrating usage.

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

That sentence could also use a tune up. Picture a person who didn't work on this code, what would "creation of events" mean to them? This code really suppresses Active Record save callbacks.

@kaspth
kaspth reviewed Feb 12, 2015
View changes
activerecord/lib/active_record/suppressor.rb Outdated
#
# See the documentation of <tt>ActiveSupport::PerThreadRegistry</tt>
# for further details.
class SuppressorRegistry

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

This class isn't for public use, add a # :nodoc: to the end of the line. Then remove the documentation about looking up the per thread registry.

@kaspth kaspth added the activerecord label Feb 12, 2015
@kaspth kaspth added this to the 5.0.0 milestone Feb 12, 2015
@perceptec
Copy link
Contributor Author

@perceptec perceptec commented Feb 12, 2015

Thanks for the feedback, @georgeclaghorn and @kaspth.

@kaspth
kaspth reviewed Feb 12, 2015
View changes
activerecord/lib/active_record/suppressor.rb Outdated
@@ -0,0 +1,57 @@
module ActiveRecord
# ActiveRecord::Suppressor allows save events to be suppressed within a

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

I still think callbacks is clearer than events here 😄

This comment has been minimized.

@perceptec

perceptec Feb 12, 2015
Author Contributor

That's more along the lines of what I originally had and again included, but lost when I moved the comment up to the class level. There's a stray line break in there too, so I'll fix once there's feedback on this commit.

If you want you can blame the original author here: #18847. 😄

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

Ah, let's give him a pass. But just this once 😉

@kaspth
kaspth reviewed Feb 12, 2015
View changes
activerecord/lib/active_record/suppressor.rb Outdated
ensure
SuppressorRegistry.suppressed[self.name] = false
end

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

✂️ this line

@kaspth
kaspth reviewed Feb 12, 2015
View changes
activerecord/lib/active_record/suppressor.rb Outdated
def save!(*args)
SuppressorRegistry.suppressed[self.class.name] ? self : super
end

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

✂️ here too

@kaspth
kaspth reviewed Feb 12, 2015
View changes
activerecord/lib/active_record/suppressor.rb Outdated
class SuppressorRegistry # :nodoc:
extend ActiveSupport::PerThreadRegistry

attr_accessor :suppressed

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

This should just be an attr_reader now.

@kaspth
kaspth reviewed Feb 12, 2015
View changes
activerecord/lib/active_record/suppressor.rb Outdated

module ClassMethods
def suppress(&block)
SuppressorRegistry.suppressed[self.name] = true

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

You can rely on implicit self here and just say name.

@kaspth
kaspth reviewed Feb 12, 2015
View changes
activerecord/test/cases/suppressor_test.rb Outdated

class SuppressorTest < ActiveRecord::TestCase

def test_executes_callbacks_without_suppression_setting

This comment has been minimized.

@kaspth

kaspth Feb 12, 2015
Member

Doesn't this test practically the same thing as the last test in this file?

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 12, 2015

Left some notes, otherwise this looks good. Thanks for working on it 😄

Just so you know, it doesn't seem like GitHub supports "closes #x" in the Pull Request title: https://github.com/blog/1506-closing-issues-via-pull-requests. But the issue will close when this is merged, because you've put the issue number in your commit message.

@perceptec perceptec changed the title Add `ActiveRecord::Base.suppress` to allow save events to be suppressed within a specified block. Closes #18847. Add `ActiveRecord::Base.suppress` to allow save events to be suppressed within a specified block. Feb 12, 2015
@mcmire
Copy link
Contributor

@mcmire mcmire commented Feb 13, 2015

Does .suppress cancel all callbacks, or only certain callbacks?

@georgeclaghorn
georgeclaghorn reviewed Feb 14, 2015
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,34 @@
* Add `ActiveRecord::Base.suppress` to allow save callbacks to be suppressed

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 14, 2015
Member

It prevents saves, not save callbacks. It is useful to prevent undesirable saves happening in callbacks.

This comment has been minimized.

@kaspth

kaspth Feb 14, 2015
Member

My bad, I suggested this. My mental model was off on what this actually does.

@mcmire
Copy link
Contributor

@mcmire mcmire commented Feb 15, 2015

I don't really agree with this change. First, I don't think that there's a legitimate use case for this, and second, I don't think this is the correct solution to this problem.

I have rarely ever seen a case where I've added a callback that involves saving another record and then for some reason I don't want that save to occur. I can see this happening in tests, but that is easily solved by stubbing the method that performs the save (Notification.create!, in the example). Now, if I did have a need to prevent this from happening in production code, it would actually tell me that these two components are too tightly coupled, and it would encourage me to break them apart into a higher level object (CreateCommentWithNotification, for instance) so that I had the freedom to only create the comment if I wanted to. In other words, I think this change encourages bad design.

Even if that were not the case, I think that the usage is weird. As a reader of this code, it seems like the context around the use of suppress would not be immediately clear. In other words, if I saw the Notification.suppress line, I'd have to know that it's tied back to the after_create... inside of Comment. I would immediately ask myself, if we are modifying the behavior of Comment, why should we have to tell Notification to do something? We're reaching inside of Notification from a completely different class. On top of this, this seems like a shotgun solution. What if there are other things happening in the block that also save notifications? Wouldn't it be better to be able to ask Comment to disable the callback in question instead of having to know what that callback does and disable its contents?

Anyway... that's a bit of a moot point, because honestly I think both problems can be solved by making a separate class that encapsulates creating a comment and also creating a notification. Is it one more class? Sure, but you don't have to disable anything, and the intention is perfectly clear to other people reading the code.

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 15, 2015

@mcmire please see the original issue #18847 for a use case and justification.

@perceptec can you add "Fixes #18847" to the description, so others will have an easier time evaluating this? 😄

@dhh
Copy link
Member

@dhh dhh commented Feb 18, 2015

Looking good @perceptec! Did @kaspth or @georgeclaghorn have any other comments, or are we good to merge?

@mcmire What @kaspth said. Use case is explained in the original issue.

@rafaelfranca
rafaelfranca reviewed Feb 18, 2015
View changes
activerecord/CHANGELOG.md Outdated
end
end

Closes #18847.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 18, 2015
Member

You don't need to point to the issue at the changelog since it is a new feature

@rafaelfranca
rafaelfranca reviewed Feb 18, 2015
View changes
activerecord/lib/active_record/suppressor.rb Outdated
@@ -0,0 +1,55 @@
module ActiveRecord
# ActiveRecord::Suppressor allows save callbacks to be suppressed within a

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 18, 2015
Member

Actually it suppers the action of saving a record, not just the callback.

@georgeclaghorn
Copy link
Member

@georgeclaghorn georgeclaghorn commented Feb 18, 2015

Looks good to me!

@dhh
Copy link
Member

@dhh dhh commented Feb 18, 2015

@mcmire The general principle I apply for this design is "the exception must carry the weight of its own work". If you create a new CreateCommentWithNotifications object, you're pushing the weight of the work onto the default case. That's not proportional in my book.

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 18, 2015

Once @georgeclaghorn's and @rafaelfranca's concerns are addressed then 👍

Oh, and we'll need a rebase @perceptec 😄

@perceptec
Copy link
Contributor Author

@perceptec perceptec commented Feb 18, 2015

This one's been slippery to define. "save callbacks" was intended more in a compound sense (if a bit of a cop-out).

Will this work? (Since there's already been discussion/confusion around the intended functionality.)

Add ActiveRecord::Base.suppress to prevent the receiver from being saved during callbacks generated by a given block.

No matter what we come up with for a definition, I think the code itself will be more illustrative...

@georgeclaghorn
Copy link
Member

@georgeclaghorn georgeclaghorn commented Feb 18, 2015

Add ActiveRecord::Base.suppress to prevent the receiver from being saved during callbacks generated by a given block.

It's not necessary to mention callbacks at all. ActiveRecord::Base.suppress prevents saves for objects of the receiving class inside the block, regardless of where the saves occur. This still suggests that it only suppresses saves in callbacks.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 18, 2015

Maybe Add ActiveRecord::Base.suppress to prevent the receiver from being saved during the given block.

@perceptec
Copy link
Contributor Author

@perceptec perceptec commented Feb 18, 2015

@georgeclaghorn True, is everybody okay with @rafaelfranca's edit?

@dhh
Copy link
Member

@dhh dhh commented Feb 18, 2015

What George said. Let’s just focus on suppress = no saving of records.

On Feb 18, 2015, at 13:26, George Claghorn notifications@github.com wrote:

Add ActiveRecord::Base.suppress to prevent the receiver from being saved during callbacks generated by a given block.

It's not necessary to mention callbacks at all. ActiveRecord::Base.suppress prevents saves for objects of the receiving class inside the block, regardless of where the saves occur. This still suggests that it only suppresses saves in callbacks.


Reply to this email directly or view it on GitHub.

@georgeclaghorn
Copy link
Member

@georgeclaghorn georgeclaghorn commented Feb 18, 2015

👍

belongs_to :commentable, polymorphic: true
after_create -> { Notification.create! comment: self,
recipients: commentable.recipients }
end

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 19, 2015
Member

Indent code examples in this file by an extra four spaces.

# belongs_to :commentable, polymorphic: true
# after_create -> { Notification.create! comment: self,
# recipients: commentable.recipients }
# end

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 19, 2015
Member

Indent code examples here by an extra two spaces.

require 'models/user'

class SuppressorTest < ActiveRecord::TestCase

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 19, 2015
Member

Delete this empty line.

@rafaelfranca rafaelfranca merged commit b9a1e9a into rails:master Feb 19, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rafaelfranca pushed a commit that referenced this pull request Feb 19, 2015
Add `ActiveRecord::Base.suppress` to allow save events to be suppressed within a specified block.
@kaspth
Copy link
Member

@kaspth kaspth commented Feb 19, 2015

Boom! Well done @perceptec 😄

Kasper

Den 19/02/2015 kl. 12.08 skrev Rafael Mendonça França notifications@github.com:

Merged #18910.


Reply to this email directly or view it on GitHub.

@perceptec perceptec deleted the perceptec:add_suppressor branch Feb 19, 2015
@dhh
Copy link
Member

@dhh dhh commented Feb 19, 2015

Great job 👏

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
@rails rails locked and limited conversation to collaborators May 23, 2016
@jeremy
Copy link
Member

@jeremy jeremy commented May 23, 2016

(Removed a disparaging comment about the feature that was thoughtlessly posted on the implementation work done here. Please contribute to discussion on the original issue #18847.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.