Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add One Transaction Report View #36934
Add One Transaction Report View #36934
Changes from 32 commits
53ada16
df1b56c
b841b6a
7ec2e5c
353b740
42adcea
66f498b
de66987
74a42f6
1bf4c04
6153bab
d436ffe
b722725
b13fc4b
452d012
cac6d81
6a08ba5
1674735
f3f5e52
90bc361
0f21504
4bb0313
35d0264
c941d23
2a02be8
b5ae144
f6cfcb3
0140912
db05afc
c274090
618b5ac
31cf018
6b2d28a
1b27a33
d36bc98
686a2dc
7994917
1003a1f
de600c8
034b93f
716023c
3b88172
45c7fc1
f9f3607
e3be337
1090c5b
a4926d3
7dd2384
fec3aa3
5438f69
1aac6e0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 single transaction view specific only for expense reports? I feel like we should treat the IOU reports the same and in that case we are missing the case of Send Money flow and
pay
IOU action type https://github.com/Expensify/Auth/blob/488096eac43b22e5a9d4a8355704105b6dfd2972/auth/command/SendMoney.cpp#L173ie it would be a test case of Send money to another user and then if you visit the iou report, its only one transaction so it should render as the single transaction view, right?
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.
Not specific to just expense reports, no. I was using the iou types from
getMostRecentIOURequestActionID()
here but can update this to includepay
👍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: I would expect this filtering to happen before the sort function as you did in for the created action. Not really a blocker but having a filter, sort and filter feels a bit odd to me instead of filter, filter sort :D
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.
We intentionally don't filter the combined reportActions and filteredTransactionThreadReport actions first because we only want to remove the
created
actions from the transaction thread report actions here.We could do it this way if you find that clearer:
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'm adding
getAllReportActions()
to the list of bad methods in #27262. Can you please remove the uses of it and use the localreportActionsByReport
collection instead?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.
Yep, I actually mentioned below here about possibly following up in a separate PR to handle this case but I'll look at using
reportActionsByReport
here and inisOneTransactionThread
. UsingreportActionsByReport
works too though!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.
@tgolen we discussed previously about how using methods like
getAllReportActions
isn't the best practice. However, I thought it would be cleaner to implement it like this for now, and make a separate PR to update all the instances ofReportUtils.getIcons()
,ReportUtils. shouldReportBeInOptionList()
, andReportUtils.shouldReportShowSubscript()
(which use these methods) to send across the appropriatereportActions
instead of justreportID
s.Lemme know if you disagree and I can add those changes in this PR instead.
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.
Why is this method being exported? It doesn't look like it is used anywhere outside of this file (and this is one of those anti-patterns we discussed). In fact, would you mind adding this method to https://github.com/Expensify/App/blob/main/tests/actions/EnforceActionExportRestrictions.ts so that it doesn't get exported and used somewhere else?
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 fact, now I'm also worried that any of the methods in this file that use
isOneTransactionReport()
are being exported and used improperly.Check failure on line 294 in src/pages/home/ReportScreen.js
GitHub Actions / Run ESLint
Check failure on line 294 in src/pages/home/ReportScreen.js
GitHub Actions / Run ESLint