{"action":"created","comment":{"url":"https://api.github.com/repos/hypothesis/h/pulls/comments/61941330","id":61941330,"diff_hunk":"@@ -42,13 +42,13 @@ def test_it_sets_appropriate_error_message_when_data_invalid(self):\n class TestAnnotationSchema(object):","path":"h/api/test/schemas_test.py","position":1,"original_position":1,"commit_id":"6985ab765e0b8dd2b65cd167471c4019975cd491","original_commit_id":"ea782bf4b73c72234ede4a5a3524c4330aaef612","user":{"login":"seanh","id":22498,"avatar_url":"https://avatars.githubusercontent.com/u/22498?v=3","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":"Yeah they're very repetitive, it does look like a strong candidate for a `parametrize()`, definitely.\r\n\r\nI _do_ think the tests add value, though. Because I think it's totally possible to create a json schema that I think does one thing, but actually doesn't do exactly what I think, and these tests make sure that it does. The json schema is a complex and very nested data structure that it's totally easy (and I did) make mistakes with. It's either test everything manually or write a set of unit tests.\r\n\r\nAlso when making changes to a jsonschema, the corresponding unit test changes in the same commit will document (probably better than the commit message) exactly what the programmer was trying to change.\r\n\r\nThese tests would also allow us to substitute jsonschema with something else (that had the same interface) and know we hadn't changed behaviour.\r\n\r\nIn what ways are they too coupled to details of jsonschema? Other than the error message strings, but I don't think there's much to be done about that, I do want to test that when the validation is giving an error it's the error that I think it's giving. Otherwise the validation when called by the test could be failing for some completely different reason (e.g. there is something else invalid about the test data), the actual thing I'm trying to test could be broken, and the test could still be passing.\r\n\r\nAlso I just want to test that we're giving out the right error messages.\r\n\r\nI don't think there's any other way to test \"jsonschema validation failed for the reason I expected it to fail\" other than asserting against the error message strings?","created_at":"2016-05-03T19:32:39Z","updated_at":"2016-05-03T19:32:39Z","html_url":"https://github.com/hypothesis/h/pull/3256#discussion_r61941330","pull_request_url":"https://api.github.com/repos/hypothesis/h/pulls/3256","_links":{"self":{"href":"https://api.github.com/repos/hypothesis/h/pulls/comments/61941330"},"html":{"href":"https://github.com/hypothesis/h/pull/3256#discussion_r61941330"},"pull_request":{"href":"https://api.github.com/repos/hypothesis/h/pulls/3256"}}},"pull_request":{"url":"https://api.github.com/repos/hypothesis/h/pulls/3256","id":68093073,"html_url":"https://github.com/hypothesis/h/pull/3256","diff_url":"https://github.com/hypothesis/h/pull/3256.diff","patch_url":"https://github.com/hypothesis/h/pull/3256.patch","issue_url":"https://api.github.com/repos/hypothesis/h/issues/3256","number":3256,"state":"open","locked":false,"title":"View and storage code for updating annotations in Postgres","user":{"login":"seanh","id":22498,"avatar_url":"https://avatars.githubusercontent.com/u/22498?v=3","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":"This pull request contains:\r\n\r\n* New `views` and `storage` code for updating annotations in Postgres.\r\n\r\n* New code to index annotation updates into the new Es index synchronously\r\n\r\n* Bug fix: don't crash when fetching an annotation with an invalid ID from postgres (iirc this happens when creating an annotation with an invalid ID in its references)\r\n\r\n* Changes to the `schemas` code for updating annotations in Postgres, which had already been merged into master. To be compatible with the legacy behaviour we need to allow partial updates (sort of `PATCH`-like) when only some of the fields of an annotation are sent in an update annotation request. On master if you omit the tags field in an update for example, for the postgres storage it will do the same thing it does on a create: it will replace any existing tags with the default value which is an empty list. Same for the rest of the fields. But the legacy behaviour in this instance is that if no tags are passed in an update (not even an empty list) then _don't change the annotation's tags_.\r\n\r\n   Updating the Postgres code to match the legacy code in this regard is actually a change to `schemas` not to `storage`. `storage` just accepts whatever fields it gets from `schemas` and saves them to the model, so if `schemas` sends `'tags': []` then `storage` will overwrite the annotation's existing tags with an empty list. So we change schemas to send no `'tags'` key in the data, if none was sent in the request.\r\n\r\n* Change to `schemas` which so that it prevents you from changing the references or group of an annotation by deleting these from the data before it gets sent to `storage`. <del>That is unfortunately currently mixed in with other changes in a commit whose commit message describes only those other changes, I'll split it out tomorrow.</del> (done)\r\n\r\n* Change to `schemas` so that `permissions` is no longer a required field for creates and updates. Makes it consistent with the rest of the fields","created_at":"2016-04-27T17:32:28Z","updated_at":"2016-05-03T19:32:39Z","closed_at":null,"merged_at":null,"merge_commit_sha":"fb9878dfd0727f3c602078fd5f6fe0ca93a5b043","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/hypothesis/h/pulls/3256/commits","review_comments_url":"https://api.github.com/repos/hypothesis/h/pulls/3256/comments","review_comment_url":"https://api.github.com/repos/hypothesis/h/pulls/comments{/number}","comments_url":"https://api.github.com/repos/hypothesis/h/issues/3256/comments","statuses_url":"https://api.github.com/repos/hypothesis/h/statuses/6985ab765e0b8dd2b65cd167471c4019975cd491","head":{"label":"hypothesis:update-annotations-in-postgres-storage-2","ref":"update-annotations-in-postgres-storage-2","sha":"6985ab765e0b8dd2b65cd167471c4019975cd491","user":{"login":"hypothesis","id":1243215,"avatar_url":"https://avatars.githubusercontent.com/u/1243215?v=3","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":3910945,"name":"h","full_name":"hypothesis/h","owner":{"login":"hypothesis","id":1243215,"avatar_url":"https://avatars.githubusercontent.com/u/1243215?v=3","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},"private":false,"html_url":"https://github.com/hypothesis/h","description":"The Internet, peer reviewed.","fork":false,"url":"https://api.github.com/repos/hypothesis/h","forks_url":"https://api.github.com/repos/hypothesis/h/forks","keys_url":"https://api.github.com/repos/hypothesis/h/keys{/key_id}","collaborators_url":"https://api.github.com/repos/hypothesis/h/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/hypothesis/h/teams","hooks_url":"https://api.github.com/repos/hypothesis/h/hooks","issue_events_url":"https://api.github.com/repos/hypothesis/h/issues/events{/number}","events_url":"https://api.github.com/repos/hypothesis/h/events","assignees_url":"https://api.github.com/repos/hypothesis/h/assignees{/user}","branches_url":"https://api.github.com/repos/hypothesis/h/branches{/branch}","tags_url":"https://api.github.com/repos/hypothesis/h/tags","blobs_url":"https://api.github.com/repos/hypothesis/h/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/hypothesis/h/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/hypothesis/h/git/refs{/sha}","trees_url":"https://api.github.com/repos/hypothesis/h/git/trees{/sha}","statuses_url":"https://api.github.com/repos/hypothesis/h/statuses/{sha}","languages_url":"https://api.github.com/repos/hypothesis/h/languages","stargazers_url":"https://api.github.com/repos/hypothesis/h/stargazers","contributors_url":"https://api.github.com/repos/hypothesis/h/contributors","subscribers_url":"https://api.github.com/repos/hypothesis/h/subscribers","subscription_url":"https://api.github.com/repos/hypothesis/h/subscription","commits_url":"https://api.github.com/repos/hypothesis/h/commits{/sha}","git_commits_url":"https://api.github.com/repos/hypothesis/h/git/commits{/sha}","comments_url":"https://api.github.com/repos/hypothesis/h/comments{/number}","issue_comment_url":"https://api.github.com/repos/hypothesis/h/issues/comments{/number}","contents_url":"https://api.github.com/repos/hypothesis/h/contents/{+path}","compare_url":"https://api.github.com/repos/hypothesis/h/compare/{base}...{head}","merges_url":"https://api.github.com/repos/hypothesis/h/merges","archive_url":"https://api.github.com/repos/hypothesis/h/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/hypothesis/h/downloads","issues_url":"https://api.github.com/repos/hypothesis/h/issues{/number}","pulls_url":"https://api.github.com/repos/hypothesis/h/pulls{/number}","milestones_url":"https://api.github.com/repos/hypothesis/h/milestones{/number}","notifications_url":"https://api.github.com/repos/hypothesis/h/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/hypothesis/h/labels{/name}","releases_url":"https://api.github.com/repos/hypothesis/h/releases{/id}","deployments_url":"https://api.github.com/repos/hypothesis/h/deployments","created_at":"2012-04-02T19:56:59Z","updated_at":"2016-05-02T20:23:37Z","pushed_at":"2016-05-03T19:17:18Z","git_url":"git://github.com/hypothesis/h.git","ssh_url":"git@github.com:hypothesis/h.git","clone_url":"https://github.com/hypothesis/h.git","svn_url":"https://github.com/hypothesis/h","homepage":"https://hypothes.is/","size":29661,"stargazers_count":628,"watchers_count":628,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":148,"mirror_url":null,"open_issues_count":68,"forks":148,"open_issues":68,"watchers":628,"default_branch":"master"}},"base":{"label":"hypothesis:master","ref":"master","sha":"dfac3a7c8da031b09eb6362e2a898f8f57c1f0f6","user":{"login":"hypothesis","id":1243215,"avatar_url":"https://avatars.githubusercontent.com/u/1243215?v=3","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":3910945,"name":"h","full_name":"hypothesis/h","owner":{"login":"hypothesis","id":1243215,"avatar_url":"https://avatars.githubusercontent.com/u/1243215?v=3","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},"private":false,"html_url":"https://github.com/hypothesis/h","description":"The Internet, peer reviewed.","fork":false,"url":"https://api.github.com/repos/hypothesis/h","forks_url":"https://api.github.com/repos/hypothesis/h/forks","keys_url":"https://api.github.com/repos/hypothesis/h/keys{/key_id}","collaborators_url":"https://api.github.com/repos/hypothesis/h/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/hypothesis/h/teams","hooks_url":"https://api.github.com/repos/hypothesis/h/hooks","issue_events_url":"https://api.github.com/repos/hypothesis/h/issues/events{/number}","events_url":"https://api.github.com/repos/hypothesis/h/events","assignees_url":"https://api.github.com/repos/hypothesis/h/assignees{/user}","branches_url":"https://api.github.com/repos/hypothesis/h/branches{/branch}","tags_url":"https://api.github.com/repos/hypothesis/h/tags","blobs_url":"https://api.github.com/repos/hypothesis/h/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/hypothesis/h/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/hypothesis/h/git/refs{/sha}","trees_url":"https://api.github.com/repos/hypothesis/h/git/trees{/sha}","statuses_url":"https://api.github.com/repos/hypothesis/h/statuses/{sha}","languages_url":"https://api.github.com/repos/hypothesis/h/languages","stargazers_url":"https://api.github.com/repos/hypothesis/h/stargazers","contributors_url":"https://api.github.com/repos/hypothesis/h/contributors","subscribers_url":"https://api.github.com/repos/hypothesis/h/subscribers","subscription_url":"https://api.github.com/repos/hypothesis/h/subscription","commits_url":"https://api.github.com/repos/hypothesis/h/commits{/sha}","git_commits_url":"https://api.github.com/repos/hypothesis/h/git/commits{/sha}","comments_url":"https://api.github.com/repos/hypothesis/h/comments{/number}","issue_comment_url":"https://api.github.com/repos/hypothesis/h/issues/comments{/number}","contents_url":"https://api.github.com/repos/hypothesis/h/contents/{+path}","compare_url":"https://api.github.com/repos/hypothesis/h/compare/{base}...{head}","merges_url":"https://api.github.com/repos/hypothesis/h/merges","archive_url":"https://api.github.com/repos/hypothesis/h/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/hypothesis/h/downloads","issues_url":"https://api.github.com/repos/hypothesis/h/issues{/number}","pulls_url":"https://api.github.com/repos/hypothesis/h/pulls{/number}","milestones_url":"https://api.github.com/repos/hypothesis/h/milestones{/number}","notifications_url":"https://api.github.com/repos/hypothesis/h/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/hypothesis/h/labels{/name}","releases_url":"https://api.github.com/repos/hypothesis/h/releases{/id}","deployments_url":"https://api.github.com/repos/hypothesis/h/deployments","created_at":"2012-04-02T19:56:59Z","updated_at":"2016-05-02T20:23:37Z","pushed_at":"2016-05-03T19:17:18Z","git_url":"git://github.com/hypothesis/h.git","ssh_url":"git@github.com:hypothesis/h.git","clone_url":"https://github.com/hypothesis/h.git","svn_url":"https://github.com/hypothesis/h","homepage":"https://hypothes.is/","size":29661,"stargazers_count":628,"watchers_count":628,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":148,"mirror_url":null,"open_issues_count":68,"forks":148,"open_issues":68,"watchers":628,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/hypothesis/h/pulls/3256"},"html":{"href":"https://github.com/hypothesis/h/pull/3256"},"issue":{"href":"https://api.github.com/repos/hypothesis/h/issues/3256"},"comments":{"href":"https://api.github.com/repos/hypothesis/h/issues/3256/comments"},"review_comments":{"href":"https://api.github.com/repos/hypothesis/h/pulls/3256/comments"},"review_comment":{"href":"https://api.github.com/repos/hypothesis/h/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/hypothesis/h/pulls/3256/commits"},"statuses":{"href":"https://api.github.com/repos/hypothesis/h/statuses/6985ab765e0b8dd2b65cd167471c4019975cd491"}}}}