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

Comment owner must be valid? #45

Open
kurtk opened this issue Dec 18, 2014 · 2 comments
Open

Comment owner must be valid? #45

kurtk opened this issue Dec 18, 2014 · 2 comments

Comments

@kurtk
Copy link

kurtk commented Dec 18, 2014

opinio/lib/opinio/opinio_model/validations.rb
validates :owner, :presence => true, :associated => true

Using just :presence => true on the owner checks the table to see if it exists, at least while using Rails 3.2.19 and testing it in the console. Trying to use an owner_id that doesn't exist will fail validation.

Using :associated => true makes it more difficult to add a new required field. For example, a phone number is now required, but forcing existing users to immediately provide one so their user record is considered valid and they can comment again is probably overkill.

Perhaps this has changed in Rails? I've read conflicting questions / answers on the topic ...

@Draiken
Copy link
Owner

Draiken commented Jan 9, 2015

I agree with you in parts. It's not that it's overkill but it is enforcing
something you might not want on your application.

As for the owner_I'd not being checked it is how I intended it to be. I
need a user so I can fetch it and do stuff with. I can't work only the id.
The id validation is done on the database level.

I suggest you remove that associated validation on your fork and if you
feel like it, submit a pull request and I'll merge it in :)

On Thu, Dec 18, 2014, 1:39 PM Kurt Kowitz notifications@github.com wrote:

opinio/lib/opinio/opinio_model/validations.rb
validates :owner, :presence => true, :associated => true

Using just :presence => true on the owner checks the table to see if it
exists, at least while using Rails 3.2.19 and testing it in the console.
Trying to use an owner_id that doesn't exist will fail validation.

Using :associated => true makes it more difficult to add a new required
field. For example, a phone number is now required, but forcing existing
users to immediately provide one so their user record is considered valid
and they can comment again is probably overkill.

Perhaps this has changed in Rails? I've read conflicting questions /
answers on the topic ...


Reply to this email directly or view it on GitHub
#45.

@Draiken
Copy link
Owner

Draiken commented Jan 9, 2015

Oh boy, I'm sorry for the delay... Found that response on my email drafts (should've been sent the same day you opened the issue but apparently it didn't)

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

No branches or pull requests

2 participants