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 for issue #594, reifying sub-classed models that use STI #1106

Closed
wants to merge 2 commits into from

Conversation

lorint
Copy link
Contributor

@lorint lorint commented Jun 5, 2018

In order to properly reify a version of a model using STI, item_type refers to the actual class instead of base_class. The previously failing example in person_spec.rb now passes, so that test has been brought back. In addition to the changes here, the five reifiers in the gem [paper_trail-association_tracking] that refer to base_class also need to be updated. See #5 for more details.

@jaredbeck
Copy link
Member

Hi @lorint, thanks for trying to fix this difficult, ongoing issue.

It sounds like your goal is to finish fixing #594, is that correct? #594 is an issue with association reification, and the entire experimental association tracking feature has recently been moved to a separate gem, paper_trail-association_tracking. So, I think you you should move this PR there. I'll close this for now, but let me know if, after talking to @westonganger, you determine that it can't be (entirely) fixed there. Thanks!

@jaredbeck jaredbeck closed this Jun 5, 2018
@lorint
Copy link
Contributor Author

lorint commented Jun 6, 2018

Although fixing the reification is a nice side benefit, my core goal is actually to improve the core PT so it accurately represents STI model names. We have a medical app with a fair amount of sub-classed models, and instead of item_type always being "Entity" or "Person", it's imperative for us that it be shown as "Hospital" or "Clinic", "Doctor" or "Patient". We can solve this by managing our own sti_item_type column, but would greatly prefer to have the actual subclassed name directly represented.

Maybe there's a different issue out there where this interest is better expressed, so #594 can be addressed in Weston's PT_AT. We had found it to be a happy side benefit that person_spec.rb worked fine after implementing our update. Over in PT_AT the only changes are to have the reifiers use just "class" instead of "class.base_class".

@westonganger
Copy link
Contributor

This PR should not be closed. If you look at the related PR in Association Tracking #5 like @lorint said only changes are to have the reifiers use just "class" instead of "class.base_class" this really does seem to be a core PT issue.

@lorint
Copy link
Contributor Author

lorint commented Jun 6, 2018

Had made a couple more Rubocop tweaks and updated the CelebrityFamily spec on my fork, but strangely the squashed commit doesn't show up here. To make things easy I can open another PR if you guys like.

@jaredbeck
Copy link
Member

We have a medical app with a fair amount of sub-classed models, and instead of item_type always being "Entity" or "Person", it's imperative for us that it be shown as "Hospital" or "Clinic", "Doctor" or "Patient".

Thanks, this helps me understand the issue.

We can solve this by managing our own sti_item_type column, but would greatly prefer to have the actual subclassed name directly represented.

Glad you found a workaround.

.. only changes are to have the reifiers use just "class" instead of "class.base_class" this really does seem to be a core PT issue.

Cool, thanks for the clarification.

To make things easy I can open another PR if you guys like.

I guess you have to, because GH won't let me reopen this. It says "The master branch was force-pushed or recreated." I've never seen that before ..

@jaredbeck
Copy link
Member

I guess you have to, because GH won't let me reopen this. It says "The master branch was force-pushed or recreated." I've never seen that before ..

Instead of using your own master branch (lorint/master) you might want to create a topic branch (lorint/my_topic).

@lorint
Copy link
Contributor Author

lorint commented Jun 7, 2018

OK sounds good -- I'll put things in lorint/item_type_gets_class_name.

Thanks much!

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.

None yet

3 participants