Skip to content
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.

Sign up for GitHub

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 App show error when open unlink in Desktop App #23738

Merged
merged 15 commits into from
Aug 11, 2023
Merged

Fix App show error when open unlink in Desktop App #23738

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
625adbe
Should not open unlink in desktop
hoangzinh Jul 27, 2023
9e7114f
Fix unable to unlink due to translation error
hoangzinh Jul 27, 2023
870c013
Fix linter
hoangzinh Jul 27, 2023
16feab4
Remove unused util
hoangzinh Jul 27, 2023
1e4cba6
Change the regex const to CONST
hoangzinh Aug 1, 2023
361f9bb
Merge branch 'main' into df/19681_2
hoangzinh Aug 4, 2023
a1bf7f3
Update comment why unlink is in the list
hoangzinh Aug 4, 2023
6f37650
Show full screen loading until the request is completed
hoangzinh Aug 4, 2023
2bcd9f5
Fix linter
hoangzinh Aug 4, 2023
4e1067f
Merge branch 'main' into df/19681_2
hoangzinh Aug 9, 2023
3f35a3d
Fix merge conflict
hoangzinh Aug 9, 2023
f9b8d0e
Fix linter
hoangzinh Aug 9, 2023
d9586c9
Group early exit conditions
hoangzinh Aug 9, 2023
befda6d
Revert using useMemo
hoangzinh Aug 10, 2023
21f34cc
Refactor code
hoangzinh Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,6 +1182,7 @@ const CONST = {

ROUTES: {
VALIDATE_LOGIN: /\/v($|(\/\/*))/,
UNLINK_LOGIN: /\/u($|(\/\/*))/,
},
},

Expand Down
13 changes: 13 additions & 0 deletions src/components/DeeplinkWrapper/index.website.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import PropTypes from 'prop-types';
import {PureComponent} from 'react';
import Str from 'expensify-common/lib/str';
import _ from 'underscore';
import * as Browser from '../../libs/Browser';
import ROUTES from '../../ROUTES';
import * as App from '../../libs/actions/App';
Expand All @@ -18,6 +19,10 @@ class DeeplinkWrapper extends PureComponent {
return;
}

if (this.isUnsupportedDeeplinkRoute()) {
return;
}

// If the current url path is /transition..., meaning it was opened from oldDot, during this transition period:
// 1. The user session may not exist, because sign-in has not been completed yet.
// 2. There may be non-idempotent operations (e.g. create a new workspace), which obviously should not be executed again in the desktop app.
Expand All @@ -33,6 +38,14 @@ class DeeplinkWrapper extends PureComponent {
return !Browser.isMobile() && typeof navigator === 'object' && typeof navigator.userAgent === 'string' && /Mac/i.test(navigator.userAgent) && !/Electron/i.test(navigator.userAgent);
}

// A function to detect if current route should be opened in Web only.
isUnsupportedDeeplinkRoute() {
return _.some([CONST.REGEX.ROUTES.UNLINK_LOGIN], (unsupportRouteRegex) => {
const routeRegex = new RegExp(unsupportRouteRegex);
return routeRegex.test(window.location.pathname);
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name looks sufficient. Instead, we could have a comment inside explaining why the unlink login path was added here. Thoughts?

cc: @hoangzinh @francoisl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Personal preference) I don't mind the comment outside the function, even if the name sounds sufficient. But overall neutral, I'm ok if we keep it or remove it.

However, yes a comment inside explaining why the unlink route is there sounds like a good idea to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the comment inside this func. Please help me review it again. Thanks


render() {
return this.props.children;
}
Expand Down
3 changes: 1 addition & 2 deletions src/libs/actions/Session/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import Log from '../../Log';
import PushNotification from '../../Notification/PushNotification';
import Timing from '../Timing';
import CONST from '../../../CONST';
import * as Localize from '../../Localize';
import Timers from '../../Timers';
import * as Pusher from '../../Pusher/pusher';
import * as Authentication from '../../Authentication';
Expand Down Expand Up @@ -632,7 +631,7 @@ function unlinkLogin(accountID, validateCode) {
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: false,
message: Localize.translateLocal('unlinkLoginForm.succesfullyUnlinkedLogin'),
message: 'unlinkLoginForm.succesfullyUnlinkedLogin',
},
},
{
Expand Down