Closed khng closed 4 years ago
@khng I've merged in #940 to fix the MySQL jobs, could you please rebase this PR?
@parndt Done! Let me know if there are any other issues.
@Ancez how does this look to you?
Nice, thank you all.
This change introduced a bug for us.
Before this change:
"Web developer"
, an associated company with title "Spotify"
, and the post's slug "web-developer-at-spotify"
""
post.to_param
still uses the original slug. All good.After this change:
"Web developer"
, an associated company with title "Spotify"
, and the post's slug "web-developer-at-spotify"
""
and the slug is set to "at-spotify"
(no job title)"at-spotify"
) is being used in the URL for the <form>
's action. (to_param
)Our implementation is rather straight forward. So I'm not sure if this should be considered a regression in FriendlyId, or our implementation. In case of the latter, I'm curious to hear suggested workarounds.
🤔 thanks @marckohlbrugge; are you willing to contribute a failing test for your case?
I am in the middle of a rails 5.0 to 5.2 upgrade and i believe i this the same issue @marckohlbrugge has. I am about to dive into the tests here because it looks like it should work.
my controller code if it makes sense.
@object = Object.find
# slug is TEST and must be uniq object params passes in TEST2 but TEST2 is taken.
@object.assign_attributes(object_params)
if @object.save
#doenst happen because of the dup slug
else
@object.to_param # returns TEST2
# return rerender form gives me the wrong post url.
end
Following this PR: https://github.com/norman/friendly_id/pull/867/files It is currently unclear to me how this would work in my case. If i repost the form it will update TEST2 with the form data now.
@parndt looking at the test in this pr https://github.com/norman/friendly_id/pull/867/files#diff-161efa3dde0ace4b376c177d76ab6998035d46447d95df00fa6942ac23ec765aR264 the difference might be the nil. I am updating the slug not nil-ing it out. so the || logic would not apply. working on running that as a test.
@Ancez how does this look to you?
looks great! Thanks, guys, sorry I wasn't able to help further, been super busy with other matters. Thanks for taking a lead on this ✨
I also ran into this issue, which was difficult to trace. The authors reversed the test intention without a good reason.
A slug should never be set or reset if validations fail, particularly on updates.
I'll work on a fix and submit it.
This PR replaces https://github.com/norman/friendly_id/pull/903. Added test to pass pipeline checks.