Open m-natarajan opened 2 weeks ago
melvin-bot[bot]
commented
2 weeks ago Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
Shahidullah-Muffakir
commented
2 weeks ago Edited by proposal-police: This proposal was edited at 2025-01-13 00:32:37 UTC.
When the user is offline:
We did not disable the Next button in the feed type selection at the following locations:
We did not disable the Assign Card button at the following location:
Disable the Assign Card button when the user is offline:
|| isOffline condition to the existing check at this location:
isDisabledAssignCardButton={!selectedFeedData || !!selectedFeedData?.errors || isOffline}Disable the Next button in the feed type selection:
BaseSelectionListProps called isConfirmButtonDisabled to convey the purpose clearly.isConfirmButtonDisabled={isOffline} at this location:
BaseSelectionList to use the new prop by setting:
disabled={isConfirmButtonDisabled}
`we may want to disable this Assign card button as well in WorkspaceCompanyCardsListHeaderButtons:
In the feed selection type, instead of disabling the next button, show the FullPageOfflineBlockingView by changing the following code in SelectFeedType.tsx as:
return (
<ScreenWrapper
testID={SelectFeedType.displayName}
includeSafeAreaPaddingBottom={false}
shouldEnablePickerAvoiding={false}
shouldEnableMaxHeight
>
<HeaderWithBackButton
title={translate('workspace.companyCards.addCards')}
onBackButtonPress={handleBackButtonPress}
/>
<FullPageOfflineBlockingView>
allgandalf
commented
2 weeks ago please assign me here, Probably not open for proposals , maybe CallStack would work on this as part of project , right @mountiny ?
melvin-bot[bot]
commented
1 week ago Job added to Upwork: https://www.upwork.com/jobs/~021878570494468361150
melvin-bot[bot]
commented
1 week ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @rojiphil (External)
mountiny
commented
1 week ago @allgandalf I think we can now export the issues to all C+ so they also start getting familiar with this flow
melvin-bot[bot]
commented
1 week ago Upwork job price has been updated to $125
mountiny
commented
1 week ago @Shahidullah-Muffakir I have updated the issue OP FYI
Shahidullah-Muffakir
commented
1 week ago @Shahidullah-Muffakir I have updated the issue OP FYI
@mountiny Thank you, Just to confirm, I’ve considered to show FullPageOfflineBlockingView when the user is offline during feed type selection.
But instead of showing this page, we should just disable the Next button. Did I understand it correctly? Thanks!
huult
commented
1 week ago When adding a direct card feed, we link you out to the bank to authenticate while offline
We did not prevent the bank connection authentication flow from opening in offline mode
To resolve this issue, we need to prevent the bank connection authentication flow from opening in offline mode and redirect to a full offline page. Once back online, the bank connection flow can be reopened, and an option to retry (e.g., ‘Please click here.’) should be provided. To implement this, we follow these steps.
update to:
if (!url || isOffline) {wrap FullPageOfflineBlockingView into this block:
<FullPageOfflineBlockingView>
<BlockingView
icon={Illustrations.PendingBank}
iconWidth={styles.pendingBankCardIllustration.width}
iconHeight={styles.pendingBankCardIllustration.height}
title={translate('workspace.moreFeatures.companyCards.pendingBankTitle')}
linkKey="workspace.moreFeatures.companyCards.pendingBankLink"
CustomSubtitle={CustomSubtitle}
shouldShowLink
onLinkPress={onOpenBankConnectionFlow}
/>
</FullPageOfflineBlockingView>With this update, we are achieving the same behavior as the native app
Optional: If we want to automatically close the authentication bank connection flow when offline, we can update the handleOpenBankConnectionFlow function as follows:
const WINDOW_WIDTH = 700;
const WINDOW_HEIGHT = 600;
const handleOpenBankConnectionFlow = (url: string) => {
if (!navigator.onLine) {
console.log('Offline mode detected. Bank connection flow cannot be opened.');
return null; // Prevent opening the flow when offline
}
const screenWidth = window.screen.width;
const screenHeight = window.screen.height;
const left = (screenWidth - WINDOW_WIDTH) / 2;
const top = (screenHeight - WINDOW_HEIGHT) / 2;
const popupFeatures = `width=${WINDOW_WIDTH},height=${WINDOW_HEIGHT},left=${left},top=${top},scrollbars=yes,resizable=yes`;
const popupWindow = window.open(url, 'popupWindow', popupFeatures);
// Function to close the popup when offline
const handleOffline = () => {
if (popupWindow && !popupWindow.closed) {
popupWindow.close();
console.log('Popup window closed due to offline mode.');
}
};
// Add an event listener for the offline event
window.addEventListener('offline', handleOffline);
// Monitor and clean up when the popup is closed manually
const popupCheckInterval = setInterval(() => {
if (popupWindow && popupWindow.closed) {
window.removeEventListener('offline', handleOffline);
clearInterval(popupCheckInterval);
console.log('Popup window closed manually.');
}
}, 1000);
return popupWindow;
};
export default handleOpenBankConnectionFlow;I think we don’t need unit tests here because this is a UI flow
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.
rojiphil
commented
1 week ago Yeah. We just need to disable the buttons using isOffline but let's use isDisabled instead of disabled for Button.
The main solution in @Shahidullah-Muffakir proposal LGTM.
🎀👀🎀 C+ reviewed
melvin-bot[bot]
commented
1 week ago Current assignee @mountiny is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
huult
commented
1 week ago @rojiphil I think we should display a full offline page on the bank account screen to ensure consistency with mobile. if we navigate to the bank account screen without displaying the full offline page, the issue will not be resolved.
mountiny
commented
1 week ago @rojiphil could you please verify the last comment? thanks!
rojiphil
commented
1 week ago I think we should display a full offline page on the bank account screen to ensure consistency with mobile.
@mountiny From a consistency point of view, I like this approach. And @huult proposal to display the full offline page on the click of the next button inside the select feed page seems to make sense. I think this is better than displaying the full offline page in place of the select feed page as per @Shahidullah-Muffakir alternative solution. We still need to disable the Assign card button but that’s a small fix though.
The other alternative is to disable the next button during offline scenarios as mentioned in OP and here. @Shahidullah-Muffakir proposal covers this aspect.
But overall I am leaning towards @huult proposal of using full offline page unless you feel otherwise.
mountiny
commented
1 week ago Yeah I think the approach from @huult looks nice
@huult please feel free to raise the PR
melvin-bot[bot]
commented
1 week ago 📣 @huult 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
huult
commented
1 week ago Thank you! I will create the PR within 24 hours.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: Reproducible in staging?: Reproducible in production?: If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @joekaufmanexpensify Slack conversation (hyperlinked to channel name): ts_external_expensify_convert
Action Performed:
Expected Result:
Authenticate via bank mode not displayed when User is in offline
Next button and the Assign card button in modal for direct feeds should be disabled when offline.
Using PAttern B is hard here as the connection expires often soon so there is high chance the call could fail
Actual Result:
Authenticate via bank mode displayed. Same result observed when you try to assign cards while offline and need to authenticate first.
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/fcd75f6f-bc7b-4a70-93ae-c237d65c999e
https://github.com/user-attachments/assets/dedf3220-d569-4a81-9eaf-33e049ee7831
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @rojiphil