GitHub PullRequestReviewCommentEvent at 2014-07-21 10:14:41
Repository: transition
Actor: rgarner
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/alphagov/transition/pulls/comments/15161812","id":15161812,"diff_hunk":"@@ -0,0 +1,74 @@\n+module Transition\n+  class ImportBatchRow\n+    include Comparable\n+\n+    attr_reader :line_number, :old_value, :new_value\n+\n+    def initialize(site, line_number, csv_row)\n+      @site = site\n+      @line_number = line_number\n+      @old_value = csv_row[0].strip\n+      @new_value = csv_row[1].present? ? csv_row[1].strip : nil\n+    end\n+\n+    def data_row?\n+      @old_value.starts_with?('/') || @old_value.starts_with?('http')\n+    end\n+\n+    def type\n+      @_type ||= case\n+                 when new_value && ((new_value.upcase == 'TNA') || new_url_is_a_national_archives_url?) then 'archive'","path":"lib/transition/import_batch_row.rb","position":20,"original_position":20,"commit_id":"e0e546bb093629eb3806e87029f82b9f509b5ee8","original_commit_id":"12d7142fc0db7b43d5014a0912742bcae19a4302","user":{"login":"rgarner","id":194511,"avatar_url":"https://avatars.githubusercontent.com/u/194511?","gravatar_id":"661a0d711ce4a64290ae2980189e9efb","url":"https://api.github.com/users/rgarner","html_url":"https://github.com/rgarner","followers_url":"https://api.github.com/users/rgarner/followers","following_url":"https://api.github.com/users/rgarner/following{/other_user}","gists_url":"https://api.github.com/users/rgarner/gists{/gist_id}","starred_url":"https://api.github.com/users/rgarner/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rgarner/subscriptions","organizations_url":"https://api.github.com/users/rgarner/orgs","repos_url":"https://api.github.com/users/rgarner/repos","events_url":"https://api.github.com/users/rgarner/events{/privacy}","received_events_url":"https://api.github.com/users/rgarner/received_events","type":"User","site_admin":false},"body":"Good point. Lazy spot. Comment retracted.","created_at":"2014-07-21T10:14:41Z","updated_at":"2014-07-21T10:14:41Z","html_url":"https://github.com/alphagov/transition/pull/334#discussion_r15161812","pull_request_url":"https://api.github.com/repos/alphagov/transition/pulls/334","_links":{"self":{"href":"https://api.github.com/repos/alphagov/transition/pulls/comments/15161812"},"html":{"href":"https://github.com/alphagov/transition/pull/334#discussion_r15161812"},"pull_request":{"href":"https://api.github.com/repos/alphagov/transition/pulls/334"}}},"pull_request":{"url":"https://api.github.com/repos/alphagov/transition/pulls/334","id":17716235,"html_url":"https://github.com/alphagov/transition/pull/334","diff_url":"https://github.com/alphagov/transition/pull/334.diff","patch_url":"https://github.com/alphagov/transition/pull/334.patch","issue_url":"https://api.github.com/repos/alphagov/transition/issues/334","number":334,"state":"open","title":"Import mappings","user":{"login":"jennyd","id":1822424,"avatar_url":"https://avatars.githubusercontent.com/u/1822424?","gravatar_id":"52e8f48e7acb85f790bac54cea6bd2a1","url":"https://api.github.com/users/jennyd","html_url":"https://github.com/jennyd","followers_url":"https://api.github.com/users/jennyd/followers","following_url":"https://api.github.com/users/jennyd/following{/other_user}","gists_url":"https://api.github.com/users/jennyd/gists{/gist_id}","starred_url":"https://api.github.com/users/jennyd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jennyd/subscriptions","organizations_url":"https://api.github.com/users/jennyd/orgs","repos_url":"https://api.github.com/users/jennyd/repos","events_url":"https://api.github.com/users/jennyd/events{/privacy}","received_events_url":"https://api.github.com/users/jennyd/received_events","type":"User","site_admin":false},"body":"[Giant story](https://www.pivotaltracker.com/n/projects/887836/stories/69025116), giant PR :mount_fuji: :whale: :ocean: :whale2: :elephant: :earth_asia: \r\n\r\n__Things to note:__\r\n- rows which don't start with something which can be parsed and canonicalized to a path are ignored silently, This means that we exclude header rows but also possibly data rows which were eg missing a leading slash. This is consistent with bulk add's behaviour.\r\n- detecting whether the input is CSV or TSV is done very simplistically.\r\n- if the second column contains a TNA URL, the mapping is made an archive but the custom URL is not preserved; we're making that behaviour a separate story.\r\n- we haven't done anything special with spaces in the input; URLs could contain unencoded spaces, so trying to ignore or split on them would be incorrect.\r\n- entries are displayed on the preview page in insert order; it's not clear what the expected ordering would otherwise be, bearing in mind that canonicalization and deduplication will have changed the apparent order from the input anyway.\r\n- we don't consider the type of existing mappings when overwriting them; if the option to overwrite is selected, all existing mappings will be overwritten even if they were eg redirects and the import says they should be unresolved. If we want different behaviour, that will be a separate story.\r\n\r\n__Things left to change/do:__\r\n- make the validation error messages a bit more specific, eg by including the relevant URL in every case\r\n- test with very large batches","created_at":"2014-06-27T17:52:31Z","updated_at":"2014-07-21T10:14:41Z","closed_at":null,"merged_at":null,"merge_commit_sha":"f06fee391e29d322fb01e9cf01b0dac1cf62e711","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/alphagov/transition/pulls/334/commits","review_comments_url":"https://api.github.com/repos/alphagov/transition/pulls/334/comments","review_comment_url":"https://api.github.com/repos/alphagov/transition/pulls/comments/{number}","comments_url":"https://api.github.com/repos/alphagov/transition/issues/334/comments","statuses_url":"https://api.github.com/repos/alphagov/transition/statuses/e0e546bb093629eb3806e87029f82b9f509b5ee8","head":{"label":"alphagov:import_mappings","ref":"import_mappings","sha":"e0e546bb093629eb3806e87029f82b9f509b5ee8","user":{"login":"alphagov","id":596977,"avatar_url":"https://avatars.githubusercontent.com/u/596977?","gravatar_id":"5618b843830194a2475a6e48823f7455","url":"https://api.github.com/users/alphagov","html_url":"https://github.com/alphagov","followers_url":"https://api.github.com/users/alphagov/followers","following_url":"https://api.github.com/users/alphagov/following{/other_user}","gists_url":"https://api.github.com/users/alphagov/gists{/gist_id}","starred_url":"https://api.github.com/users/alphagov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alphagov/subscriptions","organizations_url":"https://api.github.com/users/alphagov/orgs","repos_url":"https://api.github.com/users/alphagov/repos","events_url":"https://api.github.com/users/alphagov/events{/privacy}","received_events_url":"https://api.github.com/users/alphagov/received_events","type":"Organization","site_admin":false},"repo":{"id":12729833,"name":"transition","full_name":"alphagov/transition","owner":{"login":"alphagov","id":596977,"avatar_url":"https://avatars.githubusercontent.com/u/596977?","gravatar_id":"5618b843830194a2475a6e48823f7455","url":"https://api.github.com/users/alphagov","html_url":"https://github.com/alphagov","followers_url":"https://api.github.com/users/alphagov/followers","following_url":"https://api.github.com/users/alphagov/following{/other_user}","gists_url":"https://api.github.com/users/alphagov/gists{/gist_id}","starred_url":"https://api.github.com/users/alphagov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alphagov/subscriptions","organizations_url":"https://api.github.com/users/alphagov/orgs","repos_url":"https://api.github.com/users/alphagov/repos","events_url":"https://api.github.com/users/alphagov/events{/privacy}","received_events_url":"https://api.github.com/users/alphagov/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/alphagov/transition","description":"Managing mappings (eg redirects) for sites moving to GOV.UK.","fork":false,"url":"https://api.github.com/repos/alphagov/transition","forks_url":"https://api.github.com/repos/alphagov/transition/forks","keys_url":"https://api.github.com/repos/alphagov/transition/keys{/key_id}","collaborators_url":"https://api.github.com/repos/alphagov/transition/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/alphagov/transition/teams","hooks_url":"https://api.github.com/repos/alphagov/transition/hooks","issue_events_url":"https://api.github.com/repos/alphagov/transition/issues/events{/number}","events_url":"https://api.github.com/repos/alphagov/transition/events","assignees_url":"https://api.github.com/repos/alphagov/transition/assignees{/user}","branches_url":"https://api.github.com/repos/alphagov/transition/branches{/branch}","tags_url":"https://api.github.com/repos/alphagov/transition/tags","blobs_url":"https://api.github.com/repos/alphagov/transition/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/alphagov/transition/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/alphagov/transition/git/refs{/sha}","trees_url":"https://api.github.com/repos/alphagov/transition/git/trees{/sha}","statuses_url":"https://api.github.com/repos/alphagov/transition/statuses/{sha}","languages_url":"https://api.github.com/repos/alphagov/transition/languages","stargazers_url":"https://api.github.com/repos/alphagov/transition/stargazers","contributors_url":"https://api.github.com/repos/alphagov/transition/contributors","subscribers_url":"https://api.github.com/repos/alphagov/transition/subscribers","subscription_url":"https://api.github.com/repos/alphagov/transition/subscription","commits_url":"https://api.github.com/repos/alphagov/transition/commits{/sha}","git_commits_url":"https://api.github.com/repos/alphagov/transition/git/commits{/sha}","comments_url":"https://api.github.com/repos/alphagov/transition/comments{/number}","issue_comment_url":"https://api.github.com/repos/alphagov/transition/issues/comments/{number}","contents_url":"https://api.github.com/repos/alphagov/transition/contents/{+path}","compare_url":"https://api.github.com/repos/alphagov/transition/compare/{base}...{head}","merges_url":"https://api.github.com/repos/alphagov/transition/merges","archive_url":"https://api.github.com/repos/alphagov/transition/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/alphagov/transition/downloads","issues_url":"https://api.github.com/repos/alphagov/transition/issues{/number}","pulls_url":"https://api.github.com/repos/alphagov/transition/pulls{/number}","milestones_url":"https://api.github.com/repos/alphagov/transition/milestones{/number}","notifications_url":"https://api.github.com/repos/alphagov/transition/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/alphagov/transition/labels{/name}","releases_url":"https://api.github.com/repos/alphagov/transition/releases{/id}","created_at":"2013-09-10T13:11:17Z","updated_at":"2014-06-26T08:40:36Z","pushed_at":"2014-07-18T16:35:55Z","git_url":"git://github.com/alphagov/transition.git","ssh_url":"git@github.com:alphagov/transition.git","clone_url":"https://github.com/alphagov/transition.git","svn_url":"https://github.com/alphagov/transition","homepage":null,"size":20698,"stargazers_count":1,"watchers_count":1,"language":"Ruby","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":4,"mirror_url":null,"open_issues_count":4,"forks":4,"open_issues":4,"watchers":1,"default_branch":"master"}},"base":{"label":"alphagov:master","ref":"master","sha":"76556ae13bc3d162c2ee5a523b855c262097a373","user":{"login":"alphagov","id":596977,"avatar_url":"https://avatars.githubusercontent.com/u/596977?","gravatar_id":"5618b843830194a2475a6e48823f7455","url":"https://api.github.com/users/alphagov","html_url":"https://github.com/alphagov","followers_url":"https://api.github.com/users/alphagov/followers","following_url":"https://api.github.com/users/alphagov/following{/other_user}","gists_url":"https://api.github.com/users/alphagov/gists{/gist_id}","starred_url":"https://api.github.com/users/alphagov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alphagov/subscriptions","organizations_url":"https://api.github.com/users/alphagov/orgs","repos_url":"https://api.github.com/users/alphagov/repos","events_url":"https://api.github.com/users/alphagov/events{/privacy}","received_events_url":"https://api.github.com/users/alphagov/received_events","type":"Organization","site_admin":false},"repo":{"id":12729833,"name":"transition","full_name":"alphagov/transition","owner":{"login":"alphagov","id":596977,"avatar_url":"https://avatars.githubusercontent.com/u/596977?","gravatar_id":"5618b843830194a2475a6e48823f7455","url":"https://api.github.com/users/alphagov","html_url":"https://github.com/alphagov","followers_url":"https://api.github.com/users/alphagov/followers","following_url":"https://api.github.com/users/alphagov/following{/other_user}","gists_url":"https://api.github.com/users/alphagov/gists{/gist_id}","starred_url":"https://api.github.com/users/alphagov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alphagov/subscriptions","organizations_url":"https://api.github.com/users/alphagov/orgs","repos_url":"https://api.github.com/users/alphagov/repos","events_url":"https://api.github.com/users/alphagov/events{/privacy}","received_events_url":"https://api.github.com/users/alphagov/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/alphagov/transition","description":"Managing mappings (eg redirects) for sites moving to GOV.UK.","fork":false,"url":"https://api.github.com/repos/alphagov/transition","forks_url":"https://api.github.com/repos/alphagov/transition/forks","keys_url":"https://api.github.com/repos/alphagov/transition/keys{/key_id}","collaborators_url":"https://api.github.com/repos/alphagov/transition/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/alphagov/transition/teams","hooks_url":"https://api.github.com/repos/alphagov/transition/hooks","issue_events_url":"https://api.github.com/repos/alphagov/transition/issues/events{/number}","events_url":"https://api.github.com/repos/alphagov/transition/events","assignees_url":"https://api.github.com/repos/alphagov/transition/assignees{/user}","branches_url":"https://api.github.com/repos/alphagov/transition/branches{/branch}","tags_url":"https://api.github.com/repos/alphagov/transition/tags","blobs_url":"https://api.github.com/repos/alphagov/transition/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/alphagov/transition/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/alphagov/transition/git/refs{/sha}","trees_url":"https://api.github.com/repos/alphagov/transition/git/trees{/sha}","statuses_url":"https://api.github.com/repos/alphagov/transition/statuses/{sha}","languages_url":"https://api.github.com/repos/alphagov/transition/languages","stargazers_url":"https://api.github.com/repos/alphagov/transition/stargazers","contributors_url":"https://api.github.com/repos/alphagov/transition/contributors","subscribers_url":"https://api.github.com/repos/alphagov/transition/subscribers","subscription_url":"https://api.github.com/repos/alphagov/transition/subscription","commits_url":"https://api.github.com/repos/alphagov/transition/commits{/sha}","git_commits_url":"https://api.github.com/repos/alphagov/transition/git/commits{/sha}","comments_url":"https://api.github.com/repos/alphagov/transition/comments{/number}","issue_comment_url":"https://api.github.com/repos/alphagov/transition/issues/comments/{number}","contents_url":"https://api.github.com/repos/alphagov/transition/contents/{+path}","compare_url":"https://api.github.com/repos/alphagov/transition/compare/{base}...{head}","merges_url":"https://api.github.com/repos/alphagov/transition/merges","archive_url":"https://api.github.com/repos/alphagov/transition/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/alphagov/transition/downloads","issues_url":"https://api.github.com/repos/alphagov/transition/issues{/number}","pulls_url":"https://api.github.com/repos/alphagov/transition/pulls{/number}","milestones_url":"https://api.github.com/repos/alphagov/transition/milestones{/number}","notifications_url":"https://api.github.com/repos/alphagov/transition/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/alphagov/transition/labels{/name}","releases_url":"https://api.github.com/repos/alphagov/transition/releases{/id}","created_at":"2013-09-10T13:11:17Z","updated_at":"2014-06-26T08:40:36Z","pushed_at":"2014-07-18T16:35:55Z","git_url":"git://github.com/alphagov/transition.git","ssh_url":"git@github.com:alphagov/transition.git","clone_url":"https://github.com/alphagov/transition.git","svn_url":"https://github.com/alphagov/transition","homepage":null,"size":20698,"stargazers_count":1,"watchers_count":1,"language":"Ruby","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":4,"mirror_url":null,"open_issues_count":4,"forks":4,"open_issues":4,"watchers":1,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/alphagov/transition/pulls/334"},"html":{"href":"https://github.com/alphagov/transition/pull/334"},"issue":{"href":"https://api.github.com/repos/alphagov/transition/issues/334"},"comments":{"href":"https://api.github.com/repos/alphagov/transition/issues/334/comments"},"review_comments":{"href":"https://api.github.com/repos/alphagov/transition/pulls/334/comments"},"review_comment":{"href":"https://api.github.com/repos/alphagov/transition/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/alphagov/transition/pulls/334/commits"},"statuses":{"href":"https://api.github.com/repos/alphagov/transition/statuses/e0e546bb093629eb3806e87029f82b9f509b5ee8"}}}}