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

Allow accessing deleted associations #3

Merged
merged 4 commits into from
Sep 26, 2014
Merged

Conversation

dasch
Copy link
Contributor

@dasch dasch commented May 7, 2014

Adds a with_deleted_associations method that returns the object in a wrapper that checks all method calls. Those that correspond to associations are modified such that deleted associated records are included in the result.

@bastien please review.

@bquorning
Copy link
Member

This used to be such a nice and simple library…

Before going too far with this, please make sure that it works on #2 as well.

@dasch
Copy link
Contributor Author

dasch commented May 8, 2014

Fucking edge cases...

We're trying to avoid having to merge this in.

@jbdietrich
Copy link

@dasch could I convince you to resurrect this for: https://zendesk.atlassian.net/browse/HC-2710? @vkmita has a different approach as well, here: #4

@vkmita
Copy link

vkmita commented Sep 22, 2014

Hah I remember hacking that. It was fun.

@dasch dasch force-pushed the dasch/deleted-associations branch 2 times, most recently from 87fbfc8 to 7032179 Compare September 25, 2014 11:49
Adds a `with_deleted_associations` method that returns the object in a
wrapper that checks all method calls. Those that correspond to
associations are modified such that deleted associated records are
included in the result.
@dasch dasch force-pushed the dasch/deleted-associations branch from 7032179 to 698ea2b Compare September 25, 2014 11:50

def method_missing(name, *args, &block)
delegate_to_record(name) { @record.public_send(name, *args, &block) }
end
Copy link
Contributor

Choose a reason for hiding this comment

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

😢 😿 😢 😿 😢 😿

Copy link
Contributor

Choose a reason for hiding this comment

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

As recently as yesterday I ran into problems because of meta magic like this in Dynamic Content. Basically breaking ActiveModel::AttributeMethods and ActiveRecord::AttributeMethods reflection because we catch methods first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a war story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if DC started using BasicObject those issues would go away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have my share of scar tissue.

@jamiehodge
Copy link

@dasch why aren't you using SimpleDelegator? Currently, you don't, for example, support respond_to?.

@jamiehodge
Copy link

With some reluctance: 👍

I really wish we weren't unwrapping (through wrapping) our default scopes. Maybe this should be hard-coded at the model layer. Calling extend on instances is no longer prohibitive, due to improvements on GC. Yadayada.

@bastien
Copy link

bastien commented Sep 26, 2014

👍

dasch added a commit that referenced this pull request Sep 26, 2014
Allow accessing deleted associations
@dasch dasch merged commit 15c0b33 into master Sep 26, 2014
@dasch dasch deleted the dasch/deleted-associations branch September 26, 2014 09:09
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.

7 participants