-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Ignore hasOutstandingIOU from child iouReport #24448
Ignore hasOutstandingIOU from child iouReport #24448
Conversation
What's the status of this PR? local / merged only / staging? |
I don't think it is necessary to wait for that, both parts (frontend and backend) needs the same update, and it will be inconsistent as long as one of them has not been deployed |
@situchan thinking again about it, I guess it makes sense to wait for https://github.com/Expensify/Web-Expensify/pull/38473 to be deployed, otherwise it will be hard to test that the front end changes are doing something. |
…outstanding-iou-child-report
…outstanding-iou-child-report
Fixed tests, the Web-E PR is in staging, so this can be tested now |
@situchan Heads up that this is one of the most important PRs we can merge right now. Please start on this one before any others in your list. |
Asked in Slack for prioritization https://expensify.slack.com/archives/C02NK2DQWUX/p1692108871664589 |
Reviewer Checklist
Screenshots/VideosWeb
web.mov
web2.movMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
While testing expense request, I noticed that green dot briefly appears and then disappears in workspace chat bug.mov |
@aldo-expensify can you please check #24448 (comment)? Constantly reproducible to me |
Hmm should be on staging only, waiting for @aldo-expensify to check this out |
@situchan maybe I'm wrong about expecting a difference between staging/production, maybe this doesn't depend on the backend update to work fine. Testing... |
I also thought so. Though not sure what backend update is. |
I can see that if I connect to production, the fix is not there because Having said that, this apparently is not enough to show the green dot in the workspace chat I assume because of other conditions here: Line 1152 in 08d447c
I'm trying to make my app hit staging using |
The backend should make the |
Would be good to know why is that you see the green dot appearing briefly in the workspace chat and I don't. |
Trying to find the root cause. This video shows clear reproduction step. Screen.Recording.2023-08-16.at.2.27.19.AM.mov |
Not clear enough :P . Does this happen if you do the same with a fresh account? |
I think I found the root cause. |
Is it possible that pusher points to production while api points to staging? I think we can go ahead and merge this as this is related to backend. |
@situchan I just reproduced the bug you are pointing too, but I had to create a workspace with 2 users and request money from the member that is not the admin. I can see the pusher event that incorrectly sets I'll work on fixing that in the backend, thanks for the catch! |
Working on PR here: https://github.com/Expensify/Web-Expensify/pull/38514 , I'll add tests and make it ready for review. |
Nice, great job identifying it, I dont think we have to hold on that one though so I think we can go ahead with this PR, do you agree @aldo-expensify? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checklist: #24448 (comment)
@arosiclair Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Agree, this is a pretty safe change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me and @situchan tested thoroughly based on the comments. Feel free to merge if you want, I think we are good to go @aldo-expensify
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 1.3.55-0 🚀
|
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 1.3.56-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.56-24 🚀
|
Test with: https://github.com/Expensify/Web-Expensify/pull/38473
Details
We were looking into the linked iou/expense report to check for the property
hasOutstandingIOU
. This means that if you request money, both reports (the parent chat report and the money request report) get the green dot like there is an action pending.We are changing this to now only show the green dot in the expense/iou report and not the parent report.
The original behaviour made sense when we were not loading iou/expense reports into the LHN, but we are now, so we don't need this logic anymore
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/307581
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Before
After
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android