From ec5f07a578505b7195c1bd7c52c85ad2552b8caf Mon Sep 17 00:00:00 2001
From: Teemu Patja <tp@iki.fi>
Date: Fri, 24 Feb 2017 11:05:13 +0200
Subject: [PATCH] DB query refactoring, save PRs also when opened

* store pull-requests to DB also when opened instead of only when
  merged to allow for showing bounty claims
* refactor SQL queries
* look for PR keywords also in title
* improve PR keyword regexes
---
 ...170222143232-bounty-claim-changes.down.sql |   0
 ...20170222143232-bounty-claim-changes.up.sql |  21 ++
 resources/sql/queries.sql                     | 180 +++++++++---------
 src/clj/commiteth/db/bounties.clj             |   4 +-
 src/clj/commiteth/db/pull_requests.clj        |  17 +-
 src/clj/commiteth/routes/webhooks.clj         | 121 +++++++-----
 src/clj/commiteth/scheduler.clj               |   2 +-
 7 files changed, 206 insertions(+), 139 deletions(-)
 create mode 100644 resources/migrations/20170222143232-bounty-claim-changes.down.sql
 create mode 100644 resources/migrations/20170222143232-bounty-claim-changes.up.sql

diff --git a/resources/migrations/20170222143232-bounty-claim-changes.down.sql b/resources/migrations/20170222143232-bounty-claim-changes.down.sql
new file mode 100644
index 0000000..e69de29
diff --git a/resources/migrations/20170222143232-bounty-claim-changes.up.sql b/resources/migrations/20170222143232-bounty-claim-changes.up.sql
new file mode 100644
index 0000000..bbf98a4
--- /dev/null
+++ b/resources/migrations/20170222143232-bounty-claim-changes.up.sql
@@ -0,0 +1,21 @@
+
+ALTER TABLE "public"."issue_comment"
+ADD COLUMN "updated" timestamp DEFAULT timezone('utc'::text, now());
+
+
+ALTER TABLE "public"."pull_requests" ADD COLUMN "state" integer DEFAULT '0';
+
+CREATE TABLE "public"."pull_request_state" (
+    "id" integer,
+    "description" character varying(32),
+    PRIMARY KEY ("id")
+);
+
+INSERT INTO pull_request_state (id, description)
+VALUES(0, 'opened');
+
+INSERT INTO pull_request_state (id, description)
+VALUES(1, 'merged');
+
+INSERT INTO pull_request_state (id, description)
+VALUES(2, 'closed');
diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql
index 0c4f87e..d6cbbd9 100644
--- a/resources/sql/queries.sql
+++ b/resources/sql/queries.sql
@@ -21,7 +21,11 @@ RETURNING id, login, name, email, avatar_url, token, address, created;
 -- :name update-user! :! :n
 -- :doc updates an existing user record
 UPDATE users
-SET login = :login, name = :name, email = :email, token = :token, address = :address
+SET login = :login,
+name = :name,
+email = :email,
+token = :token,
+address = :address
 WHERE id = :id;
 
 -- :name update-user-token! :<! :1
@@ -44,9 +48,9 @@ WHERE id = :id;
 
 -- :name get-repo-owner :? :1
 SELECT *
-FROM users u
-  INNER JOIN repositories r ON r.user_id = u.id
-WHERE r.repo_id = :repo_id;
+FROM users u, repositories r
+WHERE r.repo_id = :repo_id
+AND r.user_id = u.id;
 
 -- Repositories --------------------------------------------------------------------
 
@@ -130,6 +134,8 @@ UPDATE issues
 SET transaction_hash = :transaction_hash
 WHERE issue_id = :issue_id;
 
+
+-- TODO: this is terrible
 -- :name update-contract-address :<! :1
 -- :doc updates contract-address for a given issue
 WITH t AS (
@@ -142,9 +148,9 @@ WITH t AS (
       i.repo_id          AS repo_id,
       r.login            AS login,
       r.repo             AS repo
-    FROM issues i
-      INNER JOIN repositories r ON r.repo_id = i.repo_id
-    WHERE i.issue_id = :issue_id
+    FROM issues i, repositories r
+    WHERE r.repo_id = i.repo_id
+    AND i.issue_id = :issue_id
 )
 UPDATE issues i
 SET contract_address = :contract_address
@@ -169,19 +175,26 @@ WHERE contract_address IS NULL
 
 -- Pull Requests -------------------------------------------------------------------
 
--- :name create-pull-request! :! :n
--- :doc creates pull request
-INSERT INTO pull_requests (repo_id, pr_id, pr_number, issue_number, commit_id, user_id)
-  SELECT
-    :repo_id,
-    :pr_id,
-    :pr_number,
-    :issue_number,
-    :commit_id,
-    :user_id
-  WHERE NOT exists(SELECT 1
-                   FROM pull_requests
-                   WHERE repo_id = :repo_id AND pr_id = :pr_id);
+-- :name save-pull-request! :! :n
+-- :doc inserts or updates a pull request record
+INSERT INTO pull_requests (pr_id,
+  repo_id,
+  pr_number,
+  issue_number,
+  commit_id,
+  user_id,
+  state)
+VALUES(:pr_id,
+  :repo_id,
+  :pr_number,
+  :issue_number,
+  :commit_id,
+  :user_id,
+  :state)
+ON CONFLICT (pr_id) DO UPDATE
+SET
+  state = :state,
+  commit_id = :commit_id;
 
 -- Bounties ------------------------------------------------------------------------
 
@@ -191,43 +204,40 @@ SELECT
   i.contract_address AS contract_address,
   i.issue_id         AS issue_id,
   u.address          AS payout_address
-FROM issues i
-  INNER JOIN pull_requests p
-    ON (p.commit_id = i.commit_id OR coalesce(p.issue_number, -1) = i.issue_number)
-       AND p.repo_id = i.repo_id
-  INNER JOIN users u
-    ON u.id = p.user_id
-WHERE i.execute_hash IS NULL;
+FROM issues i, pull_requests p, users u
+WHERE
+(p.commit_id = i.commit_id OR coalesce(p.issue_number, -1) = i.issue_number)
+AND p.repo_id = i.repo_id
+AND u.id = p.user_id
+AND i.execute_hash IS NULL;
 
 -- :name pending-payouts-list :? :*
 -- :doc lists all recently closed issues awaiting to be confirmed
 SELECT
   i.contract_address AS contract_address,
   i.issue_id         AS issue_id,
-  i.execute_hash     AS execute_hash
-FROM issues i
-  INNER JOIN pull_requests p
-    ON (p.commit_id = i.commit_id OR coalesce(p.issue_number, -1) = i.issue_number)
-       AND p.repo_id = i.repo_id
-  INNER JOIN users u
-    ON u.id = p.user_id
-WHERE i.confirm_hash IS NULL
-      AND i.execute_hash IS NOT NULL;
+  u.address          AS payout_address
+FROM issues i, pull_requests p, users u
+WHERE
+(p.commit_id = i.commit_id OR coalesce(p.issue_number, -1) = i.issue_number)
+AND p.repo_id = i.repo_id
+AND u.id = p.user_id
+AND i.confirm_hash IS NULL
+AND i.execute_hash IS NOT NULL;
 
 -- :name confirmed-payouts-list :? :*
 -- :doc lists all recently confirmed bounty payouts
 SELECT
   i.contract_address AS contract_address,
   i.issue_id         AS issue_id,
-  i.payout_hash      AS payout_hash
-FROM issues i
-  INNER JOIN pull_requests p
-    ON (p.commit_id = i.commit_id OR coalesce(p.issue_number, -1) = i.issue_number)
-       AND p.repo_id = i.repo_id
-  INNER JOIN users u
-    ON u.id = p.user_id
-WHERE i.payout_receipt IS NULL
-      AND i.payout_hash IS NOT NULL;
+  u.address          AS payout_address
+FROM issues i, pull_requests p, users u
+WHERE
+(p.commit_id = i.commit_id OR coalesce(p.issue_number, -1) = i.issue_number)
+AND p.repo_id = i.repo_id
+AND u.id = p.user_id
+AND i.payout_receipt IS NULL
+AND i.payout_hash IS NOT NULL;
 
 -- :name update-confirm-hash :! :n
 -- :doc updates issue with confirmation hash
@@ -254,7 +264,7 @@ SET payout_receipt = :payout_receipt
 WHERE issue_id = :issue_id;
 
 -- :name all-bounties-list :? :*
--- :doc lists all issues labeled as 'bounty'
+-- :doc open (not merged) bounty issues
 SELECT
   i.contract_address AS contract_address,
   i.issue_id         AS issue_id,
@@ -264,16 +274,13 @@ SELECT
   i.balance          AS issue_balance,
   r.login            AS owner_name,
   r.repo             AS repo_name
-FROM issues i
-  INNER JOIN repositories r
-    ON r.repo_id = i.repo_id
-WHERE i.commit_id IS NULL
-      AND NOT EXISTS(SELECT 1
-                     FROM pull_requests
-                     WHERE issue_number = i.issue_number);
+FROM issues i, repositories r
+WHERE
+r.repo_id = i.repo_id
+AND i.commit_id IS NULL;
 
 -- :name owner-bounties-list :? :*
--- :doc lists fixed issues
+-- :doc lists fixed issues (bounties awaiting maintainer confirmation)
 SELECT
   i.contract_address AS contract_address,
   i.issue_id         AS issue_id,
@@ -293,21 +300,18 @@ SELECT
   r.login            AS owner_name,
   r.repo             AS repo_name,
   o.address          AS owner_address
-FROM issues i
-  INNER JOIN pull_requests p
-    ON (p.commit_id = i.commit_id OR coalesce(p.issue_number, -1) = i.issue_number)
-       AND p.repo_id = i.repo_id
-  INNER JOIN users u
-    ON u.id = p.user_id
-  INNER JOIN repositories r
-    ON r.repo_id = i.repo_id
-  INNER JOIN users o
-    ON r.user_id = o.id
-WHERE r.user_id = :owner_id
-      AND i.confirm_hash IS NOT NULL;
+FROM issues i, pull_requests p, users u, users o, repositories r
+WHERE
+(p.commit_id = i.commit_id OR coalesce(p.issue_number, -1) = i.issue_number)
+AND p.repo_id = i.repo_id
+AND u.id = p.user_id
+AND r.repo_id = i.repo_id
+AND r.user_id = o.id
+AND r.user_id = :owner_id
+AND i.confirm_hash IS NOT NULL;
 
 -- :name owner-issues-list :? :*
--- :doc lists all not yet fixed issues in a given owner's repository
+-- :doc owner's bounty issues with no merged PR
 SELECT
   i.contract_address AS contract_address,
   i.issue_id         AS issue_id,
@@ -316,19 +320,18 @@ SELECT
   i.repo_id          AS repo_id,
   r.login            AS owner_name,
   r.repo             AS repo_name
-FROM issues i
-  INNER JOIN repositories r
-    ON r.repo_id = i.repo_id
-WHERE r.user_id = :owner_id
-      AND i.commit_id IS NULL
-      AND NOT exists(SELECT 1
-                     FROM pull_requests
-                     WHERE issue_number = i.issue_number);
+FROM issues i, repositories r
+WHERE r.repo_id = i.repo_id
+AND r.user_id = :owner_id
+AND i.commit_id IS NULL
+AND NOT exists(SELECT 1
+               FROM pull_requests
+               WHERE issue_number = i.issue_number
+               AND state = 1);
 
--- TODO: misleading name. this is used when updating bounty balances. maybe we should exclude at least bounties that have been paid?
 
--- :name wallets-list :? :*
--- :doc lists all contract ids
+-- :name open-bounty-contracts :? :*
+-- :doc bounty issues with mined bounty contracts
 SELECT
   i.contract_address AS contract_address,
   r.login            AS login,
@@ -337,22 +340,26 @@ SELECT
   i.issue_number     AS issue_number,
   i.issue_id         AS issue_id,
   i.balance          AS balance
-FROM issues i
-  INNER JOIN repositories r ON r.repo_id = i.repo_id
-WHERE contract_address IS NOT NULL;
+FROM issues i, repositories r
+WHERE r.repo_id = i.repo_id
+AND contract_address IS NOT NULL
+AND i.payout_hash IS NULL;
 
 -- :name get-bounty :? :1
+-- :doc details for a bounty issue given owner, repo and issue nunber
 SELECT
   i.contract_address AS contract_address,
   i.issue_id         AS issue_id,
   i.issue_number     AS issue_number,
   i.balance          AS balance,
   r.login            AS login,
-  r.repo             AS repo
-FROM issues i
-  INNER JOIN repositories r ON r.repo_id = i.repo_id
+  r.repo             AS repo,
+  i.state            AS state
+FROM issues i, repositories r
 WHERE i.issue_number = :issue_number
-      AND r.login = :login AND r.repo = :repo;
+AND r.repo_id = i.repo_id
+AND r.login = :login
+AND r.repo = :repo;
 
 -- :name get-balance :? :1
 -- :doc gets current balance of a wallet attached to a given issue
@@ -367,8 +374,8 @@ SET balance = :balance
 WHERE contract_address = :contract_address;
 
 
-
 -- :name save-issue-comment-image! :<! :1
+-- :doc insert or update image data for a given issue's github comment
 INSERT INTO issue_comment (issue_id, png_data)
 VALUES (:issue_id, :png_data)
 ON CONFLICT (issue_id) DO UPDATE
@@ -377,6 +384,7 @@ RETURNING id;
 
 
 -- :name get-issue-comment-image :? :1
+-- :doc retrieve image data for given issue's github comment
 SELECT png_data
 FROM issue_comment
 WHERE issue_id = :issue_id;
diff --git a/src/clj/commiteth/db/bounties.clj b/src/clj/commiteth/db/bounties.clj
index b1c6cc0..589e5b1 100644
--- a/src/clj/commiteth/db/bounties.clj
+++ b/src/clj/commiteth/db/bounties.clj
@@ -60,7 +60,7 @@
     (db/get-bounty con-db {:login user :repo repo :issue_number issue-number})))
 
 
-(defn list-wallets
+(defn open-bounty-contracts
   []
   (jdbc/with-db-connection [con-db *db*]
-    (db/wallets-list con-db)))
+    (db/open-bounty-contracts con-db)))
diff --git a/src/clj/commiteth/db/pull_requests.clj b/src/clj/commiteth/db/pull_requests.clj
index 3955f94..e706656 100644
--- a/src/clj/commiteth/db/pull_requests.clj
+++ b/src/clj/commiteth/db/pull_requests.clj
@@ -1,10 +1,17 @@
 (ns commiteth.db.pull-requests
   (:require [commiteth.db.core :refer [*db*] :as db]
             [clojure.java.jdbc :as jdbc]
-            [clojure.set :refer [rename-keys]]))
+            [clojure.tools.logging :as log]))
 
-(defn create
-  "Creates pull-request"
+
+(defn save
+  "Creates or updates a pull-request"
   [pull-request]
-  (jdbc/with-db-connection [con-db *db*]
-    (db/create-pull-request! con-db pull-request)))
+  (let [state (case (:state pull-request)
+                :opened 0
+                :merged 1
+                :closed 2)]
+    (log/debug (assoc pull-request :state state))
+    (jdbc/with-db-connection [con-db *db*]
+      (db/save-pull-request! con-db
+                             (assoc pull-request :state state)))))
diff --git a/src/clj/commiteth/routes/webhooks.clj b/src/clj/commiteth/routes/webhooks.clj
index cfd7c71..40bc444 100644
--- a/src/clj/commiteth/routes/webhooks.clj
+++ b/src/clj/commiteth/routes/webhooks.clj
@@ -60,63 +60,92 @@
       (issues/close commit-id issue-id))))
 
 (def ^:const keywords
-  [#"(?i)close\s+#(\d+)"
-   #"(?i)closes\s+#(\d+)"
-   #"(?i)closed\s+#(\d+)"
-   #"(?i)fix\s+#(\d+)"
-   #"(?i)fixes\s+#(\d+)"
-   #"(?i)fixed\s+#(\d+)"
-   #"(?i)resolve\s+#(\d+)"
-   #"(?i)resolves\s+#(\d+)"
-   #"(?i)resolved\s+#(\d+)"])
+  [#"(?i)close:?\s+#(\d+)"
+   #"(?i)closes:?\s+#(\d+)"
+   #"(?i)closed:?\s+#(\d+)"
+   #"(?i)fix:?\s+#(\d+)"
+   #"(?i)fixes:?\s+#(\d+)"
+   #"(?i)fixed:?\s+#(\d+)"
+   #"(?i)resolve:?\s?#(\d+)"
+   #"(?i)resolves:?\s+#(\d+)"
+   #"(?i)resolved:?\s+#(\d+)"])
 
 (defn extract-issue-number
-  [pr-body]
-  (mapcat #(keep
-            (fn [s]
-              (try (let [issue-number (Integer/parseInt (second s))]
-                     (when (pos? issue-number)
-                       issue-number))
-                   (catch NumberFormatException _)))
-            (re-seq % pr-body)) keywords))
+  [pr-body pr-title]
+  (let [extract (fn [source]
+                  (mapcat #(keep
+                            (fn [s]
+                              (try (let [issue-number (Integer/parseInt (second s))]
+                                     (when (pos? issue-number)
+                                       issue-number))
+                                   (catch NumberFormatException _)))
+                            (re-seq % source)) keywords))]
+    (concat (extract pr-body)
+            (extract pr-title))))
 
 
-(defn validate-issue-number
+(defn ensure-bounty-issue
   "Checks if an issue has a bounty label attached and returns its number"
   [user repo issue-number]
   (when-let [issue (github/get-issue user repo issue-number)]
     (when (bounties/has-bounty-label? issue)
       issue-number)))
 
-(defn handle-pull-request-closed
-  [{{{owner :login} :owner
+
+(defn handle-pull-request-event
+  ;; when a PR is opened, only consider it as a claim if:
+  ;; * PR references an existing bounty-issue
+  ;;
+  ;; when a PR is merged via close event, only consider it a bounty
+  ;; claim being accepted if:
+  ;; * PR exists in DB
+  ;; * PR references an existing bounty-issue
+  [event-type
+   {{{owner :login} :owner
      repo           :name
      repo-id        :id}    :repository
-    {{user-id :id
-      login   :login
-      name    :name} :user
+    {{user-id    :id
+      login      :login
+      avatar_url :avatar_url
+      name       :name} :user
      id              :id
      pr-number       :number
-     pr-body         :body} :pull_request}]
-  (log/debug "handle-pull-request-closed" owner repo repo-id login pr-body)
-  (future
-    (let [commit-id    (find-commit-id owner repo pr-number ["merged"])
-          issue-number (->>
-                        (extract-issue-number pr-body)
-                        (first)
-                        (validate-issue-number owner repo))
-          m            {:commit_id commit-id :issue_number issue-number}]
-      (log/debug "handle-pull-request-closed" commit-id issue-number)
-      (when (or commit-id issue-number)
-        (log/debug (format "Pull request %s/%s/%s closed with reference to %s"
-                           login repo pr-number
-                           (if commit-id (str "commit-id " commit-id)
-                               (str "issue-number " issue-number))))
-        (pull-requests/create (merge m {:repo_id   repo-id
-                                        :pr_id     id
-                                        :pr_number pr-number
-                                        :user_id   user-id}))
-        (users/create-user user-id login name nil nil)))))
+     pr-body         :body
+     pr-title        :title} :pull_request}]
+  (log/debug "handle-pull-request-event" event-type owner repo repo-id login pr-body pr-title)
+  (log/debug (extract-issue-number pr-body pr-title))
+  (when-let [bounty-issue-number (->>
+                                  (extract-issue-number pr-body pr-title)
+                                  (first)
+                                  (ensure-bounty-issue owner repo))]
+    (log/debug "Referenced bounty issue found" bounty-issue-number)
+    (users/create-user user-id login name nil avatar_url nil)
+    (let [pr-data {:repo_id   repo-id
+                   :pr_id     id
+                   :pr_number pr-number
+                   :user_id   user-id
+                   :issue_number bounty-issue-number
+                   :state event-type}]
+      (case event-type
+        :opened (do
+                  (log/info "PR with reference to bounty issue"
+                            bounty-issue-number "opened")
+                  (pull-requests/save (merge pr-data {:state :opened
+                                                      :commit_id nil})))
+        :closed (if-let [commit-id (find-commit-id owner
+                                                   repo
+                                                   pr-number
+                                                   ["merged"])]
+                  (do (log/info "PR with reference to bounty issue"
+                                bounty-issue-number "merged")
+                      (pull-requests/save
+                       (merge pr-data {:state :merged
+                                       :commit_id commit-id})))
+                  (do (log/info "PR with reference to bounty issue"
+                                bounty-issue-number "closed with no merge")
+                      (pull-requests/save
+                       (merge pr-data {:state :closed
+                                       :commit_id nil}))))))))
 
 
 (defn handle-issue
@@ -134,8 +163,10 @@
 
 (defn handle-pull-request
   [pull-request]
-  (when (= "closed" (:action pull-request))
-    (handle-pull-request-closed pull-request))
+  (case (:action pull-request)
+    "opened" (handle-pull-request-event :opened pull-request)
+    "closed" (handle-pull-request-event :closed pull-request)
+    nil)
   (ok))
 
 
diff --git a/src/clj/commiteth/scheduler.clj b/src/clj/commiteth/scheduler.clj
index 79101ce..a08d8a2 100644
--- a/src/clj/commiteth/scheduler.clj
+++ b/src/clj/commiteth/scheduler.clj
@@ -78,7 +78,7 @@
            comment-id       :comment_id
            issue-id         :issue_id
            old-balance      :balance
-           issue-number     :issue_number} (db-bounties/list-wallets)]
+           issue-number     :issue_number} (db-bounties/open-bounty-contracts)]
     (when comment-id
       (let [current-balance-hex (eth/get-balance-hex contract-address)
             current-balance-eth (eth/hex->eth current-balance-hex 8)