Fix/Slug Validation in unset_slug_if_invalid

[khng]

This PR replaces #903. Added test to pass pipeline checks.

[parndt]

@khng I've merged in #940 to fix the MySQL jobs, could you please rebase this PR?

[khng]

@parndt Done! Let me know if there are any other issues.

[parndt]

@Ancez how does this look to you?

[nedenwalker]

Nice, thank you all.

[marckohlbrugge]

This change introduced a bug for us.

Before this change:

1. There's a valid post with title "Web developer", an associated company with title "Spotify", and the post's slug "web-developer-at-spotify"
2. Then the post title is set to an empty string ""
3. However, because the post is invalid (title is required) the change is not saved to the database and the slug stays the same
4. Because the slug hasn't changed post.to_param still uses the original slug. All good.
5. When the form is submitted, it works as expected.

After this change:

1. There's a valid post with title "Web developer", an associated company with title "Spotify", and the post's slug "web-developer-at-spotify"
2. Then the post title is set to an empty string "" and the slug is set to "at-spotify" (no job title)
3. However, because the post is invalid these changes are not actually saved to the database
4. However, this new (unsaved) slug ("at-spotify") is being used in the URL for the <form>'s action. (to_param)
5. When the form is submitted, it cannot find a saved record with that slug.

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.

[parndt]

🤔 thanks @marckohlbrugge; are you willing to contribute a failing test for your case?

[sbeckeriv]

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.

[sbeckeriv]

@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]

@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 ✨

[xymbol]

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.

1. A record is loaded by slug.
2. Changes are applied.
3. Validation fails, but a slug is set or reset.
4. Form is re-rendered.
5. User makes corrections and re-posts the form with new changes.
6. The update fails with a record not found error as the slug never persisted.

I'll work on a fix and submit it.