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 Paranoia to work with validates_uniquness_of (rails3 branch) #122

Merged
merged 3 commits into from
Jun 20, 2015

Conversation

codeodor
Copy link
Contributor

For the rails3 branch, in pursuit of #114

@radar
Copy link
Collaborator

radar commented Mar 25, 2014

@codeodor The build on this PR passes. Does that mean that you've fixed it?

@codeodor
Copy link
Contributor Author

It was #121 that was failing, I guess. Locally it passes all the tests on Rails 4.0.2 and Ruby 2.0, but the build shows failures due to assert_nothing_raised not being there in Rails 4.1, and something wrong with test_delete_behavior_for_plain_models_callbacks.

For the failing test, I have no idea why it would pass locally and on the 4.1 tests. For the error, I think that test needs to be rewritten (but has nothing to do with this PR).

And actually, now that I look at the most recent build it appears to have the same two issues, so I'd say both are ready to go, with the two tests being a separate issue.

@codeodor
Copy link
Contributor Author

Oh, and as far as "fixed it" then yes, I did fix the issue raised in #114 on both the Rails 3 branch (this PR) and the Rails 4 branch (#121).

At first I thought by "fixed it" you might have been referring to the build failures on the Rails 4 branch that I mentioned in the original issue.

johnnylai added a commit to johnnylai/paranoia that referenced this pull request Mar 13, 2015
johnnylai added a commit to eduvo/paranoia that referenced this pull request Mar 13, 2015
@joshuapinter
Copy link
Contributor

Any status update as to when this might get merged in?

Thanks!

@radar
Copy link
Collaborator

radar commented Jun 17, 2015

screen shot 2015-06-18 at 7 31 24 am

PR will need to be rebased on the rails3 branch before it can be merged in.

@codeodor
Copy link
Contributor Author

I will rebase it when I get a chance.

@codeodor
Copy link
Contributor Author

Ok, all done.

@codeodor
Copy link
Contributor Author

@radar I also rebased #121 for the Rails 4 branch.

@codeodor codeodor changed the title Failing test that shows Paranoia not working with validates_uniquness_of (rails3 branch) Fix Paranoia to work with validates_uniquness_of (rails3 branch) Jun 18, 2015
@codeodor
Copy link
Contributor Author

This will also fix #175

@radar
Copy link
Collaborator

radar commented Jun 20, 2015

Travis is happy. I am happy. Let's merge it.

radar added a commit that referenced this pull request Jun 20, 2015
Fix Paranoia to work with validates_uniquness_of (rails3 branch)
@radar radar merged commit 926c4ff into rubysherpas:rails3 Jun 20, 2015
@codeodor
Copy link
Contributor Author

👍 Thank you!

@joshuapinter
Copy link
Contributor

Not sure of the details yet, but after using this merged version my uniqueness validation stopped working. It allows me to add multiple records with the same value.

I know as part of this PR there's a test to ensure that validates_uniqueness_of ignores rows where deleted_at is not NULL, but is there a complimentary test to ensure that validates_uniqueness_of still works as it should?

@joshuapinter
Copy link
Contributor

So, change:

def test_validates_uniqueness_only_checks_non_deleted_records
  a = Employer.create!(name: "A")
  a.destroy
  b = Employer.new(name: "A")
  assert b.valid?
end

To this:

def test_validates_uniqueness_only_checks_non_deleted_records
  a = Employer.create!(name: "A")
  b = Employer.new(name: "A")

  assert_not b.valid?

  a.destroy
  assert b.valid?
end

codeodor added a commit to codeodor/paranoia that referenced this pull request Jun 21, 2015
@codeodor
Copy link
Contributor Author

Created #246 to deal with that. Thanks @joshuapinter

radar added a commit that referenced this pull request Jun 22, 2015
Fix for #122, #121 and #175 on Rails 4 branch
radar added a commit that referenced this pull request Jun 22, 2015
Fix "normal" validates_uniqueness_of after #122 broke it
@joshuapinter
Copy link
Contributor

👍 Beauty. Thanks @codeodor.

@noahmatisoff
Copy link
Contributor

Had same problem, updated gem, fixed it. Thanks.

@ioev
Copy link

ioev commented Jun 7, 2016

Looks like this patch will raise an exception for any model that does not "acts_as_paranoid", as they will not have a paranoia_column defined. I fixed this locally with:

klass.paranoid? ? relation.and(klass.arel_table[klass.paranoia_column].eq(nil)) : relation

@codeodor
Copy link
Contributor Author

codeodor commented Jun 7, 2016

@ioev is that what #286 is for?

Edit: oh, probably not. Looks like they just revert it to its old broken behavior instead of the new broken behavior.

@ioev
Copy link

ioev commented Jun 7, 2016

I'm not sure, but I don't think so. I'm assuming that the rails 4 handles this in a different way, and so this bit of code became unnecessary?

Sorry, I should have been more specific and mentioned that I'm looking at code from the Rails 3 branch, as that is what the project I'm working on is.

@codeodor
Copy link
Contributor Author

codeodor commented Jun 7, 2016

I'd encourage a PR. I dunno if they plan to keep up any compatibility with Rails 3, but it definitely was a big oversight on my part in the original PR.

@ioev
Copy link

ioev commented Jun 7, 2016

Thanks, I'll see if I can find the time to put one together.

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.

5 participants