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

[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 this issue Dec 2, 2020 · 8 comments · May be fixed by #1026
Labels

Comments

@sbeckeriv
Copy link

sbeckeriv commented Dec 2, 2020

Dearest maintainer,

Opening a new issue following from comments in #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 9b372d7

Thanks for your hard work on this project.

Becker

sbeckeriv added a commit to sbeckeriv/friendly_id that referenced this issue Dec 2, 2020
Details in issue norman#959

in older versions this would return the slug.
@sbeckeriv sbeckeriv changed the title [bug] to_param does not work as expected when record fails to save and a new slug is set in memory [regression] to_param does not work as expected when record fails to save and a new slug is set in memory Dec 2, 2020
@sbeckeriv
Copy link
Author

As I look in to fixing this

cd879c0#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
Copy link

marckohlbrugge commented Mar 8, 2021

@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
Copy link

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
Copy link
Author

Hello @marckohlbrugge,

I am currently running a fork's branch aha-app#1 this works fine for our needs but might not work for everyone.

Thanks
Becker

@piclez
Copy link

piclez commented Mar 15, 2021

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

@stale
Copy link

stale bot commented Jun 7, 2021

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.

@stale stale bot added the stale label Jun 7, 2021
@sbeckeriv
Copy link
Author

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

@stale stale bot removed the stale label Jun 14, 2021
@parndt parndt added the pinned label Jun 22, 2021
@parndt
Copy link
Collaborator

parndt commented Jun 22, 2021

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

maxime-carbonneau added a commit to maxime-carbonneau/friendly_id that referenced this issue Nov 3, 2024
Fix if statement to run want the record is invalid.

Should fix norman#959
@maxime-carbonneau maxime-carbonneau linked a pull request Nov 3, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants