{"action":"created","comment":{"url":"https://api.github.com/repos/acquia/acquia_lift/pulls/comments/33692012","id":33692012,"diff_hunk":"@@ -689,7 +689,7 @@ function acquia_lift_report($form, &$form_state, $agent_data) {\n         );","path":"acquia_lift.admin.inc","position":1,"original_position":1,"commit_id":"46242390cbae2f6733a76db884dd400e86c006e6","original_commit_id":"46242390cbae2f6733a76db884dd400e86c006e6","user":{"login":"lkhaas","id":3512988,"avatar_url":"https://avatars.githubusercontent.com/u/3512988?v=3","gravatar_id":"","url":"https://api.github.com/users/lkhaas","html_url":"https://github.com/lkhaas","followers_url":"https://api.github.com/users/lkhaas/followers","following_url":"https://api.github.com/users/lkhaas/following{/other_user}","gists_url":"https://api.github.com/users/lkhaas/gists{/gist_id}","starred_url":"https://api.github.com/users/lkhaas/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lkhaas/subscriptions","organizations_url":"https://api.github.com/users/lkhaas/orgs","repos_url":"https://api.github.com/users/lkhaas/repos","events_url":"https://api.github.com/users/lkhaas/events{/privacy}","received_events_url":"https://api.github.com/users/lkhaas/received_events","type":"User","site_admin":false},"body":"Sorry - part of this approach of a review is that I see things that aren't in the pull request... but related.\r\nOn line 652, I'm confused by the following code:\r\n\r\n```\r\nif (empty($audience) && !isset($option_set->targeting[$audience]['osid'])) {\r\n    if (empty($audience_options)) {\r\n      return array(\r\n        'no_report' => array(\r\n          '#markup'=> t('This personalization does not contain tests, no reports to show.')\r\n        )\r\n      );\r\n    }\r\n    $audience = key($audience_options);\r\n  }\r\n```\r\n\r\nShould the other if statement actually be connected by || rather than &&?","created_at":"2015-07-01T15:44:59Z","updated_at":"2015-07-01T15:44:59Z","html_url":"https://github.com/acquia/acquia_lift/pull/188#discussion_r33692012","pull_request_url":"https://api.github.com/repos/acquia/acquia_lift/pulls/188","_links":{"self":{"href":"https://api.github.com/repos/acquia/acquia_lift/pulls/comments/33692012"},"html":{"href":"https://github.com/acquia/acquia_lift/pull/188#discussion_r33692012"},"pull_request":{"href":"https://api.github.com/repos/acquia/acquia_lift/pulls/188"}}},"pull_request":{"url":"https://api.github.com/repos/acquia/acquia_lift/pulls/188","id":38901586,"html_url":"https://github.com/acquia/acquia_lift/pull/188","diff_url":"https://github.com/acquia/acquia_lift/pull/188.diff","patch_url":"https://github.com/acquia/acquia_lift/pull/188.patch","issue_url":"https://api.github.com/repos/acquia/acquia_lift/issues/188","number":188,"state":"open","locked":false,"title":"Ch 3124","user":{"login":"katbailey","id":58409,"avatar_url":"https://avatars.githubusercontent.com/u/58409?v=3","gravatar_id":"","url":"https://api.github.com/users/katbailey","html_url":"https://github.com/katbailey","followers_url":"https://api.github.com/users/katbailey/followers","following_url":"https://api.github.com/users/katbailey/following{/other_user}","gists_url":"https://api.github.com/users/katbailey/gists{/gist_id}","starred_url":"https://api.github.com/users/katbailey/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/katbailey/subscriptions","organizations_url":"https://api.github.com/users/katbailey/orgs","repos_url":"https://api.github.com/users/katbailey/repos","events_url":"https://api.github.com/users/katbailey/events{/privacy}","received_events_url":"https://api.github.com/users/katbailey/received_events","type":"User","site_admin":false},"body":"OK, this one's a bit of a monster. I'll try to explain the approach I took with this refactoring. I basically wanted to make it as easy as possible to understand how reporting data gets turned into mark-up on the reports page. Currently the logic is very difficult to follow and the reporting markup expects a bunch of nested arrays of different reports.\r\n\r\nEach report relies on certain data, so obviously we need a method responsible for getting that data from the report data source and storing it as a class property. This data can then be accessed whenever a certain piece of the report needs something from it.\r\n\r\nWhen a report is created, it is always either for all goals or for a single goal. The same report never needs to care about both of these. It is simply a configuration option on the report that gets passed on to the data source.\r\n\r\nI've abstracted out all of the markup generation functions into a base class. These functions have much simpler parameters whose structure is not dependent on the way the data were loaded from the report source. This opens the way for different reporting extensions that wish to display their data in the same way.\r\n\r\nI've also split out the reporting classes into different files and only the interfaces are included in the .info file as the others can be loaded only when reports are being accessed.","created_at":"2015-06-30T17:10:35Z","updated_at":"2015-07-01T15:44:59Z","closed_at":null,"merged_at":null,"merge_commit_sha":"712edca70ec659c6b32f66c02d38afa248382f6f","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/acquia/acquia_lift/pulls/188/commits","review_comments_url":"https://api.github.com/repos/acquia/acquia_lift/pulls/188/comments","review_comment_url":"https://api.github.com/repos/acquia/acquia_lift/pulls/comments{/number}","comments_url":"https://api.github.com/repos/acquia/acquia_lift/issues/188/comments","statuses_url":"https://api.github.com/repos/acquia/acquia_lift/statuses/46242390cbae2f6733a76db884dd400e86c006e6","head":{"label":"acquia:CH-3124","ref":"CH-3124","sha":"46242390cbae2f6733a76db884dd400e86c006e6","user":{"login":"acquia","id":514789,"avatar_url":"https://avatars.githubusercontent.com/u/514789?v=3","gravatar_id":"","url":"https://api.github.com/users/acquia","html_url":"https://github.com/acquia","followers_url":"https://api.github.com/users/acquia/followers","following_url":"https://api.github.com/users/acquia/following{/other_user}","gists_url":"https://api.github.com/users/acquia/gists{/gist_id}","starred_url":"https://api.github.com/users/acquia/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/acquia/subscriptions","organizations_url":"https://api.github.com/users/acquia/orgs","repos_url":"https://api.github.com/users/acquia/repos","events_url":"https://api.github.com/users/acquia/events{/privacy}","received_events_url":"https://api.github.com/users/acquia/received_events","type":"Organization","site_admin":false},"repo":{"id":17344049,"name":"acquia_lift","full_name":"acquia/acquia_lift","owner":{"login":"acquia","id":514789,"avatar_url":"https://avatars.githubusercontent.com/u/514789?v=3","gravatar_id":"","url":"https://api.github.com/users/acquia","html_url":"https://github.com/acquia","followers_url":"https://api.github.com/users/acquia/followers","following_url":"https://api.github.com/users/acquia/following{/other_user}","gists_url":"https://api.github.com/users/acquia/gists{/gist_id}","starred_url":"https://api.github.com/users/acquia/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/acquia/subscriptions","organizations_url":"https://api.github.com/users/acquia/orgs","repos_url":"https://api.github.com/users/acquia/repos","events_url":"https://api.github.com/users/acquia/events{/privacy}","received_events_url":"https://api.github.com/users/acquia/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/acquia/acquia_lift","description":"Mirror of the drupal.org repository for Acquia Lift module","fork":false,"url":"https://api.github.com/repos/acquia/acquia_lift","forks_url":"https://api.github.com/repos/acquia/acquia_lift/forks","keys_url":"https://api.github.com/repos/acquia/acquia_lift/keys{/key_id}","collaborators_url":"https://api.github.com/repos/acquia/acquia_lift/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/acquia/acquia_lift/teams","hooks_url":"https://api.github.com/repos/acquia/acquia_lift/hooks","issue_events_url":"https://api.github.com/repos/acquia/acquia_lift/issues/events{/number}","events_url":"https://api.github.com/repos/acquia/acquia_lift/events","assignees_url":"https://api.github.com/repos/acquia/acquia_lift/assignees{/user}","branches_url":"https://api.github.com/repos/acquia/acquia_lift/branches{/branch}","tags_url":"https://api.github.com/repos/acquia/acquia_lift/tags","blobs_url":"https://api.github.com/repos/acquia/acquia_lift/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/acquia/acquia_lift/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/acquia/acquia_lift/git/refs{/sha}","trees_url":"https://api.github.com/repos/acquia/acquia_lift/git/trees{/sha}","statuses_url":"https://api.github.com/repos/acquia/acquia_lift/statuses/{sha}","languages_url":"https://api.github.com/repos/acquia/acquia_lift/languages","stargazers_url":"https://api.github.com/repos/acquia/acquia_lift/stargazers","contributors_url":"https://api.github.com/repos/acquia/acquia_lift/contributors","subscribers_url":"https://api.github.com/repos/acquia/acquia_lift/subscribers","subscription_url":"https://api.github.com/repos/acquia/acquia_lift/subscription","commits_url":"https://api.github.com/repos/acquia/acquia_lift/commits{/sha}","git_commits_url":"https://api.github.com/repos/acquia/acquia_lift/git/commits{/sha}","comments_url":"https://api.github.com/repos/acquia/acquia_lift/comments{/number}","issue_comment_url":"https://api.github.com/repos/acquia/acquia_lift/issues/comments{/number}","contents_url":"https://api.github.com/repos/acquia/acquia_lift/contents/{+path}","compare_url":"https://api.github.com/repos/acquia/acquia_lift/compare/{base}...{head}","merges_url":"https://api.github.com/repos/acquia/acquia_lift/merges","archive_url":"https://api.github.com/repos/acquia/acquia_lift/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/acquia/acquia_lift/downloads","issues_url":"https://api.github.com/repos/acquia/acquia_lift/issues{/number}","pulls_url":"https://api.github.com/repos/acquia/acquia_lift/pulls{/number}","milestones_url":"https://api.github.com/repos/acquia/acquia_lift/milestones{/number}","notifications_url":"https://api.github.com/repos/acquia/acquia_lift/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/acquia/acquia_lift/labels{/name}","releases_url":"https://api.github.com/repos/acquia/acquia_lift/releases{/id}","created_at":"2014-03-02T18:42:14Z","updated_at":"2014-08-25T15:09:47Z","pushed_at":"2015-07-01T15:06:53Z","git_url":"git://github.com/acquia/acquia_lift.git","ssh_url":"git@github.com:acquia/acquia_lift.git","clone_url":"https://github.com/acquia/acquia_lift.git","svn_url":"https://github.com/acquia/acquia_lift","homepage":"","size":8555,"stargazers_count":2,"watchers_count":2,"language":null,"has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":8,"mirror_url":null,"open_issues_count":10,"forks":8,"open_issues":10,"watchers":2,"default_branch":"master"}},"base":{"label":"acquia:7.x-2.x","ref":"7.x-2.x","sha":"684e1745d583f0a6f37379f2efe876287fa6cf36","user":{"login":"acquia","id":514789,"avatar_url":"https://avatars.githubusercontent.com/u/514789?v=3","gravatar_id":"","url":"https://api.github.com/users/acquia","html_url":"https://github.com/acquia","followers_url":"https://api.github.com/users/acquia/followers","following_url":"https://api.github.com/users/acquia/following{/other_user}","gists_url":"https://api.github.com/users/acquia/gists{/gist_id}","starred_url":"https://api.github.com/users/acquia/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/acquia/subscriptions","organizations_url":"https://api.github.com/users/acquia/orgs","repos_url":"https://api.github.com/users/acquia/repos","events_url":"https://api.github.com/users/acquia/events{/privacy}","received_events_url":"https://api.github.com/users/acquia/received_events","type":"Organization","site_admin":false},"repo":{"id":17344049,"name":"acquia_lift","full_name":"acquia/acquia_lift","owner":{"login":"acquia","id":514789,"avatar_url":"https://avatars.githubusercontent.com/u/514789?v=3","gravatar_id":"","url":"https://api.github.com/users/acquia","html_url":"https://github.com/acquia","followers_url":"https://api.github.com/users/acquia/followers","following_url":"https://api.github.com/users/acquia/following{/other_user}","gists_url":"https://api.github.com/users/acquia/gists{/gist_id}","starred_url":"https://api.github.com/users/acquia/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/acquia/subscriptions","organizations_url":"https://api.github.com/users/acquia/orgs","repos_url":"https://api.github.com/users/acquia/repos","events_url":"https://api.github.com/users/acquia/events{/privacy}","received_events_url":"https://api.github.com/users/acquia/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/acquia/acquia_lift","description":"Mirror of the drupal.org repository for Acquia Lift module","fork":false,"url":"https://api.github.com/repos/acquia/acquia_lift","forks_url":"https://api.github.com/repos/acquia/acquia_lift/forks","keys_url":"https://api.github.com/repos/acquia/acquia_lift/keys{/key_id}","collaborators_url":"https://api.github.com/repos/acquia/acquia_lift/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/acquia/acquia_lift/teams","hooks_url":"https://api.github.com/repos/acquia/acquia_lift/hooks","issue_events_url":"https://api.github.com/repos/acquia/acquia_lift/issues/events{/number}","events_url":"https://api.github.com/repos/acquia/acquia_lift/events","assignees_url":"https://api.github.com/repos/acquia/acquia_lift/assignees{/user}","branches_url":"https://api.github.com/repos/acquia/acquia_lift/branches{/branch}","tags_url":"https://api.github.com/repos/acquia/acquia_lift/tags","blobs_url":"https://api.github.com/repos/acquia/acquia_lift/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/acquia/acquia_lift/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/acquia/acquia_lift/git/refs{/sha}","trees_url":"https://api.github.com/repos/acquia/acquia_lift/git/trees{/sha}","statuses_url":"https://api.github.com/repos/acquia/acquia_lift/statuses/{sha}","languages_url":"https://api.github.com/repos/acquia/acquia_lift/languages","stargazers_url":"https://api.github.com/repos/acquia/acquia_lift/stargazers","contributors_url":"https://api.github.com/repos/acquia/acquia_lift/contributors","subscribers_url":"https://api.github.com/repos/acquia/acquia_lift/subscribers","subscription_url":"https://api.github.com/repos/acquia/acquia_lift/subscription","commits_url":"https://api.github.com/repos/acquia/acquia_lift/commits{/sha}","git_commits_url":"https://api.github.com/repos/acquia/acquia_lift/git/commits{/sha}","comments_url":"https://api.github.com/repos/acquia/acquia_lift/comments{/number}","issue_comment_url":"https://api.github.com/repos/acquia/acquia_lift/issues/comments{/number}","contents_url":"https://api.github.com/repos/acquia/acquia_lift/contents/{+path}","compare_url":"https://api.github.com/repos/acquia/acquia_lift/compare/{base}...{head}","merges_url":"https://api.github.com/repos/acquia/acquia_lift/merges","archive_url":"https://api.github.com/repos/acquia/acquia_lift/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/acquia/acquia_lift/downloads","issues_url":"https://api.github.com/repos/acquia/acquia_lift/issues{/number}","pulls_url":"https://api.github.com/repos/acquia/acquia_lift/pulls{/number}","milestones_url":"https://api.github.com/repos/acquia/acquia_lift/milestones{/number}","notifications_url":"https://api.github.com/repos/acquia/acquia_lift/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/acquia/acquia_lift/labels{/name}","releases_url":"https://api.github.com/repos/acquia/acquia_lift/releases{/id}","created_at":"2014-03-02T18:42:14Z","updated_at":"2014-08-25T15:09:47Z","pushed_at":"2015-07-01T15:06:53Z","git_url":"git://github.com/acquia/acquia_lift.git","ssh_url":"git@github.com:acquia/acquia_lift.git","clone_url":"https://github.com/acquia/acquia_lift.git","svn_url":"https://github.com/acquia/acquia_lift","homepage":"","size":8555,"stargazers_count":2,"watchers_count":2,"language":null,"has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":8,"mirror_url":null,"open_issues_count":10,"forks":8,"open_issues":10,"watchers":2,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/acquia/acquia_lift/pulls/188"},"html":{"href":"https://github.com/acquia/acquia_lift/pull/188"},"issue":{"href":"https://api.github.com/repos/acquia/acquia_lift/issues/188"},"comments":{"href":"https://api.github.com/repos/acquia/acquia_lift/issues/188/comments"},"review_comments":{"href":"https://api.github.com/repos/acquia/acquia_lift/pulls/188/comments"},"review_comment":{"href":"https://api.github.com/repos/acquia/acquia_lift/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/acquia/acquia_lift/pulls/188/commits"},"statuses":{"href":"https://api.github.com/repos/acquia/acquia_lift/statuses/46242390cbae2f6733a76db884dd400e86c006e6"}}}}