Command scopes, better semantics of commands, different /send and /request for groups and personal chats #1754

alwx · 2017-09-05T11:55:07Z

Ok, this PR is quite big, and there will be a lot of text here.

What happens?

This PR is a part of API refactoring/restructuring, and this PR changes the way commands look and work. It removes any need in hardcoded /send and /request-related things and also makes mailman-like behavior accessible for 3rd-party developers. It also introduces scopes for commands and removes 99% of hardcoded things. By doing it, we make our API better and remove almost all the internal "magic" which was previously unavailable for 3rd-party developers.

1. Scopes

The most important part of this PR is scopes for commands.
The best way to demonstrate scopes is this extract from the new /send:

var send = {
    name: "send",
    scope: {
        isGlobal: false,
        personalChats: true,
        groupChats: false,
        canUseForDApps: false
    },
    icon: "money_white",
    color: "#5fc48d",
    title: I18n.t('send_title'),
    description: I18n.t('send_description'),
    params: paramsSend,
    validator: validateSend,
    handler: handleSend,
    preview: previewSend,
    shortPreview: shortPreviewSend
};

The important thing is that we can create two or more commands with the same name, but different scopes and a different set of parameters. It allows us to get rid of contact selector for /send in 1-to-1 chats and allows 3rd-party devs to make commands more flexible.

2. Mailman

Mailman is no longer something special. Everybody can create the same contact whos commands will be mixed with other contact's commands. In combination with scopes, it allows creating something super crazy and flexible.

3. Less magic for /send and /request

There are still some hardcoded things for /send and /request, but the number of them has decreased drastically.

4. Commands model

I moved some common commands-related things to a separate model. It is less complicated and structured better than it was before.

5. Tons of code quality improvements

Yay!

6. One more thing — text input should work faster

We don't call :update-suggestions each time user types a character since now, so the speed of text input should increase.

janherich

I think the scopes are step in the right direction, I like the, also how it simplifies suggestions on the RN side.
The only thing which I'm little bit uneasy with is the duplication in transactor_group/bot.js and transactor_personal/bot.js commands, we should lift common code into some other file up in the hierarchy, so it's reused.

janherich · 2017-10-02T10:21:54Z

src/status_im/chat/models/commands.cljs

+            [clojure.string :as str]
+            [taoensso.timbre :as log]))
+
+(defn scope->int [{:keys [global? registered-only? personal-chats? group-chats? can-use-for-dapps?]}]


Could we rename this to scope->hash and provide proper docstring describing how hash is computed by bitwise OR operation ?
Otherwise it could be quite cryptic at first glance if the person working on the code is not familiar with how catalog of commands is structured in status.js and how scope hash is used as second level index.

This scope->int calculation is pretty self-descriptive, isn't it? And doing this kind of calculation is a common practice.

Well, when somebody looks at any place in code where it's used (mostly when constructing path for jail call), scope->int is little bit vague and she/he needs to jump to function definition before it starts making sense, but with scope->hash it's immediately clear that we are computing hash out of the scope map.
But not a big deal, I'm fine with either way.

@janherich got it. Will fix.

alwx · 2017-10-02T11:21:46Z

@janherich also don't like this kind of duplication (transactor_group and transactor_personal). However, if I combine them into one file, this file will be incredibly big. I think the best way we can solve this problem is by introducing the ability to specify entry points and have several files like bot.js.
It is not related to this task, but I will think about a better and not time-consuming way of solving this problem.

oskarth · 2017-10-02T13:20:02Z

.re-natal

@@ -1,7 +1,7 @@
 {
  "name": "StatusIm",
  "interface": "reagent",
-  "androidHost": "localhost",
+  "androidHost": "10.0.3.2",


Can you remove this from the PR?

oskarth · 2017-10-02T13:20:12Z

env/dev/env/android/main.cljs

@@ -15,8 +15,8 @@
  (re-frame.core/dispatch [:load-commands!]))

 (figwheel/watch-and-reload
-  :websocket-url "ws://localhost:3449/figwheel-ws"
+  :websocket-url "ws://10.0.3.2:3449/figwheel-ws"


Same here with entire file (and env/ios)

oskarth · 2017-10-02T13:20:41Z

ios/Podfile.lock

@@ -44,4 +44,4 @@ SPEC CHECKSUMS:

 PODFILE CHECKSUM: d05403e3fce258b6985bd3b614ffe02a0e01b9f7

-COCOAPODS: 1.3.1
+COCOAPODS: 1.2.1


Please don't downgrade COCOAPODS version

oskarth · 2017-10-02T13:21:14Z

ios/StatusIm.xcodeproj/project.pbxproj

@@ -947,6 +947,7 @@
 				A97BA941B2FB44B4B66EE6D3 /* Frameworks */,
 				1E7837547A9A40E18AD63CF3 /* Resources */,
 				5C1C8762251D6EF495FB2384 /* Pods */,
+				B280E73E1F7E6CB000B5E1A6 /* Recovered References */,


What changes does API have to do with iOS project settings?

oskarth · 2017-10-02T13:22:27Z

resources/js/bots/transactor_group/bot.js

@@ -284,7 +284,11 @@ function validateSend(params, context) {
        params["bot-db"] = {};
    }

-    if (!params["bot-db"]["public"] || !params["bot-db"]["public"]["recipient"] || !params["bot-db"]["public"]["recipient"]["address"]) {
+    console.log(context);


Does this log have sensitive info? Do we need it?

oskarth · 2017-10-02T13:40:51Z

src/status_im/chat/models/commands.cljs

+                                    (get-in contacts [identity :dapp?]))
+                                  chat-contacts))))))))
+
+(defn parse-command-message-content


Since this is in commands namespace, do we need command in name?

oskarth · 2017-10-02T13:43:28Z

src/status_im/chat/views/message/request_message.cljs

        [view st/comand-request-view
         [touchable-highlight
          {:on-press on-press-handler}
          [view st/command-request-message-view
-           (if (and @markup


Huh, lots of weird derefs in existing code here. I think this might get rid of some bugs, nice one :)

oskarth · 2017-10-02T13:44:17Z

src/status_im/data_store/realm/schemas/account/core.cljs

+               :schemaVersion 15
+               :migration     v15/migration}
+              {:schema        v16/schema
+               :schemaVersion 16


Do we need two schema bumps? What is the reason for not doing it in v15 alone?

oskarth · 2017-10-02T13:45:11Z

src/status_im/ui/screens/contacts/subs.cljs

                   (or (true? pending?)
-                       (bots-constants/hidden-bots whisper-identity))) contacts)
+                       (true? mixable?))))


Don't think you need (true? ...) here, as long as mixable is truthy or falsy, which it should be based on its name.

You're right, it's just a leftover.

oskarth · 2017-10-02T13:46:19Z

test/cljs/status_im/test/chat/models/input.cljs

@@ -3,7 +3,7 @@
            [status-im.chat.models.input :as input]))

 (def fake-db
-  {:global-commands   {:command1 {:name "global-command1"}}
+  {:global-commands   {:command1 '({:name "global-command1"})}


Can you explain why we need this quoting? It's only tests but I don't understand why

just to create a list i suppose. But maybe better to use list here

If it is just changing collection type from map to list-like a vector seems more idiomatic. Is there something list-specific about this?

Let's keep lists here because we use lists in the real db. I will replace '(...) with (list ...) to improve readability.

rasom · 2017-10-02T14:09:53Z

I would prefer to have something like

scopes: ['personal-chat'],

instead of

scope: {
        isGlobal: false,
        personalChats: true,
        groupChats: false,
        canUseForDApps: false
    },

just a suggestion

rasom · 2017-10-02T14:51:58Z

src/status_im/data_store/realm/schemas/account/v16/core.cljs

@@ -0,0 +1,40 @@
+(ns status-im.data-store.realm.schemas.account.v16.core


@alwx can you merge v15 and v16?

rasom · 2017-10-02T14:53:14Z

src/status_im/data_store/realm/schemas/account/v12/contact.cljs

@@ -0,0 +1,47 @@
+(ns status-im.data-store.realm.schemas.account.v12.contact


Why are we adding v12 contact here?

rasom · 2017-10-02T14:55:45Z

src/status_im/data_store/messages.cljs

@@ -113,7 +113,7 @@
      (data-store/save message'))))

 (defn update 
-  [{:keys [message-id] :as message}]
+  [{:keys [message-id preview] :as message}]


preview is not used

rasom · 2017-10-02T14:56:25Z

src/status_im/data_store/messages.cljs

@@ -5,7 +5,8 @@
            [status-im.utils.utils :refer [update-if-present]]
            [clojure.walk :refer [stringify-keys keywordize-keys]]
            [cljs.reader :refer [read-string]]
-            [status-im.constants :as c])
+            [status-im.constants :as c]
+            [taoensso.timbre :as log])


was any logging added in this ns?

rasom · 2017-10-02T15:44:07Z

What does canUseForDApps mean?

janherich · 2017-10-05T07:59:57Z

src/status_im/chat/events/input.cljs

-                        input-model/split-command-args
-                        rest)
+                        (input-model/split-command-args)
+                        (rest))


Nitpicking, but there is no need to add parenthesis in this case

janherich · 2017-10-05T08:05:47Z

src/status_im/chat/events/input.cljs

@@ -283,6 +282,8 @@
                         :chat-id      chat-id
                         :jail-id      (or owner-id jail-id)
                         :content      {:command (:name command)
+                                        :scope   (when-not (:to-message-id metadata)


What's the purpose of this expression, why is :scope nil when :to-message-id is present in metadata ?

Because we don't use scopes for requests and :to-message-id is the indication of request

So we have no better way of distinguishing command-request message from command message other then inspecting :to-message-id key in the metadata ?

annadanchenko · 2017-10-09T18:03:26Z

#2060 'Property must be of type: string' popup after providing confirmation code [feature/command-scopes] fixed in d71e7a3
#2061 No Unsigned Transaction screen is shown if send tx in chat with 2 members [feature/command-scopes] This is fixed in 0.9.10-215-g45b94306+ both android and ios
#2072 Icons for /send and /request commands are missing [feature/command-scopes] This is fixed in 0.9.10-215-g45b94306+ both android and ios This is fixed in 0.9.10-215-g45b94306+ both android and ios
#2075 /location command is not retrieving the map [feature/command-scopes] This is fixed in 0.9.10-215-g45b94306+ both android and ios

alwx · 2017-10-13T11:16:35Z

All fixes were merged.

jeluard · 2017-10-16T06:03:33Z

@alwx Can you rebase on develop?

alwx self-assigned this Sep 5, 2017

alwx force-pushed the feature/command-scopes branch 2 times, most recently from 91a0deb to e82184f Compare September 7, 2017 13:23

alwx force-pushed the feature/command-scopes branch from e82184f to 9cd2f82 Compare September 26, 2017 20:50

alwx changed the title ~~WIP: Command scopes, better semantics of commands, different /send and /request for groups and personal chats~~ Command scopes, better semantics of commands, different /send and /request for groups and personal chats Oct 2, 2017

alwx force-pushed the feature/command-scopes branch from 42d0fd5 to 3525ec0 Compare October 2, 2017 09:18

janherich approved these changes Oct 2, 2017

View reviewed changes

oskarth reviewed Oct 2, 2017

View reviewed changes

rasom reviewed Oct 2, 2017

View reviewed changes

alwx force-pushed the feature/command-scopes branch 3 times, most recently from 16caaad to e03e2f5 Compare October 5, 2017 06:24

janherich approved these changes Oct 5, 2017

View reviewed changes

alwx force-pushed the feature/command-scopes branch 3 times, most recently from 8bca994 to d5eccb1 Compare October 6, 2017 08:09

alwx mentioned this pull request Oct 6, 2017

Fix status-im.test.contacts.events (command scopes) #2032

Closed

alwx force-pushed the feature/command-scopes branch from d5eccb1 to d57f9bb Compare October 6, 2017 09:02

oskarth approved these changes Oct 7, 2017

View reviewed changes

asemiankevich self-assigned this Oct 9, 2017

alwx force-pushed the feature/command-scopes branch from 9a4ef51 to d7b7862 Compare October 10, 2017 07:49

asemiankevich added the Tested - Issues label Oct 11, 2017

alwx force-pushed the feature/command-scopes branch from d71e7a3 to a958c32 Compare October 13, 2017 09:47

alwx force-pushed the feature/command-scopes branch 2 times, most recently from cb62dcc to 45b9430 Compare October 13, 2017 11:14

asemiankevich added Tested - OK and removed Tested - Issues labels Oct 13, 2017

alwx added 2 commits October 16, 2017 09:39

API commands scopes, changes for API semantics (#1546)

63e479b

Fixes: #2061, #2072, #2075 (#2129)

77ef5f6

alwx force-pushed the feature/command-scopes branch from 45b9430 to 77ef5f6 Compare October 16, 2017 07:43

rasom merged commit fef984d into develop Oct 16, 2017

rasom deleted the feature/command-scopes branch October 16, 2017 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command scopes, better semantics of commands, different /send and /request for groups and personal chats #1754

Command scopes, better semantics of commands, different /send and /request for groups and personal chats #1754

alwx commented Sep 5, 2017 •

edited by rasom

Loading

janherich left a comment

janherich Oct 2, 2017

alwx Oct 2, 2017

janherich Oct 2, 2017

alwx Oct 2, 2017

alwx commented Oct 2, 2017

oskarth Oct 2, 2017

oskarth Oct 2, 2017

oskarth Oct 2, 2017

oskarth Oct 2, 2017

oskarth Oct 2, 2017

oskarth Oct 2, 2017

oskarth Oct 2, 2017

oskarth Oct 2, 2017

oskarth Oct 2, 2017

alwx Oct 2, 2017

oskarth Oct 2, 2017

rasom Oct 2, 2017

oskarth Oct 2, 2017 •

edited

Loading

alwx Oct 2, 2017

rasom commented Oct 2, 2017 •

edited

Loading

rasom Oct 2, 2017

rasom Oct 2, 2017

rasom Oct 2, 2017

rasom Oct 2, 2017

rasom commented Oct 2, 2017

janherich Oct 5, 2017

janherich Oct 5, 2017

alwx Oct 6, 2017

janherich Oct 6, 2017

annadanchenko commented Oct 9, 2017 •

edited by asemiankevich

Loading

alwx commented Oct 13, 2017

jeluard commented Oct 16, 2017

		@@ -0,0 +1,40 @@
		(ns status-im.data-store.realm.schemas.account.v16.core

		@@ -0,0 +1,47 @@
		(ns status-im.data-store.realm.schemas.account.v12.contact

Command scopes, better semantics of commands, different /send and /request for groups and personal chats #1754

Command scopes, better semantics of commands, different /send and /request for groups and personal chats #1754

Conversation

alwx commented Sep 5, 2017 • edited by rasom Loading

What happens?

1. Scopes

2. Mailman

3. Less magic for /send and /request

4. Commands model

5. Tons of code quality improvements

6. One more thing — text input should work faster

janherich left a 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

alwx commented Oct 2, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarth Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasom commented Oct 2, 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

rasom commented Oct 2, 2017

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

annadanchenko commented Oct 9, 2017 • edited by asemiankevich Loading

alwx commented Oct 13, 2017

jeluard commented Oct 16, 2017

alwx commented Sep 5, 2017 •

edited by rasom

Loading

oskarth Oct 2, 2017 •

edited

Loading

rasom commented Oct 2, 2017 •

edited

Loading

annadanchenko commented Oct 9, 2017 •

edited by asemiankevich

Loading