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

New Marker does not get updated properly while switching between same chat and chat list. #4357

Closed
3 tasks done
Santhosh-Sellavel opened this issue Aug 1, 2021 · 43 comments
Closed
3 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@Santhosh-Sellavel
Copy link
Collaborator

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


There are two issues but are similar and the same fix would solve both. So grouped as one.

Action Performed:

Issue 1

  1. Open any user chat.
  2. Tap the back arrow to navigate to the chat list.
  3. Now wait for a new message from the user.
  4. After receiving the message open the chat, the new marker is not shown.

Issue 2

  1. Open any other chat.
  2. Tap the back arrow to navigate to the chat list.
  3. Now wait for a new message from the user.
  4. After receiving the message open the chat, the new marker will be shown now.
  5. But go to the chat list, open chat again. The new marker will not hide.

Expected Result:

Issue 1

The new marker should be shown.

Issue 2

The new marker should be hidden while open the chat for the second time.

Actual Result:

Issue 1

The new marker is not shown.

New_Marker_Not_Shown_If_Wait_From_Same_Chat.mov

Issue 2

The new marker will not hide even closing and opening multiple times.

New_Marker_Will_Not_Hide_If_Switching_between_Same_Chat_And_List.mov

Workaround:

Can the user still use Expensify without this being fixed? Yes

Issue 1

Need wait from another user, then a new marker will be shown.

Issue 2

New marker only if we switch to other chat and navigate back here.

Platform:

Where is this issue occurring?

  • iOS
  • Android
  • Mobile Web

Proposal

Move the following lines to a new method let's call it maintainNewMarkerPlace (suggest new name if needed)

// Since we want the New marker to remain in place even if newer messages come in, we set it once on mount.
// We determine the last read action by deducting the number of unread actions from the total number.
// Then, we add 1 because we want the New marker displayed over the oldest unread sequence.
const oldestUnreadSequenceNumber = this.props.report.unreadActionCount === 0
? 0
: (this.props.report.maxSequenceNumber - this.props.report.unreadActionCount) + 1;
setNewMarkerPosition(this.props.reportID, oldestUnreadSequenceNumber);

then bind the method.

And invoke in the componentDidMount method (since we moved those lines from which is intended to happen).
Screenshot 2021-08-01 at 11 19 16 PM

And in shouldComponentUpdate method as below,
Screenshot 2021-08-01 at 11 14 59 PM

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

@Santhosh-Sellavel Santhosh-Sellavel added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 1, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jboniface (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 1, 2021
@Santhosh-Sellavel
Copy link
Collaborator Author

Fix Demo:

Issue.4357.Fix.Demo.mp4

@jboniface jboniface removed their assignment Aug 3, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 3, 2021
@MelvinBot
Copy link

Triggered auto assignment to @TomatoToaster (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@TomatoToaster TomatoToaster added the External Added to denote the issue can be worked on by a contributor label Aug 5, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jliexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@TomatoToaster
Copy link
Contributor

I'm putting the external label on this so we can get an upwork listing created. I think the problem makes sense and the proposal fits (but I'll have the engineer assigned double check it).

@TomatoToaster
Copy link
Contributor

TomatoToaster commented Aug 5, 2021

One thing I will say is that I don't think this method should also be in shouldComponentUpdate. That function should only handle the boolean logic on whether the props changing is relevant to rerun componentDidUpdate. Instead you should move that call of this.maintainNewMarkerPlace() to componentDidUpdate. I think it makes more sense for it to be there since you're already telling it to run componentDidUpdate with the return true in the second snippet you have.

Does that suggestion make sense?

@Santhosh-Sellavel
Copy link
Collaborator Author

Santhosh-Sellavel commented Aug 5, 2021

I will move that to componentDidUpdate it makes more sense for me too.
@TomatoToaster

@jliexpensify
Copy link
Contributor

@Santhosh-Sellavel and @TomatoToaster - I'm just catching up as I've been OOO.

Given that #4472 has been created, can I close this issue? I assume they're the same issue - correct?

@Santhosh-Sellavel
Copy link
Collaborator Author

@Santhosh-Sellavel and @TomatoToaster - I'm just catching up as I've been OOO.

Given that #4472 has been created, can I close this issue? I assume they're the same issue - correct?

@jliexpensify
IMO, they are not same issue.

@jliexpensify
Copy link
Contributor

Sorry for the delay, was checking a few things with @TomatoToaster - posted to Upworks: https://www.upwork.com/ab/applicants/1424866489594466304/job-details

@Santhosh-Sellavel - thanks for your patience. Please feel free to apply in Upworks!

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 9, 2021
@MelvinBot
Copy link

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

@jliexpensify
Copy link
Contributor

jliexpensify commented Aug 10, 2021

Hi @timszot - do you mind checking out Santhosh's proposal here, so we can get him hired? Thanks!

@Santhosh-Sellavel
Copy link
Collaborator Author

@jliexpensify I feel something is fishy going. The changes I proposed do not work for me now. Considering #4472 depend's on this issue solution and related. I would revisit the changes and update a new proposal soon, max within 3 - 4 days. If not I'll let you lets make it open for others also thanks!

@jliexpensify jliexpensify removed the Daily KSv2 label Aug 11, 2021
@MelvinBot
Copy link

Triggered auto assignment to @robertjchen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@jliexpensify
Copy link
Contributor

Looks like @timszot is OOO, so re-assigning another engineer.

@robertjchen
Copy link
Contributor

robertjchen commented Sep 1, 2021

Ah, I'm actually not familiar enough with this to provide good answers here 🤔 @marcaaron Could I trouble you to see if you have any insight here into @Santhosh-Sellavel 's questions here? #4357 (comment) Thanks!

@Santhosh-Sellavel
Copy link
Collaborator Author

@robertjchen you tagged someone else I think instead of me.

@robertjchen
Copy link
Contributor

Ah, my mistake- I've updated my tag! Hopefully we'll get some clarifications soon, appreciate your patience.

@marcaaron marcaaron added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Sep 1, 2021
@marcaaron
Copy link
Contributor

I think in this case we should have re-added the External label and not Engineering so that a contributor management engineering team member gets assigned. But looks like it's not working.

@jliexpensify
Copy link
Contributor

@marcaaron and @robertjchen - apologies, that was my mistake, I added the Engineering tag. I can post this issue in the channel to see if someone might be willing to take over from Tim?

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 2, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 2, 2021
@Santhosh-Sellavel
Copy link
Collaborator Author

@deetergp Can you answer these questions #4357 (comment)
And Review my draft PR #4887

Thanks!

@deetergp
Copy link
Contributor

deetergp commented Sep 3, 2021

Hey @Santhosh-Sellavel

Shouldn't we use the above check here in shouldComponentUpdate, tell the component to update only if it's going to be visible using nextProps instead of this.props in the check?

Does that take into account @parasharrajat's comment here?

I had two very minor comments but overall your draft PR pretty good so far.

@Santhosh-Sellavel
Copy link
Collaborator Author

@deetergp

Does that take into account @parasharrajat's comment here?

Yes partially that it should happen, I'm not clear about why because the view is hidden?

@deetergp
Copy link
Contributor

deetergp commented Sep 7, 2021

@parasharrajat Can you elaborate on your earlier comment for @Santhosh-Sellavel?

@jliexpensify
Copy link
Contributor

Bump for @parasharrajat :) Cheers!

@parasharrajat
Copy link
Member

parasharrajat commented Sep 13, 2021

I think PR is already merged and I am sorry, I have no detailed explanation for that . Rest code must be saying a better story than mine.

@jliexpensify
Copy link
Contributor

Awesome, thanks - should be good to pay Santhosh in 4 more days @deetergp (I think).

@deetergp deetergp added the Reviewing Has a PR in review label Sep 13, 2021
@kadiealexander kadiealexander added the Awaiting Payment Auto-added when associated PR is deployed to production label Sep 16, 2021
@mallenexpensify
Copy link
Contributor

Paid @Santhosh-Sellavel with bonus, apologies for delay, our new automated update messages should prevent this in the future.

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 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests