-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: [Held requests] option does not show in the preview overflow menu. #42034
feat: [Held requests] option does not show in the preview overflow menu. #42034
Conversation
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@hungvu193 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] |
hey @Krishna2323 , it's been a while, do you have any ETA for this? |
@hungvu193, I will update this today or tomorrow. Sorry for the delay, I'm not at my workplace. |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@hungvu193, can you pls give this PR a quick look, if everything seems fine to you, I will add the screenshot. |
I'll take a look in a few hours 👀 |
Please add the screenshots |
if (reportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.IOU) { | ||
return; | ||
} | ||
const moneyRequestReportID = reportAction?.originalMessage?.IOUReportID ?? 0; | ||
|
||
const moneyRequestReport = getReport(String(moneyRequestReportID)); | ||
if (!moneyRequestReportID || !moneyRequestReport) { | ||
return; | ||
} |
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.
Can we merge these 2 conditions?
if (reportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.IOU || !moneyRequestReportID || !moneyRequestReport) {
return;
}
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.
I think thats fine, reportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.IOU
check is required before defining moneyRequestReport
or moneyRequestReportID
.
little bump here @Krishna2323 |
@hungvu193, sorry for delay, I will add the recordings today. |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@Krishna2323 as part of Rule #1, Get Shit Done, please be sure to action upon any existing issues/PRs before submitting new proposals. CONTRIBUTING.md is being updated to specifically mention this too
We'll be issuing warnings for these infractions in the future. This is not a warning though, since CONTRIBUTING.md isn't updated yet (and I haven't posted in #expensify-open-source about the update either) |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@hungvu193, I have added the recordings, can you pls review once again? |
Ok. I'll take a look in next few hours |
src/libs/ReportUtils.ts
Outdated
@@ -2707,6 +2715,80 @@ function canEditReportAction(reportAction: OnyxEntry<ReportAction>): boolean { | |||
); | |||
} | |||
|
|||
function getParentReportAction(parentReportActions: OnyxEntry<ReportActions>, parentReportActionID: string | undefined): OnyxEntry<ReportAction> { |
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.
I thought you mentioned you removed this one?
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.
I re-added this as the other function is deprecated and we are using this function in ReportScreen also.
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.
@robertjchen This is my concern, we should use deprecated function or creating a new one here?
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.
Is there a reason why we can't use Use Onyx.connect() or withOnyx() instead
as recommended by the deprecation comment? 🤔
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.
@Krishna2323 Bump on the above question.
I checked ReportUtils
, I saw that we're still using ReportActionsUtils.getParentReportAction(report);
to get parentReportActions
.
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.
little bump @Krishna2323 so we can merge this one.
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.
@hungvu193, I think we can leave getParentReportAction
as it is for now. It will be removed/replaced in this issue, and we are already using getParentReportAction in multiple places.
App/src/pages/home/ReportScreen.tsx
Lines 105 to 111 in 4e4715f
function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, parentReportActionID: string | undefined): OnyxEntry<OnyxTypes.ReportAction> { | |
if (!parentReportActions || !parentReportActionID) { | |
return null; | |
} | |
return parentReportActions[parentReportActionID ?? '0']; | |
} | |
@tgolen, can you pls confirm if we are going to replace getParentReportAction
or not? I don't see any open issue/PR for that.
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.
What are your thoughts on this, @robertjchen? Since we're already using the deprecated getParentReportAction
in ReportUtils
, I'm okay with using it until we have a plan to remove it all at once.
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.
Sounds good, given the current progress on the migration- we can use the deprecated function for now 👍
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.
Thanks for confirming 😄 .
@Krishna2323 Can you update this please? Thank you 🙇
Not sure if this is related but every time I held an expense, I saw a RBR. Screen.Recording.2024-05-22.at.09.17.28.mov |
How about this one? @Krishna2323 |
@hungvu193, I need to check for that, the reply count update doesn't seem to be related but I will check and RBR is expected behaviour afaik |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.native.movAndroid: mWeb ChromemAndroid.moviOS: Nativeios.native.moviOS: mWeb SafarimSafari.movMacOS: Chrome / Safarichrome.movMacOS: Desktopdesk.mov |
Let's me know when you finish your investigation so we can merge this one. Thank you 😄 |
@Krishna2323 how did you get on? Will you complete that today? |
Let's me know if you have any new update @Krishna2323 |
@hungvu193, still trying to get the solution, it would be great if you can look once. |
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${moneyRequestReport.policyID}`] ?? null; | ||
|
||
if (isOnHold) { | ||
IOU.unholdRequest(transactionID, reportAction.childReportID ?? ''); |
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.
In here, you updated the hold expense with its childReportID
, in our BE
we didn't send Pusher event to update its parent that's why our replies didn't increase when you held the expense.
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.
This seems that we will need BE changes here cc @robertjchen.
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.
@hungvu193, thanks a lot for looking into this, I agree with you.
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.
Got it, so the backend should also push updates for the parent report here, is that correct?
Could you let me know which particular field changes in the parent report that makes the update from the backend necessary? 👀
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.
childVisibleActionCount
is the current field to count the replies.
Since we also added a comment when we held request, I think the pusher event will be exactly the same when we added a new comment inside a thread.
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.
Got it, we can investigate- can this PR go out ahead of the backend changes or is it blocked until that field is available?
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.
I think we can go ahead with this PR first, but let's leave a note for QA team that the issue about counting replies will be fixed once BE PR is deployed.
We have conflicts here @Krishna2323 |
Bump on the conflicts, thanks! |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
conflicts resolved. |
I'm unclear what we're waiting on here, @robertjchen? (coming from here) |
@trjExpensify There's ongoing discussion in the comments above, once those are wrapped up we can merge 👍 |
@Krishna2323 Just need a final change here and we should be good to go! #42034 (comment) |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Looks like a final lint issue and we should be good to merge 👍 |
All yours @robertjchen |
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.
Perfect, thank you!
✋ 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/robertjchen in version: 1.4.82-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.82-4 🚀
|
it might relate to #43589 |
Details
Fixed Issues
$ #41440
PROPOSAL: #41440 (comment)
Tests
Offline tests
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 methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4