Fix imported transactions are displayed as skeleton loader#69492
Conversation
|
@mananjadhav 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] |
trjExpensify
left a comment
There was a problem hiding this comment.
Agree with the expectation that the transactions on an open report should be deleted when the feed is deleted. 👍
|
@nkdengineer Are you able upload the screenshots now? |
|
Quick bump @nkdengineer |
@mananjadhav i still can't connect the company card. Could you please check this flow in your end? |
|
Let me try that on my end and see if it works for me. |
|
@mananjadhav Sorry but I think @c3024 should be the reviewer here since he reviewed the proposal in the issue. |
Ohh got it. I thought this was one of the issues where there was no reviewer. Cool let's have @c3024. In case they're not available I can help. |
|
It's been a week the PR has been reviewed. @trjExpensify Do you want me to finish it? (I accidentally reviewed the code already assuming I was the C+). |
|
Yep, do it. 👍 |
|
@nkdengineer The change doesn't seem to be working. I had a company card feed, which I deleted based on the steps. The transactions are still visible in the preview. But when I click to open the transaction report, I don't see them. web-card-feed-delete.movweb-card-feed-delete-2.mov |
|
I was accidentally unassigned here on the linked issue so this PR never appeared in my notifications. TY for handling this. @mananjadhav |
|
Quick bump on this @nkdengineer in case you missed. Thanks for the help @c3024 :) |
@mananjadhav When you enable |
mananjadhav
left a comment
There was a problem hiding this comment.
Approving as code change is fine and want @techievivek to take a look at the comment
|
I think then we should get this fixed on the BE and then retest on FE? |
Oh, actually. I am looking at the code, and the backend does seem to be queuing updates to clear transactions on open reports. So why are we still seeing the transactions in previews? |
I am confused about this comment in particular, what does |
|
Ok, digging further, I see the option
|
|
Yes I deleted the card feed. I didn't manually delete transactions. |
|
@nkdengineer Can you please take a look at the above comments and help me understand the change? Thanks. |
|
@techievivek From our testing, this case doesn't work as expected since the backend doesn't return an update that clears the card transaction |
|
Discussing on Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1758277505345179 |
|
Discussing on Slack here https://expensify.slack.com/archives/C01GTK53T8Q/p1758806762768339?thread_ts=1758277505.345179&cid=C01GTK53T8Q since I can't find any issue in our backend code that would handle queuing updates differently when |
|
The fix has been merged and should be deployed by end of the day today. I will confirm once it’s live, and then we can verify everything is working as expected. Thanks for flagging this 🙇 |
|
Thanks, will wait for the update. |
|
@mananjadhav @nkdengineer backend changes are deployed, can we get a re-test here? Thanks. |
|
@techievivek Works well now. |
techievivek
left a comment
There was a problem hiding this comment.
Nice, thanks for confirming.
|
@mananjadhav Can you fill up the PR reviewer checklist here? |
|
Yes I'll work on the checklist and finish the testing by weekend. |
|
Friendly bump @mananjadhav, thanks. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #69492 +/- ##
==========================================
- Coverage 35.17% 35.14% -0.04%
==========================================
Files 3290 3291 +1
Lines 107852 107963 +111
Branches 34421 34472 +51
==========================================
+ Hits 37942 37946 +4
- Misses 69725 69830 +105
- Partials 185 187 +2 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppiOS: HybridAppios-card-feed.moviOS: mWeb Safarimweb-safari-card-feed.movMacOS: Chrome / Safariweb-delete-card-feed.movMacOS: Desktopdesktop-delete-card-feed.mov |
mananjadhav
left a comment
There was a problem hiding this comment.
Issues with my android build. Just finished the checklist. @techievivek All yours.
|
✋ 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/techievivek in version: 9.2.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.30-6 🚀
|




Explanation of Change
Fixed Issues
$ #67644
PROPOSAL: #67644 (comment)
Tests
Precondition:
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop