-
-
Notifications
You must be signed in to change notification settings - Fork 592
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/Slug Validation in unset_slug_if_invalid #903
Conversation
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@Ancez hi! Could you please pull in the latest changes from master into this PR? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Will do later on today, sorry for the late response |
@nedenwalker It needs rebasing against master because it's failing CI on its current HEAD. Does the change fix your problem? |
@parndt I can confirm that the fix does eliminate the bug. When the form reloads with previous form values and error messages, the slug field is now also present. |
Wasn't sure on how to run teamcity? tests locally or rebase someone else's fork etc, so made my own fork, made the code change and submitted a new PR to get CI running in your system: #936 |
Figured out local testing issue, was running |
@parndt I see 5 test failures, all related to the code change. I cannot admit to understanding the coding enough, but in particular, I see one test that specifies the code should do what it used to do, ie wipe the slug when there is another field validation failure. This code change would alter that contract. https://github.com/norman/friendly_id/blob/master/test/slugged_test.rb#L95 Will leave it to you to determine if the functionality should change or not. |
yeah there are some tests to fix otherwise this fixes the issue of the slug being removed. If you want to you can fork this line and fix the specs for it. If not then I'll fix these specs tonight if that's alright, until then I recommend changing the base within your projects Gemfile for this gem to this PR |
Fixed the test and submitted a new PR here: #938. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
updated
unset_slug_if_invalid
to only validate errors for the slug field instead of all model fields.closes #902