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

Add where_object query to PaperTrail::Version. #380

Merged
merged 1 commit into from
Jun 27, 2014
Merged

Add where_object query to PaperTrail::Version. #380

merged 1 commit into from
Jun 27, 2014

Conversation

JacobEvelyn
Copy link
Contributor

I wasn't able to get this to work with native JSON types (an error will be raised when the query is attempted) due to not being able to accurately query for the full set of possible types (JSON's null, for instance). If anyone is able to give direction on how to make these queries work, please share!

I also wasn't sure if there's a better way to test this functionality; again, feedback welcome!

@batter batter added this to the 3.0.3 milestone Jun 9, 2014
@batter
Copy link
Collaborator

batter commented Jun 11, 2014

@bradleypriest - Any tips for how to make these queries work with the Postgres JSON column type?

@batter
Copy link
Collaborator

batter commented Jun 25, 2014

@JacobEvelyn - Finally getting some time to go through this PR and try to figure out why Travis is failing. It seems like each DB adapter can have slightly different ways of outputting that SQL syntax depending on adapter and Ruby / Arel version, plus I'm thinking these aren't great tests anyways. We really want to test for whether the method can actually locate versions where the serialized object data has values in it matching what you pass in, so I'm going to try to refactor these tests to be something more along the lines of this and see if we can get these tests passing.

I also did some research on Postgres's JSON type and it seems that not all versions of Postgres support querying the JSON objects, so that one might be more tricky. We may have to leave that off for now.

@JacobEvelyn
Copy link
Contributor Author

Thanks @batter. Let me know if I can be helpful. As I mentioned above, I wasn't sure what better way there was to test the functionality with the testing setup in place but I agree it's not really the right approach.

@batter batter merged commit 703ec9a into paper-trail-gem:master Jun 27, 2014
batter added a commit that referenced this pull request Jun 27, 2014
@JacobEvelyn
Copy link
Contributor Author

😄

@panSarin
Copy link

It would be nice to add some documentation for that.

@batter
Copy link
Collaborator

batter commented Sep 11, 2014

Agreed. I'll see what we can do. Anyone is welcome to make a PR too.

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