Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Remove unused notice #14044

Closed
wants to merge 2 commits into from

5 participants

@f-plante

The given notice is never shown because notice are only rendered on the show view

Frédéric Planté added some commits
Frédéric Planté Remove unused notice
The given notice is never shown because notice are only rendered on the show template
83fc2f9
Frédéric Planté Fixing test
c9dda49
@f-plante f-plante closed this
@f-plante f-plante reopened this
@carlosantoniodasilva

Thanks, but we don't feel it's unused. The fact that the flash only happens to be in the show view is because it was only used there, and when the notice was added to the destroy action, we should've updated the index view too.

Maybe you'd like to change your PR to add the flash the index view instead? This way the flash will be properly displayed. Let me know if you can/want to do that, otherwise I can do it myself later this week.

@f-plante

Carlos, I thought it have to be removed because a default scaffolding with jbuilder enabled produce a destroy action without any notice. So I proposed that behavior to be consistent with what scaffolding do on a standard new rails app. Please let me know what option you want, I would be very happy to contribute, even on a very minor fix/improvment like this one.

@robin850 robin850 added the railties label
@arunagw arunagw referenced this pull request from a commit in arunagw/jbuilder
@arunagw arunagw We need notice on destroy when HTML
This is same as we do without JBuilder
See this for more rails/rails#14044
8448e86
@arunagw arunagw referenced this pull request in rails/jbuilder
Merged

We need notice on destroy when HTML #191

@arunagw
Collaborator

@f-plante thanks for PR. I think we need this when we are doing things with including JBuilder.

I have added a PR here rails/jbuilder#191

@rafaelfranca
Owner

Do we? I checked the index template and we are not showing notice there.

@arunagw
Collaborator

@rafaelfranca we have it here I think https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/scaffold_controller/templates/controller.rb#L50

As PR in JBuilder is merged so this can be closed. As we have both consistent on both the places now.

Closing this now. thanks @f-plante.

@arunagw arunagw closed this
@rafaelfranca rafaelfranca reopened this
@arunagw
Collaborator

True there is no flash message when we destroy.

Sorry my bad. I made a change in JBuilder as well which needs to revert then.

@rafaelfranca
Owner

I think the proper fix is to make index show the notice

@arunagw arunagw referenced this pull request from a commit in arunagw/rails
@arunagw arunagw As we are setting notice in destroy action we should display that
For more information see rails#14044
And rails/jbuilder#191
06c9c01
@arunagw arunagw referenced this pull request from a commit in arunagw/rails
@arunagw arunagw As we are setting notice in destroy action we should display that
For more information see rails#14044
And rails/jbuilder#191

closes #14044
1532f24
@arunagw arunagw referenced this pull request from a commit in arunagw/rails
@arunagw arunagw Display notice in index.html pages in scaffolded generated views
As we are setting notice in destroy action we should display that

For more information see rails#14044
And rails/jbuilder#191

closes #14044
e197fd7
@rafaelfranca
Owner

Closed by #16048

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 13, 2014
  1. Remove unused notice

    Frédéric Planté authored
    The given notice is never shown because notice are only rendered on the show template
  2. Fixing test

    Frédéric Planté authored
This page is out of date. Refresh to see the latest.
View
2  railties/lib/rails/generators/rails/scaffold_controller/templates/controller.rb
@@ -47,7 +47,7 @@ def update
# DELETE <%= route_url %>/1
def destroy
@<%= orm_instance.destroy %>
- redirect_to <%= index_helper %>_url, notice: <%= "'#{human_name} was successfully destroyed.'" %>
+ redirect_to <%= index_helper %>_url
end
private
View
1  railties/test/generators/scaffold_controller_generator_test.rb
@@ -39,7 +39,6 @@ def test_controller_skeleton_is_created
assert_instance_method :destroy, content do |m|
assert_match(/@user\.destroy/, m)
- assert_match(/User was successfully destroyed/, m)
end
assert_instance_method :set_user, content do |m|
Something went wrong with that request. Please try again.