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 of payment 8-27][$250] Cannot scroll down after comment linking #46217

Closed
1 of 6 tasks
janicduplessis opened this issue Jul 25, 2024 · 29 comments
Closed
1 of 6 tasks
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 Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review

Comments

@janicduplessis
Copy link
Contributor

janicduplessis commented Jul 25, 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: 9.0.12-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
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: @janicduplessis
Slack conversation:

Action Performed:

  • In a chat with many messages copy the link to a message and send it
  • Clear the app cache (Troubleshoot -> Clear cache and restart)
  • Click on the message link from the first step
  • Cannot scroll down

Expected Result:

Can scroll down after linking to message

Actual Result:

Cannot scroll down

Workaround:

Scroll up, then scroll back down

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

Screen.Recording.2024-07-25.at.14.07.28.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016056b96e55e3a838
  • Upwork Job ID: 1817986329686132368
  • Last Price Increase: 2024-07-29
Issue OwnerCurrent Issue Owner: @
@janicduplessis janicduplessis added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jul 25, 2024
Copy link

melvin-bot bot commented Jul 25, 2024

Triggered auto assignment to @strepanier03 (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.

@janicduplessis
Copy link
Contributor Author

The issue seems to be related to this code, which prevents newer messages from being rendered. I am looking with the original author of this code to see why it was needed and if it can be removed.

@janicduplessis
Copy link
Contributor Author

I have a potential fix, but first need to figure out why this code was needed in the first place and if would cause a regression to remove it.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Jul 26, 2024

Looks like this is the same report as - #46222

@janicduplessis looks like you the author of the PR can @ishpaul777 be the C+?

@ishpaul777
Copy link
Contributor

ishpaul777 commented Jul 26, 2024

Yes i can review the PR, but I believe the root cause for both issues are different though we can try solving in a single PR.

Oh i see both issues have a draft PR and has different solutions, #46221 for this and #46225 for #46222

@janicduplessis
Copy link
Contributor Author

@ishpaul777 Would it be easier for you if I create a single PR that fixes all the issues?

Right now I identified 4 bugs with comment linking that all cause some slightly different issues. They all have different root cause so code wise they can be isolated, but I think it might be easier for QA to just test with all the fixes applied to see that it is now smooth.

@ishpaul777
Copy link
Contributor

It makes sense to fix in a single PR 👍 lets do it 🚀

@melvin-bot melvin-bot bot added the Overdue label Jul 29, 2024
@Christinadobrzyn
Copy link
Contributor

hi! I'm gonna add @ishpaul777 to this since both this GH and #46222 (comment) will be resolved together.

@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2024
@Christinadobrzyn Christinadobrzyn added External Added to denote the issue can be worked on by a contributor Overdue labels Jul 29, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

Job added to Upwork: https://www.upwork.com/jobs/~016056b96e55e3a838

@melvin-bot melvin-bot bot changed the title Cannot scroll down after comment linking [$250] Cannot scroll down after comment linking Jul 29, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 29, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

Current assignee @ishpaul777 is eligible for the External assigner, not assigning anyone new.

@Christinadobrzyn
Copy link
Contributor

@strepanier03 I'm happy to take this GH if that would be easier for tracking!

@strepanier03
Copy link
Contributor

@Christinadobrzyn - Sounds good to me. I haven't looked at it until now anyway. I'll assign you, thanks!

@Christinadobrzyn
Copy link
Contributor

Awesome! Sounds good Sheena!

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Jul 31, 2024

Correct me if I'm wrong but the PR for this is here - #46222 (comment), right? @ishpaul777 @janicduplessis

@janicduplessis
Copy link
Contributor Author

Yes, I consolidated the fixes around comment linking in this PR. Currently looking into janky scroll on android which I think I just found the problem, then PR should be ready.

@Christinadobrzyn Christinadobrzyn changed the title [$250] Cannot scroll down after comment linking [HOLD for 46222] [$250] Cannot scroll down after comment linking Aug 1, 2024
@Christinadobrzyn Christinadobrzyn added Weekly KSv2 and removed Daily KSv2 labels Aug 1, 2024
@Christinadobrzyn
Copy link
Contributor

I'm adding a HOLD on this because the PR is here #46222 so I think this issue can just be tested once the PR is deployed and we can make sure it's resolved too.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Aug 5, 2024
@Christinadobrzyn
Copy link
Contributor

monitoring the PR - #46315

1 similar comment
@Christinadobrzyn
Copy link
Contributor

monitoring the PR - #46315

Copy link

melvin-bot bot commented Aug 16, 2024

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

@bondydaa
Copy link
Contributor

not sure why I got assigned, looks like @roryabraham is assigned on the PR so going to move it over to you.

@bondydaa bondydaa assigned roryabraham and unassigned bondydaa Aug 16, 2024
@Christinadobrzyn
Copy link
Contributor

PR is in staging - almost there - #46315

@roryabraham
Copy link
Contributor

roryabraham commented Aug 20, 2024

PR deployed to prod

@ishpaul777
Copy link
Contributor

This is ready for payment 🎉

@Christinadobrzyn Christinadobrzyn changed the title [HOLD for 46222] [$250] Cannot scroll down after comment linking [Hold of payment 8-27][$250] Cannot scroll down after comment linking Aug 26, 2024
@Christinadobrzyn Christinadobrzyn added Daily KSv2 and removed Weekly KSv2 labels Aug 26, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Aug 26, 2024

Awesome! Looks like the 7th day of prod is tomorrow so let's prep for payment.

  • @janicduplessis you are with Margelo so paid separately, correct?
  • Do we need a regression test for this?

Payouts due:

@ishpaul777
Copy link
Contributor

Regression Test Proposal:

  1. In a chat with many messages (200 atleast) copy the link to a message and send it
  2. Clear the app cache (Troubleshoot -> Clear cache and restart)
  3. Click on the message link from the first step

Expected: User linked to correct message

  1. Try scrolling down to bottom of chat, verify new message loads as you scroll down.
  2. Return to LHN/switch to other chat and then switch again to chat in step 1, and repeat step 3, 4

Do we agree 👍 or 👎

@ishpaul777
Copy link
Contributor

Contributor+: $250 @ishpaul777 (upwork offer - https://www.upwork.com/nx/wm/offer/103686400)

@Christinadobrzyn seems the offer is same as #46222, this was a seprate issue with different root cause and solution, so i beleive payment should seperate

@janicduplessis
Copy link
Contributor Author

@Christinadobrzyn correct!

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Aug 27, 2024

Ah, sorry about that, @ishpaul777 - I paid you through the offer associated with this Upwork job #46217 (comment)

I'll create the regression test tomorrow

@Christinadobrzyn
Copy link
Contributor

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 Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

6 participants