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

Persist default values of aliased attributes #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonathanhefner
Copy link
Contributor

1f150f6 made it so that default values are always persisted. This commit adds support for aliased attributes to that change.

@@ -958,19 +958,26 @@
describe DirtyTrackingModel do
it 'stores the default on creation' do
model = DirtyTrackingModel.create!
model = DirtyTrackingModel.find(model.id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This find (or a reload) is required to exercise the fix from #95. (i.e. Without a reload, this test would not have failed prior to #95 either.)

@byroot
Copy link
Owner

byroot commented Nov 3, 2022

I considered this but wasn't expecting anyone to use aliased attributes in this case.

Is this something you are doing?

super | self.class.typed_stores.keys.map(&:to_s).select do |store|
@attributes.key?(store) && @attributes[store].value_before_type_cast.blank?
super | self.class.typed_store_attribute_names.select do |store|
read_attribute_before_type_cast(store).blank?
Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I was checking @attribute.key? in case it's a partially loaded model (I should have added a test for it TBH)

@jonathanhefner
Copy link
Contributor Author

Is this something you are doing?

No, I just wanted to make sure all the cases were covered. 😄 I wasn't sure how much aliased attributes were used at Shopify. But if they are explicitly not supported, I think it's fine.

@byroot
Copy link
Owner

byroot commented Nov 3, 2022

But if they are explicitly not supported, I think it's fine.

Yeah, I don't really see a use case for it to be honest. That said I initially took that decision in #94 because it was in much more of a hot path, it probably wouldn't matter here. We could also cache (or precompute) that array on the class.

1f150f6 made it so that default values
are always persisted.  This commit adds support for aliased attributes
to that change.
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.

2 participants