Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-01-18] [$500] Web - IOU - Clicking on blank space next to 'From' link navigates user to main report #33907

Closed
1 of 6 tasks
kbecciv opened this issue Jan 3, 2024 · 26 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jan 3, 2024

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.21-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Create an IOU report
  2. Click on the IOU preview component
  3. Click on a blank space next to from link of the IOU report header

Expected Result:

RHP should be opened and 'Details' page should be be displayed as it does for task report

Actual Result:

User is navigated to main report

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

Bug6331627_1704312285513.Screen_Recording_2024-01-03_at_9.02.31_at_night.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d27b574e0b4def7f
  • Upwork Job ID: 1742652043004137472
  • Last Price Increase: 2024-01-03
  • Automatic offers:
    • dukenv0307 | Contributor | 28089418
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 3, 2024
@melvin-bot melvin-bot bot changed the title Web - IOU - Clicking on blank space next to 'From' link navigates user to main report [$500] Web - IOU - Clicking on blank space next to 'From' link navigates user to main report Jan 3, 2024
Copy link

melvin-bot bot commented Jan 3, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 3, 2024
Copy link

melvin-bot bot commented Jan 3, 2024

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

Copy link

melvin-bot bot commented Jan 3, 2024

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

Copy link

melvin-bot bot commented Jan 3, 2024

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

@neonbhai
Copy link
Contributor

neonbhai commented Jan 3, 2024

Proposal

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

Clicking on blank space next to 'From' link navigates user to main report

What is the root cause of that problem?

This happens as the ParentNavigationSubtitle component takes the full space and the component is wrapped in PressableWithoutFeedback:

return (
<PressableWithoutFeedback
onPress={() => {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(parentReportID));
}}
accessibilityLabel={translate('threads.parentNavigationSummary', {rootReportName, workspaceName})}
role={CONST.ROLE.LINK}
style={pressableStyles}
>
<Text
style={[styles.optionAlternateText]}
numberOfLines={1}
>
<Text style={[styles.optionAlternateText, styles.textLabelSupporting]}>{`${translate('threads.from')} `}</Text>
<Text style={[styles.optionAlternateText, styles.textLabelSupporting, styles.link]}>{rootReportName}</Text>
{Boolean(workspaceName) && <Text style={[styles.optionAlternateText, styles.textLabelSupporting]}>{` ${translate('threads.in')} ${workspaceName}`}</Text>}
</Text>
</PressableWithoutFeedback>
);

The problem is, link style is applied here, but the navigation behavior wraps the whole row, making clicks on the whole line navigate back.

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

We can change the ParentNavigationSubtitle component to only navigate when the link text here is pressed:

<Text style={[styles.optionAlternateText, styles.textLabelSupporting, styles.link]}>{rootReportName}</Text>

We would use the TextLink component:

<TextLink
    onPress={() => { 
             Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(parentReportID)); 
         }} 
    style={[styles.optionAlternateText, styles.textLabelSupporting, styles.link]}
>
    {rootReportName}
</TextLink>

Alternatively

  • We can add flexShrink to button style so it only takes as much space as necessary

  • We can also change the HeaderView style here by adding flexShrink

@aim-salam
Copy link

aim-salam commented Jan 4, 2024

Proposal

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

Web - IOU - Clicking on blank space next to 'From' link navigates user to main report

What is the root cause of that problem?

ParentNavigationSubtitle follow parent component's (AvatarWithDisplayName) layout style.

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

add alignSelf:'flex-start' @ styles.alignSelfStart into ParentNavigationSubtitle style , so it out will be out of context.

style={pressableStyles}

style={[pressableStyles,styles.alignSelfStart]}
Screen.Recording.2024-01-04.at.8.11.01.AM.mov

What alternative solutions did you explore? (Optional)

@dukenv0307
Copy link
Contributor

Proposal

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

The user is navigated to main report when click on the blank space in IOU report header.

What is the root cause of that problem?

In HeaderView, we pass pressableStyles with alignSelfStart which makes ParentNavigationSubtitle only takes the enough space.

pressableStyles={[styles.alignSelfStart, styles.mw100]}

But In MoneyReportHeader, we display ParentNavigationSubtitle in AvatarWithDisplayName and we don't pass this style as we do in HeaderView so the ParentNavigationSubtitle takes the full space by default

<ParentNavigationSubtitle

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

We should pass pressableStyles={[styles.alignSelfStart, styles.mw100]} as we do in HeaderView to be consistent the width of ParentNavigationSubtitle in the header of report screen

<ParentNavigationSubtitle

What alternative solutions did you explore? (Optional)

We can add styles.alignSelfStart, styles.mw100 this style in ParentNavigationSubtitle so it always takes the enough space by default.

Result

Screen.Recording.2024-01-04.at.11.53.42.mov

@dragnoir
Copy link
Contributor

dragnoir commented Jan 4, 2024

Proposal

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

Currently, users are unable to interact with the money request header to view its details. Instead of opening the details, clicking on the header redirects users back to the previous page.

What is the root cause of that problem?

Upon examination, it is observed that the discrepancy arises due to the utilization of different components for the report page header and the money request details page. The ReportScreen.js employs the <HeaderView> component, which allows users to click and open the details in a side window. This functionality is implemented through the PressableWithoutFeedback wrapper.

However, the money request page uses the <MoneyRequestHeader> component, which lacks this click-to-open feature. This component is currently not enclosed within a PressableWithoutFeedback or similar wrapper that would facilitate the opening of detail windows upon interaction.

Reference to the current implementation can be found here:

let headerView = (
<HeaderView
reportID={reportID}
onNavigationMenuButtonClicked={goBack}
personalDetails={personalDetails}
report={report}
/>
);
if (isSingleTransactionView) {
headerView = (
<MoneyRequestHeader
report={report}
policy={policy}
personalDetails={personalDetails}
isSingleTransactionView={isSingleTransactionView}
parentReportAction={parentReportAction}
/>
);
}

The screenshot attached below highlights the difference in implementation. The <HeaderView> is nested within a PressableWithoutFeedback, enabling the opening of detail pages, which is not the case with <MoneyRequestHeader>.

image

<PressableWithoutFeedback
onPress={() => ReportUtils.navigateToDetailsPage(props.report)}

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

To resolve this issue, the <MoneyRequestHeader> should be wrapped within a PressableWithoutFeedback component, mirroring the implementation found in <HeaderView>. This modification will enable the money request header to function interactively, allowing users to access details with a click, enhancing the user experience.

@suneox
Copy link
Contributor

suneox commented Jan 4, 2024

Proposal

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

Web - IOU - Clicking on blank space next to 'From' link navigates user to main report

What is the root cause of that problem?

At the parent group of DisplayNames and subtite, we define flex column but we don't define items start and the default value is stretch for children so when we click on blank space we will click on the children element and navigate follow children link.

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

At the parent group we will define items start direction

Just add styles.alignItemsStart to this line to

  <View style={[styles.flex1, styles.flexColumn, styles.alignItemsStart, shouldShowSubscriptAvatar && !isExpenseRequest ? styles.ml4 : {}]}>

POC

33907-POC.mp4

What alternative solutions did you explore? (Optional)

@garrettmknight
Copy link
Contributor

@thesahindia can you give these a look when you get a chance?

@melvin-bot melvin-bot bot added the Overdue label Jan 8, 2024
@garrettmknight
Copy link
Contributor

@thesahindia quick bump to review these. Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Jan 8, 2024
@thesahindia
Copy link
Member

@dukenv0307's proposal looks good to me! Text will overflow if we don't set the max width to 100.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Jan 8, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 8, 2024
Copy link

melvin-bot bot commented Jan 8, 2024

📣 @dukenv0307 🎉 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 melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 9, 2024
@dukenv0307
Copy link
Contributor

@thesahindia The PR is ready for review.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 11, 2024
@melvin-bot melvin-bot bot changed the title [$500] Web - IOU - Clicking on blank space next to 'From' link navigates user to main report [HOLD for payment 2024-01-18] [$500] Web - IOU - Clicking on blank space next to 'From' link navigates user to main report Jan 11, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 11, 2024
Copy link

melvin-bot bot commented Jan 11, 2024

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

Copy link

melvin-bot bot commented Jan 11, 2024

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

If no regressions arise, payment will be issued on 2024-01-18. 🎊

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

Copy link

melvin-bot bot commented Jan 11, 2024

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:

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jan 17, 2024
@garrettmknight
Copy link
Contributor

Summary of payments:

@garrettmknight
Copy link
Contributor

Dropping to weekly for @thesahindia to suggest a regression test and request payment.

@garrettmknight garrettmknight added Weekly KSv2 and removed Daily KSv2 labels Jan 18, 2024
@thesahindia
Copy link
Member

Test steps -

  1. Create a money request
  2. Click on the report preview to open the IOU report
  3. Click on the blank space of the parent navigation subtitle in the IOU header
  4. Verify that the detail page appears
  5. Click on the parent navigation subtitle and verify that we go back to the parent report

@melvin-bot melvin-bot bot added the Overdue label Feb 1, 2024
@luacmartins
Copy link
Contributor

Test steps look good to me

@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2024
@garrettmknight
Copy link
Contributor

QA updated - on you to request payment @thesahindia

@thesahindia
Copy link
Member

I will send the request soon. We can close it.

@melvin-bot melvin-bot bot added the Overdue label Feb 15, 2024
@luacmartins
Copy link
Contributor

Closing as per previous comment!

@melvin-bot melvin-bot bot removed the Overdue label Feb 20, 2024
@JmillsExpensify
Copy link

$500 approved for @thesahindia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants