[Payment due 08-23]Emphasize completed payments with animation

[roryabraham]

slack discussion: https://expensify.slack.com/archives/CC7NECV4L/p1724447142877389

Strategy

Expensify is a financial collaboration app, and in any financial conversation, money moving hands from one person to another tends to be the most critical part. That critical moment needs to be clear, unambiguous, and confidence-inspiring.

Problem

When you receive a money request in NewDot, you’ll see a big green Pay button, and one tap instantly pays the request and changes and a checkmark appears saying that it’s paid. It’s such a crucial moment, but it’s unclear whether the payment was sent or not. As a user, I always find I have to double-take and make sure that the request was actually paid.

Solution

Update the payment flow slightly to draw more attention to it and celebrate the transfer of funds. To do this, update NewDot such that when you tap the Pay button:

1. the button animates into a Payment Complete text, which collapses a few moments later
2. (iOS/Android) we trigger some strong haptic feedback to give a tactile confirmation of the completion of the payment.
3. Once the Payment Complete text finishes collapsing:
   1. a checkmark appears (as today)
   2. Play the assets/sounds/success.mp3 sound, instead of assets/sounds/done.mp3 like we do today

Example:

Issue Owner

Current Issue Owner: @ZhenjaHorbach

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[melvin-bot]

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

[melvin-bot]

Current assignee @dubielzyk-expensify is eligible for the NewFeature assigner, not assigning anyone new.

[roryabraham]

@dubielzyk-expensify has volunteered to help finalize the design here.

[othmanekahtal]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Emphasize completed payments with animation

• update text after payment is completed
• update the sound

What is the root cause of that problem?

Improvement

What changes do you think we should make in order to solve the problem?

• change the params that passed to the function playSound in the button component from SOUNDS.DONE to SOUNDS. SUCCESS
• add the "Payment Complete" to the i18n
• update the text by updating the buttonOptions.push({ text: translate('iou.PaymentComplete') });

What alternative solutions did you explore? (Optional)

[ZhenjaHorbach]

@othmanekahtal
Thanks for your proposal !
But could you update your proposal and provide some links to the code where you plan to make changes, please?

[othmanekahtal]

@ZhenjaHorbach Hi Yauheni
thanks for noticing, i forked the repo and i made the changes take a look at this link : d2c943a

[ZhenjaHorbach]

@othmanekahtal
Thanks for your updates
But I'm not sure that it is a good idea to modify the memoized paymentButtonOptions array like this

d2c943a#diff-475fc90a9838cf6b734f4c860eb57036c3fe04ac7b4c429d1b4e361109aaa6f4R298-R300

Plus If I understand correctly
We are waiting for confirmation of designs from @dubielzyk-expensify

[roryabraham]

I think @dubielzyk-expensify and I have an understanding on the animation. What we want will be very similar to this:

[dubielzyk-expensify]

Exactly. Let's try to replicate exactly what's above 👍

[melvin-bot]

@roryabraham, @Christinadobrzyn, @ZhenjaHorbach, @dubielzyk-expensify Eep! 4 days overdue now. Issues have feelings too...

[roryabraham]

I do think we should amend the regression steps for paying expense reports and IOUs to account for this.

[shubham1206agra]

Regression here #49331

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[roryabraham]

@bernhardoj hi 👋🏼

@shawnborton noticed that we missed the animation on the checkmark itself. Can we create a follow-up PR to address that?

[bernhardoj]

@roryabraham sure, I'll open the PR tomorrow.

[bernhardoj]

PR is ready

cc: @ZhenjaHorbach

[Christinadobrzyn]

Hum, is today payment day? It looks like the regression PR was merged 3 days ago -#49438

So I think we're waiting for a few more days? Do we need a regression test for this?

[Christinadobrzyn]

I'm going to be ooo until next Monday. Since this is waiting for a regression and day 7 should be Friday, do you mind if I just keep this and pay it out on Monday?

If I understand this correctly, we are waiting on the regression PR to merge, which should be on Friday.

Payment would be the following - can we confirm?

Payouts due:

• Contributor: $125 (50% due to regression) @bernhardoj (in Newdot)
• Contributor+: $125 (50% due to regression) @ZhenjaHorbach (Upwork- https://www.upwork.com/nx/wm/offer/103991778)

[bernhardoj]

@Christinadobrzyn #49438 is not a regression PR. We just missed to animate the checkmark too.

[shubham1206agra]

Regression here #49331

I think they meant this one.

[bernhardoj]

#49331 happens because of this suggestion, @roryabraham does that count as a regression?

[shubham1206agra]

@bernhardoj Yes, since checking code implementation for bugs is your area. @roryabraham is just suggesting something.

[bernhardoj]

If that's true, then I'll keep in mind ignoring any suggestion unrelated to the issue in the future since that doesn't benefit me at all and becomes a punishment to me if there is an issue arising from that suggestion.

[shubham1206agra]

If that's true, then I'll keep in mind ignoring any suggestion unrelated to the issue in the future since that doesn't benefit me at all and becomes a punishment to me if there is an issue arising from that suggestion.

Hmmm. What's the problem if you test the code where you made the change? Why should there be immunity here?

[Christinadobrzyn]

Asking @roryabraham about the regression discussion - #48036 (comment)

Payment summary here - #48036 (comment)

[roryabraham]

Well regarding the regression, I don't think it was this suggestion per-se so much as by the migration from withOnyx -> useOnyx. I say that because it looks like the root cause was lastPaymentMethod being not loaded from Onyx yet when the component mounts, which wasn't possible with withOnyx.

I don't think @shubham1206agra is wrong here either though - the useOnyx migration is a requirement, and any time you are making code changes, you are responsible for testing that code and accountable for either fixing it or getting a regression penalty.

That said, I think we can waive any regression penalty in this case for a few reasons:

• @ZhenjaHorbach was the C+ reviewer for this issue and took care of fixing it in Fix bug with Payment option is not selected #49345
• @bernhardoj didn't hesitate to open second PR to implement a design improvement we missed the first time around. This demonstrates a commitment and work ethic that I think should be rewarded
• Frankly, it's pretty hard to spot when the render-blocking behavior of withOnyx actually matters.

I understand that doing this incremental useOnyx migration as part of PRs that might be otherwise unrelated is different from how we've operated in the past, but except in special circumstances, that's the expectation when working on PRs for now. We should try to be vigilant and do a bit of extra testing for more obscure keys, because they're less likely to be in the cache already than something like a report or session. For what it's worth though, as time goes on it will become decreasingly common that we have to do it, and the code will be simpler as a result.

[Christinadobrzyn]

Thank you for the thorough review, @roryabraham!

Paying out based on payment summary where the regression penalty isn't included.

@bernhardoj can you please request payment through NewDot or let me know if you'd like me to pay you through Upwork. TY!

[bernhardoj]

Thank you! Requested in ND.

[Christinadobrzyn]

Thanks! @bernhardoj. I forgot to ask, do we need a regression test for this?

[ZhenjaHorbach]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ZhenjaHorbach] The PR that introduced the bug has been identified. Link to the PR:

New feature

• [ @ZhenjaHorbach] 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:

New feature

• [@ZhenjaHorbach] 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:

NA

• [@ZhenjaHorbach] Determine if we should create a regression test for this bug.
• [@ZhenjaHorbach] 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.

Regression Test Proposal

• Open App
• Open your policy expense chat
• Submit a request. Approve if needed.
• Pay the money request
• Verify the pay button animates (as shown [Payment due 08-23]Emphasize completed payments with animation #48036 (comment))
• Verify there is a vibrate when pressing the pay button (Android/iOS)

Do we agree 👍 or 👎

[ZhenjaHorbach]

Thanks! @bernhardoj. I forgot to ask, do we need a regression test for this?

@Christinadobrzyn
Done !

[Christinadobrzyn]

Thanks @ZhenjaHorbach - regression test here - https://github.com/Expensify/Expensify/issues/431955

Payment summary here - #48036 (comment)

Gonna close this out since the last step is @bernhardoj being paid via ND but they've requested the payment so nothing else to wait on.

[JmillsExpensify]

$250 approved for @bernhardoj