Features/move contacts #8696
Features/move contacts #8696
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (27)
|
| (if config/use-status-go-protocol? | ||
| (load-chats-from-rpc cofx) | ||
| (initialize-chats-legacy cofx from to))) | ||
| (load-chats-from-rpc cofx from -1)) |
There was a problem hiding this comment.
it's to indicate that we want all the chats, there's pagination but it's not used currently (-1 indicates all the chats)
There was a problem hiding this comment.
hm can we use nil? i mean just do not pass this param?
There was a problem hiding this comment.
I can change that, (-1 to indicate no limit is not uncommon in apis though, while 0 might indicate that you want an empty page, null makes the parameter nillable which might not be desirable with integer data types), but if you don't mind I will do that next time I upgrade status-protocol-go (it takes 3 PRs to get stuff in status-react :) , going through 3 review process etc )
src/status_im/contact/block.cljs
Outdated
| @@ -19,6 +19,8 @@ | |||
| [{:keys [db] :as cofx} chat-id removed-chat-messages] | |||
| (let [removed-messages-ids (map :message-id removed-chat-messages) | |||
| removed-unseen-count (count (remove :seen removed-chat-messages)) | |||
| unviewed-messages-count (- (get-in db [:chats chat-id :unviewed-messages-count]) | |||
There was a problem hiding this comment.
I believe so (- nil 1) = -1
src/status_im/data_store/chats.cljs
Outdated
| :always | ||
| (dissoc :admins :contacts :members-joined))) | ||
|
|
||
| (defn- unmarshal-members [{:keys [members chatType] :as chat}] |
There was a problem hiding this comment.
why do we need all these data transformations on react side? ->rpc and <-rpc ?
There was a problem hiding this comment.
This data structures come from status-protocol-go which one of its goals is to be agnostic to status-react, so there data-structures are not quite the same, so they need to be "massaged" in shape, we can do this either in status-react or status-go, for now I picked status-react as my understanding is that status-go as well should not have much knowledge of status-react (i.e it should for example serve camelCase json rather than kebab-case which is very clojure specific), but I am happy with both, in any case if we want to change I'd rather do it separately, it should be pretty straightforward to move to status-go.
cammellos
force-pushed
the
features/move-contacts
branch
from
0eacbd2 to
0942e3c
Compare
cammellos
force-pushed
the
features/move-contacts
branch
from
0942e3c to
4b8ead5
Compare
|
@cammellos e2e build fails as well as iOS. Could you please check whats wrong? |
|
@asemiankevich not related to this PR specifically I get: |
cammellos
force-pushed
the
features/move-contacts
branch
from
4b8ead5 to
b5639c4
Compare
|
@cammellos seems profile and wallet do miss some property , so they are not shown correctly
|
cammellos
force-pushed
the
features/move-contacts
branch
from
b5639c4 to
fadcbb8
Compare
|
@cammellos i dont see any Push Notifications coming through , even i added 2 devices to contacts and setting is enabled. Could you please check that? |
|
@asemiankevich sure, thanks for spotting the issue! |
84% of end-end tests have passedFailed tests (7)Click to expand
Passed tests (37)Click to expand |
0% of end-end tests have passedFailed tests (7)Click to expand
|
Issue 2: endless spinner when joining public chat after creating accountSteps:
Logs\video here: 1. test_public_chat_messaging
Device 2: 'Username' is not found on the screenDevice sessions Device 2: Device 1: Not reproducible on nightly 06/08/2019. |
Issue 3: userpicture is not updated in 1-1 and public chatFound by e2e, see comment #8696 (comment), test Steps:
Expected result: can see custom photo of user B |
cammellos
force-pushed
the
features/move-contacts
branch
2 times, most recently
from
dd37480 to
6597130
Compare
|
@churik both issues should be fixed in the latest build, I haven't tested Push notifications as I need a real build with a real device, will do that, but in the meantime I think it's ready to be tested again (PN bug was due to issue 3 as well) |
|
@cammellos what is the expected result in this case:
Which messages should see user A? The ones that were sent after unblock only? Or the whole chat (including the messages that were sent while user B was blocked )? In this PR if i unblock user - > i see the messages before block, during block and after unblock - is this correct? |
95% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (42)Click to expand |
|
@asemiankevich I think there's an issue, will fix |
cammellos
force-pushed
the
features/move-contacts
branch
from
6597130 to
b1938e9
Compare
|
@asemiankevich should be fixed in latest commit, i haven't had time to validate yet, but will do, but if you have some spare cycles and test it , it would be very helpful. thanks! |
sure :) |
|
Checked:
all above looks okay-ish to me. |
|
Tested:
ENS names in profile can't be checked due to #8707 e2e failures are not related to PR. |
cammellos
force-pushed
the
features/move-contacts
branch
from
b1938e9 to
bdbfec6
Compare
Contacts are now in status-go, no migration of contacts is provided so all contacts will be lost upon installing this build. I have left the initialization of filters a bit sketchy (we wait that load-filters is called twice), as the next step will be to avoid calling load-filters altogether, as now that both contacts & chats are in status-go, there's no reason to call it from status-react, and can be called directly from status-go on loading. Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>
cammellos
force-pushed
the
features/move-contacts
branch
from
bdbfec6 to
fe9d4e9
Compare
Move contacts to status-go
Contacts are now in status-go, no migration of contacts is provided so all contacts will be lost upon installing this build.
I have left the initialization of filters a bit sketchy (we wait that load-filters is called twice), as the next step will be to avoid calling load-filters altogether, as now that both contacts & chats are in called directly from status-go on loading.
Testing
depends on #8679 , please review only the last commit
status: ready