[ISSUE #2287] Push Notifications: Use status-go NotifyUsers instead of Notify #2902
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@pombeirp Can we also make sure it ends up correctly on receiving end? Or make it possible for QA to check with grep on device logs. I.e.: Client A (status-react) -> status-go (trigger notification -> Firebase -> Client B (status-react) console logs. Receiving side: https://github.com/status-im/status-react/blob/develop/src/status_im/utils/notifications.cljs#L36-L47 (TODO can probably be removed now btw) |
It'd be great if we can let QA test without requiring this change, either if we can get above end to end to work with existing status-go version, or if we issue PR to status-go with your temp change + make custom build + bump status-go dep temp in this PR. Former is probably easier I think but open to both, WDYT? |
oskarth
suggested changes
Jan 4, 2018
@oskarth I added a screenshot of the receiving app on the OP above. If seeing that on their test device is enough for QA, then we could just go with the existing go version without changes. Otherwise I can try to do the change and bump the dependencies. I'll take a look at the |
|
@oskarth Strange, I'm not seeing the |
Hm, not sure. I know I had to install some special stuff on Genymotion to get notifications to work, but seems like you actually received the notification so doesn't seem to be that. Maybe it is a log stream problem? Also, if you resolve conflicts, let's move this to testing. Either QA (with TestFairy) or I can have a look then. Requesting sanity review from @rasom concurrently as it touches Java/iOS interface stuff. |
Thanks @oskarth. Do you want me to point this to a special |
If it isn't logged by default in normal status-go, that'd be great! If you push a branch/PR you should be able to use https://jenkins.status.im/job/status-go-manual/ job to get a version on form |
|
@oskarth I'll test the build I generated today and let you know if it is ready for QA. |
|
@oskarth I don't know why I stopped seeing the log entries for |
It's really hard to read, but the map after cond-> gets threaded into the (assoc...) form as first part. In the original line it goes reg-fx :send-notification used to take: where m isn't present because it is threaded from above. NOTE: note tested, just written in GH, but something like this. Can also put this in a |
oskarth
reviewed
Jan 18, 2018
| @@ -84,8 +84,8 @@ | |||
| (defn should-move-to-internal-storage? [callback] | |||
| (module-interface/-should-move-to-internal-storage? rns-module callback)) | |||
|
|
|||
| (defn notify [token callback] | |||
| (module-interface/-notify rns-module token callback)) | |||
| (defn notify-users [message payload tokens callback] | |||
There was a problem hiding this comment.
Consider doing same interface change here as ::send-notification above, i.e. [{:keys [message payload tokens] :as m} callback] as arg
|
^ nudge @rasom review as well. |
rasom
reviewed
Jan 24, 2018
| @@ -18,7 +18,7 @@ dependencies { | |||
| compile 'com.github.ericwlange:AndroidJSCore:3.0.1' | |||
| compile 'status-im:function:0.0.1' | |||
|
|
|||
| String statusGoVersion = 'develop-gd71c66a2' | |||
| String statusGoVersion = 'junk-log-NotifyUsers-g605a2123-62' | |||
There was a problem hiding this comment.
can't we use current status-go's develop here?
There was a problem hiding this comment.
This is just a temporary commit to gain additional logging on the go side to allow QA to verify.
rasom
approved these changes
Jan 24, 2018
oskarth
approved these changes
Jan 25, 2018
|
@oskarth I think the |
|
OK @oskarth I think we're ready to go. Results from
|
oskarth
approved these changes
Jan 26, 2018
Automated test results: |
Automated test results:test_password[logcat]:white_check_mark::Test Steps & Error message:test_password[mismatch]:white_check_mark::Test Steps & Error message:test_group_chat_members:x:Test Steps & Error message:
test_sign_in[invalid]:white_check_mark::Test Steps & Error message:test_sign_in[valid]:white_check_mark::Test Steps & Error message:test_commands_on_second_app_run:x:Test Steps & Error message:
test_group_chat_send_receive_messages_and_remove_user:x:Test Steps & Error message:
test_change_user_name:white_check_mark::Test Steps & Error message:test_recover_access:white_check_mark::Test Steps & Error message:test_send_funds_via_request[one_to_one_chat]:x:Test Steps & Error message:
test_send_funds_via_request[group_chat]:x:Test Steps & Error message:
test_request_transaction_from_wallet:x:Test Steps & Error message:
test_eth_and_currency_balance:white_check_mark::Test Steps & Error message:test_send_transaction_from_wallet[sign_later]:x:Test Steps & Error message:
test_wallet_error_messages:white_check_mark::Test Steps & Error message:test_one_to_one_chat:x:Test Steps & Error message:
test_transaction_send_command[group_chat]:x:Test Steps & Error message:
test_send_transaction_from_wallet[sign_now]:x:Test Steps & Error message:
test_transaction_send_command[wrong_password]:x:Test Steps & Error message:
test_send_transaction_from_daap:x:Test Steps & Error message:
test_password[short]:white_check_mark::Test Steps & Error message:test_transaction_send_command[one_to_one_chat]:x:Test Steps & Error message:
|
Automated test results:test_send_transaction_from_daap:white_check_mark::Test Steps & Error message:test_send_eth_from_wallet_sign_later:white_check_mark::Test Steps & Error message:test_transaction_send_command_group_chat:white_check_mark::Test Steps & Error message:test_send_stt_from_wallet_via_enter_contact_code:white_check_mark::Test Steps & Error message:test_send_eth_from_wallet_sign_now:x:Test Steps & Error message:
test_send_eth_to_request_from_wallet:x:Test Steps & Error message:
test_send_eth_to_request_in_group_chat:white_check_mark::Test Steps & Error message:test_transaction_send_command_wrong_password:white_check_mark::Test Steps & Error message: |
|
@annadanchenko do you think we can get this tested? I know it is low-priority, but it's been in test for 20 days now and it's the third time I've had to solve merge conflicts to keep it testable. |
|
@pombeirp and again a merge conflict sorry :D |
Automated test results:test_browse_link_entering_url_in_dapp_view:x:Test Steps & Error message:
test_contact_profile_view:white_check_mark::Test Steps & Error message:test_qr_code_and_its_value:white_check_mark::Test Steps & Error message:test_network_switch:white_check_mark::Test Steps & Error message:test_send_eth_from_wallet_sign_now:white_check_mark::Test Steps & Error message:test_transaction_send_command_wrong_password:white_check_mark::Test Steps & Error message:test_send_eth_from_wallet_sign_later:white_check_mark::Test Steps & Error message:test_send_transaction_from_daap:white_check_mark::Test Steps & Error message:test_send_stt_from_wallet_via_enter_contact_code:white_check_mark::Test Steps & Error message:test_transaction_send_command_group_chat:white_check_mark::Test Steps & Error message:test_transaction_send_command_one_to_one_chat:x:Test Steps & Error message:
test_send_eth_to_request_in_one_to_one_chat:white_check_mark::Test Steps & Error message:test_send_eth_to_request_from_wallet:white_check_mark::Test Steps & Error message:test_send_eth_to_request_in_group_chat:x:Test Steps & Error message:
|
|
Can get notification if send a message in 1:1 chat to the added contact (both iOs and Android) |
|
@pombeirp please squash commits |
|
@flexsurfer Done |
Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2287
Summary:
This PR updates the react-side code to use
Statusgo.NotifyUsers(message, payloadJSON, tokensArray *C.char)instead ofStatusgo.Notify(token *C.char).Review notes:
I've made some temporary changes to logging code and set the payload to some bogus data, so this should be reset before merging to
develop.Testing notes:
In order to test that the unencrypted payload has arrived correctly to the Go side, I changed the following code in
geth/api/api.goon my machine:You need to make sure that the
LOG_LEVEL_STATUS_GOfeature flag is set to at leastwarnlevel.Steps to test:
adb logcat | grep -e ReactNativeJS -e notifyUsers -e send-notification -e NotifyUsers -e StatusModuleYou should see something similar to the following:
There's initially the call to
send-notificationon the CLJS side, then the fakeWARNlog message on thegethside, and then the success callback result appearing back on the CLJS side (send-notification cb result: {"status":true}).On the receiving device, you can see the title and content that come from the CLJS code:
status: ready