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 2023-02-24] New message green line appears after a brief moment when opening the app from background #14520

Closed
1 task
kavimuru opened this issue Jan 24, 2023 · 23 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Jan 24, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to any chat
  2. Mark as read first message then swipe from bottom then open the app again.
  3. Notice that mark as read line disappears then appears again.

Expected Result:

Mark as read line should consistently.

Actual Result:

Mark as read line disappears then appears again.

Workaround:

unknown

Platforms:

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

  • iOS / native

Version Number: 1.2.58-3
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
Notes/Photos/Videos:

MZBZ5035.MP4
RPReplay_Final1674551444.MP4

Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674552168801719

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ecfec059338b828f
  • Upwork Job ID: 1623441417954881536
  • Last Price Increase: 2023-02-08
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 24, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 24, 2023
@MelvinBot
Copy link

@puneetlath this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Feb 7, 2023
@MelvinBot
Copy link

@puneetlath 12 days overdue now... This issue's end is nigh!

@puneetlath
Copy link
Contributor

I think this likely needs to be Internal.

@melvin-bot melvin-bot bot removed the Overdue label Feb 8, 2023
@puneetlath puneetlath added the Internal Requires API changes or must be handled by Expensify staff label Feb 8, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 8, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Triggered auto assignment to Contributor Plus for review of internal employee PR - @eVoloshchak (Internal)

@gedu
Copy link
Contributor

gedu commented Feb 9, 2023

Hi, Eduardo from Callstack, can I take this one?

@mountiny
Copy link
Contributor

mountiny commented Feb 9, 2023

@gedu as mentioned in Slack feel free to give this a try but there is a chance it requires backend changes, nevertheless post your findings!

@gedu
Copy link
Contributor

gedu commented Feb 10, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.
After marking a chat message as unread, when you put the app into the background and open it again, the new green line hide and shows up again

What is the root cause of that problem?
In ReportsActionView we are subscribing to Visibility.onVisibilityChange and as soon the app becomes visible we are cleaning the newMarkerReportActionID, when that happens the new green line disappears, and later when the list is fully visible, it checks if was manually marked didManuallyMarkReportAsUnread and it shows up the green line again

What changes do you think we should make in order to solve the problem?
In ReportsActionView we are subscribing to Visibility.onVisibilityChange check if still unread before clearing the newMarkerReportActionID instead of assuming that the user already read the message

Screen Shot 2023-02-10 at 11 41 56

greenLine-aftervisible.mp4
greenLine-aftervisible-hide.mp4

What alternative solutions did you explore? (Optional)
Maybe instead of using a ReportActionID using the lastReadTime, so we avoid to use kind of 2 properties to mark a message as unread, this will require a lot of code changes and testing to avoid regression bugs or breaking the working features

@puneetlath
Copy link
Contributor

Ah yes that is interesting. The comment says "If the app user becomes active and they have no unread actions" but then we don't actually check whether or not the chat is unread before clearing the marker. That solution sounds good to me. @eVoloshchak what do you think?

@melvin-bot melvin-bot bot added the Overdue label Feb 13, 2023
@mountiny
Copy link
Contributor

@gedu I think this solution makes sense, thank you, I would say you can go ahead and create a PR for this. You got two approvals 🙌

@melvin-bot melvin-bot bot added Reviewing Has a PR in review and removed Overdue labels Feb 13, 2023
@gedu
Copy link
Contributor

gedu commented Feb 13, 2023

I have read the CLA Document and I hereby sign the CLA

francoisl added a commit that referenced this issue Feb 15, 2023
This is a follow-up of #14520 / #15097, where we changed the app's
behavior to keep the unread indicator present after backgrounding
and reopening the app.
@MelvinBot
Copy link

@puneetlath, @gedu, @eVoloshchak Whoops! This issue is 2 days overdue. Let's get this updated quick!

@eVoloshchak
Copy link
Contributor

Not overdue, PR was deployed to production 3 days ago

@mountiny mountiny changed the title New message green line appears after a brief moment when opening the app from background [HOLD for payment 2023-02-24] New message green line appears after a brief moment when opening the app from background Feb 22, 2023
@mountiny mountiny added the Awaiting Payment Auto-added when associated PR is deployed to production label Feb 22, 2023
@mountiny
Copy link
Contributor

Updated the title to reflect this.

@puneetlath
Copy link
Contributor

@eVoloshchak sent you a contract offer.

@eVoloshchak
Copy link
Contributor

Offer accepted👌

@puneetlath
Copy link
Contributor

All done. Thanks everyone!

@hungvu193
Copy link
Contributor

@puneetlath please invite me, I'm the reporter

@eVoloshchak
Copy link
Contributor

eVoloshchak commented Feb 26, 2023

@puneetlath, the PR was merged within 2 business days. I didn't review proposals, only the PR, but it looks like @gedu might be eligible for the bonus

josemak25 pushed a commit to josemak25/expensify that referenced this issue Feb 26, 2023
This is a follow-up of Expensify#14520 / Expensify#15097, where we changed the app's
behavior to keep the unread indicator present after backgrounding
and reopening the app.
@mountiny
Copy link
Contributor

Just gonna reopen this so we dont forget to handle the reporting bonus

@mountiny mountiny reopened this Feb 27, 2023
@puneetlath
Copy link
Contributor

puneetlath commented Feb 27, 2023

@puneetlath, the PR was merged within 2 business days. I didn't review proposals, only the PR, but it looks like @gedu might be eligible for the bonus

@eVoloshchak in this case, this was handled by a vendor contributor from Callstack, so the process is more similar to our Internal PR process.

@hungvu193 sorry for forgetting about you! Can you apply here: https://www.upwork.com/jobs/~01ecfec059338b828f

For me: https://www.upwork.com/ab/applicants/1623441417954881536/applicants

@hungvu193
Copy link
Contributor

@puneetlath, the PR was merged within 2 business days. I didn't review proposals, only the PR, but it looks like @gedu might be eligible for the bonus

@eVoloshchak in this case, this was handled by a vendor contributor from Callstack, so the process is more similar to our Internal PR process.

@hungvu193 sorry for forgetting about you! Can you apply here: https://www.upwork.com/jobs/~01ecfec059338b828f

For me: https://www.upwork.com/ab/applicants/1623441417954881536/applicants

Thank you, I've just applied!

@puneetlath
Copy link
Contributor

Paid! Should actually be done now. Thanks everyone!

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. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants