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

Update callbacks with upstream improvements #1028

Merged
merged 5 commits into from
Mar 21, 2016
Merged

Update callbacks with upstream improvements #1028

merged 5 commits into from
Mar 21, 2016

Conversation

cbeer
Copy link
Member

@cbeer cbeer commented Mar 18, 2016

No description provided.

end

def _update_record(_options = {})
return false unless content_changed?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 3f213e4, I've changed this from true to false (to cancel after_update callbacks, like we're doing above). It seems like this was introduced in c76c6ca, but it's unclear why create and update are different. @hectorcorrea @jcoyne?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, it should return true if there is nothing to save as ActiveRecord does. See https://gist.github.com/jcoyne/074969310111653ae02c

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create record is different because it would not save anything if there is no content to save, that would effectively be an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbeer I don't really care what _update_record returns as long as save() returns true in this instance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 I can work with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at how Rails handles callbacks -- they don't suppress the after_update hook if nothing changed (and, presumably, neither do we, in AF::Persistence). I've updated this behavior in 5bae75f.

@cbeer cbeer force-pushed the update-callbacks branch 3 times, most recently from 3f213e4 to c5f5ccb Compare March 19, 2016 02:11
#
# There's a series of callbacks associated with #destroy!. If the
# <tt>before_destroy</tt> callback throws +:abort+ the action is cancelled
# and #destroy! raises ActiveRecord::RecordNotDestroyed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this line and the next to ActiveFedora instead of ActiveRecord

jcoyne added a commit that referenced this pull request Mar 21, 2016
Update callbacks with upstream improvements
@jcoyne jcoyne merged commit d9e3c9a into master Mar 21, 2016
@jcoyne jcoyne deleted the update-callbacks branch March 21, 2016 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants