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

Fixes #15476 - Improve validations for policy #179

Merged
merged 1 commit into from Sep 1, 2016

Conversation

xprazak2
Copy link
Contributor

No description provided.

@shlomizadok
Copy link
Member

@xprazak2 will you please rebase? Thanks

elsif new_record? && !wizard_initiated?
true
else
!id.blank?
Copy link
Member

Choose a reason for hiding this comment

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

should be equal to persisted? I think

@shlomizadok
Copy link
Member

@xprazak2 - please note that tests are failing :/

@wizard_initiated
end

def update_attrs
Copy link
Member

Choose a reason for hiding this comment

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

update_attrs strikes me as too general name (e.g., I expected to find more attributes to update here) where this update deals only with period. Would you consider renaming this method?

@shlomizadok
Copy link
Member

@xprazak2 - Thank you for this improvement. Tested and it works well. I have only one nitpick regarding a method name (let me know what you thing - I may be wrong...)
👍

@xprazak2
Copy link
Contributor Author

xprazak2 commented Sep 1, 2016

I renamed the method to update_period_attrs. You are right @shlomizadok, it deals only with period attributes and the name was a bit misleading.

@shlomizadok
Copy link
Member

Merging, Thanks a lot @xprazak2 !

@shlomizadok shlomizadok merged commit 2d244ad into theforeman:master Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants