[HOLD for payment 2024-06-05] [$250] CRITICAL: Tapping on Push Notification fails to retrieve Onyx updates

[muttmuure]

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.62-3
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @muttmuure
Slack conversation:

Action Performed:

Tap a push notification

Expected Result:

100% of the time, the message that generated that push should be rendered in the chat when you tap it

Actual Result:

Since we removed GetMissingOnyxMessages from push and reconnect app, messages fail to be inserted into the client.

Workaround:

Opening chats and closing them retrieves the updates successfully, further suggesting that the command queue is to blame

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• 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/~01fa6dcd0bbb47aa32
• Upwork Job ID: 1790746770223431680
• Last Price Increase: 2024-05-15

Issue Owner

Current Issue Owner: @lschurr

[melvin-bot]

Triggered auto assignment to @lschurr (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.

[muttmuure]

The proposed solution to this from @quinthar is:

1. Get a list of all updates
2. Sort them by updateID
3. Apply as many as you can in order
4. If there are any gaps, call GetMissingOnyxMessages once

[muttmuure]

#38748

[lschurr]

On hold.

[lschurr]

This is on hold.

[quinthar]

#38748 is deployed; taking this off hold!

[arosiclair]

Getting onyx updates from active push notifications is easy with Airship's Airship.push.getActiveNotifications API. So on selecting a push notification, we'll just

1. Get all OnyxUpdatesFromServer in current push notifications
2. Add them to the new deferredUpdates queue
3. Call validateAndApplyDeferredUpdates to apply them

I do think the deferredUpdates queue could be "dressed up" a bit to make it a bit easier to use and maintain. I'm thinking of adding proper enqueue(), clear() and run() functions to encapsulate it's functionality a bit better. cc @chrispader

This is only about 2-3 days of work however, I'm prioritizing the QBO errors design doc and we also have a deploy freeze EOW. So the ETA is more realistically end of next week.

[chrispader]

I do think the deferredUpdates queue could be "dressed up" a bit to make it a bit easier to use and maintain. I'm thinking of adding proper enqueue(), clear() and run() functions to encapsulate it's functionality a bit better. cc @chrispader

I agree, we can for sure "dress up" the deferred updates logic to add functions like enqueue(), clear() and run().

I think there are two scenarios with receiving push notifications (with the OnyxUpdatesFromServer as the payload?):

1. The app is already running and we're adding it to the deferred updates queue. I think this makes sense in general.
2. When the app is not running yet, shouldn't these updates be retrieved from the server by default? Since on startup we need to get all the data anyways, right?

[chrispader]

I'll open up a PR to add these extra functions to the deferred updates Queue/OnyxUpdateManager.

[chrispader]

Thinking about the design of these additional encapsulated functions. When fetching the push notification updates with Airship.push.getActiveNotifications(), we receive the updates from all push notifications right?

I think it makes sense to automatically trigger the validateAndApplyDeferredUpdates process once deferred updates got queued. Otherwise we might incorporate some bad behaviors when deferred updates get queued but are not processed immediately.

Therefore, using OnyxUpdateManager.enqueueDeferredUpdate() would automatically trigger processDeferredUpdates(). I'll add an additional flag to this function, with which you can deactivate this automatic processing.

[chrispader]

I just created a PR that adds this functionality here: #42044

[arosiclair]

Thinking about the design of these additional encapsulated functions. When fetching the push notification updates with Airship.push.getActiveNotifications(), we receive the updates from all push notifications right?

I think it makes sense to automatically trigger the validateAndApplyDeferredUpdates process once deferred updates got queued. Otherwise we might incorporate some bad behaviors when deferred updates get queued but are not processed immediately.

Therefore, using OnyxUpdateManager.enqueueDeferredUpdate() would automatically trigger processDeferredUpdates(). I'll add an additional flag to this function, with which you can deactivate this automatic processing.

Yup that all sounds right to me. I'll review the changes

[chrispader]

@arosiclair created a PR for this here: #42510

I'm not sure if we actually have to use DeferredOnyxUpdates queue directly. Using applyOnyxUpdatesReliably synchronously should also achieve the same thing, while hiding internal code.

@arosiclair could you give the PR a look and let me know, if you think, that we actually need to use DeferredOnyxUpdates queue?

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.76-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Add deferred updates queue functions to OnyxUpdateManager to manually apply updates (e.g. from push notifications) #42044

If no regressions arise, payment will be issued on 2024-06-05. 🎊

For reference, here are some details about the assignees on this issue:

• @chrispader does not require payment (Contractor)
• @allroundexperts requires payment through NewDot Manual Requests

[chrispader]

i think @MelvinBot got this wrong. The solution has only been applied partially. Still waiting on #42510 to be merged

[arosiclair]

Yeah this is not done but @allroundexperts should get a payment for his C+ review on that first PR. @lschurr

[arosiclair]

Alright upon further investigation and testing, I've found that we should not add functionality to apply onyx updates when selecting a push notification. We have a PushReceived handler that applies the onyx update and this should work even when the app killed (the handler runs in the background OR the next time the app is re-opened). I confirmed that with Airship here. So adding extra functionality on select would be redundant.

However, I did notice two other issues

• Push notification Onyx updates are not applied correctly when there are no gaps detected. This is because the update is applied with a placeholder eventType here which results in a no-op since it's unrecognized here. I'll fix this with a separate code flow for applying Airship onyx updates rather than re-using the pusher flow.
• Background refresh doesn't seem to be working on iOS anymore. The app doesn't wake in the background and only processes push notification events once you re-open the app.

I'm going to create two follow up issues to address those and leave this open since we still need to handle payment for the last C+ review

[arosiclair]

Follow up issues:

• [HOLD for payment 2024-06-28] Push notification Onyx updates are not applied when there is no gap detected #43047
• iOS: Push notifications do not wake the app in the background #43048

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @chrispader is from an agency-contributor and not due payment
• Reviewer: @allroundexperts owed $250 via NewDot

BugZero Checklist (@lschurr)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1790746770223431680/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[lschurr]

The payment summary is correct. No payments will happen in Upwork. @allroundexperts you can request via NewDot :)

[lschurr]

Oh wait, should this one still be open @arosiclair? #40227 (comment)

[arosiclair]

As long as this payment was handled then we should be all set

[lschurr]

Yep, that's handled in the payment summary here: #40227 (comment)

Going to close :)

[JmillsExpensify]

$250 approved for @allroundexperts