[HybridApp] Clicking expense violation notifications cause the iOS app to crash

[Julesssss]

@AndrewGable @trjExpensify

Problem

Clicking expense violation notifications cause the iOS HybridApp to crash. NewDot users should not be receiving OldDot notifications.

Reproduction
I only have HybridApp installed, I think it’s:

• Have a workspace on New Expensify that has violations enabled
• SmartScan a receipt that has violations (e.g. our limit is $10 and this SmartScan was $100)
• Click on push

Solution

Figure out if this should block the upcoming HybridApp release.

Fix.

[Julesssss]

I'm struggling to locate the crash in Firebase.

The higher frequency crashes exist in pre-hybrid releases, or seem very unrelated (shadowview example).

[AndrewGable]

I have been reporting this via the TestFlight crash dialog, I am not sure Firebase is initialized yet

[Julesssss]

So for now would our desired solution to be stop sending the notification for NewDot violations, rather than implementing NewDot violation handling?

Secondly, is the desired behaviour to only send push notifications for the experience the user is set to? For example, a NewDot user would not receive a notification for an OldDot violation, and a NewDot user would not (currently) receive any violation notification.

[AndrewGable]

I think we want to stop sending and we will figure out how to handle violation push notifications in a different solution. I think we are discussing them here: https://expensify.slack.com/archives/C06ML6X0W9L/p1728408150428299

[AndrewGable]

Can you confirm the second paragraph here @trjExpensify?

[trjExpensify]

What's a "NewDot violation", and what's an "OldDot violation", just so I'm clear?

I'm not aware of a system message to notify when a receipt upload contains workspace violations in NewDot. @JmillsExpensify is this supposed to exist?

I know there's a system message for a failed SmartScan that should be added to the transaction thread that would notify the submitter when it fails. It looks like this:

But interestingly, I just tested that, and it seems to not be added anymore? 🤔

Secondly, is the desired behaviour to only send push notifications for the experience the user is set to? For example, a NewDot user would not receive a notification for an OldDot violation, and a NewDot user would not (currently) receive any violation notification.

On this, yes.. if you're set to the NewDot experience you shouldn't receive push notifications for the OldDot experience and the same the other way around. Otherwise, you would get pulled into the wrong experience when tapping the notification.

[trjExpensify]

Code for SmartScan failure is here, I believe: https://github.com/Expensify/Auth/blob/44606fb94035b15742c95973346b3d468269e4bc/auth/command/EditMoneyRequest.cpp#L431-L445

[Julesssss]

What's a "NewDot violation", and what's an "OldDot violation", just so I'm clear?

I wasn't sure where they originated from (are the OldDot notifications coming from a NewDot violation request?) -- but your answer has cleared this up.

On this, yes.. if you're set to the NewDot experience you shouldn't receive push notifications for the OldDot experience and the same the other way around.

Okay thanks. Our logic assumes otherwise as I thought we wanted to avoid dropping OldDot notifications when users first switch to NewDot. Currently we'll send OldDot styled notifications (receipt needs approval, violation) to NewDot users and we can simply switch this off.

Do we have any issues or plans to re-introduce NewDot notifications though? As we ramp up NewDot redirection users are going to be blissfully unaware of violations, ect.

[trjExpensify]

Yeah, I can't see any. So I think what we need to do is:

• Get that actionable system message when SmartScan fails sending again (which will then send a push notification to the submitter and open the transaction thread)
• Send an actionable system message when there's a policy violation on the receipt uploaded (which will then send a push notification to the submitter and open the transaction thread)
• Keep sending the Expensify Card push notification(s) at the point of sale, but if you're set to the NewDot experience, open the camera/transaction view in NewDot.

Do you guys agree with that?

[JmillsExpensify]

That sounds good to me as a starting point.

[trjExpensify]

Alright, so what's the next step here and who's taking it on?

[Julesssss]

First lets solve the crash:

Currently we'll send OldDot styled notifications (receipt needs approval, violation) to NewDot users and we can simply switch this off.

I'll happily do this, but need to finish up attendee tracking first.

[trjExpensify]

Okay, does that block defaulting new sign-ups to the NewDot experience though? If so, I think it's higher priority than attendee tracking in terms of priority, as these final issues to get this done come into #convert now.

[Julesssss]

Okay, does that block defaulting new sign-ups to the NewDot experience though?

Yeah, it probably should. But as both are critical WhatsNext tasks associated with deadlines, I think I should finish what I started before picking up this task -- attendee tracking has already missed one deadline, and this would risk missing a second.

[Julesssss]

After catching up with the new prioritisations, I can pick this up once I have wound down attendee tracking and left it in a good place.

[trjExpensify]

Great stuff!

[trjExpensify]

Jules, are you good with the plan of action here? Which I think is:

1. Solve the crash. Stop sending the OldDot styled notifications when there's a violation on the receipt uploaded when the user is defaulted to the NewDot experience.
2. Add the NewDot system messages to the transaction thread, so they notify the submitter incl. bringing back the SmartScan failure one.

SmartScan failure:

Expensify Today at 10:49AM
receipt scanning failed, please enter the details manually.

Violation on receipt upload:

Expensify Today at 09:02AM
that %merchant% receipt you just added needs your attention, please take a look. 👀

3. Check Expensify Card "point of sale" notifications are still delivered and don't crash on open, make them open the camera/transaction view in the NewDot experience when applicable.

[Julesssss]

Yeah, this all seems fine. I would prefer to solve the crash first (1), and plan the remaining tasks (2,3) for later (after or during DevX). But I could continue to working on these if we're okay delaying DevX/making HybridApp external. A couple of clarifications:

Add the NewDot system messages to the transaction thread

I might have to configure the notification title/grouping. This would be minor text formatting though.

Check Expensify Card "point of sale" notifications are still delivered

To be 100% sure, we still will send the OldDot POS notification to users on NewDot but won't yet build out the NewDot POS notifications.

---

Notes:

• Remove notification duplication logic when source is OldDot
• Build a param to override this (for example, for the POS notification)

[trjExpensify]

I might have to configure the notification title/grouping. This would be minor text formatting though.

We had a system message for SmartScan failures already that we should bring back (or figure out why it's not there anymore), so I don't think this would be too hard to follow for this NewDot paradigm.

But I could continue to working on these if we're okay delaying DevX/making HybridApp external.

I think these items are probably more important as they impact customers using the NewDot experience on HybridApp. Ecard transactions POS notifications have a fraud component to them as well really, so I think this is the right priority personally. 👍

To be 100% sure, we still will send the OldDot POS notification to users on NewDot but won't yet build out the NewDot POS notifications.

Yeah, I view the Expensify Card POS notifications as being a feature of the Expensify Card not OldDot specifically. If you receive one, it should open in the right experience depending on which one that is.

[melvin-bot]

Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Julesssss]

Focusing on a more critical HybridApp issue first.

[Julesssss]

I'll need to try again, but I believe I tried both cases -- Initially I wasn't receiving the notification when set to NewDot, so I switched to these steps.

[Julesssss]

Thanks @kavimuru. So as of right now the crash isn't reproducable. Please anyone correct me if they disagree.

I'm too busy with daily App Deploy to pick this up yet but I my next step will be to make sure NewDot users aren't sent OldDot violation notifications.

1. Solve the crash. Stop sending the OldDot styled notifications when there's a violation on the receipt uploaded when the user is defaulted to the NewDot experience.

@trjExpensify if the crash isn't occurring then there's nothing blocking our *final HybridApp production deploy.

[trjExpensify]

@trjExpensify if the crash isn't occurring then there's nothing blocking our *final HybridApp production deploy.

That's awesome news, let's confirm and ship!

[Julesssss]

I confirmed that this is no longer crashing for Andrew or myself. Here's the final HybridApp release for reference.

[melvin-bot]

@Julesssss Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Julesssss]

Finishing up other tasks. The priority might come below other tasks now that the crash isn't occurring.

[melvin-bot]

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

[melvin-bot]

@Julesssss Still overdue 6 days?! Let's take care of this!

[Julesssss]

Unassigning for now, while I work on other issues.

[melvin-bot]

Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

10 days overdue. I'm getting more depressed than Marvin.

[melvin-bot]

12 days overdue now... This issue's end is nigh!

[melvin-bot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[melvin-bot]

This issue has not been updated in over 15 days. eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[trjExpensify]

This can be closed now. The crash was fixed. Jason is progressing Ecard notifications elsewhere.