Skip to content

Fixes requests specs generated by scaffold with namespace resource #2492

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 2, 2021
Merged

Fixes requests specs generated by scaffold with namespace resource #2492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5468bee
Refactor show_helper in request scaffold generators
PedroAugustoRamalhoDuarte Apr 11, 2021
749aefa
Removes unused .tr call in edit_helper
PedroAugustoRamalhoDuarte Apr 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/generators/rspec/scaffold/scaffold_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ def template_file(folder:, suffix: '')
def banner
self.class.banner
end

def show_helper(resource_name = file_name)
"#{singular_route_name}_url(#{resource_name})"
end
end
end
end
10 changes: 5 additions & 5 deletions lib/generators/rspec/scaffold/templates/api_request_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
describe "GET /show" do
it "renders a successful response" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
get <%= show_helper.tr('@', '') %>, as: :json
get <%= show_helper %>, as: :json
expect(response).to be_successful
end
end
Expand Down Expand Up @@ -93,15 +93,15 @@

it "updates the requested <%= ns_file_name %>" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
patch <%= show_helper.tr('@', '') %>,
patch <%= show_helper %>,
params: { <%= singular_table_name %>: new_attributes }, headers: valid_headers, as: :json
<%= file_name %>.reload
skip("Add assertions for updated state")
end

it "renders a JSON response with the <%= ns_file_name %>" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
patch <%= show_helper.tr('@', '') %>,
patch <%= show_helper %>,
params: { <%= singular_table_name %>: new_attributes }, headers: valid_headers, as: :json
expect(response).to have_http_status(:ok)
expect(response.content_type).to match(a_string_including("application/json"))
Expand All @@ -111,7 +111,7 @@
context "with invalid parameters" do
it "renders a JSON response with errors for the <%= ns_file_name %>" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
patch <%= show_helper.tr('@', '') %>,
patch <%= show_helper %>,
params: { <%= singular_table_name %>: invalid_attributes }, headers: valid_headers, as: :json
expect(response).to have_http_status(:unprocessable_entity)
expect(response.content_type).to match(a_string_including("application/json"))
Expand All @@ -123,7 +123,7 @@
it "destroys the requested <%= ns_file_name %>" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
expect {
delete <%= show_helper.tr('@', '') %>, headers: valid_headers, as: :json
delete <%= show_helper %>, headers: valid_headers, as: :json
}.to change(<%= class_name %>, :count).by(-1)
end
end
Expand Down
16 changes: 8 additions & 8 deletions lib/generators/rspec/scaffold/templates/request_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
describe "GET /show" do
it "renders a successful response" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
get <%= show_helper.tr('@', '') %>
get <%= show_helper %>
expect(response).to be_successful
end
end
Expand All @@ -56,7 +56,7 @@
describe "GET /edit" do
it "renders a successful response" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
get <%= edit_helper.tr('@','') %>
get <%= edit_helper %>
expect(response).to be_successful
end
end
Expand All @@ -71,7 +71,7 @@

it "redirects to the created <%= ns_file_name %>" do
post <%= index_helper %>_url, params: { <%= ns_file_name %>: valid_attributes }
expect(response).to redirect_to(<%= show_helper.gsub("\@#{file_name}", class_name+".last") %>)
expect(response).to redirect_to(<%= show_helper(class_name+".last") %>)
end
end

Expand All @@ -97,14 +97,14 @@

it "updates the requested <%= ns_file_name %>" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
patch <%= show_helper.tr('@', '') %>, params: { <%= singular_table_name %>: new_attributes }
patch <%= show_helper %>, params: { <%= singular_table_name %>: new_attributes }
<%= file_name %>.reload
skip("Add assertions for updated state")
end

it "redirects to the <%= ns_file_name %>" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
patch <%= show_helper.tr('@', '') %>, params: { <%= singular_table_name %>: new_attributes }
patch <%= show_helper %>, params: { <%= singular_table_name %>: new_attributes }
<%= file_name %>.reload
expect(response).to redirect_to(<%= singular_table_name %>_url(<%= file_name %>))
end
Expand All @@ -113,7 +113,7 @@
context "with invalid parameters" do
it "renders a successful response (i.e. to display the 'edit' template)" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
patch <%= show_helper.tr('@', '') %>, params: { <%= singular_table_name %>: invalid_attributes }
patch <%= show_helper %>, params: { <%= singular_table_name %>: invalid_attributes }
expect(response).to be_successful
end
end
Expand All @@ -123,13 +123,13 @@
it "destroys the requested <%= ns_file_name %>" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
expect {
delete <%= show_helper.tr('@', '') %>
delete <%= show_helper %>
}.to change(<%= class_name %>, :count).by(-1)
end

it "redirects to the <%= table_name %> list" do
<%= file_name %> = <%= class_name %>.create! valid_attributes
delete <%= show_helper.tr('@', '') %>
delete <%= show_helper %>
expect(response).to redirect_to(<%= index_helper %>_url)
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/generators/rspec/scaffold/scaffold_generator_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
before { run_generator %w[admin/posts] }
it { is_expected.to exist }
it { is_expected.to contain(/^RSpec.describe "\/admin\/posts", #{type_metatag(:request)}/) }
it { is_expected.to contain('admin_post_url(admin_post)') }
it { is_expected.to contain('admin_post_url(post)') }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why?

      accounts_user = Accounts::User.create! valid_attributes
      get accounts_user_url(accounts_user), as: :json

It makes it clear what namespace a certain model belongs to.
Accounting::User and Reader::User might be quite different.

If RSpec is used as a project documentation tool, I'd prefer it to be more specific by default.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before my changes the generator output was:

# Output
user = Accounts::User.create! valid_attributes
get accounts_user_url(accounts_user), as: :json

# Source
# <%= file_name #>  returns 'user'
<%= file_name %> = <%= class_name %>.create! valid_attributes
get <%= show_helper %>, as: :json

To keep the pattern from another templates, example controller_spec ,i have use the file_name to be argument from path. In my pratice i use more the small name like user, but thinking as project documentation tool is better keep the full_name from resource, do you like that i change all file_name calls from generator source to be ns_file_name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonRowe @benoittgt What do you guys think? accounts_user or just user for Accounts::User?

Copy link
Member

@JonRowe JonRowe Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be the same and the spec should work either way? e.g. I think this report is that one is user and one is accounts_user and thats wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I explained the problem with more details in this issue #2491 . We can fix the spec generator to use accounts_user or just user, in my PR, I chose user.
In order to change to accounts_user and keep the pattern from another templates generators, we need to change some other lines.

it { is_expected.to contain('Admin::Post.create') }
end

Expand Down