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.56k stars 2.9k forks source link

[$500] IOU - Description, merchant, date order changed after creating split bill in group chat. #34566

Closed kbecciv closed 10 months ago

kbecciv commented 10 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.25-4 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:

  1. Launch app
  2. Tap on a group chat
  3. Tap plus icon near compose
  4. Tap split bill
  5. Enter an amount and tap next
  6. Tap show more & enter description, merchant
  7. Note the order of display of description, merchant and date
  8. Tap split
  9. Tap the split created to open details page 10.Tap show more
  10. Note the order of display changed to description, date and merchant

Expected Result:

Description, merchant, date order must not be changed after creating split bill in group chat.

Actual Result:

Description, merchant, date order changed after creating split bill in group chat.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/5bb3e836-c25a-4f5b-856e-fc010e615528

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bf7e0226a4739f95
  • Upwork Job ID: 1747238369577791488
  • Last Price Increase: 2024-01-16
melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

abzokhattab commented 10 months ago

Proposal

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

IOU - Description, merchant, date order changed after creating split bill in group chat

What is the root cause of that problem?

Date is switched with the merchant in this componet: https://github.com/Expensify/App/blob/61c7e0bcddac907b50e3600ff651655c5eb8d2e1/src/components/MoneyRequestConfirmationList.js#L666-L685

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

Date should be move to the button of the component to be the last item:

details ```js {shouldShowAllFields && ( <> {props.isDistanceRequest && ( Navigation.navigate(ROUTES.MONEY_REQUEST_DISTANCE.getRoute(props.iouType, props.reportID))} disabled={didConfirm || !isTypeRequest} interactive={!props.isReadOnly} /> )} {shouldShowMerchant && ( { if (props.isEditingSplitBill) { Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(props.reportID, props.reportActionID, CONST.EDIT_REQUEST_FIELD.MERCHANT)); return; } Navigation.navigate(ROUTES.MONEY_REQUEST_MERCHANT.getRoute(props.iouType, props.reportID)); }} disabled={didConfirm} interactive={!props.isReadOnly} brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} error={shouldDisplayMerchantError || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''} /> )} {shouldShowCategories && ( { if (props.isEditingSplitBill) { Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(props.reportID, props.reportActionID, CONST.EDIT_REQUEST_FIELD.CATEGORY)); return; } Navigation.navigate(ROUTES.MONEY_REQUEST_CATEGORY.getRoute(props.iouType, props.reportID)); }} style={[styles.moneyRequestMenuItem]} titleStyle={styles.flex1} disabled={didConfirm} interactive={!props.isReadOnly} rightLabel={canUseViolations && Boolean(props.policy.requiresCategory) ? translate('common.required') : ''} /> )} {shouldShowTags && ( { if (props.isEditingSplitBill) { Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(props.reportID, props.reportActionID, CONST.EDIT_REQUEST_FIELD.TAG)); return; } Navigation.navigate(ROUTES.MONEY_REQUEST_TAG.getRoute(props.iouType, props.reportID)); }} style={[styles.moneyRequestMenuItem]} disabled={didConfirm} interactive={!props.isReadOnly} rightLabel={canUseViolations && Boolean(props.policy.requiresTag) ? translate('common.required') : ''} /> )} {shouldShowTax && ( Navigation.navigate( ROUTES.MONEY_REQUEST_STEP_TAX_RATE.getRoute(props.iouType, props.transaction.transactionID, props.reportID, Navigation.getActiveRouteWithoutParams()), ) } disabled={didConfirm} interactive={!props.isReadOnly} /> )} {shouldShowTax && ( Navigation.navigate( ROUTES.MONEY_REQUEST_STEP_TAX_AMOUNT.getRoute(props.iouType, props.transaction.transactionID, props.reportID, Navigation.getActiveRouteWithoutParams()), ) } disabled={didConfirm} interactive={!props.isReadOnly} /> )} {shouldShowBillable && ( {translate('common.billable')} )} {shouldShowDate && ( { if (props.isEditingSplitBill) { Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(props.reportID, props.reportActionID, CONST.EDIT_REQUEST_FIELD.DATE)); return; } Navigation.navigate(ROUTES.MONEY_REQUEST_DATE.getRoute(props.iouType, props.reportID)); }} disabled={didConfirm} interactive={!props.isReadOnly} brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isCreatedMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} error={shouldDisplayFieldError && TransactionUtils.isCreatedMissing(transaction) ? translate('common.error.enterDate') : ''} /> )} )} ```

OR We can just put the date after the merchant item to be in sync with the other confrimationlist:

details ```js {props.isDistanceRequest && ( Navigation.navigate(ROUTES.MONEY_REQUEST_DISTANCE.getRoute(props.iouType, props.reportID))} disabled={didConfirm || !isTypeRequest} interactive={!props.isReadOnly} /> )} {shouldShowMerchant && ( { if (props.isEditingSplitBill) { Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(props.reportID, props.reportActionID, CONST.EDIT_REQUEST_FIELD.MERCHANT)); return; } Navigation.navigate(ROUTES.MONEY_REQUEST_MERCHANT.getRoute(props.iouType, props.reportID)); }} disabled={didConfirm} interactive={!props.isReadOnly} brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} error={shouldDisplayMerchantError || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''} /> )} {shouldShowDate && ( { if (props.isEditingSplitBill) { Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(props.reportID, props.reportActionID, CONST.EDIT_REQUEST_FIELD.DATE)); return; } Navigation.navigate(ROUTES.MONEY_REQUEST_DATE.getRoute(props.iouType, props.reportID)); }} disabled={didConfirm} interactive={!props.isReadOnly} brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isCreatedMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} error={shouldDisplayFieldError && TransactionUtils.isCreatedMissing(transaction) ? translate('common.error.enterDate') : ''} /> )} ```

Result:

image
DylanDylann commented 10 months ago

Proposal

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

Description, merchant, date order changed after creating split bill in group chat.

What is the root cause of that problem?

The order in 2 places are different https://github.com/Expensify/App/blob/2f1b770016a185d4e9d444f2e8aaa1759ee7e1b3/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L744-L780

https://github.com/Expensify/App/blob/2f1b770016a185d4e9d444f2e8aaa1759ee7e1b3/src/components/MoneyRequestConfirmationList.js#L666-L684

In the Confirmation Page, we migrate to use the new component MoneyTemporaryForRefactorRequestConfirmationList instead of MoneyRequestConfirmationList but in Split Detail Page we still use old component MoneyRequestConfirmationList , It caused inconsistency.

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

I suggest in the split bill page we also should use MoneyTemporaryForRefactorRequestConfirmationList instead of MoneyRequestConfirmationList

What alternative solutions did you explore? (Optional)

NA

rushatgabhane commented 10 months ago

Yes, looks like we created a temporary copy of MoneyRequestConfirmationList to avoid merge conflicts. https://github.com/Expensify/App/pull/28618

rushatgabhane commented 10 months ago

@tgolen im trying to gain clarity here. In https://github.com/Expensify/App/pull/28618 we created a temporary copy of MoneyRequestConfirmationList.

But MoneyRequestConfirmationList and MoneyTemporaryForRefactorRequestConfirmationList are out of sync now, it is causing this inconsistency "bug".

I'm sure there is a plan to sync them when refactor is done. Could you please let us know if we should hold this issue or proceed with fixing it? Thank you so much 🙇

aimane-chnaif commented 10 months ago

yes, this will be handled in #29107

sync
rushatgabhane commented 10 months ago

Thank you so much @aimane-chnaif!

@lschurr let's close this issue :)

lschurr commented 10 months ago

Great, thanks!