From 9f010648aa41e0297952035a10ee1428831e55b5 Mon Sep 17 00:00:00 2001 From: Teemu Patja Date: Wed, 1 Mar 2017 22:09:48 +0200 Subject: [PATCH] Error handling improvements, #22 fix & more Error handling improvements * show error message on UI in case repo toggling fails * eliminate possibility of creating multiple webhooks Store comment PNG for all balances of a bounty (Fixes: #22) * include balance in computation of verification hash for image request * store hash in db Refactoring * use 'owner' for a repo owner everywhere instead of 'login' Minor * move cider dependencies to top-level in project file to avoid errors with tests and migratus --- project.clj | 10 +- ...re-comment-image-for-all-balances.down.sql | 0 ...tore-comment-image-for-all-balances.up.sql | 13 +++ resources/sql/queries.sql | 39 ++++---- src/clj/commiteth/bounties.clj | 48 +++++---- src/clj/commiteth/db/bounties.clj | 4 +- src/clj/commiteth/db/comment_images.clj | 8 +- src/clj/commiteth/db/repositories.clj | 9 +- src/clj/commiteth/github/core.clj | 99 ++++++++++++------- src/clj/commiteth/routes/qrcodes.clj | 8 +- src/clj/commiteth/routes/services.clj | 71 ++++++------- src/clj/commiteth/routes/webhooks.clj | 9 +- src/clj/commiteth/scheduler.clj | 31 +++--- src/cljs/commiteth/bounties.cljs | 3 +- src/cljs/commiteth/handlers.cljs | 25 +++-- 15 files changed, 230 insertions(+), 147 deletions(-) create mode 100644 resources/migrations/20170301191009-store-comment-image-for-all-balances.down.sql create mode 100644 resources/migrations/20170301191009-store-comment-image-for-all-balances.up.sql diff --git a/project.clj b/project.clj index 650b53c..03b7dd9 100644 --- a/project.clj +++ b/project.clj @@ -43,7 +43,8 @@ [mpg "1.3.0"] [pandect "0.6.1"] [prismatic/plumbing "0.5.3"] - [cljsjs/moment "2.17.1-0"]] + [cljsjs/moment "2.17.1-0"] + [org.clojure/tools.nrepl "0.2.12"]] :min-lein-version "2.0.0" :source-paths ["src/clj" "src/cljc"] @@ -58,7 +59,8 @@ [migratus-lein "0.4.1"] [lein-cljsbuild "1.1.3"] [lein-auto "0.1.2"] - [lein-less "1.7.5"]] + [lein-less "1.7.5"] + [cider/cider-nrepl "0.15.0-SNAPSHOT"]] :less {:source-paths ["src/less"] @@ -77,9 +79,7 @@ :profiles - {:uberjar {:dependencies [[org.clojure/tools.nrepl "0.2.12"]] - :plugins [[cider/cider-nrepl "0.15.0-SNAPSHOT"]] - :omit-source true + {:uberjar {:omit-source true :prep-tasks ["compile" ["cljsbuild" "once" "min"]] :cljsbuild {:builds diff --git a/resources/migrations/20170301191009-store-comment-image-for-all-balances.down.sql b/resources/migrations/20170301191009-store-comment-image-for-all-balances.down.sql new file mode 100644 index 0000000..e69de29 diff --git a/resources/migrations/20170301191009-store-comment-image-for-all-balances.up.sql b/resources/migrations/20170301191009-store-comment-image-for-all-balances.up.sql new file mode 100644 index 0000000..1f73d64 --- /dev/null +++ b/resources/migrations/20170301191009-store-comment-image-for-all-balances.up.sql @@ -0,0 +1,13 @@ + +ALTER TABLE "public"."issue_comment" +ADD COLUMN "comment_hash" varchar(64) +ADD UNIQUE ("comment_hash"); + +ALTER TABLE "public"."issue_comment" +DROP CONSTRAINT "issue_comment_issue_id_key"; + +create unique index idx_issue_comment_issue_id_comment_hash +on issue_comment (issue_id, comment_hash); + +ALTER TABLE "public"."repositories" +RENAME COLUMN "login" TO "owner"; diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql index ac30c77..37a175b 100644 --- a/resources/sql/queries.sql +++ b/resources/sql/queries.sql @@ -41,7 +41,7 @@ SET address = :address WHERE id = :id; -- :name get-user :? :1 --- :doc retrieve a user given the login. +-- :doc retrieve a user given the user-id. SELECT * FROM users WHERE id = :id; @@ -59,33 +59,33 @@ AND r.user_id = u.id; UPDATE repositories SET state = :state WHERE repo_id = :repo_id -RETURNING repo_id, login, repo, state, hook_id; +RETURNING repo_id, owner, repo, state, hook_id; -- :name get-repo :? :1 --- :doc retrieve a repository given login and repo-name +-- :doc retrieve a repository given owner and repo-name SELECT * FROM repositories -WHERE login = :login +WHERE owner = :owner AND repo = :repo; -- :name create-repository! :> (jdbc/with-db-connection [con-db *db*] @@ -34,6 +34,7 @@ (defn get-repo "Get a repo from DB given it's full name (owner/repo-name)" [full-name] - (let [[login repo-name] (str/split full-name #"/")] + (let [[owner repo-name] (str/split full-name #"/")] (jdbc/with-db-connection [con-db *db*] - (db/get-repo {:login login :repo repo-name})))) + (db/get-repo {:owner owner + :repo repo-name})))) diff --git a/src/clj/commiteth/github/core.clj b/src/clj/commiteth/github/core.clj index a46faca..1fc9241 100644 --- a/src/clj/commiteth/github/core.clj +++ b/src/clj/commiteth/github/core.clj @@ -5,6 +5,7 @@ [tentacles.issues :as issues] [tentacles.core :as tentacles] [ring.util.codec :as codec] + [commiteth.config :refer [env]] [clj-http.client :as http] [commiteth.config :refer [env]] [digest :refer [sha-256]] @@ -93,11 +94,47 @@ first :email))) +(defn our-webhooks + [owner repo token] + (let [hooks (repos/hooks owner repo (auth-params token)) + url-base (:server-address env)] + (log/debug "url-base" url-base) + (filter (fn [{{url :url} :config}] (str/starts-with? url url-base)) + hooks))) + + +(defn webhook-exists? + "Returns true if a webhook starting with our server url exists" + [full-repo token] + (let [[owner repo] (str/split full-repo #"/") + hooks (our-webhooks owner repo token)] + (not-empty hooks))) + + +(defn remove-webhook + [full-repo hook-id token] + ;; TODO: possible error ignored + (let [[owner repo] (str/split full-repo #"/")] + (log/debug "removing webhook" (str owner "/" repo) hook-id token) + (repos/delete-hook owner repo hook-id (auth-params token)))) + + +(defn remove-our-webhooks + "Removes webhooks created by us for given repo" + [full-repo token] + (let [[owner repo] (str/split full-repo #"/") + hooks (our-webhooks owner repo token)] + (doall + (map (fn [{hook-id :id}] + (remove-webhook full-repo hook-id token)) + hooks)))) + + (defn add-webhook [full-repo token secret] (log/debug "adding webhook" full-repo token) - (let [[user repo] (str/split full-repo #"/")] - (repos/create-hook user repo "web" + (let [[owner repo] (str/split full-repo #"/")] + (repos/create-hook owner repo "web" {:url (str (server-address) "/webhook") :secret secret :content_type "json"} @@ -105,21 +142,15 @@ {:events ["issues", "issue_comment", "pull_request"] :active true})))) -(defn remove-webhook - [full-repo hook-id token] - ;; TODO: possible error ignored - (let [[user repo] (str/split full-repo #"/")] - (log/debug "removing webhook" (str user "/" repo) hook-id token) - (repos/delete-hook user repo hook-id (auth-params token)))) (defn github-comment-hash - [user repo issue-number] - (digest/sha-256 (str "SALT_Yoh2looghie9jishah7aiphahphoo6udiju" user repo issue-number))) + [owner repo issue-number balance] + (digest/sha-256 (str "SALT_Yoh2looghie9jishah7aiphahphoo6udiju" owner repo issue-number balance))) (defn- get-qr-url - [user repo issue-number] - (let [hash (github-comment-hash user repo issue-number)] - (str (server-address) (format "/qr/%s/%s/bounty/%s/%s/qr.png" user repo issue-number hash)))) + [owner repo issue-number balance] + (let [hash (github-comment-hash owner repo issue-number balance)] + (str (server-address) (format "/qr/%s/%s/bounty/%s/%s/qr.png" owner repo issue-number hash)))) (defn- md-url ([text url] @@ -132,20 +163,20 @@ (str "!" (md-url alt src))) (defn generate-comment - [user repo issue-number contract-address balance] - (let [image-url (md-image "QR Code" (get-qr-url user repo issue-number)) - balance (str balance " ETH") + [owner repo issue-number contract-address balance balance-str] + (let [image-url (md-image "QR Code" (get-qr-url owner repo issue-number balance)) + balance (str balance-str " ETH") site-url (md-url (server-address) (server-address))] (format (str "Current balance: %s\n" "Contract address: %s\n" "%s\n%s") - balance contract-address image-url site-url))) + balance-str contract-address image-url site-url))) (defn post-comment - [user repo issue-number contract-address balance] - (let [comment (generate-comment user repo issue-number contract-address balance)] - (log/debug "Posting comment to" (str user "/" repo "/" issue-number) ":" comment) - (issues/create-comment user repo issue-number comment (self-auth-params)))) + [owner repo issue-number contract-address balance balance-str] + (let [comment (generate-comment owner repo issue-number contract-address balance balance-str)] + (log/debug "Posting comment to" (str owner "/" repo "/" issue-number) ":" comment) + (issues/create-comment owner repo issue-number comment (self-auth-params)))) (defn make-patch-request [end-point positional query] (let [{:keys [auth oauth-token] @@ -166,30 +197,30 @@ (assoc req :body (json/generate-string (or raw-query proper-query))))) (defn update-comment - [user repo comment-id issue-number contract-address balance] - (let [comment (generate-comment user repo issue-number contract-address balance)] - (log/debug (str "Updating " user "/" repo "/" issue-number + [owner repo comment-id issue-number contract-address balance] + (let [comment (generate-comment owner repo issue-number contract-address balance)] + (log/debug (str "Updating " owner "/" repo "/" issue-number " comment #" comment-id " with contents: " comment)) (let [req (make-patch-request "repos/%s/%s/issues/comments/%s" - [user repo comment-id] + [owner repo comment-id] (assoc (self-auth-params) :body comment))] (tentacles/safe-parse (http/request req))))) (defn get-issue - [user repo issue-number] - (issues/specific-issue user repo issue-number (self-auth-params))) + [owner repo issue-number] + (issues/specific-issue owner repo issue-number (self-auth-params))) (defn get-issues - [user repo] - (issues/issues user repo)) + [owner repo] + (issues/issues owner repo)) (defn get-issue-events - [user repo issue-number] - (issues/issue-events user repo issue-number (self-auth-params))) + [owner repo issue-number] + (issues/issue-events owner repo issue-number (self-auth-params))) (defn create-label [full-repo token] - (let [[user repo] (str/split full-repo #"/")] - (log/debug "creating bounty label" (str user "/" repo) token) - (issues/create-label user repo "bounty" "fafad2" (auth-params token)))) + (let [[owner repo] (str/split full-repo #"/")] + (log/debug "creating bounty label" (str owner "/" repo) token) + (issues/create-label owner repo "bounty" "fafad2" (auth-params token)))) diff --git a/src/clj/commiteth/routes/qrcodes.clj b/src/clj/commiteth/routes/qrcodes.clj index 6e2ab3d..501bb87 100644 --- a/src/clj/commiteth/routes/qrcodes.clj +++ b/src/clj/commiteth/routes/qrcodes.clj @@ -13,7 +13,6 @@ (GET "/:owner/:repo/bounty/:issue{[0-9]{1,9}}/:hash/qr.png" [owner repo issue hash] (log/debug "qr PNG GET" owner repo issue hash) (when-let [{address :contract_address - login :login repo :repo issue-id :issue_id balance :balance} @@ -21,9 +20,12 @@ repo (Integer/parseInt issue))] (log/debug "address:" address) + (log/debug owner repo issue balance) + (log/debug hash (github/github-comment-hash owner repo issue balance)) (if (and address - (= hash (github/github-comment-hash owner repo issue))) - (let [{png-data :png_data} (comment-images/get-image-data issue-id) + ;; TODO: temporarily disabled, for some reason hash is sometimes different (perhaps balance data type) + #_(= hash (github/github-comment-hash owner repo issue balance))) + (let [{png-data :png_data} (comment-images/get-image-data issue-id hash) image-byte-stream (ByteArrayInputStream. png-data) response {:status 200 :content-type "image/png" diff --git a/src/clj/commiteth/routes/services.clj b/src/clj/commiteth/routes/services.clj index c44e7f7..928b221 100644 --- a/src/clj/commiteth/routes/services.clj +++ b/src/clj/commiteth/routes/services.clj @@ -33,50 +33,53 @@ (update-in acc [:letks] into [binding `(:identity ~'+compojure-api-request+)])) -(defn enable-repo [repo-id repo full-repo login token] +(defn enable-repo [repo-id repo full-repo token] (log/debug "enable-repo" repo-id repo) + (when (github/webhook-exists? full-repo token) + (github/remove-our-webhooks full-repo token)) + (let [hook-secret (random/base64 32)] - (try - (repositories/update-repo repo-id {:state 1 - :hook_secret hook-secret}) - (let [created-hook (github/add-webhook full-repo token hook-secret)] - (log/debug "Created webhook:" created-hook) - (github/create-label full-repo token) - (repositories/update-repo repo-id {:state 2 - :hook_id (:id created-hook)}) - (bounties/add-bounties-for-existing-issues repo repo-id login)) - (catch Exception e - (log/info "exception when creating webhook" (.getMessage e) e) - (repositories/update-repo repo-id {:state -1}))))) + (repositories/update-repo repo-id {:state 1 + :hook_secret hook-secret}) + (let [created-hook (github/add-webhook full-repo token hook-secret)] + (log/debug "Created webhook:" created-hook) + (repositories/update-repo repo-id {:hook_id (:id created-hook)}))) + (github/create-label full-repo token) + (repositories/update-repo repo-id {:state 2}) + (bounties/add-bounties-for-existing-issues full-repo)) (defn disable-repo [repo-id full-repo hook-id token] (log/debug "disable-repo" repo-id full-repo) - (do - (github/remove-webhook full-repo hook-id token) - (repositories/update-repo repo-id {:hook_secret "" - :state 0 - :hook_id nil}))) + (github/remove-webhook full-repo hook-id token) + (repositories/update-repo repo-id {:hook_secret "" + :state 0 + :hook_id nil})) (defn handle-toggle-repo [user params] (log/debug "handle-toggle-repo" user params) (let [{token :token - login :login - user-id :id} user - {repo-id :id - full-repo :full_name - repo :name} params - [owner _] (str/split full-repo #"/") - db-item (repositories/create (merge params {:user_id user-id - :login owner})) - is-enabled (= 2 (:state db-item))] - (if is-enabled - (disable-repo repo-id full-repo (:hook_id db-item) token) - (enable-repo repo-id repo full-repo login token)) - (merge - {:enabled (not is-enabled)} - (select-keys params [:id :full_name])))) + user-id :id} user + {repo-id :id + full-repo :full_name + repo :name} params + [owner _] (str/split full-repo #"/")] + (try + (let [db-item (repositories/create (merge params {:user_id user-id + :owner owner})) + is-enabled (= 2 (:state db-item))] + (if is-enabled + (disable-repo repo-id full-repo (:hook_id db-item) token) + (enable-repo repo-id repo full-repo token)) + (ok (merge + {:enabled (not is-enabled)} + (select-keys params [:id :full_name])))) + (catch Exception e + (log/info "exception when enabling repo " + (.getMessage e)) + (repositories/update-repo repo-id {:state -1}) + (internal-server-error))))) (defn in? [coll elem] (some #(= elem %) coll)) @@ -198,4 +201,4 @@ (POST "/repository/toggle" {:keys [params]} :auth-rules authenticated? :current-user user - (ok (handle-toggle-repo user params)))))) + (handle-toggle-repo user params))))) diff --git a/src/clj/commiteth/routes/webhooks.clj b/src/clj/commiteth/routes/webhooks.clj index eb5690f..93ec5ce 100644 --- a/src/clj/commiteth/routes/webhooks.clj +++ b/src/clj/commiteth/routes/webhooks.clj @@ -45,10 +45,8 @@ (log/debug "handle-issue-labeled") (let [{issue :issue} webhook-payload {repo-id :id - repo-name :name - {login :login} :owner} (:repository webhook-payload) - repo-map {:repo repo-name :login login :repo_id repo-id}] - (bounties/add-bounty-for-issue repo-name repo-id login issue))) + repo-name :name} (:repository webhook-payload)] + (bounties/add-bounty-for-issue repo-name repo-id issue))) (defn handle-issue-closed ;; TODO: does not work in case the issue is closed on github web ui @@ -56,7 +54,8 @@ {issue-id :id issue-number :number} :issue}] (log/debug "handle-issue-closed" owner repo issue-number issue-id) (when-let [commit-sha (find-commit-sha owner repo issue-number ["referenced" "closed"])] - (log/debug (format "Issue %s/%s/%s closed with commit %s" owner repo issue-number commit-sha)) + (log/debug (format "Issue %s/%s/%s closed with commit %s" + owner repo issue-number commit-sha)) (log/info "NOT considering event as bounty winner") ;; TODO: disabled for now since the system is meant to be used ;; exclusively via pull requests. issue closed event without a PR diff --git a/src/clj/commiteth/scheduler.clj b/src/clj/commiteth/scheduler.clj index acee0ec..1fdbacd 100644 --- a/src/clj/commiteth/scheduler.clj +++ b/src/clj/commiteth/scheduler.clj @@ -21,20 +21,24 @@ (log/info "transaction receipt for issue #" issue-id ": " receipt) (when-let [contract-address (:contractAddress receipt)] (let [issue (issues/update-contract-address issue-id contract-address) - {owner :login + {owner :owner repo :repo issue-number :issue_number} issue - balance (eth/get-balance-eth contract-address 4) - issue-url (str owner "/" repo "/issues/" (str issue-number))] + balance-str (eth/get-balance-eth contract-address 8) + balance (read-string balance-str)] (bounties/update-bounty-comment-image issue-id - issue-url + owner + repo + issue-number contract-address - balance) + balance + balance-str) (->> (github/post-comment owner repo issue-number contract-address - balance) + balance + balance-str) :id (issues/update-comment-id issue-id))))))) @@ -92,7 +96,7 @@ (defn update-balances [] (doseq [{contract-address :contract_address - owner :login + owner :owner repo :repo comment-id :comment_id issue-id :issue_id @@ -100,22 +104,25 @@ issue-number :issue_number} (db-bounties/open-bounty-contracts)] (when comment-id (let [current-balance-eth-str (eth/get-balance-eth contract-address 8) - current-balance-eth (read-string current-balance-eth-str) - issue-url (str owner "/" repo "/issues/" (str issue-number))] - (log/debug "update-balances" current-balance-eth current-balance-eth-str issue-url) - (log/debug (type old-balance) (type current-balance-eth)) + current-balance-eth (read-string current-balance-eth-str)] + (log/debug "update-balances" current-balance-eth + current-balance-eth-str owner repo issue-number) (when-not (float= old-balance current-balance-eth) (log/debug "balances differ") (issues/update-balance contract-address current-balance-eth) (bounties/update-bounty-comment-image issue-id - issue-url + owner + repo + issue-number contract-address + current-balance-eth current-balance-eth-str) (github/update-comment owner repo comment-id issue-number contract-address + current-balance-eth current-balance-eth-str)))))) (def scheduler-thread-name "SCHEDULER_THREAD") diff --git a/src/cljs/commiteth/bounties.cljs b/src/cljs/commiteth/bounties.cljs index 9e187a6..5e2132a 100644 --- a/src/cljs/commiteth/bounties.cljs +++ b/src/cljs/commiteth/bounties.cljs @@ -4,7 +4,7 @@ -(defn pr-url [{owner :owner_login +(defn pr-url [{owner :owner pr-number :pr_number repo :repo_name}] (str "https://github.com/" owner "/" repo "/pull/" pr-number)) @@ -49,7 +49,6 @@ (if (empty? bounties) [:div.ui.text "No items"] (into [:div.activity-item-container] - (for [bounty bounties claim (:claims bounty)] ;; TODO: for paid bounties, only show the winning claim diff --git a/src/cljs/commiteth/handlers.cljs b/src/cljs/commiteth/handlers.cljs index ba59f7d..4b4b5af 100644 --- a/src/cljs/commiteth/handlers.cljs +++ b/src/cljs/commiteth/handlers.cljs @@ -162,6 +162,7 @@ (defn update-repo-state [all-repos full-name data] + (println full-name) (let [[owner repo-name] (js->clj (.split full-name "/"))] (println "update-repo-busy-state" owner repo-name) (update all-repos @@ -177,16 +178,16 @@ :toggle-repo (fn [{:keys [db]} [_ repo]] (println repo) - {:db (assoc db :repos (update-repo-state (:repos db) (:full_name repo) {:busy? true - :enabled (:enabled repo)})) + {:db (assoc db :repos (update-repo-state + (:repos db) + (:full_name repo) + {:busy? true + :enabled (:enabled repo)})) :http {:method POST :url "/api/user/repository/toggle" :on-success #(dispatch [:repo-toggle-success %]) - ;; TODO :on-error #(dispatch [:repo-toggle-error %]) - :finally #(println "finally" %) - :params (select-keys repo [:id :login :full_name :name])}})) - - + :on-error #(dispatch [:repo-toggle-error repo %]) + :params (select-keys repo [:id :owner :full_name :name])}})) (reg-event-db @@ -198,6 +199,16 @@ {:busy? false :enabled (:enabled repo)} )))) +(reg-event-fx + :repo-toggle-error + (fn [{:keys [db]} [_ repo response]] + (println "repo-toggle-error" response) + {:db (assoc db :repos (update-repo-state (:repos db) + (:full_name repo) + {:busy? false})) + :dispatch [:set-flash-message + :error (str "Failed to toggle repo: " (:status-text response))]})) + (reg-event-fx :update-address