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

[lanitochka17]

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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6512799_1718348053624.bandicam_2024-06-14_14-53-23-432.mp4

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]

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]

@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]

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.

App/src/components/MoneyRequestConfirmationList.tsx

Lines 1073 to 1080 in 2e85794

	<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)}
	/>
	</View>

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

billable_lock_icon.mp4

[Krishna2323]

Proposal Updated

• Updated main solution and added alternative.

[melvin-bot]

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

[melvin-bot]

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

[cretadn22]

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

App/src/components/MoneyRequestConfirmationList.tsx

Lines 1075 to 1079 in bdb9789

	<Switch
	accessibilityLabel={translate('common.billable')}
	isOn={iouIsBillable}
	onToggle={(isOn) => onToggleBillable?.(isOn)}
	/>

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

Simply adding disabled={isReadOnly} to the MoneyRequestConfirmationList Component

App/src/components/MoneyRequestConfirmationList.tsx

Lines 1075 to 1079 in bdb9789

	<Switch
	accessibilityLabel={translate('common.billable')}
	isOn={iouIsBillable}
	onToggle={(isOn) => onToggleBillable?.(isOn)}
	/>

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

Ghi.Man.hinh.2024-06-15.luc.01.54.49.mov

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

App/src/components/MoneyRequestConfirmationList.tsx

Lines 1075 to 1079 in bdb9789

	<Switch
	accessibilityLabel={translate('common.billable')}
	isOn={iouIsBillable}
	onToggle={(isOn) => onToggleBillable?.(isOn)}
	/>

App/src/components/ReportActionItem/MoneyRequestView.tsx

Lines 561 to 566 in bdb9789

	<Switch
	accessibilityLabel={translate('common.billable')}
	isOn={!!transactionBillable}
	onToggle={saveBillable}
	disabled={!canEdit}
	/>

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]

Proposal Updated

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

[melvin-bot]

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

[ishpaul777]

@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]

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

[melvin-bot]

📣 @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]

📣 @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]

@ishpaul777, PR ready for review.

[melvin-bot]

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

[melvin-bot]

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

• fix: Billable - Billable toggle should have a locked icon in split details view. #43889

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

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]

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]

@Krishna2323 has been paid.

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

[melvin-bot]

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

• fix: Billable - Billable toggle should have a locked icon in split details view. #43889

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

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]

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]

not overdue, will complete Checklist soon

[puneetlath]

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

[ishpaul777]

[@ishpaul777] The PR that introduced the bug has been identified. Link to the PR: #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]

@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]

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]

update above ^ melvin

[ishpaul777]

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

[puneetlath]

Paid!