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

Fix #previously_new_record? on destroyed records #48796

Merged

Conversation

adrianna-chang-shopify
Copy link
Contributor

Motivation / Background

Ref: #48794
cc @eizengan

Based on the documentation for #previously_new_record?:

Returns true if this object was just created – that is, prior to the last save, the object didn't exist in the database and new_record? would have returned true.

IMO a record that was previously created and has since been destroyed and removed from the database should no longer be considered "just created", although one could argue that a DELETE is different from a "save".

Detail

Set @previously_new_record to false in #destroy and #delete methods.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@eizengan
Copy link

eizengan commented Jul 24, 2023

IMO a record that was previously created and has since been destroyed and removed from the database should no longer be considered "just created", although one could argue that a DELETE is different from a "save".

My take as well.

prior to the last save, the object didn't exist in the database and new_record? would have returned true

feels less useful than e.g.

prior to the last call which permanently modified the object, the object didn't exist in the database and new_record? would have returned true

Copy link
Contributor

@nvasilevski nvasilevski left a comment

Choose a reason for hiding this comment

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

Makes sense 👍

activerecord/test/cases/base_test.rb Outdated Show resolved Hide resolved
activerecord/test/cases/base_test.rb Outdated Show resolved Hide resolved
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

I thought about this for a bit and I'm kind of on the fence - it would depend whether we think a save and delete are different for this context. I think it is since a double save makes previously_new_record? return false. I think the docs should be updated though to reflect this change. Also with the slight change in behavior that might be surprising, can you add a CHANGELOG entry too?

Ref: rails#48794

`#previously_new_record?` should return false for records that are
created and then destroyed.
@adrianna-chang-shopify
Copy link
Contributor Author

it would depend whether we think a save and delete are different for this context.

Yeah, I was on the fence about this too. I ended up leaning towards these changes because the primary use case around #previously_new_record? seems to be for triggering callbacks on records that were newly created. Even though "save" and "delete" may be considered different in this context, we'd want to prevent a callback from firing off on the first "create" and then again on a destroy, as @eizengan demonstrated in the example in his issue.

I've updated the docs and added a CHANGELOG entry -- let me know what you think 😄

@eileencodes eileencodes merged commit 0c78ab2 into rails:main Jul 25, 2023
9 checks passed
@eileencodes eileencodes deleted the ac-fix-previously-new-record branch July 25, 2023 20:56
rafaelfranca pushed a commit that referenced this pull request Aug 2, 2023
…ly-new-record

Fix `#previously_new_record?` on destroyed records
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants