-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 card assignment flow for one card and several assignees #55151
Fix card assignment flow for one card and several assignees #55151
Conversation
@jayeshmangwani 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] |
@VickyStash Could you add @jayeshmangwani as admin on the direct feed policy so they can test this too? Thanks! @jayeshmangwani what email could Vicky use? |
@VickyStash @mountiny please add jm98289517+44@gmail.com |
@jayeshmangwani Done, you should have access to this workspace |
Thanks, now i can access the ws company cards! |
@VickyStash Do I need to follow any additional setup? Pressing 'Assign Card' opens a new tab for me. company-card-assignment.mov |
|
@VickyStash It seems this doesn’t work. To test this, I tried the card assignment flow on the production app on the web (https://new.expensify.com/), and it also redirects to the Amex authentication page. |
@jayeshmangwani If you do login after I set the bank connection, I need to set it one more time. I just did it again. Try now, but keep in mind, if you do the login flow for jm98289517+44@gmail.com account - the connection will need to be set up again. |
The expiration is just 5 minutes so that is expected |
Right now, its working |
@mountiny I was able to test this on the web, but it seems to expire after a few minutes. What would be the best way to test the card assignment flow for C+ in this case? |
@VickyStash To test this PR right now, if I log in on all other platforms (iOS, Android, mWeb, and desktop) with jm98289517+44@gmail.com and then you set the bank connection again, it will work for all platforms, correct? |
@jayeshmangwani I'm not 100% sure just to be true |
@jayeshmangwani I already worked on the direct feed before and I usually create mock data and simulate a trigger for testing PRs that relate to Direct Feed. You also can try it |
@jayeshmangwani yes mocking is good, for now I think web should be enough |
If we mock data on a useEffect, we can trigger it on all platforms |
Thanks for the guidance, @DylanDylann! I can now test the flow by mocking data |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: NativeiOS.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.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.
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.
Thanks!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.86-0 🚀
|
tested and confirmed it works! |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.86-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.86-3 🚀
|
Explanation of Change
Fix card assignment flow for one card and several assignees
Fixed Issues
$ #55100
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
Same, as in the Tests section.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
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.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4