{"action":"created","comment":{"url":"https://api.github.com/repos/hypothesis/lms/pulls/comments/229423791","pull_request_review_id":169922256,"id":229423791,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDIyOTQyMzc5MQ==","diff_hunk":"@@ -1,66 +1,63 @@\n-# -*- coding: utf-8 -*-\n-\n from pyramid import httpexceptions\n from pyramid import i18n\n-from pyramid.view import view_config\n+from pyramid.view import exception_view_config\n from pyramid.view import view_defaults\n-from pyramid.settings import asbool\n \n from lms.exceptions import LTILaunchError\n+\n _ = i18n.TranslationStringFactory(__package__)\n \n \n-@view_defaults(renderer='lms:templates/error.html.jinja2')\n+@view_defaults(renderer=\"lms:templates/error.html.jinja2\")\n class ErrorViews:\n-    \"\"\"Show user an error page and an appropriate message.\"\"\"\n-\n     def __init__(self, exc, request):\n-        \"\"\"Store request and exception information.\"\"\"\n         self.exc = exc\n         self.request = request\n \n-    @view_config(context=httpexceptions.HTTPError)\n-    @view_config(context=httpexceptions.HTTPServerError)\n+    @exception_view_config(httpexceptions.HTTPError)\n+    @exception_view_config(httpexceptions.HTTPServerError)\n     def httperror(self):\n-        self.request.response.status_int = self.exc.status_int\n-        # If code raises an HTTPError or HTTPServerError we assume this was\n-        # deliberately raised and:\n-        # 1. Show the user an error page including specific error message\n-        # 2. _Do not_ report the error to Sentry\n-        return {'message': str(self.exc)}\n+        \"\"\"\n+        Handle an HTTP 4xx or 5xx exception.\n \n-    @view_config(context=Exception)\n-    def error(self):\n-        # In debug mode re-raise exceptions so that they get printed in the\n-        # terminal.\n-        if asbool(self.request.registry.settings.get('debug')):\n-            raise self.exc\n+        If code raises an HTTP client or server error we assume this was\n+        deliberately raised We show the user an error page including specific\n+        error message but _do not_ report the error to Sentry\n+        \"\"\"\n+        self.request.response.status_int = self.exc.status_int\n+        return {\"message\": str(self.exc)}\n \n-        self.request.response.status_int = 500\n+    @exception_view_config(LTILaunchError)\n+    def missing_lti_param_error(self):\n+        \"\"\"\n+        Handle LTILaunchErrors.\n \n-        # If code raises a non-HTTPException exception we assume it was a bug\n-        # and:\n-        # 1. Show the user a generic error page\n-        # 2. Report the details of the error to Sentry\n+        If code raises an LTILaunchError we assume this was deliberately raised\n+        because an invalid LTI launch request was received. For example a\n+        required LTI launch parameter was missing. We return a 400 Bad Request\n+        response, show the user an error page with the specific error message,\n+        and report the issue to Sentry.\n+        \"\"\"\n+        self.request.response.status_int = 400\n         self.request.raven.captureException()\n-        return {'message': _(\"Sorry, but something went wrong. \"\n-                             \"The issue has been reported and we'll try to \"\n-                             \"fix it.\")}\n+        return {\"message\": str(self.exc)}\n \n-\n-    @view_config(context=LTILaunchError)\n-    def missing_lti_param_error(self):\n+    @exception_view_config(Exception)\n+    def error(self):\n         \"\"\"\n-        Catch MissingLTILaunchParamError and MissingLTIContentItemParamError, render a 400 page and report the exception to Sentry.\n-\n-        If code raises a MissingLTILaunchParamError it means that a required parameter was missing\n-        from an LTI launch request that we received.\n-        If code raises a MissingLTIContentItemParamError it means that a required parameter was missing\n-        from an LTI content item selection request that we received:\n+        Handle an unexpected exception.\n \n-        1. Show the user an error page including specific error message\n-        2. Report the error to Sentry\n+        If the code raises an unexpected exception (anything not caught by any\n+        of the more specific exception views above) then we assume it was a\n+        bug.  We show the user a generic error page and report the exception to\n+        Sentry.\n         \"\"\"\n-        self.request.response.status_int = 400\n+        self.request.response.status_int = 500\n         self.request.raven.captureException()\n-        return {'message': str(self.exc)}\n+        return {\n+            \"message\": _(\n+                \"Sorry, but something went wrong. \"","path":"lms/views/error.py","position":102,"original_position":102,"commit_id":"9329d6486d0f52fddf54db93391c4d633792cd95","original_commit_id":"9329d6486d0f52fddf54db93391c4d633792cd95","user":{"login":"seanh","id":22498,"node_id":"MDQ6VXNlcjIyNDk4","avatar_url":"https://avatars3.githubusercontent.com/u/22498?v=4","gravatar_id":"","url":"https://api.github.com/users/seanh","html_url":"https://github.com/seanh","followers_url":"https://api.github.com/users/seanh/followers","following_url":"https://api.github.com/users/seanh/following{/other_user}","gists_url":"https://api.github.com/users/seanh/gists{/gist_id}","starred_url":"https://api.github.com/users/seanh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seanh/subscriptions","organizations_url":"https://api.github.com/users/seanh/orgs","repos_url":"https://api.github.com/users/seanh/repos","events_url":"https://api.github.com/users/seanh/events{/privacy}","received_events_url":"https://api.github.com/users/seanh/received_events","type":"User","site_admin":false},"body":"I think this needs to be fixed in a separate PR. The problem is that the h error page has different templating and styling than the LMS one. It uses a separate `<h1>` and `<p>` for the above error message. The LMS app would wrap it all together in one paragraph and center-align it all. Needs a follow-up PR to improve the design of the error page","created_at":"2018-10-30T18:06:35Z","updated_at":"2018-10-30T18:06:36Z","html_url":"https://github.com/hypothesis/lms/pull/304#discussion_r229423791","pull_request_url":"https://api.github.com/repos/hypothesis/lms/pulls/304","author_association":"COLLABORATOR","_links":{"self":{"href":"https://api.github.com/repos/hypothesis/lms/pulls/comments/229423791"},"html":{"href":"https://github.com/hypothesis/lms/pull/304#discussion_r229423791"},"pull_request":{"href":"https://api.github.com/repos/hypothesis/lms/pulls/304"}},"in_reply_to_id":229370721},"pull_request":{"url":"https://api.github.com/repos/hypothesis/lms/pulls/304","id":226145926,"node_id":"MDExOlB1bGxSZXF1ZXN0MjI2MTQ1OTI2","html_url":"https://github.com/hypothesis/lms/pull/304","diff_url":"https://github.com/hypothesis/lms/pull/304.diff","patch_url":"https://github.com/hypothesis/lms/pull/304.patch","issue_url":"https://api.github.com/repos/hypothesis/lms/issues/304","number":304,"state":"open","locked":false,"title":"Fix exception handling using pyramid_exclogger","user":{"login":"seanh","id":22498,"node_id":"MDQ6VXNlcjIyNDk4","avatar_url":"https://avatars3.githubusercontent.com/u/22498?v=4","gravatar_id":"","url":"https://api.github.com/users/seanh","html_url":"https://github.com/seanh","followers_url":"https://api.github.com/users/seanh/followers","following_url":"https://api.github.com/users/seanh/following{/other_user}","gists_url":"https://api.github.com/users/seanh/gists{/gist_id}","starred_url":"https://api.github.com/users/seanh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seanh/subscriptions","organizations_url":"https://api.github.com/users/seanh/orgs","repos_url":"https://api.github.com/users/seanh/repos","events_url":"https://api.github.com/users/seanh/events{/privacy}","received_events_url":"https://api.github.com/users/seanh/received_events","type":"User","site_admin":false},"body":"Fix the in-browser exception views (error page) and printing of exceptions to the console in dev.\r\n\r\nThis was all just completely broken. Fix it by using `pyramid_exclog` to log exceptions to the console, and also fix up the exception views / error page to not crash, and tidy up the code and docstrings.\r\n\r\nSee:\r\n\r\n* <https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/logging.html#logging-exceptions>\r\n* <https://docs.pylonsproject.org/projects/pyramid_exclog/en/latest/>","created_at":"2018-10-26T15:52:34Z","updated_at":"2018-10-30T18:06:36Z","closed_at":null,"merged_at":null,"merge_commit_sha":"c2915d7f7d23021d8610809d20533a9a76dbd95c","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/hypothesis/lms/pulls/304/commits","review_comments_url":"https://api.github.com/repos/hypothesis/lms/pulls/304/comments","review_comment_url":"https://api.github.com/repos/hypothesis/lms/pulls/comments{/number}","comments_url":"https://api.github.com/repos/hypothesis/lms/issues/304/comments","statuses_url":"https://api.github.com/repos/hypothesis/lms/statuses/9329d6486d0f52fddf54db93391c4d633792cd95","head":{"label":"hypothesis:fix-error-page","ref":"fix-error-page","sha":"9329d6486d0f52fddf54db93391c4d633792cd95","user":{"login":"hypothesis","id":1243215,"node_id":"MDEyOk9yZ2FuaXphdGlvbjEyNDMyMTU=","avatar_url":"https://avatars1.githubusercontent.com/u/1243215?v=4","gravatar_id":"","url":"https://api.github.com/users/hypothesis","html_url":"https://github.com/hypothesis","followers_url":"https://api.github.com/users/hypothesis/followers","following_url":"https://api.github.com/users/hypothesis/following{/other_user}","gists_url":"https://api.github.com/users/hypothesis/gists{/gist_id}","starred_url":"https://api.github.com/users/hypothesis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hypothesis/subscriptions","organizations_url":"https://api.github.com/users/hypothesis/orgs","repos_url":"https://api.github.com/users/hypothesis/repos","events_url":"https://api.github.com/users/hypothesis/events{/privacy}","received_events_url":"https://api.github.com/users/hypothesis/received_events","type":"Organization","site_admin":false},"repo":{"id":108909388,"node_id":"MDEwOlJlcG9zaXRvcnkxMDg5MDkzODg=","name":"lms","full_name":"hypothesis/lms","private":false,"owner":{"login":"hypothesis","id":1243215,"node_id":"MDEyOk9yZ2FuaXphdGlvbjEyNDMyMTU=","avatar_url":"https://avatars1.githubusercontent.com/u/1243215?v=4","gravatar_id":"","url":"https://api.github.com/users/hypothesis","html_url":"https://github.com/hypothesis","followers_url":"https://api.github.com/users/hypothesis/followers","following_url":"https://api.github.com/users/hypothesis/following{/other_user}","gists_url":"https://api.github.com/users/hypothesis/gists{/gist_id}","starred_url":"https://api.github.com/users/hypothesis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hypothesis/subscriptions","organizations_url":"https://api.github.com/users/hypothesis/orgs","repos_url":"https://api.github.com/users/hypothesis/repos","events_url":"https://api.github.com/users/hypothesis/events{/privacy}","received_events_url":"https://api.github.com/users/hypothesis/received_events","type":"Organization","site_admin":false},"html_url":"https://github.com/hypothesis/lms","description":"LTI app for integrating with learning management systems","fork":false,"url":"https://api.github.com/repos/hypothesis/lms","forks_url":"https://api.github.com/repos/hypothesis/lms/forks","keys_url":"https://api.github.com/repos/hypothesis/lms/keys{/key_id}","collaborators_url":"https://api.github.com/repos/hypothesis/lms/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/hypothesis/lms/teams","hooks_url":"https://api.github.com/repos/hypothesis/lms/hooks","issue_events_url":"https://api.github.com/repos/hypothesis/lms/issues/events{/number}","events_url":"https://api.github.com/repos/hypothesis/lms/events","assignees_url":"https://api.github.com/repos/hypothesis/lms/assignees{/user}","branches_url":"https://api.github.com/repos/hypothesis/lms/branches{/branch}","tags_url":"https://api.github.com/repos/hypothesis/lms/tags","blobs_url":"https://api.github.com/repos/hypothesis/lms/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/hypothesis/lms/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/hypothesis/lms/git/refs{/sha}","trees_url":"https://api.github.com/repos/hypothesis/lms/git/trees{/sha}","statuses_url":"https://api.github.com/repos/hypothesis/lms/statuses/{sha}","languages_url":"https://api.github.com/repos/hypothesis/lms/languages","stargazers_url":"https://api.github.com/repos/hypothesis/lms/stargazers","contributors_url":"https://api.github.com/repos/hypothesis/lms/contributors","subscribers_url":"https://api.github.com/repos/hypothesis/lms/subscribers","subscription_url":"https://api.github.com/repos/hypothesis/lms/subscription","commits_url":"https://api.github.com/repos/hypothesis/lms/commits{/sha}","git_commits_url":"https://api.github.com/repos/hypothesis/lms/git/commits{/sha}","comments_url":"https://api.github.com/repos/hypothesis/lms/comments{/number}","issue_comment_url":"https://api.github.com/repos/hypothesis/lms/issues/comments{/number}","contents_url":"https://api.github.com/repos/hypothesis/lms/contents/{+path}","compare_url":"https://api.github.com/repos/hypothesis/lms/compare/{base}...{head}","merges_url":"https://api.github.com/repos/hypothesis/lms/merges","archive_url":"https://api.github.com/repos/hypothesis/lms/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/hypothesis/lms/downloads","issues_url":"https://api.github.com/repos/hypothesis/lms/issues{/number}","pulls_url":"https://api.github.com/repos/hypothesis/lms/pulls{/number}","milestones_url":"https://api.github.com/repos/hypothesis/lms/milestones{/number}","notifications_url":"https://api.github.com/repos/hypothesis/lms/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/hypothesis/lms/labels{/name}","releases_url":"https://api.github.com/repos/hypothesis/lms/releases{/id}","deployments_url":"https://api.github.com/repos/hypothesis/lms/deployments","created_at":"2017-10-30T21:15:37Z","updated_at":"2018-10-30T14:07:52Z","pushed_at":"2018-10-30T14:07:53Z","git_url":"git://github.com/hypothesis/lms.git","ssh_url":"git@github.com:hypothesis/lms.git","clone_url":"https://github.com/hypothesis/lms.git","svn_url":"https://github.com/hypothesis/lms","homepage":null,"size":5297,"stargazers_count":2,"watchers_count":2,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1,"mirror_url":null,"archived":false,"open_issues_count":32,"license":{"key":"bsd-2-clause","name":"BSD 2-Clause \"Simplified\" License","spdx_id":"BSD-2-Clause","url":"https://api.github.com/licenses/bsd-2-clause","node_id":"MDc6TGljZW5zZTQ="},"forks":1,"open_issues":32,"watchers":2,"default_branch":"master"}},"base":{"label":"hypothesis:master","ref":"master","sha":"1f0a26ae9b7716eed9df278c4120603d45bab4fa","user":{"login":"hypothesis","id":1243215,"node_id":"MDEyOk9yZ2FuaXphdGlvbjEyNDMyMTU=","avatar_url":"https://avatars1.githubusercontent.com/u/1243215?v=4","gravatar_id":"","url":"https://api.github.com/users/hypothesis","html_url":"https://github.com/hypothesis","followers_url":"https://api.github.com/users/hypothesis/followers","following_url":"https://api.github.com/users/hypothesis/following{/other_user}","gists_url":"https://api.github.com/users/hypothesis/gists{/gist_id}","starred_url":"https://api.github.com/users/hypothesis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hypothesis/subscriptions","organizations_url":"https://api.github.com/users/hypothesis/orgs","repos_url":"https://api.github.com/users/hypothesis/repos","events_url":"https://api.github.com/users/hypothesis/events{/privacy}","received_events_url":"https://api.github.com/users/hypothesis/received_events","type":"Organization","site_admin":false},"repo":{"id":108909388,"node_id":"MDEwOlJlcG9zaXRvcnkxMDg5MDkzODg=","name":"lms","full_name":"hypothesis/lms","private":false,"owner":{"login":"hypothesis","id":1243215,"node_id":"MDEyOk9yZ2FuaXphdGlvbjEyNDMyMTU=","avatar_url":"https://avatars1.githubusercontent.com/u/1243215?v=4","gravatar_id":"","url":"https://api.github.com/users/hypothesis","html_url":"https://github.com/hypothesis","followers_url":"https://api.github.com/users/hypothesis/followers","following_url":"https://api.github.com/users/hypothesis/following{/other_user}","gists_url":"https://api.github.com/users/hypothesis/gists{/gist_id}","starred_url":"https://api.github.com/users/hypothesis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hypothesis/subscriptions","organizations_url":"https://api.github.com/users/hypothesis/orgs","repos_url":"https://api.github.com/users/hypothesis/repos","events_url":"https://api.github.com/users/hypothesis/events{/privacy}","received_events_url":"https://api.github.com/users/hypothesis/received_events","type":"Organization","site_admin":false},"html_url":"https://github.com/hypothesis/lms","description":"LTI app for integrating with learning management systems","fork":false,"url":"https://api.github.com/repos/hypothesis/lms","forks_url":"https://api.github.com/repos/hypothesis/lms/forks","keys_url":"https://api.github.com/repos/hypothesis/lms/keys{/key_id}","collaborators_url":"https://api.github.com/repos/hypothesis/lms/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/hypothesis/lms/teams","hooks_url":"https://api.github.com/repos/hypothesis/lms/hooks","issue_events_url":"https://api.github.com/repos/hypothesis/lms/issues/events{/number}","events_url":"https://api.github.com/repos/hypothesis/lms/events","assignees_url":"https://api.github.com/repos/hypothesis/lms/assignees{/user}","branches_url":"https://api.github.com/repos/hypothesis/lms/branches{/branch}","tags_url":"https://api.github.com/repos/hypothesis/lms/tags","blobs_url":"https://api.github.com/repos/hypothesis/lms/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/hypothesis/lms/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/hypothesis/lms/git/refs{/sha}","trees_url":"https://api.github.com/repos/hypothesis/lms/git/trees{/sha}","statuses_url":"https://api.github.com/repos/hypothesis/lms/statuses/{sha}","languages_url":"https://api.github.com/repos/hypothesis/lms/languages","stargazers_url":"https://api.github.com/repos/hypothesis/lms/stargazers","contributors_url":"https://api.github.com/repos/hypothesis/lms/contributors","subscribers_url":"https://api.github.com/repos/hypothesis/lms/subscribers","subscription_url":"https://api.github.com/repos/hypothesis/lms/subscription","commits_url":"https://api.github.com/repos/hypothesis/lms/commits{/sha}","git_commits_url":"https://api.github.com/repos/hypothesis/lms/git/commits{/sha}","comments_url":"https://api.github.com/repos/hypothesis/lms/comments{/number}","issue_comment_url":"https://api.github.com/repos/hypothesis/lms/issues/comments{/number}","contents_url":"https://api.github.com/repos/hypothesis/lms/contents/{+path}","compare_url":"https://api.github.com/repos/hypothesis/lms/compare/{base}...{head}","merges_url":"https://api.github.com/repos/hypothesis/lms/merges","archive_url":"https://api.github.com/repos/hypothesis/lms/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/hypothesis/lms/downloads","issues_url":"https://api.github.com/repos/hypothesis/lms/issues{/number}","pulls_url":"https://api.github.com/repos/hypothesis/lms/pulls{/number}","milestones_url":"https://api.github.com/repos/hypothesis/lms/milestones{/number}","notifications_url":"https://api.github.com/repos/hypothesis/lms/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/hypothesis/lms/labels{/name}","releases_url":"https://api.github.com/repos/hypothesis/lms/releases{/id}","deployments_url":"https://api.github.com/repos/hypothesis/lms/deployments","created_at":"2017-10-30T21:15:37Z","updated_at":"2018-10-30T14:07:52Z","pushed_at":"2018-10-30T14:07:53Z","git_url":"git://github.com/hypothesis/lms.git","ssh_url":"git@github.com:hypothesis/lms.git","clone_url":"https://github.com/hypothesis/lms.git","svn_url":"https://github.com/hypothesis/lms","homepage":null,"size":5297,"stargazers_count":2,"watchers_count":2,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1,"mirror_url":null,"archived":false,"open_issues_count":32,"license":{"key":"bsd-2-clause","name":"BSD 2-Clause \"Simplified\" License","spdx_id":"BSD-2-Clause","url":"https://api.github.com/licenses/bsd-2-clause","node_id":"MDc6TGljZW5zZTQ="},"forks":1,"open_issues":32,"watchers":2,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/hypothesis/lms/pulls/304"},"html":{"href":"https://github.com/hypothesis/lms/pull/304"},"issue":{"href":"https://api.github.com/repos/hypothesis/lms/issues/304"},"comments":{"href":"https://api.github.com/repos/hypothesis/lms/issues/304/comments"},"review_comments":{"href":"https://api.github.com/repos/hypothesis/lms/pulls/304/comments"},"review_comment":{"href":"https://api.github.com/repos/hypothesis/lms/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/hypothesis/lms/pulls/304/commits"},"statuses":{"href":"https://api.github.com/repos/hypothesis/lms/statuses/9329d6486d0f52fddf54db93391c4d633792cd95"}},"author_association":"COLLABORATOR"}}
{"actor":{"display_login":"seanh"}}