Configure Confirm step for IOU #2120
Conversation
|
@iwiznia I've mentioned the steps to check the flow for Split! |
barun1997
force-pushed
the
iou-confirm-step
branch
from
2870c86
to
f74592c
Compare
barun1997
force-pushed
the
iou-confirm-step
branch
from
f74592c
to
15f9b08
Compare
|
Hi @barun1997
Is this where the background is grey and you can't close the Modal? If so there's a workaround for testing as this doesn't occur if you launch the modal from the menu. Make these changes locally to see the IOU options in the menus -- but please don't commit them :) |
|
@Julesssss Yup just tested it. Works like a charm! :) The modal closes successfully as required |
Julesssss
changed the title
barun1997
changed the title
|
@Julesssss Sorry about the title change. I didn't figure you'd changed the name. I was confused why it was named that, for a second! Haha |
src/pages/iou/IOUModal.js
Outdated
| return Navigation.dismissModal(); | ||
| } | ||
|
|
||
| // Object.keys(this.props.iousReport) |
There was a problem hiding this comment.
@Julesssss This didn't work with Onyx.mergeCollection() so I instead opted to use state.isTransactionComplete to figure out when to close the modal. What do you suggest I should do?
The logic works perfectly well in the case of Request, when I merge just a single report key, but when I merge the entire object, it doesn't get called. I'm thinking it's mutability issue, and I need to look more into it. But just wanted to know if it was okay to instead use state to know when to close the modal?
There was a problem hiding this comment.
Oh, so you didn't get notified about the report when mergeCollection was called? Odd.
We want to avoid returning from an Actions (Promises or callbacks) so I'd suggest looking into the reason that mergeCollection doesn't work first before we look for alternatives. I'll take a look at this issue tomorrow and let you know if I find anything.
There was a problem hiding this comment.
Ahh, yep makes sense! Will look deeper into it then :)
There was a problem hiding this comment.
I didn't find any time for this today unfortunately. Will try again tomorrow. Any luck on your side?
There was a problem hiding this comment.
@Julesssss I faced the same issue when I looked at it briefly today morning. Will take a deeper look now!
There was a problem hiding this comment.
This was a bug in Onyx, which has just been fixed. We've just bumped the Onyx version in Expensify.cash, so you should be able to resolve your issue after merging master.
There was a problem hiding this comment.
@Julesssss Got it, thank you!
There was a problem hiding this comment.
Hi @Julesssss, just to give you an update, I have merged with the master but the issue still seems to persist on my end. However, I will look into it more to see if the bug is with how I've used mergeCollection. Please feel free to add suggestions if you have any.
It seems to work with IOURequest, like before where just merge is used. I used _.isEqual for now -- just to make sure the componentDidUpdate implementation wasn't the problem.
I've attached a video for the two instances below:
Screen.Recording.2021-04-02.at.11.09.01.mov
There was a problem hiding this comment.
Hi @barun1997, we're now tracking this here.
There was a problem hiding this comment.
@Julesssss Thank you Jules, I will take a look.
|
@barun1997 any updates on this? |
I should be ready to go as soon as this PR is up! |
| * Returns selected options with all participant logins | ||
| * @returns {Array} | ||
| */ | ||
| getAllOptionsAsSelected() { |
There was a problem hiding this comment.
I'm a bit confused by this.
Returns selected options with all participant logins
What exactly does 'selectedOptions' mean?
There was a problem hiding this comment.
I get options. Selected meaning the IOU participants?
There was a problem hiding this comment.
I meant that it selects all IOUParticipants for the display in IOUConfirm’s split flow. I will revise the comment to explain it better :)
|
Hi @Julesssss Thank you for the review! I will revise the PR with updated commit first thing tomorrow morning! :) |
|
Bumping @AndrewGable @timszot for reviews |
|
@Julesssss Made the final change as well :) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Heads up this PR has caused a regression that prevents the LHN from scrolling. |
|
Offending commit ee4d245 |
|
hi, @marcaaron I’ll take a look. I had to do that that because the List wasn’t displaying in iOS/Android. I will revert the change in OptionList and work it out in ConfirmationPage itself. |
|
🚀 [Deployed](https://github.com/Expensify/Expensify.cash
|
|
This PR is pretty old, but it seems that this bug has been there all along which allowed users to send empty comments in IOU requests. |
<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>
Details
Fixed Issues
#1988
Fixes #1988
Tests
FOR IOU Request:
Since IOURequest has a bug, the modal doesn't pop after completion.
For IOU Split:
QA Steps
Tested On
Screenshots
Web
Mobile Web
iOS
Screen.Recording.2021-04-12.at.01.04.01.mov
Screen.Recording.2021-04-12.at.01.04.47.mov
Android