norman / friendly_id

FriendlyId is the “Swiss Army bulldozer” of slugging and permalink plugins for ActiveRecord. It allows you to create pretty URL’s and work with human-friendly strings as if they were numeric ids for ActiveRecord models.
http://norman.github.io/friendly_id/
MIT License
6.15k stars 591 forks source link

[regression] to_param does not work as expected when record fails to save and a new slug is set in memory #959

Open sbeckeriv opened 3 years ago

sbeckeriv commented 3 years ago

Dearest maintainer,

Opening a new issue following from comments in https://github.com/norman/friendly_id/pull/938 , The tldr is that if you update the slug but it fails to validate or save the to_param reads the in memory data and if you use that object again for a form it will post to the incorrect endpoint. The test appear to pass because the test use nil. I will create a pr to demo this.

The related commit https://github.com/norman/friendly_id/commit/9b372d75159422e6aa80cf6d2bb1804a7ae6f247

Thanks for your hard work on this project.

Becker

sbeckeriv commented 3 years ago

As I look in to fixing this

https://github.com/norman/friendly_id/commit/cd879c09775e50c775a17fa212c033f43bff94d8#diff-82407ad0a1f69c6572ae294573e99782980e71014022e444cbdd5251fab2b433R537

Testing in rails 5.0 the prefix fails to save because J2 is assigned in the database. Save fails and my to_params still returns the old value. I am unsure why the line of test I link changed the result.

(byebug) @p.changes
{"prefix"=>["J1", "J2"]}
(byebug) @p.to_param
"J1"
(byebug) @p.valid?
false
marckohlbrugge commented 3 years ago

@sbeckeriv did you find a workaround?

I'm running into the same problem in newer versions of FriendlyId. 5.3.0 works fine, but with 5.4.2 (and I assume even some versions before this) my tests are failing. The issue is like described above.

It seems like a common occurrence to re-render a form with an invalid record, so I'm curious why not more people seem to run into this issue.

Edit: pinging @khng who seems to have introduced the change. Curious to get their view.

marckohlbrugge commented 3 years ago

I've now worked around it by ensuring there are no slug candidates when the record's title is empty. This isn't ideal as I'm basically duplicating my validations, but it does the job.

sbeckeriv commented 3 years ago

Hello @marckohlbrugge,

I am currently running a fork's branch https://github.com/aha-app/friendly_id/pull/1 this works fine for our needs but might not work for everyone.

Thanks Becker

piclez commented 3 years ago

I'm experiencing this issue @marckohlbrugge on a brand new Rails 6.

stale[bot] commented 3 years ago

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.

sbeckeriv commented 3 years ago

Stale but relevant? If its pinged as stale again I wont re-comment and assume the matter to be the way it works now.

parndt commented 3 years ago

Stale bot is just a bot - I've marked this as pinned so it won't do it again.