Part 7 - Thread delegateEmail through addComment/addAttachment/pressLockedBankAccount callers#87968
Part 7 - Thread delegateEmail through addComment/addAttachment/pressLockedBankAccount callers#87968Krishna2323 wants to merge 8 commits intoExpensify:mainfrom
Conversation
…ockedBankAccount callers Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…cAddCommentReportAction Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…ions and add PR numbers to TODO comments Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe23a3e012
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@DylanDylann 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] |
|
No product review needed |
| successData: successCreateWorkspaceTaskData, | ||
| failureData: failureCreateWorkspaceTaskData, | ||
| // Will be refactored in next PR; buildOptimisticTaskReportAction falls back to module-level Onyx.connect value; tracked in https://github.com/Expensify/App/issues/66417 | ||
| // Will be refactored in PR 15; buildOptimisticTaskReportAction falls back to module-level Onyx.connect value; tracked in https://github.com/Expensify/App/issues/66425 |
There was a problem hiding this comment.
Please pay close attention to avoid missing this in the future. Let’s specify which issue handles which parameter.
There was a problem hiding this comment.
Since there are multiple undefined values, we should specify which ones need to be handled later.
| const delegateAccountDetails = getPersonalDetailByEmail(delegateEmail); | ||
| // Falls back to module-level delegateEmail (from Onyx.connect) for callers not yet migrated; will be removed in https://github.com/Expensify/App/issues/66425 | ||
| const effectiveDelegateEmail = delegateEmailParam ?? delegateEmail; | ||
| const delegateAccountDetails = effectiveDelegateEmail ? getPersonalDetailByEmail(effectiveDelegateEmail) : undefined; |
There was a problem hiding this comment.
@Krishna2323 In this case, passing delegateAccountID looks better
There was a problem hiding this comment.
Passing delegateAccountID directly would be better, but it requires updating all the previous PRs' functions too (Task.ts, ReportWorkflow.ts, PaymentUtils.ts, and ~15 component files) since the parameter needs to be renamed through the entire call chain. To keep this PR focused, lets handle the full delegateEmail -> delegateAccountID conversion in a separate PR. WDYT?
There was a problem hiding this comment.
@Krishna2323 Why does this relate to other PRs? It looks like delegateEmailParam was introduced in buildOptimisticAddCommentReportAction in this PR. Anyways, I think we should check again to use delegateAccountID directly instead of the current way
…tionableButtons Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@codex review |
Explanation of Change
Fixed Issues
$ #66425
PROPOSAL:
Tests
delegateAccountIDis setdelegateAccountIDis not set when there's no active delegationOffline tests
QA Steps
Same as Tests
Verify that no errors appear in the JS console
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
Monosnap.screencast.2026-04-16.02-32-47.mp4