kbecciv commented 9 months 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!

Version Number: v1.4.12-0 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal team Slack conversation:

Action Performed:

Open the app
Open any workspace chat and scroll on top
Observe the text 'You can also use the + button to send money, request money, or assign a task!' has send money even though send money is not possible to workspace chat
Confirm the options by clicking on plus besides compose box and observe that it does not have 'send money' option

Expected Result:

App should not display 'Send money' text in workspace chat as send money to workspace is not possible

Actual Result:

App displays 'Send money' text in workspace chat even though send money to workspace is not possible

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/31ead2c4-6a38-445a-8af6-eeb45ff58a50

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0184e5f006c16a747a
Upwork Job ID: 1735020266743328768
Last Price Increase: 2023-12-20
Automatic offers:

Ollyws | Reviewer | 28067635
ZhenjaHorbach | Contributor | 28067636

Issue Owner

Current Issue Owner: @miljakljajic

melvin-bot[bot] commented 9 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0184e5f006c16a747a

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 9 months ago

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

Berndy commented 9 months ago

Proposal

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

The displayed text mentions being able to send money, despite this not being the case in workspace chats.

What is the root cause of that problem?

We check if money request options contain send OR request to decide whether we display the text or not.

https://github.com/Expensify/App/blob/904cf5635b819971b7b24bc78c3e516beea21239/src/components/ReportWelcomeText.js#L133-L135

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

We need to add another text for workspace chats, which doesn't mention sending money. Then decide which text to render based on which money request options we have available

https://github.com/Expensify/App/blob/904cf5635b819971b7b24bc78c3e516beea21239/src/languages/en.ts#L467

https://github.com/Expensify/App/blob/904cf5635b819971b7b24bc78c3e516beea21239/src/languages/es.ts#L460

I suggest we add usePlusButtonWorkspace, then display the current text only if we can send and request, or the workspace text if we can only request.

What alternative solutions did you explore? (Optional)

ZhenjaHorbach commented 9 months ago

Proposal

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

Chat - Send money text displayed in workspace chat

What is the root cause of that problem?

We have static text for usePlusButton

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

To fix this we can synchronize with the available buttons we have in the AttachmentPicker

https://github.com/Expensify/App/blob/904cf5635b819971b7b24bc78c3e516beea21239/src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.js#L141-L143

For this, we can use logic from AttachmentPicker and get a list of available moneyRequestOptions

https://github.com/Expensify/App/blob/3df907ce12d1085b71231aff126cb5288a5e8145/src/components/ReportWelcomeText.js#L73

And then update usePlusButton translate to be able to pass custom properties (

https://github.com/Expensify/App/blob/904cf5635b819971b7b24bc78c3e516beea21239/src/components/ReportWelcomeText.js#L134

        usePlusButton: ({options}: {options: string[]}) => {
            const actions = options.map((item: string) => `${item} money`).join(', '); // We can move this logic inside ReportWelcomeText and pass only additional text instead options 
            const additionalText = actions ? `${actions}, or` : '';
            return `\n\nYou can also use the + button to ${additionalText} assign a task!`;
        },

Plus It's strange that we ignore the split option. I think we also need to enable this option for showing in translate

What alternative solutions did you explore? (Optional)

As alternative we can create translate for every case And depending on the availability of a particular button, show the translation

yh-0218 commented 9 months ago

Proposal

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

Chat - Send money text displayed in workspace chat

What is the root cause of that problem?

Because we use static text here https://github.com/Expensify/App/blob/2782381be1cf4e8e4e697ae6d95b28e2e79817c3/src/components/ReportWelcomeText.js#L134

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

first we need to get dynamic text.

// this is test, we need to update more.
const addedText = useMemo(() => {
    let temp = '';
    moneyRequestOptions.forEach((option) => {
        if(option.includes(CONST.IOU.TYPE.SPLIT)) {
            temp = temp? `${temp}, split bill` : 'split bill'
        }
        if(option.includes(CONST.IOU.TYPE.SEND)) {
            temp = temp? `${temp}, send money` : 'send money'
        }
        if(option.includes(CONST.IOU.TYPE.REQUEST)) {
            temp = temp? `${temp}, request money` : 'request money'
        } 
    });
    return temp;
}, [moneyRequestOptions])

we need to change to dynamic text

    {(addedText) && (
                <Text>{props.translate('reportActionsView.usePlusButton', {addedText})}</Text>
     )}

What alternative solutions did you explore? (Optional)

https://github.com/Expensify/App/assets/65789015/1ce42301-a75c-42eb-8b2e-cb5b0b6cd5ad

Ollyws commented 9 months ago

I like @ZhenjaHorbach's proposal but better to go with We can move this logic inside ReportWelcomeText and pass only additional text instead options as suggested in the comment. 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 9 months ago

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

yh-0218 commented 9 months ago

hi, @Ollyws Did you check my proposal. I think I am first on here. I hope you check edited date. Thanks.

ZhenjaHorbach commented 9 months ago

@yh-0218 I don't agree) In the original proposal I suggested using array from ReportUtils.getMoneyRequestOptions(props.report, participantAccountIDs);

And later wrote the example how we can make it Which looks completely different

Ollyws commented 9 months ago

@yh-0218 It seems @ZhenjaHorbach posted the first implementation suggesting custom translate params about an hour before yours, as far as I can see.

yh-0218 commented 9 months ago

@Ollyws I had taken much time to test all and create correct proposal. If we are possible edit, then I will do from next with that way. I think if anyone did edit, then must sent update proposal message when he edit.

situchan commented 9 months ago

Agree. Here's example. Just posting here based on https://github.com/Expensify/App/issues/15486#issuecomment-1480395027

ZhenjaHorbach commented 9 months ago

@yh-0218 I think it's good practice to leave comments. Next time I'll do that But at the time of writing your comment, I already suggested using ReportUtils.getMoneyRequestOptions(props.report, participantAccountIDs) array to generate translation And my changes were related only to adding details And my solution also works correct )

miljakljajic commented 9 months ago

@Ollyws what are your thoughts - will we move forward with @ZhenjaHorbach? Let me know and I'll assign/send a contract.

Ollyws commented 9 months ago

@miljakljajic Yeah they were first to provide an adequate solution.

yh-0218 commented 9 months ago

I am not sure why this is correct solution.

    const reportParticipantIDs = _.without(lodashGet(props.report, 'participantAccountIDs', []), props.personalDetails.accountID);

   conts possibleFunctions = ReportUtils.getMoneyRequestOptions(props.report, reportParticipantIDs)

melvin-bot[bot] commented 9 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 9 months ago

@puneetlath, @Ollyws, @miljakljajic Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 9 months ago

📣 @Ollyws 🎉 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 9 months ago

📣 @ZhenjaHorbach 🎉 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 📖

miljakljajic commented 9 months ago

Thank you @Ollyws and @ZhenjaHorbach - go ahead and accept those offers and we'll move forward :)

ZhenjaHorbach commented 9 months ago

Thank you @Ollyws and @ZhenjaHorbach - go ahead and accept those offers and we'll move forward :)

Pr will be ready today

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

https://github.com/Expensify/App/pull/33520

If no regressions arise, payment will be issued on 2024-01-17. :confetti_ball:

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

@Ollyws requires payment automatic offer (Reviewer)
@ZhenjaHorbach requires payment automatic offer (Contributor)

melvin-bot[bot] commented 8 months 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:

[ ] [@Ollyws] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@Ollyws] 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:
[ ] [@Ollyws] 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:
[ ] [@Ollyws] Determine if we should create a regression test for this bug.
[ ] [@Ollyws] 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.
[ ] [@miljakljajic] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

miljakljajic commented 8 months ago

@Ollyws can you accept the offer? @ZhenjaHorbach , paid!

Ollyws commented 8 months ago

BugZero Checklist:

[x] The PR that introduced the bug has been identified. Link to the PR:

We've been using a static message for this since it was implemented so I don't think we can really pin it on a PR, it's more of a new feature.

[x] 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:

N/A

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

Yes.

[x] 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

1. Open the app
2. Open any workspace chat and scroll on top
3. Observe the text 'You can also use the + button to send money, request money, or assign a task!' has synchronized with available money options in + menu

Do we agree 👍 or 👎

Expensify / App

[HOLD for payment 2024-01-17] [$500] Chat - Send money text displayed in workspace chat #33003

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

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?

What alternative solutions did you explore? (Optional)

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?

What alternative solutions did you explore? (Optional)

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?

What alternative solutions did you explore? (Optional)