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

#77 validation added. parent presence validation, title presence, uniqueness... #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

angdev
Copy link
Member

@angdev angdev commented Aug 27, 2014

... (especially, project) (w/o join table)
#77

title이랑 상위 객체 (history면 속해 있는 project) 가 있는지 없는지 검사하는 부분을 추가했고, project의 경우는 title이 unique 해야하는 점을 추가했습니다. project title에는 조건을 달아서 정규식 체크도 해야할 것 같습니다.

한편 일반적으로 상위 객체에 대한 validation을 넣는지랑 join table에는 어떻게 넣어주는게 보통인지가 궁금합니다. (질문을 먼저 이슈에 하고 작업하는게 좋았으려나요?)

@shaynekang
Copy link
Contributor

수고하셨습니다~

상위 객체에 대한 validation을 하고 싶다면 Custom Validation을 사용해야 합니다. ㅎㅎ

Validation은 잘 거신 것 같은데, 이 기능을 추가함으로서 생기는 에러도 함께 처리해 줘야 할 것 같습니다. 아마 폼 입력과 관련한 많은 부분에서 에러가 날 것 같네요. ㅎㅎ

@minhyeok4dev
Copy link

확인했습니다. 근데 User모델에 거는 validation만큼 마이그레이션도 조정해줘야 하지 않을까요? (null: false 등)

@MoojinChae
Copy link
Contributor

확인 했습니다!

@yhoonkim
Copy link
Contributor

확인했습니다.
입력값에 띄어쓰기나 .이 들어갈 경우 validation오류를 낼지 아니면 알아서 replace할지 정해야 할 것 같습니다.
route에서 title을 쓰다보니까 .이 들어가면 망함...

@shaynekang
Copy link
Contributor

@yhoonkim 사용자 Nickname에 '.'이 들어가도 라우팅에 문제 없게 해 주는 기능을 @MujinChae 멘티님이 추가했을 겁니다. 어디 브랜치였는지는 기억 안 나지만...(..)

@MoojinChae
Copy link
Contributor

@yhoonkim 지금 마스터 브랜치에 user profile 나오는 부분에 있을겁니다 ㅎㅎ

@angdev
Copy link
Member Author

angdev commented Aug 28, 2014

@shaynekang 상위 객체의 존재 여부는 persence를 걸면 될 것 같고, 상위 객체에 validation이 잘 짜여져있다고 할 때는 딱히 걸 일이 없을 것 같고, 이 상위 객체에 속해도 되는지 체크할 때 Custom Validation을 쓰는 경우가 떠오르는데 이게 맞나요?

한편, 상위 객체 혹은 하위 객체에 validates-associated 를 거는 경우는 보통 어떤 경우가 있는지 궁금합니다. 생각해보면 상위 객체에 걸 일이 있을 것 같지는 않지만요.

@shaynekang
Copy link
Contributor

@minhyeok92 분야에 따라 조금 다른데, 저는 데이터를 정말 깐간하게 검증해야 하는 분야가 아니라면(예: 의료, 증권, 결제) validation을 2중 ~ 3중으로 걸 필요는 없다고 봅니다. 오히려 생산성이 떨어지는 역효과가 난다고 생각해요. DB에 contraint는 걸어주지 않아도 무방할 것 같습니다. ㅎㅎ

@fegs 아, 제가 코멘트를 잘못 이해했습니다. 저는 상위 객체에 validation을 넣는 방법에 대해서 질문하신 줄 알았네요. ㅎㅎ 경우에 따라서는 상위 객체가 반드시 들어가야 하는 경우도 있다고 생각합니다. 가령 지금의 Project같은 경우, 적어도 한 명의 Owner(User)가 있어야겠죠. 이런 경우는 validates-associated가 필요하다고 생각합니다.ㅎㅎ

@angdev
Copy link
Member Author

angdev commented Aug 28, 2014

현재는 history, todo에는 user, project presence 조건이 걸려있는데 이를 validates-associated로도 처리를 할 수 있다는 말씀이신가요?

@shaynekang
Copy link
Contributor

아, 맞다. validates_associated는 연관 모델의 데이터 검증을 하는거였죠?
음... 이걸 사용하는 경우가 있을겁니다. 가령 모델의 데이터를 변경할 때, 연관 모델의 데이터도 함께 변경해줘야 하는 경우도 있거든요. ㅎㅎ
다만, 저같은 경우 이런 상황에서는 Form Object를 사용합니다. 두 개 이상의 모델을 동시에 변경할 경우, form_forform_tag는 굉장히 불편하거든요.(정확히는 fields_foraccepts_nested_attributes_for 조합이 참 좋지 않습니다;)

@shaynekang
Copy link
Contributor

레일즈캐스트 자료는 결제를 해야 되네요.(..)
7 Patterns to Refactor Fat ActiveRecord Models에 Form Object에 관련한 내용이 있습니다. ㅎㅎ

@angdev
Copy link
Member Author

angdev commented Aug 29, 2014

@shaynekang 음.. 조인 테이블의 경우는 별도로 validation을 걸지는 않나요? (..)

@shaynekang
Copy link
Contributor

음... 조인 테이블 거는 경우는 잘 못 봤네요. ㅎㄷ

저는 사실 Validation은 정말 필요한 것만 걸면 된다는 주의거든요. ㅋ(데이터가 중요한 의료, 금융, 결제 등이 아니라면)

만약 조인 테이블이나 두 개 이상의 테이블을 동시에 Create(or Update)하는 상황이 생긴다면, Form Object로 다 해결할 것 같아요. ㅎㅎ

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