[HybridApp] - Redirected Hybrid app users missing Push Notifications

muttmuure commented 1 month ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: v1.4.74-4 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: @muttmuure Slack conversation: https://expensify.enterprise.slack.com/archives/C05LX9D6E07/p1715102667514389

Action Performed:

Break down in numbered steps

Create a new public domain account
- or apply the ClassicRedirect NVP to your account in Expensify Classic
Sign into the Hybrid app, you should see the NewDot experience
Enable notifications for under OS app settings
From a different account, Send a message to yourself
Kill the app
From a different account, Send a message to yourself

Expected Result:

You should receive a push notificaion

Actual Result:

You do not receive a notification

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

None

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[ ] Android: mWeb Chrome
[x] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01b96cd2222bc21292
Upwork Job ID: 1792637809065209856
Last Price Increase: 2024-05-27

Issue Owner

Current Issue Owner: @sobitneupane

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

CortneyOfstad commented 1 month ago

Was able to recreate so going to get eyes on this!

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~01b96cd2222bc21292

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

jrstanley commented 1 month ago

I've been unable to replicate this in production with a physical iOS device

drantis commented 1 month ago

Cannot view logs. SSO required

melvin-bot[bot] commented 1 month ago

📣 @drantis! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

Julesssss commented 1 month ago

Unable to reproduce:

IMG_4DFA58EED59F-1 IMG_AC08FC6D2D0D-1

Julesssss commented 1 month ago

The OS only lets a certain amount of push notifications to reach the app and this depends on many things like Battery, Amount of push notifications we have recently sent, etc. perhaps that is the cause, or perhaps we're missing some extra steps required to reproduce this?

CortneyOfstad commented 1 month ago

Was not able to reproduce (details here)

CortneyOfstad commented 1 month ago

Talking to @Julesssss and @muttmuure in Slack, I'm going to need to create a new Expensify account under a public domain email. Testing this now 👍

CortneyOfstad commented 1 month ago

Alright I was able to recreate! I ran the snippet for cort2007@gmail.com and signed in via the mobile app. Enabled notifications, quit the app. From cortney@expensify.com, I attempted to send a test message and it never notified me on iOS

sobitneupane commented 1 month ago

Unable to reproduce on latest staging:

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

@Julesssss, @CortneyOfstad, @sobitneupane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

muttmuure commented 1 month ago

The reproduction steps here are to dismiss the redirect (using Switch to Expensify Classic) and then you don't receive notifications anymore.

muttmuure commented 1 month ago

So sign up on a public domain which will set the redirect, then navigate back to Classic. This disables notifications for you on the back end and you no longer receive them

Julesssss commented 1 month ago

This member invitation redirection is higher priority. But I will get back to this soon

sobitneupane commented 1 month ago

@Julesssss are we expecting proposals on this, or will you be working on the fix?

Julesssss commented 1 month ago

Hey @sobitneupane, I'm pretty sure this is a backend issue. So you can unassign and unsubscribe

Julesssss commented 1 month ago

I'm finding it hard to verify currently. I see no difference in NVPs between users. I believe re-authenticating could work, but I'd like to confirm Matt and Cortney's NVP data before we confirm that fix.

The Airship push notificationID is set to the users accountID
On App sign-in, we enable/disable push notifications depending on the notificationID being present in Onyx
NVP is set from Web-E when we redirect

It looks like the NVP data is identical for both 'unsubscribed' users and regular users:

Unsbscribed


"private_pushNotificationID": "ptetgDGrF0KHB2ev",

"pushNotificationsEnabled": {
        "iPhone8,4_C5A88FFA-C9E0-4D6C-8969-96DABCA6D1CE": [
            {
                "isEnabled": true,
                "timestamp": "2024-05-31 10:24:25"
            }
        ]
    },

**Subscribed**
// ID
"private_pushNotificationID": "cKWxpD68HHuwAtHx",

"EnablePushNotifications": { // This doesn't seem to be used anywhere...
            "completed": {
                "data": {
                    "value": "allow"
                },
                "time": 1638377726
            },
            "dismissed": {
                "data": [],
                "time": 1701962990
            }
        },

"pushNotificationsEnabled": {
        "bluejay002ce64b-2937-13c3-6a13-a80b5c37ef1f": [
            {
                "isEnabled": true,
                "timestamp": "2023-03-16 10:30:51"
            }
        ],
        ...

CortneyOfstad commented 4 weeks ago

Not overdue!

Julesssss commented 4 weeks ago

Busy with other chores today. Will carry on when I'm back on Wednesday

melvin-bot[bot] commented 3 weeks ago

@Julesssss @CortneyOfstad this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Julesssss commented 3 weeks ago

I'm still working on chores, reviews, ect and didn't have time to pickup today. I'll have time for this tomorrow.

mvtglobally commented 3 weeks ago

Issue not reproducible during KI retests. (First week)

Julesssss commented 3 weeks ago

I'm afraid I have again been too busy with chores and other daily/hourly tasks to get to this. I will make time tomorrow.

@CortneyOfstad in the meantime, it would be helpful to confirm:

That you triggered the redirection by creating a new account on a public domain?
Which App/website did you use to create the account?
Whether you get notifications for web/desktop on that same account?

I find it confusing that magic codes aren't sent to new accounts on first entering your email; AFAICT, you must wait 30 seconds and then try again to receive it...

Julesssss commented 3 weeks ago

I can now reproduce on Android too. This only works for hybrid app. I see the same log that Matt reported here.

Signing into expensify.com and being redirected doesn't work as far as I can tell,

[App] 2024-06-07T13:24:01.533Z - [info] [PushNotification] Unsubscribing from push notifications. ~~ parameters: '' userAgent: 'okhttp/4.9.2'

muttmuure commented 3 weeks ago

👍

CortneyOfstad commented 3 weeks ago

Not overdue!

Julesssss commented 3 weeks ago

The UserAPI::createTemporaryLogin function attempts to retrieve the private_pushNotificationID NVP, here. But this is being retrieved as null or ''. This seems to be reproducible 100% of the time in prod.

But reproducing locally is tricky because the unsubscription only occurs on mobile. When debugging web I do in fact see the NVP being set 😕

A recent change wraps the fault NVP retrieval behind an isValidatedUser condition, so I need to figure out how this will continue to be testable
Could this be due to SSO auth options?.. I need to verify that the bug occurs for the SSO, but first I need apply this local configuration

Julesssss commented 2 weeks ago

Once again I have been pulled away to finish some other things. And new instances of a previous fire...

For now, I think the eventual solutions will be:

A) Register for push notifications on app start instead of sign in/out. This is not dangerous and will allow past and future users to have the issue auto-resolved on the next app open
B) Figure out why the NVP is not being retrieved within createTemporaryLogin (secondary improvement)

note: I should try temp-mail for throwaway accounts

Julesssss commented 2 weeks ago

I continued investigating today. A lot of my time is being spent building the Hybrid app locally for debugging...

Here is the specific line of code causes us to de-register from Airship. I don't yet know why, but the app behaves differently when being run within hybrid app:

NewDot app: nvp_private_pushNotificationID exists, so we register for push notifications
Hybrid app: nvp_private_pushNotificationID is null, so we deregister for push notifications

I think this is because a a unique flow is used for the Hybrid app flow, where the user's accountID is returned (via createTemporaryLogin). This function gets the users accountID and returns it as Onyx data. Perhaps this is called too early for it to work within Hybrid app...

Next steps:

Hardcode the accountID to empty and see if this reproduces the issue in NewDot
Debug Web-E and monitor the onyx data -- compare NewDot to Hybrid app if possible
Try and mount PushNotifications on app open
Share ideas in Slack and see if anyone else has a suggestion

Julesssss commented 2 weeks ago

I think Mariusz will eventually hit this issue when resolving Hybrid notifications as this problem seems to be that we deregister from Airship due to the missing Onyx data.

My best guess so far is that the mounting of PushNotification is happening too early, or in the wrong order when NewDot is running within Hybrid.

I'll remain subscribed.

AndrewGable commented 2 weeks ago

cc @staszekscp

staszekscp commented 2 weeks ago

I had a bit of time to have a look at the issue 🙂 If the issue is indeed related to calling PushNotification.deregister() I think the key would be to change the callback for ONYXKEYS.NVP_PRIVATE_PUSH_NOTIFICATION_ID key. Currently we register to push notifications, even though we do it already by calling registerOnStartup on the native OldDot side - therefore the user is already registered whenever we enter NewDot.

It means that theoretically we could skip registration/deregistration of PushNotifications on NewDot side. Especially if we're not going to let users to logout on NewDot. It would require some testing, but after some testing it seems that it would work.

FYI: I'm going to be out of office for the next two weeks, but I hope that it'll help you debug this issue 😄

CortneyOfstad commented 1 week ago

@Julesssss any update? Last one I can see is from Slack here — thanks!

Julesssss commented 1 week ago

Hey @CortneyOfstad, this is held on wider Hybrid notification changes. Which I believe will be investigated by @staszekscp when he is back.

@AndrewGable I looked for a Hybrid notification issues in the board and tracker but can't see anything. Are we missing a wider issue for this?

CortneyOfstad commented 6 days ago

Thanks @Julesssss!

@staszekscp — if you have any questions, or need anything — just let me know!

Expensify / App