[HOLD for payment 2024-07-31] [$250] Billable - Billable toggle should have a locked icon in split details view

lanitochka17 commented 3 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: 1.4.83-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: https://expensify.testrail.io/index.php?/tests/view/4631196 Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Billable option is set to default to billable on Old Dot
1. Go to staging.new.expensify.com
2. Go to workspace chat
3. Split a manual expense with billable toggled enabled
4. Click on the split preview
5. Click on the toggle

Expected Result:

Billable toggle should appear with a locked icon if billable toggle cannot be edited after splitting the bill

Actual Result:

The billable toggle does not have a locked icon, is clickable but not responsive

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/78819774/ec4cc55d-8a87-4aa8-807f-5018a0119fc1

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015cebf7a04f37f63d
Upwork Job ID: 1801675898344474551
Last Price Increase: 2024-06-14
Automatic offers:

ishpaul777 | Reviewer | 102769577
Krishna2323 | Contributor | 102769579

Issue Owner

Current Issue Owner: @ishpaul777

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 3 months ago

@puneetlath FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

Krishna2323 commented 3 months ago

Proposal

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

What is the root cause of that problem?

We haven't added the showLockIcon & disabled prop. https://github.com/Expensify/App/blob/2e857947c0b4b9a4c13f40e6e12c8bb6818d8cbf/src/components/MoneyRequestConfirmationList.tsx#L1073-L1080

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

Add showLockIcon & disabled prop.

<View style={[styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter, styles.ml5, styles.mr8, styles.optionRow]}>
    <Text color={!iouIsBillable ? theme.textSupporting : undefined}>{translate('common.billable')}</Text>
    <Switch
        accessibilityLabel={translate('common.billable')}
        isOn={iouIsBillable}
        onToggle={(isOn) => onToggleBillable?.(isOn)}
        showLockIcon={isReadOnly}
        disabled={isReadOnly}
    />
</View>

We can also modify Switch component to accept a new prop disabledStyle and pass it to PressableWithFeedback. This should be done incase we want to show the regular cursor instead of disabled one.

We should also check for other components using Switch which may have similar issue.

We might also want to update the text color based on the isReadOnly prop.

What alternative solutions did you explore? (Optional)

We can use ToggleSettingOptionRow.

Result

https://github.com/Expensify/App/assets/85894871/59619d6b-f103-4a3a-bb4e-6bac64a9eb5d

Krishna2323 commented 3 months ago

Proposal Updated

Updated main solution and added alternative.

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~015cebf7a04f37f63d

melvin-bot[bot] commented 3 months ago

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

cretadn22 commented 3 months ago

Proposal

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

Billable toggle should have a locked icon in split details view

What is the root cause of that problem?

SplitBillDetailsPage is using the MoneyRequestConfirmationList Component. However, the disabled property hasn't been included in the MoneyRequestConfirmationList Component. And since SplitBillDetailsPage doesn't provide the onToggleBillable function to MoneyRequestConfirmationList, it doesn't trigger any action when clicking toggle button

https://github.com/Expensify/App/blob/bdb978990df4a8e173b9790a2fa75c2a0f1d6be3/src/components/MoneyRequestConfirmationList.tsx#L1075-L1079

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

Simply adding disabled={isReadOnly} to the MoneyRequestConfirmationList Component

https://github.com/Expensify/App/blob/bdb978990df4a8e173b9790a2fa75c2a0f1d6be3/src/components/MoneyRequestConfirmationList.tsx#L1075-L1079

To clarify, the lock icon is displayed when either the disabled prop is true or the showLockIcon prop is true. The showLockIcon prop should only be used when we want to display the lock icon without disabling the toggle, as in this particular use case

https://github.com/Expensify/App/assets/168617111/a242ec42-16df-41ec-97f0-a8bb79557c22

Another consideration is that we've recently introduced the ToggleSettingOptionRow Component. We could also consider migrating these places to use the new component

https://github.com/Expensify/App/blob/bdb978990df4a8e173b9790a2fa75c2a0f1d6be3/src/components/MoneyRequestConfirmationList.tsx#L1075-L1079

https://github.com/Expensify/App/blob/bdb978990df4a8e173b9790a2fa75c2a0f1d6be3/src/components/ReportActionItem/MoneyRequestView.tsx#L561-L566

The current way seems fine to me. However, if we aim for consistency, there shouldn't be any issues. I've just tested it locally, and it's functioning smoothly.

Krishna2323 commented 3 months ago

Proposal Updated

Added back the reference code in RCA which was deleted mistakenly during an edit.

melvin-bot[bot] commented 3 months ago

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

ishpaul777 commented 3 months ago

@Krishna2323's Proposal LGTM! only thing is that we do not need to pass both showLockIcon & disabled prop only adding disabled should work. Migrating the component to ToggleSettingOptionRow would be nice-to-have 👍

🎀 👀 🎀

melvin-bot[bot] commented 3 months ago

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 3 months ago

📣 @ishpaul777 🎉 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 3 months ago

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

Krishna2323 commented 3 months ago

@ishpaul777, PR ready for review.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.1-19 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/43889

If no regressions arise, payment will be issued on 2024-07-02. :confetti_ball:

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

@Krishna2323 requires payment automatic offer (Contributor)
@ishpaul777 requires payment automatic offer (Reviewer)

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

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

puneetlath commented 2 months ago

@Krishna2323 has been paid.

@ishpaul777 bump on the checklist so we can pay you too.

melvin-bot[bot] commented 2 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.3-7 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/43889

If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:

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

@Krishna2323 requires payment automatic offer (Contributor)
@ishpaul777 requires payment automatic offer (Reviewer)

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

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

ishpaul777 commented 2 months ago

not overdue, will complete Checklist soon

puneetlath commented 2 months ago

@ishpaul777 please complete the checklist ASAP. I'd like to close this issue out.

ishpaul777 commented 2 months ago

[@ishpaul777] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/27172/ [@ishpaul777] 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/27172/files#r1679832517 [@ishpaul777] 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: No need [@ishpaul777] Determine if we should create a regression test for this bug. - Not required [@ishpaul777] 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

ishpaul777 commented 2 months ago

@puneetlath Please hold my payment until 7/22 i'll bump once i am ready to accept payment, we can move this to weekly for now

ishpaul777 commented 2 months ago

Sadly i'd still have to keep my payment on hold (Expected End of this month) i'll ping once i am ready to accept. Thanks for you patience.

ishpaul777 commented 2 months ago

update above ^ melvin

ishpaul777 commented 1 month ago

Thanks for holding the payment 🙇 , please release payment whenever you get the chance @puneetlath

puneetlath commented 1 month ago

Paid!

Expensify / App