{"action":"created","issue":{"url":"https://api.github.com/repos/opentrials/processors/issues/69","repository_url":"https://api.github.com/repos/opentrials/processors","labels_url":"https://api.github.com/repos/opentrials/processors/issues/69/labels{/name}","comments_url":"https://api.github.com/repos/opentrials/processors/issues/69/comments","events_url":"https://api.github.com/repos/opentrials/processors/issues/69/events","html_url":"https://github.com/opentrials/processors/pull/69","id":181241727,"number":69,"title":"[#432] Validate urls before writing to database","user":{"login":"georgiana-b","id":8862002,"avatar_url":"https://avatars.githubusercontent.com/u/8862002?v=3","gravatar_id":"","url":"https://api.github.com/users/georgiana-b","html_url":"https://github.com/georgiana-b","followers_url":"https://api.github.com/users/georgiana-b/followers","following_url":"https://api.github.com/users/georgiana-b/following{/other_user}","gists_url":"https://api.github.com/users/georgiana-b/gists{/gist_id}","starred_url":"https://api.github.com/users/georgiana-b/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/georgiana-b/subscriptions","organizations_url":"https://api.github.com/users/georgiana-b/orgs","repos_url":"https://api.github.com/users/georgiana-b/repos","events_url":"https://api.github.com/users/georgiana-b/events{/privacy}","received_events_url":"https://api.github.com/users/georgiana-b/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/opentrials/processors/labels/4.%20Ready%20for%20Review","name":"4. Ready for Review","color":"ededed"}],"state":"open","locked":false,"assignee":{"login":"vitorbaptista","id":76945,"avatar_url":"https://avatars.githubusercontent.com/u/76945?v=3","gravatar_id":"","url":"https://api.github.com/users/vitorbaptista","html_url":"https://github.com/vitorbaptista","followers_url":"https://api.github.com/users/vitorbaptista/followers","following_url":"https://api.github.com/users/vitorbaptista/following{/other_user}","gists_url":"https://api.github.com/users/vitorbaptista/gists{/gist_id}","starred_url":"https://api.github.com/users/vitorbaptista/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vitorbaptista/subscriptions","organizations_url":"https://api.github.com/users/vitorbaptista/orgs","repos_url":"https://api.github.com/users/vitorbaptista/repos","events_url":"https://api.github.com/users/vitorbaptista/events{/privacy}","received_events_url":"https://api.github.com/users/vitorbaptista/received_events","type":"User","site_admin":false},"assignees":[{"login":"vitorbaptista","id":76945,"avatar_url":"https://avatars.githubusercontent.com/u/76945?v=3","gravatar_id":"","url":"https://api.github.com/users/vitorbaptista","html_url":"https://github.com/vitorbaptista","followers_url":"https://api.github.com/users/vitorbaptista/followers","following_url":"https://api.github.com/users/vitorbaptista/following{/other_user}","gists_url":"https://api.github.com/users/vitorbaptista/gists{/gist_id}","starred_url":"https://api.github.com/users/vitorbaptista/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vitorbaptista/subscriptions","organizations_url":"https://api.github.com/users/vitorbaptista/orgs","repos_url":"https://api.github.com/users/vitorbaptista/repos","events_url":"https://api.github.com/users/vitorbaptista/events{/privacy}","received_events_url":"https://api.github.com/users/vitorbaptista/received_events","type":"User","site_admin":false}],"milestone":null,"comments":0,"created_at":"2016-10-05T19:17:58Z","updated_at":"2016-10-19T10:48:49Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/opentrials/processors/pulls/69","html_url":"https://github.com/opentrials/processors/pull/69","diff_url":"https://github.com/opentrials/processors/pull/69.diff","patch_url":"https://github.com/opentrials/processors/pull/69.patch"},"body":"opentrials/opentrials#432"},"comment":{"url":"https://api.github.com/repos/opentrials/processors/issues/comments/254778644","html_url":"https://github.com/opentrials/processors/pull/69#issuecomment-254778644","issue_url":"https://api.github.com/repos/opentrials/processors/issues/69","id":254778644,"user":{"login":"vitorbaptista","id":76945,"avatar_url":"https://avatars.githubusercontent.com/u/76945?v=3","gravatar_id":"","url":"https://api.github.com/users/vitorbaptista","html_url":"https://github.com/vitorbaptista","followers_url":"https://api.github.com/users/vitorbaptista/followers","following_url":"https://api.github.com/users/vitorbaptista/following{/other_user}","gists_url":"https://api.github.com/users/vitorbaptista/gists{/gist_id}","starred_url":"https://api.github.com/users/vitorbaptista/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vitorbaptista/subscriptions","organizations_url":"https://api.github.com/users/vitorbaptista/orgs","repos_url":"https://api.github.com/users/vitorbaptista/repos","events_url":"https://api.github.com/users/vitorbaptista/events{/privacy}","received_events_url":"https://api.github.com/users/vitorbaptista/received_events","type":"User","site_admin":false},"created_at":"2016-10-19T10:48:49Z","updated_at":"2016-10-19T10:48:49Z","body":"I understand the reasoning on mocking `helpers.validate_remote_url` on the writers' tests, so the test won't depend on the implementation of that method. However, it creates a drawback: this test now depends on that method existing and being used. If/when we refactor the helpers, we'll need to change not only the helpers themselves and who calls  them (all good and expected), but also all those tests.\r\n\r\nIf, instead of testing *how* the code is doing it's job, we focus on the outputs, that won't change. So the tests could be simplified to something like:\r\n\r\n```python\r\ndef test_writes_document_with_valid_url(self):\r\n    document = {\r\n      url: 'http://example.org',\r\n       // ...\r\n    }\r\n    conn = _get_mock_conn()\r\n    conn['database']['documents'].find_one.return_value = None\r\n\r\n    assert writers.write_document(conn, document) is not None\r\n\r\ndef test_skips_documents_with_invalid_url(self):\r\n    document = {\r\n      url: 'invalid-url',\r\n       // ...\r\n    }\r\n    conn = _get_mock_conn()\r\n    conn['database']['documents'].find_one.return_value = None\r\n\r\n    assert writers.write_document(conn, document) is None\r\n```\r\n\r\nNotice we're not even testing that the `write_document` returned the `id` of the document, but only that it was created, which is the focus of these tests. This simplifies the test code quite a bit, and makes them easier to maintain (as they won't be breaking unnecessarily).\r\n\r\nAlso, you just wrote tests for sources and documents, but the code also changes records, publications, and file. Please add tests for the others."}}
{"actor":{"display_login":"vitorbaptista"}}