-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Move selected transactions #61839
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
Move selected transactions #61839
Conversation
@MarioExpensify 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] |
@waterim I used your implementation here and move UI to separate component. I think you'll work on follow up so please don't be surprised 😅 @sumo-slonik I slightly modified logic here so please take a look 😇 cc @luacmartins |
@waterim I made a mistake and wrote that I'll work on follow up 😅 I think you're the one who's doing it so I don't want to interfere 😅 but of course I can help with it :D |
const policyID = policyIDToCheck ?? getPolicyIDFromState(navigationRef.getRootState() as State<RootNavigatorParamList>); | ||
const policyMemberAccountIDs = getPolicyEmployeeAccountIDs(policyID); | ||
const shouldOpenAllWorkspace = isEmptyObject(targetReport) ? true : !doesReportBelongToWorkspace(targetReport, policyMemberAccountIDs, policyID); | ||
isNavigationReady().then(() => { |
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 change was approved by @WojtekBoman himself 🙇
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.
the man, the legend
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.
Looking good. Left a few minor comments
assets/images/document-merge.svg
Outdated
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 asset doesn't seem to follow the same pattern as others. @jnowakow did you just extract it directly from figma?
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.
Yes and then I removed attributes that were not present in assets/images/document-plus.svg
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.
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 have a different process to export these assets. @Expensify/design could you please assist?
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.
Yeah, who exported this? Did it come from the design team or not?
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.
Try this one: document-move.svg.zip
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.
Yeah, who exported this? Did it come from the design team or not?
I don't think so, I think @jnowakow just got it from figma
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 this addressed @jnowakow?
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Outdated
Show resolved
Hide resolved
options.push({ | ||
text: 'Move Expenses', | ||
icon: Expensicons.DocumentMerge, | ||
value: 'MOVE', |
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.
Let's use a const for this, probably declared in CONST.REPORT.SECONDARY_ACTIONS.MOVE_TRANSACTIONS
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'll declare it as const on top of the file because it's ignored anyway
const policyID = policyIDToCheck ?? getPolicyIDFromState(navigationRef.getRootState() as State<RootNavigatorParamList>); | ||
const policyMemberAccountIDs = getPolicyEmployeeAccountIDs(policyID); | ||
const shouldOpenAllWorkspace = isEmptyObject(targetReport) ? true : !doesReportBelongToWorkspace(targetReport, policyMemberAccountIDs, policyID); | ||
isNavigationReady().then(() => { |
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.
the man, the legend
|
||
function IOURequestEditReportCommon({backTo, transactionReport, selectReport}: Props) { | ||
const {translate} = useLocalize(); | ||
const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {selector: (c) => mapOnyxCollectionItems(c, reportSelector), canBeMissing: true}); |
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 know this is not your code, but what is c
? Can we use a better variable name in this case?
Where do we have the most up-to-date videos for review? Otherwise we can make a test build when this is ready too. Just let us know! |
🚧 @luacmartins has triggered a test app build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-05-14.at.9.51.18.AM.movAndroid: mWeb ChromeScreen.Recording.2025-05-14.at.9.52.28.AM.moviOS: HybridAppScreen.Recording.2025-05-14.at.9.43.43.AM.moviOS: mWeb SafariScreen.Recording.2025-05-14.at.9.45.54.AM.movMacOS: Chrome / SafariScreen.Recording.2025-05-14.at.9.40.15.AM.movMacOS: DesktopScreen.Recording.2025-05-14.at.9.39.01.AM.mov |
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.
nit picks, will go forward with platform testing now
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Show resolved
Hide resolved
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Show resolved
Hide resolved
@@ -1212,6 +1212,7 @@ const translations = { | |||
dates: 'Fechas', | |||
rates: 'Tasas', | |||
submitsTo: ({name}: SubmitsToParams) => `Se envía a ${name}`, | |||
moveExpenses: () => ({one: 'Mover gasto', other: 'Mover gastos'}), |
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.
are these translations confirmed
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.
Yes, by JamieGPT. I've posted to slack channel here
Screen.Recording.2025-05-14.at.9.53.55.AM.movI think the button should be hidden if there is only a single report. thoughts @trjExpensify @luacmartins |
Screen.Recording.2025-05-14.at.9.56.24.AM.mov |
It was done this way before so I didn't alter this behaviour |
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 also cannot reproduce the second bug with new report! thanks
@luacmartins @trjExpensify should it be possible to move all expenses from submitted report? It leads to situation where there's a report without expenses and there's no option to add expense to it Screen.Recording.2025-05-14.at.10.50.16.mov |
Ahh okey after #61465 is merged it will be possible to bring expenses back but this |
Hmm interesting, I would think that in that case above, we should probably just delete the report since it no longer has expenses on it? Curious what Tom thinks though. |
Yeah, it seems pretty unlikely that someone is going to move all expenses off a report and then add new ones to that old report? Curious what Tom thinks though for sure. |
Yea, this is possible.
We don't do this in OldDot, so I don't think we should start it now. Let's just keep the empty report. |
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.
LGTM!
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.
LGTM
const canMoveExpense = canEditFieldOfMoneyRequest(iouReportAction, CONST.EDIT_REQUEST_FIELD.REPORT); | ||
|
||
return canMoveExpense; |
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.
NAB
const canMoveExpense = canEditFieldOfMoneyRequest(iouReportAction, CONST.EDIT_REQUEST_FIELD.REPORT); | |
return canMoveExpense; | |
return canEditFieldOfMoneyRequest(iouReportAction, CONST.EDIT_REQUEST_FIELD.REPORT); |
@jnowakow we have conflicts |
@luacmartins conflicts solved |
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.
LGTM
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Explanation of Change
Fixed Issues
$ #61770
PROPOSAL: N/A
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectioncanBeMissing
param foruseOnyx
toggleReport
and notonIconClick
)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: mWeb Chrome
android-web.mov
iOS: Native
iOS: mWeb Safari
ios.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop