search

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.51k stars 2.87k forks source link

[PAID] [$500] Web - Composer send button is enabled even when the input contains only space #26433

Closed kbecciv closed 1 year 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. Open any chat
  2. Type multiple spaces and verify the send button is still disabled
  3. Close the chat or navigate to another chat
  4. Open back the chat

Expected Result:

The send button stays disabled because it contains only space

Actual Result:

The send button is enabled

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.60.1 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/d2d7a800-a5e8-4bdf-9b9e-98b25c5d7367

https://github.com/Expensify/App/assets/93399543/126c6c8e-c746-496d-af70-9fd86c545b31

Expensify/Expensify Issue URL: Issue reported by: @bernhardoj Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693037242762969

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0148ad3caa7badc613
  • Upwork Job ID: 1697749457182527488
  • Last Price Increase: 2023-09-01
  • Automatic offers:
    • fedirjh | Reviewer | 26518770
    • bernhardoj | Contributor | 26518772
    • bernhardoj | Reporter | 26518773
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @strepanier03 (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)

kbecciv commented 1 year ago

Proposal by: @bernhardoj Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693037242762969

Proposal

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

The composer send button is enabled even when it contains only space.

What is the root cause of that problem?

We disable the send button if isCommentEmpty state is true. The initial value of isCommentEmpty comes from isCommentEmptyProp. https://github.com/Expensify/App/blob/c16995257c738761206ea07645e863ee717ecbc0/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L131 https://github.com/Expensify/App/blob/c16995257c738761206ea07645e863ee717ecbc0/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#[…]8 Notice that we check using _.isEmpty which considers space only as not empty. This is different from what we do when typing something in the composer. https://github.com/Expensify/App/blob/c16995257c738761206ea07645e863ee717ecbc0/src[…]ages/home/report/ReportActionCompose/ComposerWithSuggestions.js

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

This issue happens after the ReportActionCompose refactor. Previously, when the composer mounted, we always called updateComment to make sure isCommentEmpty had a correct value, but this approach raised this issue. Even though the issue is marked as not a bug, I still think it's a bug and we shouldn't use the old approach which is a workaround. To fully solve the issue, we should replace _.isEmpty with the regex we use when typing in the composer.

isCommentEmpty: {
    selector: (comment) => !comment || !!comment.match(/^(\s)*$/)
// comment could be undefined, so we also use !comment
alifhaider commented 1 year ago

Proposal

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

Composer send button is not disabled when typed text is only spaces

What is the root cause of that problem?

Send Button is only checking for disabled if field is empty or not. https://github.com/Expensify/App/blob/64ff471fc614a57645edf34b5d2ac346046f12c1/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L452-L455

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

We can trim the comment and then update isCommentEmpty. By this, it will remove the start and ending spaces of text, and then make the button enabled and disabled based on that. 

isCommentEmpty: {
            key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
            selector: (comment) => _.isEmpty(comment) || _.isEmpty(comment.trim()),
        },

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 1 year ago

📣 @alifhaider! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format: 
    Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
alifhaider commented 1 year ago

Contributor details Your Expensify account email: alifhaider57@gmail.com Upwork Profile Link: (https://www.upwork.com/freelancers/~014066482556c551be)

melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

alphaboss1104 commented 1 year ago

Proposal

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

Composer send button is not disabled when typed text is only spaces

What is the root cause of that problem?

We disable the send button if isCommentEmpty state is true. We set the isCommentEmpty 's value by comment is empty or not when the chat is reopened or navigated back to.

https://github.com/Expensify/App/blob/ec95ab05e2c9d14ac91aa33946a5beeb39099e1f/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L452-L455

This is the root cause of this issue.

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

We should also check if the comment is a string full of spaces like this.

        isCommentEmpty: {
            key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
            selector: (comment) => _.isEmpty(comment) || !!comment.match(/^(\s)*$/),
        },

What alternative solutions did you explore? (Optional)

We can also be resolved by adding a check in the useEffect hook that handles the reopening or navigation back to chat here.

    useEffect(() => {
        if (value.length === 0) {
            return;
        }

+       setIsCommentEmpty(value.trim().length === 0);

        Report.setReportWithDraft(reportID, true);
    }, []);
melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0148ad3caa7badc613

melvin-bot[bot] commented 1 year ago

Current assignee @strepanier03 is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

muradkhan101 commented 1 year ago

Proposal

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

The Composer send button is disabled correctly when entering spaces inside of a chat but it is incorrectly enabled when re-entering that same chat room

What is the root cause of that problem?

When loading the page initially, the send button's disabled state is set by the initial value of isCommentEmptyProp, which is set here using _.isEmpty.

https://github.com/Expensify/App/blob/9701fbe5ec24bf5031da390075b2a9fb125b3ce3/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L454

However, when the state is updating while chatting, the value is set by using regex. https://github.com/Expensify/App/blob/9701fbe5ec24bf5031da390075b2a9fb125b3ce3/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js#L185

The issue is that there is no common helper function to check whether a comment is empty so two different implementations currently exist.

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

I think we should create a common isTextInputEmpty helper method that will use the regex to determine if the comment is empty. Then we update all code locations to use this helper instead of having their own implementation.

What alternative solutions did you explore? (Optional)

Just update all locations to use the regex but this issue can happen again later if a new engineer doesn't know to use the regex. The common helper method should resolve that issue

rsiota commented 1 year ago

Proposal

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

When we open a chat and type multiple spaces in the composer, the send button remains disabled. But if we switch chats and navigate back to the original chat, the send button becomes enabled, instead of remaining disabled.

What is the root cause of that problem?

The root cause of the problem is that the check used to determine if a comment is empty, uses the _.isEmpty() underscore function, which, for strings and array-like objects, checks if the length property is 0. This function doesn't account for white spaces within a string.

The current check happens on ReportActionCompose.js, in the following lines:

452    isCommentEmpty: {
453        key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
454        selector: (comment) => _.isEmpty(comment),
455    },

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

I would propose to add a further check to determine if the empty comment has white spaces. I think that it would be useful to have a utility function that could be composed when doing the check.

I was looking for a place on the basecode that might be appropriate for adding the utitlity function, and I thought of ReportUtil.js, but, without knowing the basecode in depth, I understand, there might be other files, where this utility function, might be more appropriately placed.

I would suggest the following changes:

ReportUtil.js

/**
* Checks if a comment has white space
*
* @param {String} comment
* @returns {Boolean}
*/
function hasWhiteSpace(comment) {
    return /\s/g.test(comment);
} 
export {
    ...
    hasWhiteSpace,
    ...
};

ReportActionCompose.js

isCommentEmpty: {
    key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
    selector: (comment) => _.isEmpty(comment) || ReportUtils.hasWhiteSpace(comment),
},

What alternative solutions did you explore? (Optional)

An alternative solution would be to inline the check for white spacing directly, instead of using a separate function. I thought that using a separate function might help with readability.

melvin-bot[bot] commented 1 year ago

📣 @rsiota! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format: 
    Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
rsiota commented 1 year ago

Contributor details Your Expensify account email: rsiota@gmail.com Upwork Profile Link: (https://www.upwork.com/en-gb/freelancers/~016641e7b5affe3105)

melvin-bot[bot] commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

melvin-bot[bot] commented 1 year ago

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

fedirjh commented 1 year ago

Thanks everyone for the proposal.

Looks like @bernhardoj's solution has a straightforward fix for this issue. Let's proceed with it.

🎀 👀 🎀 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.

aldo-expensify commented 1 year ago

I'm not being able to assign @bernhardoj for the moment

image

I was having some problems with Github crashing a few minutes ago, maybe is related to that, I'll try later.

strepanier03 commented 1 year ago

Hmm, I gave it a go and can't assign them either 🤔

strepanier03 commented 1 year ago

@bernhardoj - Can you comment on this GH, please? I ran into this issue on another GH in the past and commenting allowed assigning, not sure why.

bernhardoj commented 1 year ago

Comment

melvin-bot[bot] commented 1 year ago

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

Offer link Upwork job

melvin-bot[bot] commented 1 year ago

📣 @bernhardoj 🎉 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

📣 @bernhardoj 🎉 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

bernhardoj commented 1 year ago

PR is ready

cc: @fedirjh

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 🚀

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.74-3 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-05. :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.

Bonus assessment discussion: I took a look at the PR and it looks like this was ready for our review on September 6 and other factors delayed the merge.

  • C+ approved on 9/6
  • We placed the PR on hold with reason "Merge Freeze" on 9/6
  • The C+ bumped the engineer that it was ready to be merged on 9/12
  • We held until the next deploy due to the number of PRs on 9/12
  • The C+ bumped the engineer to merge on 9/22
  • The PR was merged on 9/22

In light of this, I am going to pay the urgency bonus.

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:

strepanier03 commented 1 year ago

@bernhardoj - You've been paid in Upwork. I left a note on the bonus payment but I thought we had two separate contracts for this job for you so I added the bonus to pay the reporting payment after the fact. Let me know if you have any questions.

@fedirjh - I'll keep my eyes open for an update on the checklist and move forward once there's one.

Thanks everyone!

bernhardoj commented 1 year ago

@strepanier03 got it. Thanks! I think you can close the Reporter contract then.

fedirjh commented 1 year ago

BugZero Checklist:

Regression Test Proposal

  1. Open any chat
  2. Type space into the composer.
  3. Verify the send button is disabled.
  4. Switch to another chat
  5. Open the chat back

  6. Verify the send button is still disabled.

    • Do we agree 👍 or 👎
fedirjh commented 1 year ago

@strepanier03 The offer was expired for me, could you please send me another offer 🙏🏼

strepanier03 commented 1 year ago

Sure thing @fedirjh, I'll do that now.

strepanier03 commented 1 year ago

@fedirjh - Sent!

Working on reg test GH now.

strepanier03 commented 1 year ago

@bernhardoj - I'll work on closing that next week. For some reason both contracts linked here are for the same contract and paid already.

fedirjh commented 1 year ago

@strepanier03 Accepted, Thank you.

strepanier03 commented 1 year ago

@fedirjh - I have finished paying you as well. Thanks for accepting that.

Thank you both for being part of our community and working so hard to help us achieve the New Expensify dream.