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

Create MissingExactTemplate exception with separate template #29286

Merged

Conversation

@vinistock
Copy link
Contributor

@vinistock vinistock commented May 30, 2017

This PR is supposed to address issue #29280.

Create new exception class MissingExactTemplate with it's own HTML template and raise it in the implicit_render class.

This is my first contribution, so please let me know if I'm missing something or if I'm breaking any code style standards.

@rails-bot
Copy link

@rails-bot rails-bot commented May 30, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Copy link
Member

@matthewd matthewd left a comment

This is looking about the right shape 👍🏻

Still need this bit:

Turn the long exception message into a custom HTML template

@@ -51,4 +51,7 @@ class UnknownHttpMethod < ActionControllerError #:nodoc:

class UnknownFormat < ActionControllerError #:nodoc:
end

class MissingExactTemplate < ActionControllerError #:nodoc:
Copy link
Member

@matthewd matthewd May 30, 2017

Inherit from UnknownFormat, so the new exception is still rescued by anything that currently matches it

@vinistock
Copy link
Contributor Author

@vinistock vinistock commented May 30, 2017

@matthewd Thanks for reviewing the PR. I addressed your comment about the inheritance and fixed the respective tests.

Although I must say I don't quite understand the bit about turning the message into an html template. Is the entire message moving to a template? Will the exception not use it for instantiation anymore? Sorry, not really sure what is expected on this portion.

@kaspth
Copy link
Member

@kaspth kaspth commented May 30, 2017

@vinistock try triggering the exception in a newly generated Rails 5.1 app. Notice how the exception is just blurted out as one big chunk of text? We want to do better.

We want the custom template to communicate what the exception means better than what the one paragraph exception message can do. To "show" rather than "tell".

The existing exception message can be a spring board, but you can almost do with it as you please, it's not required for instantiation anymore (that's not fully true, but lets get the template going first then we can hone details).

@vinistock
Copy link
Contributor Author

@vinistock vinistock commented Jun 1, 2017

@kaspth I've customized the template a bit and changed the message. I also added what the expected path for the template would be, I thought that would make the exception cause more obvious to the user.

Let me know if this is aligned with the expectations and if the exception messages / explanations are as descriptive and concise as they should be.

The layout remains similar to the majority of Rails error pages. If any changes concerning this are wanted, give me a heads up.

@vinistock vinistock force-pushed the create_missing_exact_template_exception branch from 0b3944c to e069e26 Jun 5, 2017
Copy link
Member

@matthewd matthewd left a comment

Sorry, I spectacularly dropped the ball on this 😟

I've added a few small notes, but this seems to be just about good to go. I haven't yet actually caused the error locally to see the resulting page, but the HTML sounds good! (If you happen to have it open locally and can add a screenshot, that'd be awesome; if not, that's fine too.)

Sorry again I left this unanswered for so long!


raise ActionController::UnknownFormat, message
message = "#{self.class.name}\##{action_name} is missing a template for request formats: #{request.formats.map(&:to_s).join(',')}"
expected_template_path = "app/views/#{self.class.controller_path}/#{action_name}.html.erb"
Copy link
Member

@matthewd matthewd Feb 1, 2018

This is an interesting idea, but making a more accurate guess is a lot harder than this, so I think it's best to leave it off for now. (Notably, it has checked a lot more than one filename.)

@@ -23,7 +23,8 @@ class ExceptionWrapper
"ActionView::MissingTemplate" => "missing_template",
"ActionController::RoutingError" => "routing_error",
"AbstractController::ActionNotFound" => "unknown_action",
"ActionView::Template::Error" => "template_error"
"ActionView::Template::Error" => "template_error",
"ActionController::MissingExactTemplate" => "missing_exact_template"
Copy link
Member

@matthewd matthewd Feb 1, 2018

Let's add a trailing comma, so this line doesn't have to change next time we add an entry.

Also, I think we need to add our new exception to rescue_responses above as well (pointing to :not_acceptable, same as the existing UnknownFormat)

@@ -0,0 +1,23 @@
<header>
<h1>Missing exact template</h1>
Copy link
Member

@matthewd matthewd Feb 1, 2018

"exact" sounds a bit odd in a user-facing context here. How about we title it "No template for interactive request"?

@vinistock vinistock force-pushed the create_missing_exact_template_exception branch from 4c6c334 to 76d79e7 Feb 9, 2018
@vinistock
Copy link
Contributor Author

@vinistock vinistock commented Feb 9, 2018

@matthewd I've addressed your comments and rebased with the current master. Let me know if any other changes are needed.

Here's the screenshot of a sample application raising the error.

missing_exact_template

@@ -50,4 +50,10 @@ class UnknownHttpMethod < ActionControllerError #:nodoc:

class UnknownFormat < ActionControllerError #:nodoc:
end

class MissingExactTemplate < UnknownFormat #:nodoc:
def initialize(message)
Copy link
Member

@rafaelfranca rafaelfranca Apr 12, 2018

Any reason why we need to define an initialize?


raise ActionController::UnknownFormat, message
message = "#{self.class.name}\##{action_name} is missing a template for request formats: #{request.formats.map(&:to_s).join(',')}"
raise ActionController::MissingExactTemplate.new(message)
Copy link
Member

@rafaelfranca rafaelfranca Apr 12, 2018

can't we can keep the raise ActionController::MissingExactTemplate, message pattern here?

@@ -26,7 +27,8 @@ class ExceptionWrapper
"ActionController::RoutingError" => "routing_error",
"AbstractController::ActionNotFound" => "unknown_action",
"ActiveRecord::StatementInvalid" => "invalid_statement",
"ActionView::Template::Error" => "template_error"
"ActionView::Template::Error" => "template_error",
"ActionController::MissingExactTemplate" => "missing_exact_template",
Copy link
Member

@rafaelfranca rafaelfranca Apr 12, 2018

can we align all other values or maybe a next PR un-align all the values?

to be rendered for such requests. If that's the case, carry on.
</p>

<%= render template: "rescues/_source" %>
Copy link
Member

@rafaelfranca rafaelfranca Apr 12, 2018

Not sure if the source and the trace is going to be useful for this exception. The source is always inside Rails. I'd also remove the request_and_response. This page is more informative than actually an application error.

@vinistock
Copy link
Contributor Author

@vinistock vinistock commented Apr 13, 2018

@rafaelfranca Thanks for the review. I believe all your comments make sense and applied all the requested changes. Let me know if there's anything else to be done.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

This looks good to me. Can you squash your commits?

@vinistock vinistock force-pushed the create_missing_exact_template_exception branch from e4c7954 to 297a579 Apr 20, 2018
@vinistock
Copy link
Contributor Author

@vinistock vinistock commented Apr 20, 2018

@rafaelfranca all done.

@rafaelfranca rafaelfranca merged commit 297a579 into rails:master Apr 20, 2018
2 checks passed
rafaelfranca added a commit that referenced this issue Apr 20, 2018
…e_exception

Create MissingExactTemplate exception with separate template
utilum added a commit to utilum/rails that referenced this issue Feb 4, 2019
Changes the Getting Started guide explanation for
`ActionController::MissingExactTemplate` error, to reflect the current message.

Follow up for rails#29286, rails#35148

[ci skip]
scpike added a commit to scpike/rails that referenced this issue Feb 4, 2019
Changes the Getting Started guide explanation for
`ActionController::MissingExactTemplate` error, to reflect the current message.

Follow up for rails#29286, rails#35148

[ci skip]
@vinistock vinistock deleted the create_missing_exact_template_exception branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants