Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.47k stars 2.82k forks source link

[HOLD for payment 2023-10-16] Chat - Expand button disappear after clicking even when composer has error #27779

Closed kbecciv closed 12 months ago

kbecciv commented 1 year 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!


Action Performed:

  1. Go to any report
  2. Paste the message that has over 15,000 characters (To make the error shows).
  3. Now the composer will have red border and expand button is visible, click on the Send button.
  4. Notice that now the Composer doesn’t have Expand button.

Expected Result:

Expand button should be visible after clicking Send button

Actual Result:

Expand button disappear after clicking even when composer has error

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.71.5 Reproducible in staging?: y Reproducible in production?: y 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/ea69cd7e-99b2-47be-abcc-e918e153341f

https://github.com/Expensify/App/assets/93399543/4560eefb-e5fa-4c5f-8d6c-e6e5661a0f5b

Expensify/Expensify Issue URL: Issue reported by: @hungvu193 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694937956481939 https://expensify.slack.com/archives/C049HHMV9SM/p1695124790291359

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e5b7548a5c671a9e
  • Upwork Job ID: 1704120906245259264
  • Last Price Increase: 2023-09-19
  • Automatic offers:
    • DylanDylann | Contributor | 26978024
    • hungvu193 | Reporter | 26978028
DylanDylann commented 1 year ago

Proposal

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

What is the root cause of that problem?

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

If you could update your proposal so that send button won't be disabled during chat loading, but be disabled only after clicking on it

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @sakluger (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

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

kbecciv commented 1 year ago

Proposal by: @hungvu193 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694937956481939

Proposal

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

Composer: Clicking on Send button make the error disappear while actually it should do nothing.

What is the root cause of that problem?

We're setting disable style for our Pressable inside SendButton here. https://github.com/Expensify/App/blob/c77814fd5e643cba78e8c8294370d78f633a59c5/src/pages/home/report/ReportActionCompose/SendButton.js#L57-L64 However, we're using GestureDetector's gesture to handle the click on Send button, so even the send button is disabled, it still perform the gesture and in our Tap gesture which caused this issue: https://github.com/Expensify/App/blob/c77814fd5e643cba78e8c8294370d78f633a59c5/src/pages/home/report/ReportActionCompose/SendButton.js#L36-L49

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

Solution 1: We should disable the Tap gesture if isDisabledProp is true:

const Tap = Gesture.Tap()
        .enabled(!isDisabledProp)
        .onEnd(() => {

Solution 2: Add an early return when isDisabledProp is true inside onEnd function:

 .onEnd(() => {
            'worklet';
             if (isDisabledProp) {
                 return;
             }

What alternative solutions did you explore? (Optional)

N/A

akinwale commented 1 year ago

This has the same root cause as https://github.com/Expensify/App/issues/26960 where if the send button is set to disabled, the tap action still gets executed.

Pointed out here, and a working proposal for the fix.

jliexpensify commented 1 year ago

@robertKozik based on the comment above, this sounds like it could be a dupe? Shall we close this or maybe put this on hold?

DylanDylann commented 1 year ago

@jliexpensify I think App allows to write and send message in skeleton view is expected behavious

robertKozik commented 1 year ago

No, it's not a duplicate. The issue that @akinwale linked did contain a proposal with a solution that would also fix our issue. However, that issue as a whole was closed because it pertained to expected behavior. I'll review the proposals today.

jliexpensify commented 1 year ago

Any thoughts on the proposals @robertKozik ?

melvin-bot[bot] commented 1 year ago

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

robertKozik commented 1 year ago

Hi @DylanDylann @hungvu193! Both of you siggested simillar solution for this problem, which potencially suffers from the same problem. Per #26960 issue we want the send button to be accessible when chat is loading. @hungvu193 your solution is passing the isDisabledProp which would disable the send button while loading @DylanDylann is the isSendDisabled variable identical with isDisabledProp? If yes, it will case the same problem.

If you could update your proposal so that send button won't be disabled during chat loading, but be disabled only after clicking on it

DylanDylann commented 1 year ago

@robertKozik Also please note that my proposal will fix the case pressing the ENTER button, not only clicking on "Send" button. Because pressing the ENTER button or clicking on the Send button should be the same, right?

DylanDylann commented 1 year ago

@robertKozik

If you could update your proposal so that send button won't be disabled during chat loading, but be disabled only after clicking on it

hungvu193 commented 1 year ago

Ok, Updated my proposal:

Proposal

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

Composer: Clicking on Send button make the error disappear while actually it should do nothing.

What is the root cause of that problem?

We're setting disable style for our Pressable inside SendButton here. https://github.com/Expensify/App/blob/c77814fd5e643cba78e8c8294370d78f633a59c5/src/pages/home/report/ReportActionCompose/SendButton.js#L57-L64 However, we're using GestureDetector's gesture to handle the click on Send button, so even the send button is disabled, it still perform the gesture and in our Tap gesture which caused this issue: https://github.com/Expensify/App/blob/c77814fd5e643cba78e8c8294370d78f633a59c5/src/pages/home/report/ReportActionCompose/SendButton.js#L36-L49

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

First of all we should update the logic of isSendDisabled as below:

// with isReportReadyForDisplay is passed down from ReportScreen.
const isSendDisabled = !isReportReadyForDisplay || isCommentEmpty || isBlockedFromConcierge || disabled || hasExceededMaxCommentLength;

After that we can do as below: Solution 1: We should disable the Tap gesture if isDisabledProp is true:

const Tap = Gesture.Tap()
        .enabled(!isDisabledProp)
        .onEnd(() => {

Solution 2: Add an early return when isDisabledProp is true inside onEnd function:

 .onEnd(() => {
            'worklet';
             if (isDisabledProp) {
                 return;
             }

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 1 year ago

@jliexpensify, @robertKozik Eep! 4 days overdue now. Issues have feelings too...

robertKozik commented 1 year ago

Thank you both for updating the proposals! I think I will suggest the @DylanDylann proposal, as it's not only fixes the issue, but as well refactor the code with unifying the submit functions.

Selected proposal: https://github.com/Expensify/App/issues/27779#issuecomment-1725481804

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @aldo-expensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 1 year ago

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 1 year ago

📣 @hungvu193 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

DylanDylann commented 1 year ago

@robertKozik PR https://github.com/Expensify/App/pull/28681 is ready for review

melvin-bot[bot] commented 1 year ago

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

On to the next one 🚀

jliexpensify commented 1 year ago

Should @DylanDylann be paid a reporting bonus here? It looks like there was a ~3 day delay for @robertKozik to review the PR cc @aldo-expensify for your thoughts

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-10-16. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

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:

robertKozik commented 1 year ago

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:

  • [X] [@robertKozik] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/25758
  • [X] [@robertKozik] 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: https://github.com/Expensify/App/pull/25758/files#r1351875313
  • [X] [@robertKozik] 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: N/A
  • [X] [@robertKozik] Determine if we should create a regression test for this bug. I don't think we need new test here
  • [X] [@robertKozik] 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. N/A
DylanDylann commented 1 year ago

@aldo-expensify Please help check https://github.com/Expensify/App/issues/27779#issuecomment-1752353186 when you have a chance

jliexpensify commented 12 months ago

Hi @DylanDylann - I actually queried this with another GH (same situation, delay in C+) and was told that bonuses only apply if the Expensify Engineer is delayed. Sorry!

jliexpensify commented 12 months ago

Payment Summary:

Upwork job

DylanDylann commented 12 months ago

Yeah thanks @jliexpensify

jliexpensify commented 12 months ago

All paid and job closed!