Implementation of remote android notifications #13028
Merged
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
Jenkins BuildsClick to see older builds (93)
|
|
This should be an opt-in option, since Google listens to notifications, which would compromise anonymity and privacy of users. |
Yes, it will be 👍 |
|
hey @Parveshdhull are you considering conditioning this for f-droid builds ? there must be no google services dependencies |
Hi @flexsurfer, first we trying to see if we can make remote android notifications work for android. Once it is complete, I will make sure to condition google dependencies before merging 👍 |
Parveshdhull
force-pushed
the
feature/remote-android-notifications
branch
6 times, most recently
from
9614bb0 to
4d58002
Compare
Parveshdhull
changed the title
Parveshdhull
commented
Feb 1, 2022
| @@ -27,6 +27,7 @@ | |||
| "emojilib": "^2.4.0", | |||
| "eth-phishing-detect": "^1.1.13", | |||
| "functional-red-black-tree": "^1.0.1", | |||
| "hermes-engine": "0.5.2-rc1", | |||
There was a problem hiding this comment.
TODO - remove hermes-engine 0.5.2-rc1 once react-native is upgraded to 0.64+ (will create separate issue for this)
wix/react-native-notifications#455 (comment)
87% of end-end tests have passedFailed tests (11)Click to expand
Passed tests (71)Click to expand
|
jakubgs
reviewed
Feb 2, 2022
Parveshdhull
force-pushed
the
feature/remote-android-notifications
branch
4 times, most recently
from
54d0fe3 to
5bdffc2
Compare
jakubgs
reviewed
Feb 7, 2022
jakubgs
approved these changes
Feb 7, 2022
|
sorry probably this was discussed before, but settings look strange |
|
Hi @pavloburykh, Thanks for adding steps to reproduce 2nd issue. I think currently remote PN is only supported for messages, not wallet transactions (issue also exists in the ios - nightly version). Maybe we can work on this issue separately? wdyt |
|
@Parveshdhull I'm receiving wallet PN on IOS nightly version (attaching video below). But anyway, I agree with you, that we can add remote wallet PN for Android by separate issue in order not to block this PR. telegram-cloud-document-2-5202025987612611585.mp4 |
@Parveshdhull in terms of this comment I would like to add, that currently Notification switcher seems to have 2 functions - enables/disables notifications overall and enables background notifications in case if Remote notification switcher is disabled (attaching video below demonstrating current behaviour). It might be a little bit confusing for a user. And better decision would be to rename Notification switcher to smth like Local notifications as was proposed by @flexsurfer but also to make this switcher independent and responsible only for enabling/disabling Background notifications. telegram-cloud-document-2-5199941120292820514.mp4I think we might think to improve above issues in case if Notifications version implemented by this PR is considered to be final version. In case if we consider this version to be temporary one and we are planning to improve it in nearest future then we are good to go at this moment without additional UX/UI fixes. |
Parveshdhull
force-pushed
the
feature/remote-android-notifications
branch
from
c42a3a4 to
25201b8
Compare
|
Hi @pavloburykh, I updated the UI as requested by you and @flexsurfer. |
88% of end-end tests have passedFailed tests (10)Click to expand
Passed tests (73)Click to expand
|
80% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (8)Click to expand
|
This is now replaced by |
Thanx @Parveshdhull we will update test. |
churik
added a commit
that referenced
this pull request
Feb 14, 2022
|
@Parveshdhull Thanx for all fixes. PR is now ready for merge. |
This was referenced Feb 14, 2022
churik
added a commit
that referenced
this pull request
Feb 14, 2022
Signed-off-by: Churikova Tetiana <churikova.tm@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 #12866
PR implements android remote notifications
Code Review Requests:
I tried to make sure google dependencies are removed in the f-droid release build. Please, let me know if I missed something. cc @jakubgs
QA test request:
Changes for android remote notifications may have affected ios remote notifications, please let me know if anything is not working as expected.
Known issue:
PR disables remote notifications for an account at the time of logout. But if the user logs in into another account, without logging out or disabling notifications manually (by logging in while opening the app), then the user will keep receiving notifications for another account
Extras:
#12866 also mentions migrating ios library too. I will create a separate issue/pr for this.
status-go PR
status-im/status-go#2527
status: ready