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

igrishaev · 2017-12-27T18:16:40Z

Fixes #193

The following PR adds a new option to exclude the current user from the top hunters list and activity feed. Here some screenshots below.

Also, while working on that task, I fixed a couple of bugs and borrowed some features my current Luminus-driven project that help a lot (see my comments below).

igrishaev · 2017-12-27T18:20:23Z

.gitignore

@@ -24,3 +24,6 @@ profiles.clj
 resources/contracts
 node_modules
 .DS_Store
+src/java
+yarn.lock
+env/dev/resources/config.edn


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.

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.

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?

tpatja · 2017-12-27T18:20:39Z

Thank you for the PR. Unfortunately, there has been a misunderstanding, as I already have an implementation for this feature. In general, we welcome contributions for issues that have a bounty on them.

There are several nice things in this PR not related to the feature that I like (makefile, bug fixes etc) that we certainly want merged. We should figure out how to merge the overlapping changes related to the feature that we now have. I'll get back to this tomorrow.

igrishaev · 2017-12-27T18:23:03Z

Makefile

@@ -0,0 +1,41 @@
+


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 ....

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?

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.

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

@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.

igrishaev · 2017-12-27T18:25:02Z

project.clj

@@ -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"]


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.

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

igrishaev · 2017-12-27T18:25:56Z

resources/migrations/20171226182259-hide-user.down.sql

+ALTER TABLE users
+    DROP COLUMN is_hidden;
+
+-- restore the previous version of the view


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

igrishaev · 2017-12-27T18:28:00Z

src/clj/commiteth/config.clj


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


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.

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).

igrishaev · 2017-12-27T18:29:48Z

src/clj/commiteth/db/core.clj

  (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")


duplication line

igrishaev · 2017-12-27T18:30:49Z

src/cljs/commiteth/handlers.cljs

@@ -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])


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

pablodip · 2017-12-29T02:43:59Z

.gitignore

@@ -24,3 +24,6 @@ profiles.clj
 resources/contracts
 node_modules
 .DS_Store
+src/java
+yarn.lock


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

right, we're not, will drop that.

pablodip · 2017-12-29T03:05:58Z

src/clj/commiteth/routes/services.clj

@@ -234,6 +235,14 @@
                              (if (= 1 result)
                                (ok)
                                (internal-server-error)))))
+
+                    (POST "/hidden" []


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.

Agree, will merge those two handlers.

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

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

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.

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

pablodip · 2018-01-31T13:09:50Z

@igrishaev Are you willing to continue and finish this PR? We would like to have it merged. :)

We would basically need to only have things related with the hide issue (not additional things, which you can of course propose in different PRs), and to only have one endpoint to send the user profile data.

Thanks! :)

igrishaev · 2018-01-31T13:53:11Z

@pablodip yes, sorry for the delay. I wan't sure if should move further with it. Let me adapt it quickly.

pablodip · 2018-01-31T14:23:04Z

@igrishaev No worries at all, just let us know when it is ready to review it. :)

Btw, please also make sure that it is up to date with develop, and also to indicate the issue that fixes in the description.

Thanks a lot! :)

pablodip · 2018-02-12T14:28:54Z

@siphiuel Good catch! :) Given that we have the Update button, it probably makes sense to update the info only when pressing that button.

denis-sharypin · 2018-02-12T14:46:24Z

@siphiuel hey This button should updates both parameters – address and settings, but this page requires a quick design update. I believe Update button should be placed under both items.

…to 193-hide-myself

igrishaev · 2018-02-12T15:17:05Z

@pablodip @denis-sharypin Merged with the latest develop and fixed UI. Here how it looks like:

denis-sharypin · 2018-02-12T15:20:04Z

If heading Settings is there, I'm happy

igrishaev · 2018-02-12T15:22:52Z

@denis-sharypin It is, please update the page (I re-attached the image).

vitvly

Looks good. Thank you!

…to 193-hide-myself

igrishaev · 2018-02-16T12:22:38Z

@pablodip or anyone else, is there something more required on that task? And for some reason, the issue hasn't been claimed. It's a bit unclear to me how to claim it. Should I send some ETH to the contract address or...? Sorry, it's the first time I contribute to Status.

igrishaev · 2018-02-16T13:23:41Z

The last part of my previous is not action. There was no Fixes clause at the top of a PR.

vitvly · 2018-02-16T14:01:46Z

All is good! I've merged it now. This should be claimed by you now in Open Bounty. Again, thank you for your contribution!

churik · 2018-02-20T14:19:56Z

Environments:

macOS HighSierra 10.13, Chrome 64
macOS HighSierra 10.13, Safari 11
macOS HighSierra 10.13, FireFox 58
windows 10, IE 11

Tested functionality:

enable/disable hiding

Regression:

checked that option doesn't affect to Ethereum address field

igrishaev added 12 commits December 26, 2017 18:14

193 gitignore files added + readme updated

264a83c

193 makefile added

7dbd327

193 sql migrations added

460eab8

193 cider plugin removed

287922d

193 ignoring dev config

58de6ae

193 start-end pairs added

0eba6fe

193 server handler added

e44639a

193 ui widget added

b77a231

193 migrations updated

bea4321

193 style added

fea8d4f

193 checkbox updated

559f4f4

193 transaction removed

78d8b28

igrishaev commented Dec 27, 2017

View reviewed changes

igrishaev changed the title ~~193 hide myself~~ 193 Hide myself from the top hunters and activity lists Dec 27, 2017

pablodip reviewed Dec 29, 2017

View reviewed changes

pablodip mentioned this pull request Jan 23, 2018

Dev fix: add cljs source paths to :dev profile and remove CIDER dep #228

Merged

vitvly force-pushed the develop branch 2 times, most recently from 2b88442 to 2f24317 Compare January 26, 2018 16:38

193 rolled back

085e8ef

igrishaev added 3 commits February 12, 2018 18:07

193 review fixes

4717a1a

Merge branch 'develop' of https://github.com/status-im/open-bounty in…

34c1645

…to 193-hide-myself

193 merge artifacts

092713a

193 h3 added

9732b7c

vitvly approved these changes Feb 13, 2018

View reviewed changes

pablodip approved these changes Feb 13, 2018

View reviewed changes

igrishaev added 2 commits February 16, 2018 14:56

Merge branch 'develop' of https://github.com/status-im/open-bounty in…

38ecb91

…to 193-hide-myself

Merge branch 'develop' into 193-hide-myself

a8cdaa4

Merge branch 'develop' into 193-hide-myself

187b748

vitvly merged commit 166c125 into status-im:develop Feb 16, 2018

churik self-assigned this Feb 20, 2018

churik added the Tested-OK label Feb 20, 2018

churik mentioned this pull request Feb 23, 2018

Fix Develop #307

Closed

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

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

Conversation

igrishaev commented Dec 27, 2017 • edited by andytudhope Loading

igrishaev Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpatja commented Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrishaev Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrishaev Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrishaev Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablodip commented Jan 31, 2018

igrishaev commented Jan 31, 2018

pablodip commented Jan 31, 2018

pablodip commented Feb 12, 2018

denis-sharypin commented Feb 12, 2018 • edited Loading

igrishaev commented Feb 12, 2018 • edited Loading

denis-sharypin commented Feb 12, 2018

igrishaev commented Feb 12, 2018

vitvly left a comment

Choose a reason for hiding this comment

igrishaev commented Feb 16, 2018

igrishaev commented Feb 16, 2018

vitvly commented Feb 16, 2018

churik commented Feb 20, 2018

Environments:

Tested functionality:

Regression:

igrishaev commented Dec 27, 2017 •

edited by andytudhope

Loading

igrishaev Dec 27, 2017 •

edited

Loading

tpatja commented Dec 27, 2017 •

edited

Loading

igrishaev Dec 29, 2017 •

edited

Loading

igrishaev Dec 27, 2017 •

edited

Loading

igrishaev Dec 29, 2017 •

edited

Loading

denis-sharypin commented Feb 12, 2018 •

edited

Loading

igrishaev commented Feb 12, 2018 •

edited

Loading