Skip to content
This repository has been archived by the owner on Apr 26, 2019. It is now read-only.

193 Hide myself from the top hunters and activity lists #199

Merged
merged 35 commits into from
Feb 16, 2018
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
264a83c
193 gitignore files added + readme updated
igrishaev Dec 26, 2017
7dbd327
193 makefile added
igrishaev Dec 26, 2017
460eab8
193 sql migrations added
igrishaev Dec 26, 2017
287922d
193 cider plugin removed
igrishaev Dec 26, 2017
58de6ae
193 ignoring dev config
igrishaev Dec 26, 2017
0eba6fe
193 start-end pairs added
igrishaev Dec 26, 2017
e44639a
193 server handler added
igrishaev Dec 27, 2017
b77a231
193 ui widget added
igrishaev Dec 27, 2017
bea4321
193 migrations updated
igrishaev Dec 27, 2017
fea8d4f
193 style added
igrishaev Dec 27, 2017
559f4f4
193 checkbox updated
igrishaev Dec 27, 2017
78d8b28
193 transaction removed
igrishaev Dec 27, 2017
085e8ef
193 rolled back
igrishaev Jan 31, 2018
07bb457
193 user exists query added
igrishaev Feb 6, 2018
50e3045
193 address + hidden routes mixed
igrishaev Feb 6, 2018
8937816
193 query field updated
igrishaev Feb 6, 2018
2b7dff9
193 UI updated
igrishaev Feb 6, 2018
6107fec
193 gitignore fix
igrishaev Feb 6, 2018
fb95fe6
193 print removed
igrishaev Feb 6, 2018
c860db5
193 config.example deleted
igrishaev Feb 6, 2018
b162bba
193 config file restored
igrishaev Feb 6, 2018
a0073b2
Merge branch 'develop' of https://github.com/status-im/open-bounty in…
igrishaev Feb 6, 2018
a61885d
193 get rid of passing user-id
igrishaev Feb 7, 2018
596e6c9
Merge branch 'develop' of https://github.com/status-im/open-bounty in…
igrishaev Feb 8, 2018
c66691c
193 review fixes
igrishaev Feb 8, 2018
56d1425
193 sql fix
igrishaev Feb 8, 2018
21c869a
193 blank line removed
igrishaev Feb 8, 2018
c2d6094
Merge branch 'develop' of https://github.com/status-im/open-bounty in…
igrishaev Feb 9, 2018
4717a1a
193 review fixes
igrishaev Feb 12, 2018
34c1645
Merge branch 'develop' of https://github.com/status-im/open-bounty in…
igrishaev Feb 12, 2018
092713a
193 merge artifacts
igrishaev Feb 12, 2018
9732b7c
193 h3 added
igrishaev Feb 12, 2018
38ecb91
Merge branch 'develop' of https://github.com/status-im/open-bounty in…
igrishaev Feb 16, 2018
a8cdaa4
Merge branch 'develop' into 193-hide-myself
igrishaev Feb 16, 2018
187b748
Merge branch 'develop' into 193-hide-myself
Feb 16, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,6 @@ profiles.clj
resources/contracts
node_modules
.DS_Store
src/java
yarn.lock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know we're not using yarn, are we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, we're not, will drop that.

env/dev/resources/config.edn
Copy link
Contributor Author

@igrishaev igrishaev Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When changing that config file, it hangs in git status causing a chance to commit my credentials eventually. The better approach, I believe, would be to provide a sample of that file giving instructions in readme.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the sample file! And we could even separate normal config from sensitive config by using two config files, maybe config.edn and config-private.edn, so that we are more aware of that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or how about instead of having a sample having a config-default.edn as the sample, which could be imported alongside config-private.edn`, this one overwriting the custom values from the other one, and this one not being versioned?

41 changes: 41 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the following makefile helps to perform different tasks on the project without forcing you to keep their syntax in mind. For example, to create a new migration, you write: make create-migration, it promts for its name and creates it as well. Most of modern shells show possible make targets when pressing tab key after make ....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just using lein aliases so that we avoid new tools and have more power by being able to access our project and language in lein in case we need it?

Copy link
Contributor Author

@igrishaev igrishaev Dec 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, please do not take me wrong, I'm not against lein aliases. There are some advantages of makefile over lein that I'm going to share.

First, starring any lein command takes a REALLY long time. Even without auto-tasks, the delay might be about 5 to 10 seconds. Now Imagine you made a misprint and should call it and wait again.

Then, with auto-tasks specified in lein project, the process of starting repl takes up to minute, because of compiling contracts to Java classes. But I just wanted to start REPL. Would it be better to move compiling process into a separate task? Make target?

Finally, lein utility was created for managing clojure-driven project, but not a common project. For example, in make file you may declare dependencies, saying something like make uberjar that automatically calls building less, js, contracts, etc tracking their success status.

Makefile is rather a collection of actions that might be performed over the project in general. Say, compiling CSS files is not about clojure, it is more general part. This file is easier to read because of its clear declarative syntax rather then lein project file that usually brings lots of additional stuff.

But still, if you guys do not want to have makefile in the project, there no any claims from my side. I'll just keep it locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, a funny misprint: I'm not against aliases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igrishaev

Thanks for the explanations! :)

I'm okay with the makefile as long as it is just to run commands in an easier way, not to develop more logics there.

As for the time that lein takes to run commands and the management those commands, we could probably overcome that with lumo and/or boot to continue having the power of clojure and the integration with the whole project, but that is probably not a priority right now.

all: uberjar

.PHONY: contracts
contracts:
./build_contracts.sh

less:
lein less once

less-auto:
lein less auto

repl:
lein repl

uberjar:
lein uberjar

yarn:
yarn install

cljsbuild: yarn
lein cljsbuild once

figwheel: yarn
rlwrap lein figwheel

.PHONY: test
test:
lein test

migrate:
lein migratus migrate

create-migration:
@read -p "Enter migration name: " migration \
&& lein migratus create $$migration

orig:
find . -name '*.orig' -delete
9 changes: 8 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,14 @@ Web3j [2.3.0](https://github.com/web3j/web3j/releases/tag/v2.3.0) is required an

## Running

Make sure `env/dev/resources/config.edn` is correctly populated.
Prepare your local git-ignored config file:

```
cd env/dev/resources
cp config.example.edn config.edn
```

Make sure `config.edn` is correctly populated.

Lauch a local geth node with the bot account unlocked:

Expand Down
2 changes: 0 additions & 2 deletions project.clj
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,8 @@
[lein-auto "0.1.2"]
[lein-less "1.7.5"]
[lein-shell "0.5.0"]
[cider/cider-nrepl "0.15.0-SNAPSHOT"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since every developer has their own cider version installed, a good practice would be to have in local profiles in ~/.lein/profiles.clj. For example, I've got 0.15.1 version installed that causes warning messages when connecting to repl.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this change for now? We plan to address that here: #255

[lein-sha-version "0.1.1"]]


:less {:source-paths ["src/less"]
:target-path "resources/public/css"}

Expand Down
35 changes: 35 additions & 0 deletions resources/migrations/20171226182259-hide-user.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
BEGIN;

ALTER TABLE users
DROP COLUMN is_hidden;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better a bit more specific name, something like: is_hidden_in_hunters


-- restore the previous version of the view
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and below, I had to redefine existing DB view to add non-hidden user clause.

CREATE OR REPLACE VIEW "public"."claims_view" AS
SELECT
i.title AS issue_title,
i.issue_number,
r.repo AS repo_name,
r.owner AS repo_owner,
COALESCE(u.name, u.login) AS user_name,
u.avatar_url AS user_avatar_url,
i.payout_receipt,
p.updated,
i.updated AS issue_updated,
i.balance_eth,
i.tokens,
i.value_usd,
p.state AS pr_state,
i.is_open AS issue_open,
(case when u.address IS NULL THEN false ELSE true END) AS user_has_address
FROM issues i,
users u,
repositories r,
pull_requests p
WHERE r.repo_id = i.repo_id
AND p.issue_id = i.issue_id
AND p.user_id = u.id
AND i.contract_address IS NOT NULL
AND i.comment_id IS NOT NULL
ORDER BY p.updated;

COMMIT;
35 changes: 35 additions & 0 deletions resources/migrations/20171226182259-hide-user.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
BEGIN;

ALTER TABLE users
ADD COLUMN is_hidden BOOLEAN NOT NULL DEFAULT FALSE;

CREATE OR REPLACE VIEW "public"."claims_view" AS
SELECT
i.title AS issue_title,
i.issue_number,
r.repo AS repo_name,
r.owner AS repo_owner,
COALESCE(u.name, u.login) AS user_name,
u.avatar_url AS user_avatar_url,
i.payout_receipt,
p.updated,
i.updated AS issue_updated,
i.balance_eth,
i.tokens,
i.value_usd,
p.state AS pr_state,
i.is_open AS issue_open,
(case when u.address IS NULL THEN false ELSE true END) AS user_has_address
FROM issues i,
users u,
repositories r,
pull_requests p
WHERE r.repo_id = i.repo_id
AND p.issue_id = i.issue_id
AND p.user_id = u.id
AND i.contract_address IS NOT NULL
AND i.comment_id IS NOT NULL
AND NOT u.is_hidden -- added
ORDER BY p.updated;

COMMIT;
1 change: 1 addition & 0 deletions resources/sql/queries.sql
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,7 @@ WHERE
pr.commit_sha = i.commit_sha
AND u.id = pr.user_id
AND i.payout_receipt IS NOT NULL
AND NOT u.is_hidden
GROUP BY u.id
ORDER BY total_usd DESC
LIMIT 5;
Expand Down
8 changes: 7 additions & 1 deletion src/clj/commiteth/config.clj
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
(ns commiteth.config
(:require [cprop.core :refer [load-config]]
[cprop.source :as source]
[mount.core :refer [args defstate]]))
[mount.core :refer [args defstate] :as mount]))

(defstate env :start (load-config
:merge
[(args)
(source/from-system-props)
(source/from-env)]))

Copy link
Contributor Author

@igrishaev igrishaev Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below, it would be great to allow a developer run exactly those subsystems that he or she really needs when working on a task. Having start! and stop! functions helps when you'd like to just start/stop the database for example.

While working on that task, I didn't manage to run some of components due to configuration issues. Instead, I just launched config, db and http-server which were enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about describing the different combinations/groups that could be useful instead of adding a fn to start/stop states independently? In the end starting/stopping states independently can be done easily in the REPL. For example we could have a group to start just the web app without any connection to Ethereum (and without things that depend on that).

(defn start! []
(mount/start #'env))

(defn stop! []
(mount/stop #'env))
16 changes: 11 additions & 5 deletions src/clj/commiteth/core.clj
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,23 @@
:parse-fn #(Integer/parseInt %)]])

(mount/defstate
http-server
http-server
:start
(http/start
(-> env
(assoc :handler (handler/app))
(update :port #(or (-> env :options :port) %))))
(-> env
(assoc :handler (handler/app))
(update :port #(or (-> env :options :port) %))))
:stop
(http/stop http-server))

(defn start! []
(mount/start #'http-server))

(defn stop! []
(mount/stop #'http-server))

(mount/defstate ^{:on-reload :noop}
repl-server
repl-server
:start
(when-let [nrepl-port (env :nrepl-port)]
(log/info "Starting NREPL server on port" nrepl-port)
Expand Down
21 changes: 17 additions & 4 deletions src/clj/commiteth/db/core.clj
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
[clojure.java.jdbc :as jdbc]
[conman.core :as conman]
[commiteth.config :refer [env]]
[mount.core :refer [defstate]]
[mount.core :refer [defstate] :as mount]
[migratus.core :as migratus]
[mpg.core :as mpg]
[clojure.string :as str])
Expand All @@ -20,21 +20,26 @@

(mpg/patch)

(defn start []
(defn db-start []
(let [db (env :jdbc-database-url)
migratus-config {:store :database
:migration-dir "migrations/"
:migration-table-name "schema_migrations"
:db db}]
(migratus/migrate migratus-config)
(conman/bind-connection db "sql/queries.sql")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplication line

(conman/connect! {:jdbc-url db})
db))

(defstate ^:dynamic *db*
:start (start)
:start (db-start)
:stop (conman/disconnect! *db*))

(defn start! []
(mount/start #'*db*))

(defn stop! []
(mount/stop #'*db*))

(conman/bind-connection *db* "sql/queries.sql")

(defn to-date [^java.sql.Date sql-date]
Expand Down Expand Up @@ -85,3 +90,11 @@
(sql-value [value] (to-pg-json value))
IPersistentVector
(sql-value [value] (to-pg-json value)))

(defmacro with-trx [& body]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this macro!

Just I think that the abbreviation of transaction is normally tx.

"Performs a set of queries in transaction."
`(conman/with-transaction [*db*]
~@body))

(defn update! [& args]
(apply jdbc/update! *db* args))
9 changes: 9 additions & 0 deletions src/clj/commiteth/routes/services.clj
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
[compojure.api.meta :refer [restructure-param]]
[buddy.auth.accessrules :refer [restrict]]
[buddy.auth :refer [authenticated?]]
[commiteth.db.core :as db]
[commiteth.db.users :as users]
[commiteth.db.usage-metrics :as usage-metrics]
[commiteth.db.repositories :as repositories]
Expand Down Expand Up @@ -234,6 +235,14 @@
(if (= 1 result)
(ok)
(internal-server-error)))))

(POST "/hidden" []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using just one endpoint to update data for the user? Before we just had the address, but now this too, and in the future probably more things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, will merge those two handlers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talk to @tpatja first, since he said here that he was already implementing this feature.

Copy link
Contributor Author

@igrishaev igrishaev Dec 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, let me ask here
@tpatja where did you update that handler? In your local branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said in my previous comment, I implemented this whole feature (backend and frontend changes) already last week. My changes are still on my local repo, will clean up and push once I return to work. Will be on holiday until Jan 8th.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that sounds great. But still, I'll refactor that PR to make it satisfy your and @pablanopete suggestions.

:auth-rules authenticated?
:body-params [user-id :- Long, hidden :- Boolean]
:summary "(Un)mark a user as being hidden (not visible in rating tables)."
(db/update! :users {:is_hidden hidden} ["id = ?" user-id])
(ok))

(GET "/repositories" {:keys [params]}
:auth-rules authenticated?
:current-user user
Expand Down
20 changes: 18 additions & 2 deletions src/cljs/commiteth/handlers.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,6 @@
{:db db
:dispatch [:set-active-page :update-address]}))


(reg-event-fx
:save-user-address
(fn [{:keys [db]} [_ user-id address]]
Expand All @@ -320,7 +319,7 @@
:http {:method POST
:url "/api/user/address"
:on-success #(do
(dispatch [:assoc-in [:user [:address] address]])
(dispatch [:assoc-in [:user :address] address])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a bug here, when the wrong key was updated so nothing was shown in UI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

(dispatch [:set-flash-message
:success
"Address saved"]))
Expand All @@ -332,6 +331,23 @@
:finally #(dispatch [:clear-updating-address])
:params {:user-id user-id :address address}}}))

(reg-event-fx
:mark-user-hidden
(fn [{:keys [db]} [_ user-id hidden]]
{:http {:method POST
:url "/api/user/hidden"
:on-success #(do
(dispatch [:assoc-in [:user :is_hidden] hidden])
(dispatch [:set-flash-message
:success
"Settings saved"]))
:on-error #(do
(dispatch [:set-flash-message
:error (:response %)]))
:params {:user-id user-id :hidden hidden}}}))



(reg-event-db
:clear-updating-address
(fn [db _]
Expand Down
22 changes: 18 additions & 4 deletions src/cljs/commiteth/update_address.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@
[reagent.crypt :as crypt]
[cljs-web3.eth :as web3-eth]))


(defn update-address-page []
(let [db (rf/subscribe [:db])
user (rf/subscribe [:user])
updating-address (rf/subscribe [:get-in [:updating-address]])
address (r/atom @(rf/subscribe [:get-in [:user :address]]))]
address (r/atom @(rf/subscribe [:get-in [:user :address]]))
hidden (rf/subscribe [:get-in [:user :is_hidden]])]

(fn []
(let [web3 (:web3 @db)
web3-accounts (when web3
(web3-eth/accounts web3))]
(println "web3-accounts" web3-accounts)
[:div.ui.container.grid
[:div.ui.form.sixteen.wide.column
[:h3 "Update address"]
Expand All @@ -41,4 +41,18 @@
:class (str "ui button small update-address-button"
(when @updating-address
" busy loading"))})
"UPDATE"]]]))))
"UPDATE"]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some blank lines here and below.

[:h3 "Settings"]
[:div

[:input
{:type :checkbox
:id :input-hidden
:checked @hidden
:on-change
(fn [e]
(let [value (-> e .-target .-checked)]
(rf/dispatch [:mark-user-hidden (:id @user) value])))}]

[:label {:for :input-hidden} "Disguise myself from the top hunters and activity lists."]]]]))))
10 changes: 9 additions & 1 deletion src/less/style.less
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,20 @@
}
}

label[for="input-hidden"] {
padding-left: 8px;
}

#input-hidden {
vertical-align: bottom;
position: relative;
top: -4px;
}

.login-button {
background-color: rgba(255,255,255,0.2)!important;
}


.commiteth-header {
background-color: #57a7ed!important;
border-radius: 0em;
Expand Down