[$2000] BUG: unread indicator count is not reset after logout reported by @parasharrajat

[kavimuru]

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. Open the app on web.
2. Login with any account that has some unread messages. (to get unread messages send some messages from a different account to the account being logged in).
3. Now wait a couple seconds.
4. Observe there are unread message count on the Tab.
5. Do not go to the chat that has unread messages.
6. Now Sign out.
7. Wait a couple seconds.
8. Unread count is shown back after log out.

Optional:
9. If count is not shown by step 8, login back with the same account without refreshing the page.
10. Wait for the count to show up.
11. Log out now.
12. Wait a couple seconds, observe the Unread count on the tab bar.

Expected Result:

No unread count and indicator is shown after logout

Actual Result:

Unread count and indicator are shown after logout on login page.

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Desktop App

Version Number: 1.2.12-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/194615034-efb34a05-7dfd-4644-b813-d49dbc39827d.mp4

Recording.639.2.mp4

Expensify/Expensify Issue URL:
Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665066610938259

View all open jobs on GitHub

[melvin-bot]

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

[tjferriss]

I'm going to pass this over to engineering as I think it's a pass. However, I am on the fence as to whether this issue really hits the value part of the review:

Value — in other words, why is this particular issue important? Are there reasonable workarounds? Is the value of resolution greater than the resources required to reach it?

This seems like a polish item and if it requires more than a super small amount of work then I'd suggest we pass for now.

[melvin-bot]

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

[Justicea83]

This can be external, we set the unreadCount on the Web App here.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[tienifr]

Proposal

Problem

Now, we're using listenForReportChanges function to listen the changing of the reports and then using function throttledUpdatePageTitleAndUnreadCount to update the pageTitle and unreadCount. In function listenForReportChanges we check if (!report || !report.reportID) we do nothing. That why when we logout we don't reset the reports.

##Solution

we'll create new function resetReportAfterLogout to reset the reports and use UnreadIndicatorUpdater.resetReportAfterLogout() in componentDidMount of SignInPage

In https://github.com/Expensify/App/blob/main/src/libs/UnreadIndicatorUpdater/index.js

+function resetReportAfterLogout(){
+    Object.keys(reports).forEach(k => delete reports[k])
+}

export default {
    listenForReportChanges,
    stopListeningForReportChanges,
    throttledUpdatePageTitleAndUnreadCount,
+    resetReportAfterLogout
};

In https://github.com/Expensify/App/blob/main/src/pages/signin/SignInPage.js#L48

-        setTimeout(() => updateUnread(0), 0);
+        setTimeout(() => {
+            UnreadIndicatorUpdater.resetReportAfterLogout()
+            updateUnread(0)
+        }, 0);

Screen.Recording.2022-10-11.at.18.08.51.mp4

[marcaaron]

@Justicea83 why did we unassign @CortneyOfstad?
@sobitneupane why did we add the Help Wanted label?

Did I miss a process change? Seems like we need to wait for the Upwork job before the Help Wanted label gets added (which should happen automatically when the Exported label is added).

[CortneyOfstad]

Sounds good — just about to head out for the day, but will tackle this ASAP in the morning to get the Upwork job created 👍

[Justicea83]

@Justicea83 why did we unassign @CortneyOfstad? @sobitneupane why did we add the Help Wanted label?

Did I miss a process change? Seems like we need to wait for the Upwork job before the Help Wanted label gets added (which should happen automatically when the Exported label is added).

That must be a Github bug, I remember only unassigning myself 😁

[JmillsExpensify]

Coming back to this issue though, let's like we're down to one PR, and working through one trailing bug/consideration before merging. It definitely looks close!

[melvin-bot]

@tgolen, @puneetlath, @CortneyOfstad, @sobitneupane, @fedirjh 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

@tgolen, @puneetlath, @CortneyOfstad, @sobitneupane, @fedirjh 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[tgolen]

This issue is going to require two different PRs to fix. The first PR is a refactoring of how the unread reports are counted. However, there was one report of Onyx data not fully clearing when the sign-out happens, which kept some unread reports sitting in Onyx and the unread count remained.

Well, at least the count is accurately tracking what is in Onyx!

I was not able to reproduce the issue of Onyx.clear() not working correctly, but if we can reproduce it reliably, and fix it, then this issue can be fully closed. There are quite a few changes being made to Onyx.clear() right now after switching over to SQLite and the multiSet() implementation being reverted, so it's a little murky on whether or not there is indeed a problem or not.

[fedirjh]

@tgolen i have reported that before #11671 (comment) , and i believe this issue was present from long time ago #7397 (comment)

There was some reported bugs (#11901 and #12055) which got closed due to hard replication and i believe they are related to Onyx.clear

@puneetlath if i will suggest an updated proposal i would suggest a change to the Onyx.clear logic . there are other bugs related to how Onyx.clear works like #11901 and #12055 , i have started a thread on the slack channel

[tgolen]

@fedirjh Thanks for those reminders. I think what I would like to do is open a new issue that specifically tracks the problems with Onyx.clear() and not just the symptoms of it. I'll work on creating that GH and I will CC you on the issue.

If you have the time, I'd appreciate your review and help with testing Expensify/react-native-onyx#219

[tgolen]

I've opened #13884 to address issues related to Onyx.clear() not fully clearing out data

[fedirjh]

@tgolen thank for creating new I issue for Onyx.clear() , unfortunately that conversation (#13884) is locked and I can't reply there.

[tgolen]

OK, I've unlocked it for now.

…

On Sat, Dec 31, 2022 at 9:33 AM Fedi Rajhi ***@***.***> wrote: @tgolen <https://github.com/tgolen> thank for creating new I issue for Onyx.clear() , unfortunately that's conversation (#13884 <#13884>) is locked and I can't reply there. — Reply to this email directly, view it on GitHub <#11671 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMABZAUPP5WVMGRI5Y6DLWQBU6PANCNFSM6AAAAAAQ7Y6GQY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[melvin-bot]

@tgolen, @CortneyOfstad, @sobitneupane, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sobitneupane]

PR deployed to production

[sobitneupane]

@CortneyOfstad This should be ready for payment. Associated PR was deployed to production last week.

[CortneyOfstad]

@sobitneupane I am so sorry for the delay on this! For the payment, it would be like this, correct?

@parasharrajat gets $250 for reporting
@sobitneupane and @fedirjh each get $2000 for contributing and proposing the solution.

Are there any bonuses that I am missing?

TIA!

[sobitneupane]

@CortneyOfstad
Actually, we end up solving this issue internally (PR). So, Maybe

Rajat $250 for reporting.
Sobit $1000 for Internal PR review.

[CortneyOfstad]

Oh okay, thank you for the clarification! I'll get the contracts sent over in UpWork ASAP to have them accepted and I'll get payment sent out right away this morning. Again, I apologize for the delay on this!!

[CortneyOfstad]

Alright, contracts have been sent out @parasharrajat and @sobitneupane! Thanks!

[sobitneupane]

Again, I apologize for the delay on this!!

No Problem @CortneyOfstad.

Alright, contracts have been sent out

Accepted the offer.

[CortneyOfstad]

Both contracts have been accepted and paid! Closing!

[fedirjh]

Oh, was there not any announcement about this? As far as I know, we have been encouraged to take things "internal" at our discretion to help expedite as our roadmap is blocked on these bugs and unblocking it is our highest priority atm. It does not affect compensation if you have contributed to the solution then you will get paid. So, it's less work for you and the same benefit smile

@sobitneupane , @CortneyOfstad , from this comment #12963 (comment) , I think I am eligible for compensation too.

[CortneyOfstad]

^^ @sobitneupane can you confirm?

[sobitneupane]

At first we decided to go with @fedirjh's proposal. But later we found some issues in the proposal. So, we opened it for new proposals. Since we were not getting good proposals, @tgolen decided to make it internal.

It was solved with a different approach than initially proposed by @fedirjh. But @fedirjh was involved in this issue from the very beginning.

@CortneyOfstad This is the context. I am not sure if @fedirjh is eligible for the payment.

[CortneyOfstad]

Okay, let me check with the team on this. Thank you!

[CortneyOfstad]

Actually @fedirjh I misspoke!

[CortneyOfstad]

Our team has decided that this does qualify for payment and you'll get $250. I'll get that contract over to you ASAP!

[CortneyOfstad]

@fedirjh Contract sent over!

[fedirjh]

@CortneyOfstad Thank you! Accepted.

[CortneyOfstad]

Paid! Thank you!