Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #7432 remove redirect to undefined chat #7960

Closed
wants to merge 4 commits into from

Conversation

6 participants
@trowacat
Copy link
Contributor

commented Apr 12, 2019

fixes #7432

Summary

User was able to be redirected to an unknown chat if using a notification for a deleted chat or a blocked user.

  • Android
  • iOS

Steps to test

Scenario 1:

Open Status
Start 1-1 chat with user A
Put app to background. When receive PN, don't open it.
Block user A
Open PN

Scenario 2:

Open Status
Start 1-1 chat with user A
Put app to background. When receive PN, don't open it.
Delete chat
Open PN

status: ready

@status-github-bot status-github-bot bot added this to REVIEW in Pipeline for QA Apr 12, 2019

@status-im-auto

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Jenkins Builds

Click to see older builds (18)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c911b86 #1 2019-04-12 00:37:39 ~14 min macos 📦 dmg
✔️ c911b86 #1 2019-04-12 00:42:45 ~19 min linux 📦 App
✔️ c911b86 #1 2019-04-12 00:42:56 ~19 min android 📦 apk
✔️ c911b86 #1 2019-04-12 00:43:36 ~20 min android-e2e 📦 apk
✔️ c911b86 #1 2019-04-12 00:48:41 ~25 min windows 📦 exe
✔️ c911b86 #1 2019-04-12 00:48:54 ~25 min ios 📦 ipa
7ec4850 #2 2019-04-12 17:03:25 ~3 min ios 📄 log
7ec4850 #2 2019-04-12 17:03:25 ~3 min macos 📄 log
7ec4850 #2 2019-04-12 17:06:31 ~7 min linux 📄 log
abe4b07 #3 2019-04-12 17:10:24 ~4 min ios 📄 log
abe4b07 #3 2019-04-12 17:10:41 ~4 min macos 📄 log
a78436f #5 2019-04-12 17:15:37 ~3 min macos 📄 log
a78436f #5 2019-04-12 17:15:43 ~3 min ios 📄 log
a78436f #5 2019-04-12 17:18:06 ~6 min linux 📄 log
a78436f #5 2019-04-12 17:18:13 ~6 min windows 📄 log
a78436f #5 2019-04-12 17:21:28 ~9 min android 📄 log
a78436f #5 2019-04-12 17:21:28 ~9 min android-e2e 📄 log
a78436f #6 2019-04-29 18:51:45 ~28 sec android 📄 log
Commit #️⃣ Finished (UTC) Duration Platform Result
780d760 #7 2019-05-25 14:15:40 ~27 sec android 📄 log
780d760 #6 2019-05-25 14:15:40 ~31 sec android-e2e 📄 log
780d760 #6 2019-05-25 14:15:46 ~31 sec ios 📄 log
780d760 #6 2019-05-25 14:15:52 ~34 sec linux 📄 log
780d760 #6 2019-05-25 14:15:55 ~30 sec windows 📄 log
780d760 #6 2019-05-25 14:15:56 ~35 sec macos 📄 log
f292d78 #7 2019-05-25 14:18:07 ~20 sec android-e2e 📄 log
f292d78 #8 2019-05-25 14:18:12 ~20 sec android 📄 log
f292d78 #7 2019-05-25 14:18:17 ~22 sec ios 📄 log
f292d78 #7 2019-05-25 14:18:24 ~27 sec linux 📄 log
f292d78 #7 2019-05-25 14:18:28 ~27 sec macos 📄 log
f292d78 #7 2019-05-25 14:18:29 ~25 sec windows 📄 log
@cammellos
Copy link
Member

left a comment

Looks good, thanks for the contribution! Have you checked what happens if you receive a push notifications from someone you never received a message from (not blocked, nor deleted). What happens in the current-app vs this PR

Thanks!

(fx/merge cofx
(navigation/navigate-to-cofx :chat-modal {})
(preload-chat-data chat-id))
(when (= (is-active? (get-chat cofx chat-id)) true)

This comment has been minimized.

Copy link
@cammellos

cammellos Apr 12, 2019

Member

No need to (= .. true) (when (is-active? ...) should do, unless you really mean to check it against true (i.e is-active? might return some truthy values that you don't want to consider, but that does not seem to be the case).

This comment has been minimized.

Copy link
@trowacat

trowacat Apr 12, 2019

Author Contributor

Thanks for the suggestion I will update this. I just wanted to say that I have not been able to receive notifications unless the person is already a contact in both versions. Is this intended?

This comment has been minimized.

Copy link
@cammellos

cammellos May 9, 2019

Member

yes, so to receive a notification from A when A sends you a message, you need first to add A to your contacts

@trowacat trowacat force-pushed the trowacat:issue7432 branch from 7ec4850 to abe4b07 Apr 12, 2019

@trowacat trowacat force-pushed the trowacat:issue7432 branch from 90ea392 to a78436f Apr 12, 2019

@flexsurfer flexsurfer requested a review from yenda Apr 26, 2019

@flexsurfer

This comment has been minimized.

Copy link
Member

commented May 9, 2019

(fx/merge cofx
(navigation/navigate-to-cofx :chat-modal {})
(preload-chat-data chat-id))
(when ((is-active? (get-chat cofx chat-id)))

This comment has been minimized.

Copy link
@cammellos

cammellos May 9, 2019

Member

looks like there's an extra set of parentheses here

This comment has been minimized.

Copy link
@yenda

yenda May 9, 2019

Member

I was about to say the same, and the second arity of is-active? would avoid calling get-chat there.

This comment has been minimized.

Copy link
@flexsurfer

This comment has been minimized.

Copy link
@bitsikka

bitsikka May 9, 2019

Contributor

btw merged PR #7820 introduced function active-chat? an exact(apart from name) duplicate of is-active?

This comment has been minimized.

Copy link
@yenda

yenda May 9, 2019

Member

@bitsikka yeah good point, this PR probably needs a rebase because I couldn't find active-chat? though I was pretty sure I saw it somewhere already

This comment has been minimized.

Copy link
@trowacat

trowacat May 9, 2019

Author Contributor

Thank you all, I will look into resolving what you have pointed out and rebase tonight.

@status-github-bot

This comment has been minimized.

Copy link

commented May 11, 2019

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?
@flexsurfer

This comment has been minimized.

Copy link
Member

commented May 24, 2019

hey @trowacat any updates on this? :)

@trowacat trowacat requested a review from status-im/status-core as a code owner May 25, 2019

@flexsurfer

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

hey @trowacat feel free to reopen PR if needed or file a new one, thanks for your effort

@flexsurfer flexsurfer closed this Jun 7, 2019

Pipeline for QA automation moved this from REVIEW to DONE Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.