{"action":"created","comment":{"url":"https://api.github.com/repos/alphagov/govwifi/pulls/comments/140279677","pull_request_review_id":64329144,"id":140279677,"diff_hunk":"@@ -93,40 +94,43 @@ public function writeToDB($stop = true) {\n         if ($stop) {\n             $stopField = \"stop=now(),\";\n         }\n-        $addAP = false;\n-        $apText = \"\";\n-        if (! empty($this->ap)) {\n-            $addAP = true;\n-            $apText = \" and ap=:ap \";\n-            error_log(\"Added AP: [\" . $this->ap. \"]\");\n-        }\n         $db = DB::getInstance();\n         $dbLink = $db->getConnection();\n         $handle = $dbLink->prepare(\n                 \"update session set \" . $stopField .\n-                    \" inMB=:inMB, outMB=:outMB, building_identifier=:buildingID \"\n-                . \"where siteIP=:siteIP and username=:username \"\n-                    . \"and stop is null and mac=:mac \" . $apText\n-                    . \"and start between :startmin and :startmax\");\n+                \" inMB=:inMB, outMB=:outMB, building_identifier=:buildingID \" .\n+                \"where siteIP=:siteIP and username=:username \" .\n+                    \"and stop is null and mac=:mac \" .\n+                    \"and start between :startmin and :startmax\");\n         // Some (300/11500) sessions are started multiple times in the same second, can\n         // NOT use \"order by start desc limit 1\" here for now.\n         // TODO (afoldesi-gds): Validate this logic further.\n         $startMin = strftime('%Y-%m-%d %H:%M:%S',$this->startTime-$window);\n         $startMax = strftime('%Y-%m-%d %H:%M:%S',$this->startTime+$window);\n         error_log(\n-                \"Updating record between \"\n-                . $startMin . \" and \" . $startMax . \" for \" . $this->login);\n+                \"Updating record between \" .\n+                $startMin . \" and \" . $startMax . \" for \" . $this->login);\n         $handle->bindValue(':startmin',   $startMin,                 PDO::PARAM_STR);\n         $handle->bindValue(':startmax',   $startMax,                 PDO::PARAM_STR);\n         $handle->bindValue(':siteIP',     $this->siteIP,             PDO::PARAM_STR);\n         $handle->bindValue(':username',   $this->login,              PDO::PARAM_STR);\n         $handle->bindValue(':mac',        $this->mac,                PDO::PARAM_STR);\n-        if ($addAP) {\n-            $handle->bindValue(':ap',     $this->ap,                 PDO::PARAM_STR);\n-        }\n         $handle->bindValue(':inMB',       $this->inMB(),             PDO::PARAM_INT);\n         $handle->bindValue(':outMB',      $this->outMB(),            PDO::PARAM_INT);\n         $handle->bindValue(':buildingID', $this->buildingIdentifier, PDO::PARAM_STR);\n-        $handle->execute();\n+        $success = false;\n+        try {\n+            $success = $handle->execute();\n+        } catch (PDOException $e) {\n+            error_log(\"Exception while updating session: \" .\n+                $e->getMessage() . \"|Trace: \" . implode(\"|\" . $e->getTrace()));\n+        }\n+        if ($success) {\n+            error_log(\"Session record updated. \" .\n+                $startMin . \" and \" . $startMax . \" for \" . $this->login);\n+        } else {\n+            error_log(\"Session update failed. \" .","path":"src/Session.php","position":49,"original_position":64,"commit_id":"131836cdbe3ed5258d6a0e5a07d100e98d512a8f","original_commit_id":"131836cdbe3ed5258d6a0e5a07d100e98d512a8f","user":{"login":"afoldesi-gds","id":22799567,"avatar_url":"https://avatars1.githubusercontent.com/u/22799567?v=4","gravatar_id":"","url":"https://api.github.com/users/afoldesi-gds","html_url":"https://github.com/afoldesi-gds","followers_url":"https://api.github.com/users/afoldesi-gds/followers","following_url":"https://api.github.com/users/afoldesi-gds/following{/other_user}","gists_url":"https://api.github.com/users/afoldesi-gds/gists{/gist_id}","starred_url":"https://api.github.com/users/afoldesi-gds/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/afoldesi-gds/subscriptions","organizations_url":"https://api.github.com/users/afoldesi-gds/orgs","repos_url":"https://api.github.com/users/afoldesi-gds/repos","events_url":"https://api.github.com/users/afoldesi-gds/events{/privacy}","received_events_url":"https://api.github.com/users/afoldesi-gds/received_events","type":"User","site_admin":false},"body":"The first point reduces the risk of deadlock as we only update the records when we have a building ID (which is the point of the update: we want to capture this as soon as possible)\r\n\r\nThe information gathering is of course so that we can \r\n- fail gracefully instead of an exception\r\n- gather data about the successful-ness of the update\r\nI'm hoping that this actually returns false when the update ran successfully but updated zero records. Is that not something you would expect.\r\n \r\n ","created_at":"2017-09-21T15:39:38Z","updated_at":"2017-09-21T15:39:38Z","html_url":"https://github.com/alphagov/govwifi/pull/101#discussion_r140279677","pull_request_url":"https://api.github.com/repos/alphagov/govwifi/pulls/101","author_association":"COLLABORATOR","_links":{"self":{"href":"https://api.github.com/repos/alphagov/govwifi/pulls/comments/140279677"},"html":{"href":"https://github.com/alphagov/govwifi/pull/101#discussion_r140279677"},"pull_request":{"href":"https://api.github.com/repos/alphagov/govwifi/pulls/101"}},"in_reply_to_id":140272546},"pull_request":{"url":"https://api.github.com/repos/alphagov/govwifi/pulls/101","id":141601671,"html_url":"https://github.com/alphagov/govwifi/pull/101","diff_url":"https://github.com/alphagov/govwifi/pull/101.diff","patch_url":"https://github.com/alphagov/govwifi/pull/101.patch","issue_url":"https://api.github.com/repos/alphagov/govwifi/issues/101","number":101,"state":"open","locked":false,"title":"Reduce risk of deadlock on accounting update","user":{"login":"afoldesi-gds","id":22799567,"avatar_url":"https://avatars1.githubusercontent.com/u/22799567?v=4","gravatar_id":"","url":"https://api.github.com/users/afoldesi-gds","html_url":"https://github.com/afoldesi-gds","followers_url":"https://api.github.com/users/afoldesi-gds/followers","following_url":"https://api.github.com/users/afoldesi-gds/following{/other_user}","gists_url":"https://api.github.com/users/afoldesi-gds/gists{/gist_id}","starred_url":"https://api.github.com/users/afoldesi-gds/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/afoldesi-gds/subscriptions","organizations_url":"https://api.github.com/users/afoldesi-gds/orgs","repos_url":"https://api.github.com/users/afoldesi-gds/repos","events_url":"https://api.github.com/users/afoldesi-gds/events{/privacy}","received_events_url":"https://api.github.com/users/afoldesi-gds/received_events","type":"User","site_admin":false},"body":"- Only save session on start request if we have a building identifier to save. \r\n- Add AP to query conditions if we have an AP to add.\r\n- Remove AP condition as it proved inconsistent in staging tests, add exception handling and custom success message so we can gauge the extent of failed updates without hitting the generic alarm.","created_at":"2017-09-18T13:24:43Z","updated_at":"2017-09-21T15:39:38Z","closed_at":null,"merged_at":null,"merge_commit_sha":"0b5fe494611db9a98b4719aab293bd8a8edae133","assignee":null,"assignees":[],"requested_reviewers":[],"milestone":null,"commits_url":"https://api.github.com/repos/alphagov/govwifi/pulls/101/commits","review_comments_url":"https://api.github.com/repos/alphagov/govwifi/pulls/101/comments","review_comment_url":"https://api.github.com/repos/alphagov/govwifi/pulls/comments{/number}","comments_url":"https://api.github.com/repos/alphagov/govwifi/issues/101/comments","statuses_url":"https://api.github.com/repos/alphagov/govwifi/statuses/131836cdbe3ed5258d6a0e5a07d100e98d512a8f","head":{"label":"alphagov:reduce-risk-of-deadlock-on-acct-update","ref":"reduce-risk-of-deadlock-on-acct-update","sha":"131836cdbe3ed5258d6a0e5a07d100e98d512a8f","user":{"login":"alphagov","id":596977,"avatar_url":"https://avatars0.githubusercontent.com/u/596977?v=4","gravatar_id":"","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":70912308,"name":"govwifi","full_name":"alphagov/govwifi","owner":{"login":"alphagov","id":596977,"avatar_url":"https://avatars0.githubusercontent.com/u/596977?v=4","gravatar_id":"","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/govwifi","description":"GovWifi","fork":false,"url":"https://api.github.com/repos/alphagov/govwifi","forks_url":"https://api.github.com/repos/alphagov/govwifi/forks","keys_url":"https://api.github.com/repos/alphagov/govwifi/keys{/key_id}","collaborators_url":"https://api.github.com/repos/alphagov/govwifi/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/alphagov/govwifi/teams","hooks_url":"https://api.github.com/repos/alphagov/govwifi/hooks","issue_events_url":"https://api.github.com/repos/alphagov/govwifi/issues/events{/number}","events_url":"https://api.github.com/repos/alphagov/govwifi/events","assignees_url":"https://api.github.com/repos/alphagov/govwifi/assignees{/user}","branches_url":"https://api.github.com/repos/alphagov/govwifi/branches{/branch}","tags_url":"https://api.github.com/repos/alphagov/govwifi/tags","blobs_url":"https://api.github.com/repos/alphagov/govwifi/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/alphagov/govwifi/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/alphagov/govwifi/git/refs{/sha}","trees_url":"https://api.github.com/repos/alphagov/govwifi/git/trees{/sha}","statuses_url":"https://api.github.com/repos/alphagov/govwifi/statuses/{sha}","languages_url":"https://api.github.com/repos/alphagov/govwifi/languages","stargazers_url":"https://api.github.com/repos/alphagov/govwifi/stargazers","contributors_url":"https://api.github.com/repos/alphagov/govwifi/contributors","subscribers_url":"https://api.github.com/repos/alphagov/govwifi/subscribers","subscription_url":"https://api.github.com/repos/alphagov/govwifi/subscription","commits_url":"https://api.github.com/repos/alphagov/govwifi/commits{/sha}","git_commits_url":"https://api.github.com/repos/alphagov/govwifi/git/commits{/sha}","comments_url":"https://api.github.com/repos/alphagov/govwifi/comments{/number}","issue_comment_url":"https://api.github.com/repos/alphagov/govwifi/issues/comments{/number}","contents_url":"https://api.github.com/repos/alphagov/govwifi/contents/{+path}","compare_url":"https://api.github.com/repos/alphagov/govwifi/compare/{base}...{head}","merges_url":"https://api.github.com/repos/alphagov/govwifi/merges","archive_url":"https://api.github.com/repos/alphagov/govwifi/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/alphagov/govwifi/downloads","issues_url":"https://api.github.com/repos/alphagov/govwifi/issues{/number}","pulls_url":"https://api.github.com/repos/alphagov/govwifi/pulls{/number}","milestones_url":"https://api.github.com/repos/alphagov/govwifi/milestones{/number}","notifications_url":"https://api.github.com/repos/alphagov/govwifi/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/alphagov/govwifi/labels{/name}","releases_url":"https://api.github.com/repos/alphagov/govwifi/releases{/id}","deployments_url":"https://api.github.com/repos/alphagov/govwifi/deployments","created_at":"2016-10-14T13:23:18Z","updated_at":"2017-07-05T17:25:05Z","pushed_at":"2017-09-20T16:40:05Z","git_url":"git://github.com/alphagov/govwifi.git","ssh_url":"git@github.com:alphagov/govwifi.git","clone_url":"https://github.com/alphagov/govwifi.git","svn_url":"https://github.com/alphagov/govwifi","homepage":"https://www.gov.uk/government/publications/govwifi/govwifi","size":501,"stargazers_count":0,"watchers_count":0,"language":"PHP","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":4,"mirror_url":null,"open_issues_count":4,"forks":4,"open_issues":4,"watchers":0,"default_branch":"master"}},"base":{"label":"alphagov:master","ref":"master","sha":"ea4873452b6d1855f4b190367ac2f821d6e81336","user":{"login":"alphagov","id":596977,"avatar_url":"https://avatars0.githubusercontent.com/u/596977?v=4","gravatar_id":"","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":70912308,"name":"govwifi","full_name":"alphagov/govwifi","owner":{"login":"alphagov","id":596977,"avatar_url":"https://avatars0.githubusercontent.com/u/596977?v=4","gravatar_id":"","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/govwifi","description":"GovWifi","fork":false,"url":"https://api.github.com/repos/alphagov/govwifi","forks_url":"https://api.github.com/repos/alphagov/govwifi/forks","keys_url":"https://api.github.com/repos/alphagov/govwifi/keys{/key_id}","collaborators_url":"https://api.github.com/repos/alphagov/govwifi/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/alphagov/govwifi/teams","hooks_url":"https://api.github.com/repos/alphagov/govwifi/hooks","issue_events_url":"https://api.github.com/repos/alphagov/govwifi/issues/events{/number}","events_url":"https://api.github.com/repos/alphagov/govwifi/events","assignees_url":"https://api.github.com/repos/alphagov/govwifi/assignees{/user}","branches_url":"https://api.github.com/repos/alphagov/govwifi/branches{/branch}","tags_url":"https://api.github.com/repos/alphagov/govwifi/tags","blobs_url":"https://api.github.com/repos/alphagov/govwifi/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/alphagov/govwifi/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/alphagov/govwifi/git/refs{/sha}","trees_url":"https://api.github.com/repos/alphagov/govwifi/git/trees{/sha}","statuses_url":"https://api.github.com/repos/alphagov/govwifi/statuses/{sha}","languages_url":"https://api.github.com/repos/alphagov/govwifi/languages","stargazers_url":"https://api.github.com/repos/alphagov/govwifi/stargazers","contributors_url":"https://api.github.com/repos/alphagov/govwifi/contributors","subscribers_url":"https://api.github.com/repos/alphagov/govwifi/subscribers","subscription_url":"https://api.github.com/repos/alphagov/govwifi/subscription","commits_url":"https://api.github.com/repos/alphagov/govwifi/commits{/sha}","git_commits_url":"https://api.github.com/repos/alphagov/govwifi/git/commits{/sha}","comments_url":"https://api.github.com/repos/alphagov/govwifi/comments{/number}","issue_comment_url":"https://api.github.com/repos/alphagov/govwifi/issues/comments{/number}","contents_url":"https://api.github.com/repos/alphagov/govwifi/contents/{+path}","compare_url":"https://api.github.com/repos/alphagov/govwifi/compare/{base}...{head}","merges_url":"https://api.github.com/repos/alphagov/govwifi/merges","archive_url":"https://api.github.com/repos/alphagov/govwifi/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/alphagov/govwifi/downloads","issues_url":"https://api.github.com/repos/alphagov/govwifi/issues{/number}","pulls_url":"https://api.github.com/repos/alphagov/govwifi/pulls{/number}","milestones_url":"https://api.github.com/repos/alphagov/govwifi/milestones{/number}","notifications_url":"https://api.github.com/repos/alphagov/govwifi/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/alphagov/govwifi/labels{/name}","releases_url":"https://api.github.com/repos/alphagov/govwifi/releases{/id}","deployments_url":"https://api.github.com/repos/alphagov/govwifi/deployments","created_at":"2016-10-14T13:23:18Z","updated_at":"2017-07-05T17:25:05Z","pushed_at":"2017-09-20T16:40:05Z","git_url":"git://github.com/alphagov/govwifi.git","ssh_url":"git@github.com:alphagov/govwifi.git","clone_url":"https://github.com/alphagov/govwifi.git","svn_url":"https://github.com/alphagov/govwifi","homepage":"https://www.gov.uk/government/publications/govwifi/govwifi","size":501,"stargazers_count":0,"watchers_count":0,"language":"PHP","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":4,"mirror_url":null,"open_issues_count":4,"forks":4,"open_issues":4,"watchers":0,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/alphagov/govwifi/pulls/101"},"html":{"href":"https://github.com/alphagov/govwifi/pull/101"},"issue":{"href":"https://api.github.com/repos/alphagov/govwifi/issues/101"},"comments":{"href":"https://api.github.com/repos/alphagov/govwifi/issues/101/comments"},"review_comments":{"href":"https://api.github.com/repos/alphagov/govwifi/pulls/101/comments"},"review_comment":{"href":"https://api.github.com/repos/alphagov/govwifi/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/alphagov/govwifi/pulls/101/commits"},"statuses":{"href":"https://api.github.com/repos/alphagov/govwifi/statuses/131836cdbe3ed5258d6a0e5a07d100e98d512a8f"}},"author_association":"COLLABORATOR"}}
{"actor":{"display_login":"afoldesi-gds"}}