{"action":"opened","issue":{"url":"https://api.github.com/repos/konadave/com.klangsoft.flexiblejobs/issues/1","labels_url":"https://api.github.com/repos/konadave/com.klangsoft.flexiblejobs/issues/1/labels{/name}","comments_url":"https://api.github.com/repos/konadave/com.klangsoft.flexiblejobs/issues/1/comments","events_url":"https://api.github.com/repos/konadave/com.klangsoft.flexiblejobs/issues/1/events","html_url":"https://github.com/konadave/com.klangsoft.flexiblejobs/issues/1","id":123890448,"number":1,"title":"Extension review","user":{"login":"PalanteJon","id":1796012,"avatar_url":"https://avatars.githubusercontent.com/u/1796012?v=3","gravatar_id":"","url":"https://api.github.com/users/PalanteJon","html_url":"https://github.com/PalanteJon","followers_url":"https://api.github.com/users/PalanteJon/followers","following_url":"https://api.github.com/users/PalanteJon/following{/other_user}","gists_url":"https://api.github.com/users/PalanteJon/gists{/gist_id}","starred_url":"https://api.github.com/users/PalanteJon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PalanteJon/subscriptions","organizations_url":"https://api.github.com/users/PalanteJon/orgs","repos_url":"https://api.github.com/users/PalanteJon/repos","events_url":"https://api.github.com/users/PalanteJon/events{/privacy}","received_events_url":"https://api.github.com/users/PalanteJon/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":0,"created_at":"2015-12-25T17:00:00Z","updated_at":"2015-12-25T17:00:00Z","closed_at":null,"body":"Hi Dave,\r\n\r\nI'm on the CiviCRM extension review team.  Thanks for submitting this!\r\n\r\nOverall, it looks good.  Here's the feedback I have.\r\n* I struggled a bit with setting the date.  I didn't realize dates in the past wouldn't work (I was trying to use them so I could test immediately), so I thought I couldn't figure out the date format it was expecting.  I finally saw the code that was confounding me - but not until I converted that field to a date picker.  I'll submit that as a PR.\r\n* A few minor civilint issues - but tbh, civix generates code with some of the same errors.  I'm ignoring this.\r\n* The biggest issue is the use of the core file overrides, in particular SelectValues.php.  This makes me worry about this breaking a point release upgrade - and of course, using this with 4.7 as-is isn't an option.\r\n* The other reason this can't be used with 4.7 is your code makes use of the group_name column in civicrm_setting, which is removed in 4.7.\r\n\r\nSo with that all said, I'm going to approve this immediately, but if you want to incorporate my PR, I think it's a worthwhile improvement! \r\n\r\nFinally - I think these features would be welcome in core, and would be easier to maintain than PHP overrides.  For the future, perhaps."}}