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 uniqueness validation after source code changes #78

Closed
wants to merge 0 commits into from
Closed

Fix uniqueness validation after source code changes #78

wants to merge 0 commits into from

Conversation

hgani
Copy link
Contributor

@hgani hgani commented Nov 11, 2016

This fixes an issue that has been reported multiple times:
#17
#24

There has been a suggested workaround, but the workaround doesn't work for me.

This pull request aims to fix this problem properly so no workaround is needed.

Basically what it does is use class name as the key for expose instead of using the class itself, which will break when Rails autoloads the class, because then the existing key/class will not match with the reloaded class.

Thanks

@hgani
Copy link
Contributor Author

hgani commented Nov 11, 2016

FYI, I will look into fixing the failed tests once I get a green light for pull request.

@jamesmk
Copy link
Collaborator

jamesmk commented Nov 11, 2016

Thanks for the PR @hgani!

I don't see anything wrong with this solution. I can take another look when you get the tests passing.

thanks!

@hgani
Copy link
Contributor Author

hgani commented Nov 11, 2016

@jamesmk

When running some of the tests (e.g. rspec ./spec/models/validation_spec.rb:30), I got the following error.

ActiveRecord::StatementInvalid:
  SQLite3::SQLException: no such table: users: SELECT "users".* FROM "users"
# --- Caused by: ---
# SQLite3::SQLException:
#   no such table: users
#   ../.rvm/gems/ruby-2.3.0/gems/sqlite3-1.3.12/lib/sqlite3/database.rb:91:in `initialize'

Do you know why this happens? Do I need to run something before running the test?

@jamesmk
Copy link
Collaborator

jamesmk commented Nov 11, 2016

@hgani did you create the test DB and load the schema? You may have to run rake db:setup RAILS_ENV=test.

thanks

@hgani
Copy link
Contributor Author

hgani commented Nov 11, 2016

@jamesmk yes that's it, I haven't setup the DB. Thanks.

I've pushed a bug fix commit so tests are all passing now, with the exception of one test rspec ./spec/confirmation_validator_spec.rb:28, which I don't think is related to my changes and I am not sure how to fix properly.

Please review.

Thanks

attrs.concat(attributes).uniq!
end

def exposed
@@exposed
@@exposed.map { |k, v| [k.constantize, v] }.to_h
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hgani It looks like you added this to get some tests to pass, but I'm thinking it may be the tests that need to be adjusted. First, it looks like Ruby 1.9.3 doesn't like the usage of to_h here (whether or not we should be still supporting 1.9.3 is another topic). When I reverted this change and updated the tests to use a stringed key, instead of class, they passed.

Judge.config.exposed['User'].should eql [:username, :email]

I don't think Judge.config.exposed is part of the documented external API and the only usage internal.

@joecorcoran Any opinions here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds better to change the test, yep. Aside: does this PR mean that either a constant or a string can be used?

Copy link
Contributor Author

@hgani hgani Nov 13, 2016

Choose a reason for hiding this comment

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

Okay, sounds good to me that we fix the tests instead, given that exposed is an internal API so it doesn't matter that it's not returning the same type (i.e. string) as what it was getting (i.e. class).

@joecorcoran the PR aims to maintain the exact same usage, so it's meant to be used only with a model class, e.g. expose Post, :title, :body. Developers don't need to know that keys are stored as strings now.

@jamesmk Just want to confirm so we don't wait on each other, I assume that you'll be taking this PR forward? Let me know if there's anything I can help.

Thanks

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