-
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
handle offline mode for bank connection #55338
handle offline mode for bank connection #55338
Conversation
@huult Is the above implemented? Also, there are conflicts to be addressed. Please resolve them too. Thanks. |
…eed-bank-authentication
I will update it tomorrow |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
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.
@huult I have left just one comment. Otherwise, code LGTM. Please have a look. Thanks.
src/pages/workspace/companyCards/WorkspaceCompanyCardsFeedAddedEmptyPage.tsx
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari55338-web-safari-001.mp4MacOS: Desktop55338-desktop-001.mp4Android: Native55338-android-native-001.mp4Android: mWeb Chrome55338-mweb-chrome-001.mp4iOS: Native55338-ios-native-001.mp4iOS: mWeb Safari55338-mweb-mweb-safari-001.mp4 |
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.
@@ -36,7 +38,7 @@ function WorkspaceCompanyCardsFeedAddedEmptyPage({handleAssignCard, isDisabledAs | |||
buttonAction: handleAssignCard, | |||
icon: Expensicons.Plus, | |||
success: true, | |||
isDisabled: isDisabledAssignCardButton, | |||
isDisabled: isOffline || isDisabledAssignCardButton, |
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.
@huult is this for both direct and custom feeds? this change should only be for direct feeds because only then we might be opening the popuo window
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.
Oops! I missed interpreting the expectation correctly here. Thanks for pointing out.
On thinking further, I am not that sure if we would get an empty page for direct feeds. If the empty cards page is not applicable for direct feeds, no changes are needed here. But looks like some more understanding is needed here.
And it looks like we may have to make changes in the header buttons here by considering isOffline
and isCustomFeed
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.
On thinking further, I am not that sure if we would get an empty page for direct feeds. If the empty cards page is not applicable for direct feeds, no changes are needed here. But looks like some more understanding is needed here.
Feed added empty state
can occur for direct feeds also. The test video in OP also demonstrates this. So, we need to disable Assign card
here only for Direct
feed.
And it looks like we may have to make changes in the header buttons here by considering isOffline and isCustomFeed
I think no change has to be made to the Assign card
button in the card list header.
@mountiny Is this understanding correct?
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.
@joekaufmanexpensify @dubielzyk-expensify curious for your take on this one. We have 3 cases in this flow to consider:
Do you agree with the last case? Is it ok to just make the button disabled in this case when offline? |
Yep, those all make sense to me. No need to allow clicking to assign a card offline if that is just going to prompt you to connect to your bank (which you cannot do while offline). |
All those scenarios sound reasonable to me. I don't mind throwing up a Offline RHP on the last scenario though cause at least it's telling you why you can't click the button instead of just disabling it. cc @Expensify/design and Offline Expert @trjExpensify |
Just so I'm sure I'm following your thoughts.. When you're online, if your connection has expired, clicking So the consideration here is that when you're offline, we don't know if your feed is broken or not (?), so if we let you click the button and open the RHP with the offline blocking form. If you come back online with it open, you shouldn't be seeing the actual contents of that page because of your broken connection, so now we're in a bit of a pickle because we let you open the RHP offline? |
This is just about credentials expired, which is different from broken connection. The former happens in the ordinary course of business. Most bank credentials expire after ~15 minutes and transactions still import after the credentials expire. We don't show an error here, we just prompt the customer to authenticate with their bank before they can assign more cards. We do know if their credentials are expired before they click assign card. If their credentials are expired and they're offline, we don't want to let them start the assign card flow because the first step will be linking them to connect to their bank (which can't be done offline). Broken connection is more rare and happens when there is an actual issue with the connection. There we throw an error to authenticate the connection and explain why transactions are not importing. I imagine fixing the broken connection error offline would be similar, where we disable the button to log into your bank. LMK if that makes sense @trjExpensify |
I'm good with that too. I wasn't aware we had that existing component. |
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.
@huult Thanks for the updates. Just one more small change though as mentioned here. Can you please make the success button styling in downloads
to match with the one we used? Below is the screenshot that demonstrates the need of styling change.
Also, I have left one NAB comment. Please have a look.Thanks.
Otherwise the code looks good and the changes also works well. Here are the test videos. I will complete the checklist once the review comments are addressed. 55338-web-safari-002.mp455338-web-safari-003.mp4 |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
🚧 @mountiny has triggered a test hybrid app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
🚧 @mountiny has triggered a test hybrid app build. You can view the workflow run here. |
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 think this is looking better now, I think we just need to clean up the cancel button
@rojiphil I have updated your feedback. Could you check it again? |
confirmText={translate('common.buttonConfirm')} | ||
prompt={translate('common.offlinePrompt')} | ||
shouldShowCancelButton={false} | ||
success={false} |
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! This change is not required as we need to keep the color green.
I was referring to the download
action in search
page where the button size is small and color is grey. There, button size needs to be large and color green as per comment. Is this a quick change? If not, maybe we can do this in a separate PR as mentioned here
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Yes, this is DecisionModal. I have changed the offline modal to use the DecisionModal component. This is the same as the image above.
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.
@rojiphil But the button is grey. If we want this button to be green, we must introduce new props.Do we implement this in this pull request?
App/src/components/DecisionModal.tsx
Lines 67 to 71 in ab40264
<Button | |
style={[styles.mt3, styles.noSelect]} | |
onPress={onSecondOptionSubmit} | |
text={secondOptionText} | |
/> |
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.
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.
Yeah lets do that separately
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari55338-web-safari-004.mp4MacOS: Desktop55338-desktop-002.mp4Android: Native55338-android-native-002.mp4Android: mWeb Chrome55338-mweb-chrome-002.mp4iOS: Native55338-ios-native-002.mp4iOS: mWeb Safari55338-mweb-safari-002.mp4 |
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.
@huult Thanks for the updates. Just that we need a small change in test case description to replace
Verify that the Assign Card button in the modal for direct feeds is disabled when offline.
with something like this:
When offline, verify that the
Assign Card button invokes an offline modal for direct feeds
@mountiny The changes LGTM and works well too. The only pending issue is as mentioned here where the default modal look needs to change for the button (i.e. size and color). But may be we can do this as a separate PR as mentioned here. Otherwise, over to you for review. Thanks.
Thanks everyone |
✋ 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.95-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.95-6 🚀
|
Details
MOBILE-EXPENSIFY:https://github.com/Expensify/Mobile-Expensify/pull/13401
Fixed Issues
$ #55096
PROPOSAL: #55096 (comment)
Tests
Same QA step
Offline tests
QA Steps
and
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
Screen.Recording.2025-01-16.at.10.33.06.mp4
Android: mWeb Chrome
Screen.Recording.2025-01-16.at.10.34.47.mp4
iOS: Native
Screen.Recording.2025-01-16.at.10.37.45.mp4
iOS: mWeb Safari
Screen.Recording.2025-01-16.at.10.38.51.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-01-16.at.10.39.27.mp4
MacOS: Desktop
Screen.Recording.2025-01-16.at.10.40.46.mp4