melvin-bot[bot]
commented
2 months ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)
Closed mountiny closed 1 month ago
melvin-bot[bot]
commented
2 months ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)
melvin-bot[bot]
commented
2 months ago Current assignee @mountiny is eligible for the AutoAssignerNewDotQuality assigner, not assigning anyone new.
melvin-bot[bot]
commented
2 months ago Triggered auto assignment to @twisterdotcom (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.
melvin-bot[bot]
commented
2 months ago Job added to Upwork: https://www.upwork.com/jobs/~01c0a47577add24a8e
melvin-bot[bot]
commented
2 months ago Current assignee @eVoloshchak is eligible for the External assigner, not assigning anyone new.
puneetlath
commented
2 months ago I also have this same domain room in my foucs mode LHN even though its not unread. Here's its onyx data
daledah
commented
2 months ago [CRITICAL] [UX Reliability] Domain chat room is showing in focus mode in LHN despite not having any new messages
This early return is the problem https://github.com/Expensify/App/blob/main/src/libs/SidebarUtils.ts#L120-L122.
The condition is checking only participantAccountIDs.includes(CONST.ACCOUNT_ID.NOTIFICATIONS), it should've targeted only the system message (for Expensify account onboarding chat). But participantAccountIDs.includes(CONST.ACCOUNT_ID.NOTIFICATIONS) is targeting other chat types too. The Expensify domain chat room referenced in https://github.com/Expensify/App/issues/43782#issuecomment-2168485189 is one such chat. It also contains CONST.ACCOUNT_ID.NOTIFICATIONS as a participant, hence will early return true and included in the chat list regardless of focus mode.
Limit the condition to only the system chat https://github.com/Expensify/App/blob/main/src/libs/SidebarUtils.ts#L120, afterwards it will still work well for the Expensify account onboarding chat and will not affect other chats like domain room.
if (currentUserAccountID && AccountUtils.isAccountIDOddNumber(currentUserAccountID) && participantAccountIDs.includes(CONST.ACCOUNT_ID.NOTIFICATIONS) && isSystemChat) {
return true;
}The only change was to add && isSystemChat
AbdulRehmanMehar
commented
2 months ago [CRITICAL] [UX Reliability] Domain chat room is showing in focus mode in LHN despite not having any new messages
The early return condition in the code at SidebarUtils.ts#L120 doesn't check whether the focus mode is active or not and displays system chat regardless of the priority mode.
We need to add a check for priority mode at SidebarUtils.ts#L121. It should display the report only if it's unread while the user is in focus mode.
N/A
melvin-bot[bot]
commented
2 months ago 📣 @AbdulRehmanMehar! 📣 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:
Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>Contributor details Your Expensify account email: mehars.6925@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~010d76b8839f89f4a3
melvin-bot[bot]
commented
2 months ago ✅ Contributor details stored successfully. Thank you for contributing to Expensify!
twisterdotcom
commented
2 months ago @eVoloshchak how are these proposals?
eVoloshchak
commented
2 months ago As both contributors have pointed out, the problem is caused by an early return in https://github.com/Expensify/App/blob/main/src/libs/SidebarUtils.ts#L120-L122.
It was added in https://github.com/Expensify/App/pull/41343. Left a comment on the PR, it might be resolved as part of that issue, since PR was deployed to prod 5 days ago and is still in 7-day regression period
daledah
commented
2 months ago @twisterdotcom @eVoloshchak IMHO it'll be fair to fix here, as this issue was promoted as Help Wanted and contributors spent time providing the help and proposals based on that premise 😄
cc @mountiny @puneetlath
mountiny
commented
2 months ago @koko57 this seems to be still in the regression period so would make sense if you can fix it with @eVoloshchak
koko57
commented
2 months ago @mountiny ok, so I can take care of this
melvin-bot[bot]
commented
2 months ago Reviewing label has been removed, please complete the "BugZero Checklist".
melvin-bot[bot]
commented
2 months ago The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.1-19 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-07-02. :confetti_ball:
For reference, here are some details about the assignees on this issue:
melvin-bot[bot]
commented
2 months ago BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
melvin-bot[bot]
commented
2 months ago melvin-bot[bot]
commented
2 months ago The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.3-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:
For reference, here are some details about the assignees on this issue:
melvin-bot[bot]
commented
2 months ago BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
mountiny
commented
2 months ago I think this would be $125 to @eVoloshchak for their review. That is because this regression stemmed from PR they reviewed, however, its something they could not really test for easily so I err to the side of partial payment
melvin-bot[bot]
commented
2 months ago Upwork job price has been updated to $125
twisterdotcom
commented
2 months ago Payment Summary:
daledah
commented
2 months ago @twisterdotcom @mountiny I think I'm also due a partial compensation here because my proposal was used to fix the issue, and this issue was Help Wanted (actively seeking help) at the time I posted the proposal
mountiny
commented
2 months ago That is fair, the Help Wanted label was on the issue and similar solution was used so $50 to @daledah cc @twisterdotcom please
daledah
commented
2 months ago @mountiny I think same as the reviewer ($125) should be more fair. I think it takes equal (or more) effort to come up with the original RCA and working solution than to review the PR with existing solution.
What do you think?
mountiny
commented
2 months ago @daledah Sorry, $50 is more in line with how we handle such partial rewards. I do not think its valid to issue to full reward of this issue given you did not have to create the PR and complete the checklist/ test and record on all platforms, which is the time consuming part. I hope that makes sense
daledah
commented
2 months ago I do not think its valid to issue to full reward of this issue
@mountiny The issue was for $250, $125 is half the reward though.
Anyway I'd be happy with any final decision made here 👍
mountiny
commented
2 months ago Yeah, I'm sorry, but $50 was being issued elsewhere, even for $250. I'm also sorry that the help wanted label was added, and then it was handled by the original PR author. This risk always exists as the PR author is supposed to fix regressions.
Hopefully, you can land the full reward on some other task where we merge your PR 💪 thanks for contributing.
melvin-bot[bot]
commented
1 month ago @eVoloshchak @twisterdotcom @koko57 @mountiny this issue is now 4 weeks old, please consider:
Thanks!
eVoloshchak
commented
1 month ago Regression Test Proposal
Do we agree 👍 or 👎
eVoloshchak
commented
1 month ago I think this would be $125 to @eVoloshchak for their review. That is because this regression stemmed from PR they reviewed, however, its something they could not really test for easily so I err to the side of partial payment
@mountiny, this regression stems from https://github.com/Expensify/App/pull/41343, which I wasn't involved in
twisterdotcom
commented
1 month ago Okay, as that PR wasn't reviewed by @eVoloshchak, I think we can do another $125, right @mountiny?
mountiny
commented
1 month ago @twisterdotcom @eVoloshchak sounds good, apologies for missing this!
twisterdotcom
commented
1 month ago Just a note then, I have updated the Payment Summary here for you to refer back to @eVoloshchak.
mountiny
commented
1 month ago Sorry for missing that
JmillsExpensify
commented
1 month ago $250 approve for @eVoloshchak
twisterdotcom
commented
1 month ago @daledah same as this issue: https://github.com/Expensify/App/issues/44150#issuecomment-2249029743. I need a valid upwork profile link for you to send a payment contract to please. Could you drop a link to your profile or DM ted on Slack with it please?
I updated the Payment Summary here in the meantime too.
daledah
commented
1 month ago @twisterdotcom I'm having some issue with my Upwork account and actively resolving it, I'll give an update here early next week once it's resolved.
daledah
commented
1 month ago Could you drop a link to your profile
@twisterdotcom Sure, here it is https://www.upwork.com/freelancers/~0138d999529f34d33f. Please help send an offer to my profile there.
Thanks
twisterdotcom
commented
1 month ago Offer sent. Payment Summary updated with link.
daledah
commented
1 month ago @twisterdotcom I accepted that offer, thanks for helping 🙏
Problem
In focus mode, only pinned chats, drafts, GBR, unread chats should be showing in LHN. There is now also a domain room chat that is not unread, has not new messages, no GBR or RBR in it. Here is the onyx data.
Why is it showing?
Solution
Investigate why the domain chat is showing in focus mode based on the report data above.
Issue Owner
Current Issue Owner: @Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @twisterdotcom