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
fix: unify TBD-related logic for Distance requests #34135
fix: unify TBD-related logic for Distance requests #34135
Changes from all commits
f4aff81
152b569
b30a5e7
f68e06e
27e9415
46dec59
c05c602
b87642a
5a24138
dd94a01
cda5367
373c670
634b5fa
f2a7a06
98ec065
5443b83
c6ed187
6cdea23
0d72dde
3f3564c
98c7b9b
9452819
db6e2a7
26da439
16f6b54
646c97f
19d97aa
42757dc
2cd2c52
3ff6f9a
e63553c
e88d6d5
516ae47
d22cfe5
a020a23
1c849c3
534b2c1
82bb06e
749a395
8d493e2
2e4c40f
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.
Why isn't this using
isFetchingWaypointsFromServer()
?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
isFetchingWaypointsFromServer
depends on this variable, not vice versa:App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js
Lines 477 to 479 in e63553c
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 feel like that's kind of an anti pattern. I think we should only call actions based directly on user action, not based on data changing. Do you agree Tim?
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.
But we already do call these actions in multiple places when landing on different steps of the IOU creation. Since in all these places we update the draft transaction, it looks more like a way to transfer the state between the IOU creation steps, and not an actual data modification.
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.
Ah, I think you're both right. @paultsimura you're correct that this is more akin to storing the form state between steps. @neil-marcellini I think you might be onto something about this looking like an anti-pattern.
What do you think about this?
IOU.setMoneyRequestPendingFields(transaction.transactionID, {waypoints: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD});
on this page, set that data on the previous page when the user submits the form. I think it could be part of the data that gets set here:App/src/pages/iou/request/step/IOURequestStepDistance.js
Line 159 in e83c24a
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 issue here is the following scenario:
The user completes the first steps up until the confirmation step while being offline. Then, on the confirmation step, they come back online. Here, the map should get fetched, and we no longer should have the waypoints pending (even though when we landed on this page, the map was still pending).
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.
Oh, that darn offline flow gets confusing! Thanks for pointing that out. It's probably OK then, but I would ask that you also put that context into a code comment where it happens.
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 you, thanks for taking your time on this. Updated ✔️
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.
Thank you for explaining. I think that flow makes sense, but how does that relate to this condition? If we are offline and set the waypoints pending when they are updated (as Tim suggested), then when the user is navigated to this page the pending fields will show correctly. Once they go back online I would think that the GetRoute response should clear the pending fields for the waypoints (and or the route), and then all data on the confirmation page would no longer show as pending. Does that make sense?
I'm going to leave this as a NAB. We could fix it in a follow up if needed.
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.
Probably yes, but I'm not sure where to attach the workspace rate being still loaded in this case. That being said, I would really appreciate it if we could merge this PR as-is if it's an NAB. It's my longest one so far 🙂