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

[$500] Task - Chat is not scrolled to bottom when edit a task #38833

Closed
4 of 6 tasks
lanitochka17 opened this issue Mar 22, 2024 · 36 comments
Closed
4 of 6 tasks

[$500] Task - Chat is not scrolled to bottom when edit a task #38833

lanitochka17 opened this issue Mar 22, 2024 · 36 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Mar 22, 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.56-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: https://expensify.testrail.io/index.php?/runs/view/20318&group_by=cases:section_id&group_order=asc&group_id=291935
Email or phone of affected tester (no customers): shussain+web@applausemail.com
Issue reported by: Applause - Internal Team

Action Performed:

  1. Open an existing chat
  2. Click the '+' > Assign Task
  3. Give the task as Title, Description and Assignee > Confirm Task
  4. Click on the created task in the chat
  5. Change the name > Save
  6. Verify there's an audit trail of the change in the chat thread
  7. Make more changes to the task (you can change the title, description, assignee) > click Save
  8. Verify that eventually the chat isn't fully scrolled to the bottom of last message (or audit trail change)

Expected Result:

Chat should always scroll to the bottom of the last message ( you should see the last message after making a change)

Actual Result:

After making multiple edits to a task, the chat thread isn't scrolled to the bottom.

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

Bug6423531_1711133132795.2024-03-22_23-44-10.mp4
2024-03-26_12-57-52.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d64def74796adc96
  • Upwork Job ID: 1772488534557900800
  • Last Price Increase: 2024-03-26
  • Automatic offers:
    • fedirjh | Reviewer | 0
    • gijoe0295 | Contributor | 0
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 22, 2024
Copy link

melvin-bot bot commented Mar 22, 2024

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

@lanitochka17
Copy link
Author

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

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@gijoe0295
Copy link
Contributor

gijoe0295 commented Mar 22, 2024

Proposal

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

Chat is not scrolled to bottom when edit a task

What is the root cause of that problem?

Whenever there's any new report action in the report, we should call the notifyNewAction:

function notifyNewAction(reportID: string, accountID?: number, reportActionID?: string) {

to trigger scrollToBottom:

InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom());

That's what we've been doing for IOU requests (for eg here). But for edit task flow (editTask), we did not call it.

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

Call Report.notifyNewAction(report.reportID, currentUserAccountID) after editing the task in editTask.

We should also do it for whatever actions create new report action/message in the chat. For example: editTaskAssignee, reopenTask, completeTask.

What alternative solutions did you explore? (Optional)

NA

@bernhardoj

This comment was marked as outdated.

@gijoe0295
Copy link
Contributor

That's not correct @bernhardoj. This is a new behavior not on production and has different RCA with #38855.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Mar 25, 2024

Hum, I'm not able to reproduce this, it looks like the chat is at the bottom and the cursor is in the chat field when making edits to an assigned task.

2024-03-25_11-36-13.mp4

Asking QA for some clarification on the expected result vs actual result - https://expensify.slack.com/archives/C9YU7BX5M/p1711337939301019

@gijoe0295
Copy link
Contributor

gijoe0295 commented Mar 25, 2024

@Christinadobrzyn you need to scroll the task report up then make changes there. They expect that when there're new changes in that task report, it should scroll to bottom (just like regular chat report).

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Mar 26, 2024

Ah okay, I think I'm able to reproduce, I updated the OP so it's clearer how to reproduce. I think this can be external and part of vip-vsb

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Mar 26, 2024
@melvin-bot melvin-bot bot changed the title Task - Chat is not scrolled to bottom when edit a task [$500] Task - Chat is not scrolled to bottom when edit a task Mar 26, 2024
Copy link

melvin-bot bot commented Mar 26, 2024

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

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

melvin-bot bot commented Mar 26, 2024

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 26, 2024

Proposal

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

After making multiple edits to a task, the chat thread isn't scrolled to the bottom.

What is the root cause of that problem?

We don't call notifyNewAction after we edit the task.

function notifyNewAction(reportID: string, accountID?: number, reportActionID?: string) {
const actionSubscriber = newActionSubscribers.find((subscriber) => subscriber.reportID === reportID);

If it's currently a bug, it also happens when we edit a money request.

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

After edit a task or edit a money request, we should call notifyNewAction to scroll to bottom of the report.

function completeTask(taskReport: OnyxEntry<OnyxTypes.Report>) {

function reopenTask(taskReport: OnyxEntry<OnyxTypes.Report>) {

function editTask(report: OnyxTypes.Report, {title, description}: OnyxTypes.Task) {

function editTaskAssignee(

function deleteTask(report: OnyxEntry<OnyxTypes.Report>) {

function updateDistanceRequest(

function updateMoneyRequestDescription(

function updateMoneyRequestCategory(

function updateMoneyRequestTag(

function updateMoneyRequestMerchant(

function updateMoneyRequestBillable(

function updateMoneyRequestDate(

function updateMoneyRequestAmountAndCurrency(

function putOnHold(transactionID: string, comment: string, reportID: string) {

function unholdRequest(transactionID: string, reportID: string) {

What alternative solutions did you explore? (Optional)

NA

@0018akhil
Copy link

0018akhil commented Mar 26, 2024

Contributor details
Your Expensify account email: 0018akhil@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/akhilachary

I'm on Windows OS,
the issue is only happening in the staging environment.

Below is the video comparing both staging and the new website.

Scrollbar.Problem.mp4

I found this third-party cookies warning in the staging website this might be causing the issue(scrolling).

image

Copy link

melvin-bot bot commented Mar 26, 2024

📣 @0018akhil! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@0018akhil
Copy link

0018akhil commented Mar 26, 2024

Contributor details
Your Expensify account email: 0018akhil@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/akhilachary

Try pointing staging.new.expensify.com(staging website) to new.expensify.com URL temporarily, third party cookies warning will disappear and test it (scrollbar problem).

If you use Cloudflair this can be done easily.

Copy link

melvin-bot bot commented Mar 26, 2024

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

@fedirjh
Copy link
Contributor

fedirjh commented Mar 27, 2024

If it's currently a bug, it also happens when we edit a money request.

@nkdengineer Can you replicate this bug with the money request editing flow? I tried, but couldn't reproduce it.

@nkdengineer
Copy link
Contributor

@fedirjh The reason is here

useEffect(() => {
if (!isFocused || prevIsFocused || !ReportUtils.isChatThread(report) || report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
return;
}
Report.openReport(report.reportID);

The transaction thread report is a chat thread and If we don't add any comment in transaction thread report, the notification of this report is hidden. When we edit the money request, the report is unfocused and then it's focused again after we submit the edit. That makes this useEffect above is triggered when we edit the money request and then the chat scrolls to bottom.

I think it's another bug and we should add isSingleTransactionView check in this useEffect to prevent openReport is called.

To replicate this bug, we can send a message in this report and edit the money request again.

Screen.Recording.2024-03-27.at.13.12.19.mov

@0018akhil
Copy link

@nkdengineer
Is this issue happening only on the staging site?

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Mar 28, 2024

@0018akhil This appears to be happening in production too at https://new.expensify.com/

2024-03-28_12-59-29.mp4

@0018akhil
Copy link

0018akhil commented Mar 28, 2024

Hi @Christinadobrzyn . Okay

So your in Mac. Can you send me the screenshot of your console(any warnings).

@Christinadobrzyn
Copy link
Contributor

I see this in the console when changing the task not sure if those are helpful

image

@fedirjh
Copy link
Contributor

fedirjh commented Mar 29, 2024

I think it's another bug and we should add isSingleTransactionView check in this useEffect to prevent openReport is called.

@nkdengineer Thanks for confirming this. I was not able to replicate the bug with the money request edit flow. Hence, I don't think we should extend the fix to cover that flow.

@fedirjh
Copy link
Contributor

fedirjh commented Mar 29, 2024

I think we should proceed with this proposal from @gijoe0295. the solution looks good to me.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Mar 29, 2024

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 29, 2024

I was not able to replicate the bug with the money request edit flow. Hence, I don't think we should extend the fix to cover that flow.

To replicate this bug, we can send a message in this report and edit the money request again.

@fedirjh As I mentioned above, we can replicate this bug on latest main or staging without any code change by sending a message in transaction thread report before we edit the money request. I think we should fix all of them in an issue to verify that this bug doesn't happen in the further.

@gijoe0295
Copy link
Contributor

gijoe0295 commented Mar 29, 2024

Sorry for being late here.

We should also do it for whatever actions create new report action/message in the chat.

Whichever approach we proceed, they were already covered in my proposal. I think that finding these places are not difficult and not significant enough to count as a completely new proposal. It took more time to find the RCA than finding every places as the implementation is straightforward. Doing this would discourage contributors from investigating the issues and create precedence for tricks like extending former proposals to include "similar" cases. We have many jobs that finding all specific places to fix does not make a difference. We can always handle those in PR implementation.

@melvin-bot melvin-bot bot added the Overdue label Apr 1, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Apr 2, 2024

Not overdue,

@madmax330 can you review the proposal here - #38833 (comment)

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

melvin-bot bot commented Apr 2, 2024

📣 @fedirjh 🎉 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

Copy link

melvin-bot bot commented Apr 2, 2024

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

@gijoe0295
Copy link
Contributor

@Christinadobrzyn I think we're good for payment here because PR has been on production for nearly 10 days.

@Christinadobrzyn
Copy link
Contributor

Thanks @gijoe0295 - not sure why the payment auto-message didn't trigger for this.

Payouts due:

Upwork job is here.

@fedirjh or @gijoe0295 do we need a regression test for this?

@Christinadobrzyn Christinadobrzyn added Daily KSv2 and removed Weekly KSv2 labels Apr 22, 2024
@fedirjh
Copy link
Contributor

fedirjh commented Apr 22, 2024

@Christinadobrzyn It seems like this bug was discovered using a regression test. Please review the OP for more information:

If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/runs/view/20318&group_by=cases:section_id&group_order=asc&group_id=291935

@Christinadobrzyn
Copy link
Contributor

Ah thanks @fedirjh, if it was caught during a regression test then no payments should be made. I just requested a refund through Upwork for the payments sent earlier today. Let me know if I'm missing anything!

@fedirjh
Copy link
Contributor

fedirjh commented Apr 22, 2024

@Christinadobrzyn Sorry for the confusion. My above answer was related to this question :

do we need a regression test for this?

We don't need a regression test as we already have one listed in the OP.

if it was caught during a regression test, then no payments should be made

This is not the case for this issue. it is not caught during the regression period.

@Christinadobrzyn
Copy link
Contributor

Oh thank you for clarifying! Okay, gotcha!

Please ignore my refund requests. Closing since we don't need a regression. Let me know if I'm missing anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants