[Hold for payment - 2024-03-07] MEDIUM: Custom message sound not consistent on mobile

[Julesssss]

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: 1.4.42-0
Reproducible in staging?: Yes

Action Performed:

• Send a message to a user signed in on mobile in three different app states:
  • App Killed
  • App Open
  • App in background

Expected Result:

• App Killed: Custom sound
• App Open: Custom sound
• App in background: Custom sound

Actual Result:

• App Killed: Default notification sound
• App Open: Custom sound
• App in background: Default notification sound

Workaround:

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

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

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[kirillzyusko]

Thanks for bringing this up! As I have written that feature let me take some time trying to reproduce. I'll let you know if I found anything 👀

[Julesssss]

I can also help with backend changes. As you mentioned we may need to start sending a reportType variable -- though I also think we don't send IOU notifications currently

[Julesssss]

@kirillzyusko and I discussed. We can drop the message received sound on mobile apps as this is being handled by the OS push notification itself.

[Julesssss]

Assigning @abdulrahuman5196 based on this comment

[kirillzyusko]

@Julesssss since the PR was merged - what should we do with that issue? Should we re-test and close it?

[Julesssss]

Yeah, I'll try and find time tomorrow to do this. If you're able to verify iOS that would be very helpful.

[kirillzyusko]

@Julesssss does the app publicly available or do I need to get the invite (to TestFlight, for example)? If everyone can test, then would you mind to share a link here? 👀 (sorry for being asking such simple questions, but it's a first time when I'm testing the app deployed to staging)

[Julesssss]

No problem. You would need to get invited to Testflight to test staging, but the PR has already been deployed to production, so you can use the App Store app to verify 👍

[Julesssss]

I'm going to close this issue as fixed, but let's discuss the other bugs on their respective issues.

[abdulrahuman5196]

Hi, @Julesssss C+ payment processing would be pending on this issue.

Kindly re-open the issue.

cc: @CortneyOfstad

[Julesssss]

Awaiting C+ payment

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

Not a regression. Update to existing implementation

Determine if we should create a regression test for this bug.

Yes.

If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Open app on two devices (A - iOS, B - Android)
2. Send a message from device A to device B
3. Be sure that you hear a custom sound only when message is received (plain messages)
4. Send message from device B to device A
5. Be sure that you hear a custom sound only when message is received (plain messages)
6. Open app on two devices (any platforms, exclude iOS/Android platform)
7. Send a message from device A to device B
8. Be sure that sound can be heard

[Julesssss]

We're still making changes to the push notification logic so this test will soon be outdated. We'll create one seperately once the changes are complete.

[CortneyOfstad]

Sorry, was OoO last week — handling the payment now!

[CortneyOfstad]

@Julesssss in regards to the comment here, is a regression test needed here if there will be separate tests created after the logic is adjusted? Just trying to confirm — thank you!

[CortneyOfstad]

@abdulrahuman5196 I sent you a job offer here — https://www.upwork.com/jobs/~01d297cc64a730c705

Not sure why the job did not post originally, but please let me know once you accept and I'll get that paid ASAP. Thanks!

[abdulrahuman5196]

@CortneyOfstad accepted the offer. Thank you

[CortneyOfstad]

Paid!

Payment Summary

@abdulrahuman5196 — paid $500 via Upwork!

[Julesssss]

Hi @CortneyOfstad, no test needed here

[CortneyOfstad]

Perfect! This can be closed — thanks everyone!