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

Redirect to @record or path in controller generator #569

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

jeromedalbert
Copy link
Contributor

@jeromedalbert jeromedalbert commented Sep 1, 2024

Partially addresses rails/rails#52756

Problem

There is some discrepancy between Rails controller generators and jbuilder controller generators when it comes to redirects.

Solution

This PR changes the controller generator to generate redirects with redirect_to @post or _path, in order to match the Rails codebase generators as close as possible.

@jeromedalbert
Copy link
Contributor Author

jeromedalbert commented Sep 1, 2024

Tests fail for Rails 5 because singular_route_name is not available in that version. I am wondering if a better fix would be to stop testing against Ruby version 2.7 and Rails 5 since both version are end of life. If so I can update this PR or make a separate one.

@skipkayhil
Copy link
Member

skipkayhil commented Sep 1, 2024

Would redirect_to @post work in this case? I think that's what the Rails' templates are generating now (however, redirect_to @post is still redirecting with a full URL which maybe(?) DHH is wanting to change)

@dhh
Copy link
Member

dhh commented Sep 1, 2024

Yes, let's change to redirect_to @post so it matches the generator in Rails itself. They should be as close as possible.

@jeromedalbert jeromedalbert changed the title Change controller generators to use _path instead of _url for redirects Redirect to @record or path in controller generator Sep 2, 2024
@jeromedalbert jeromedalbert force-pushed the path-in-controller-generator branch 2 times, most recently from a1c5222 to 298056d Compare September 2, 2024 04:08
@jeromedalbert
Copy link
Contributor Author

jeromedalbert commented Sep 2, 2024

PR updated. Tests are failing for Rails 5.0 and 5.1 because redirect_resource_name looks like it was added in Rails 5.2.

I would suggest not testing against these end of life versions any more. Here is a PR to do so: #570.

@dhh dhh merged commit 8dbc23f into rails:main Sep 14, 2024
28 of 38 checks passed
@jeromedalbert jeromedalbert deleted the path-in-controller-generator branch September 14, 2024 19:15
derricklannaman referenced this pull request in powerhome/power-web-development-interview Sep 24, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [jbuilder](https://redirect.github.com/rails/jbuilder)
([changelog](https://redirect.github.com/rails/jbuilder/releases/tag/v2.13.0))
| `2.12.0` -> `2.13.0` |
[![age](https://developer.mend.io/api/mc/badges/age/rubygems/jbuilder/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/rubygems/jbuilder/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/rubygems/jbuilder/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/rubygems/jbuilder/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>rails/jbuilder (jbuilder)</summary>

###
[`v2.13.0`](https://redirect.github.com/rails/jbuilder/releases/tag/v2.13.0)

[Compare
Source](https://redirect.github.com/rails/jbuilder/compare/v2.12.0...v2.13.0)

#### What's Changed

- Redirect to `@record` or path in controller generator by
[@&#8203;jeromedalbert](https://redirect.github.com/jeromedalbert) in
[https://github.com/rails/jbuilder/pull/569](https://redirect.github.com/rails/jbuilder/pull/569)
- Return early from collection partial rendering if blank by
[@&#8203;tylerjc](https://redirect.github.com/tylerjc) in
[https://github.com/rails/jbuilder/pull/560](https://redirect.github.com/rails/jbuilder/pull/560)
- Add missing ':see_other' status code in generated destroy controller
method by [@&#8203;ldeld](https://redirect.github.com/ldeld) in
[https://github.com/rails/jbuilder/pull/538](https://redirect.github.com/rails/jbuilder/pull/538)
- Remove OpenStruct references from Jbuilder by
[@&#8203;mtsmfm](https://redirect.github.com/mtsmfm) in
[https://github.com/rails/jbuilder/pull/567](https://redirect.github.com/rails/jbuilder/pull/567)
- Use new `params.expect` syntax instead of `params.require` by
[@&#8203;jeromedalbert](https://redirect.github.com/jeromedalbert) in
[https://github.com/rails/jbuilder/pull/573](https://redirect.github.com/rails/jbuilder/pull/573)

#### New Contributors

- [@&#8203;jeromedalbert](https://redirect.github.com/jeromedalbert)
made their first contribution in
[https://github.com/rails/jbuilder/pull/570](https://redirect.github.com/rails/jbuilder/pull/570)
- [@&#8203;tylerjc](https://redirect.github.com/tylerjc) made their
first contribution in
[https://github.com/rails/jbuilder/pull/560](https://redirect.github.com/rails/jbuilder/pull/560)
- [@&#8203;ldeld](https://redirect.github.com/ldeld) made their first
contribution in
[https://github.com/rails/jbuilder/pull/538](https://redirect.github.com/rails/jbuilder/pull/538)
- [@&#8203;mtsmfm](https://redirect.github.com/mtsmfm) made their first
contribution in
[https://github.com/rails/jbuilder/pull/567](https://redirect.github.com/rails/jbuilder/pull/567)

**Full Changelog**:
rails/jbuilder@v2.12.0...v2.13.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/powerhome/power-web-development-interview).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC43NC4xIiwidXBkYXRlZEluVmVyIjoiMzguNzQuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants