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

Jump to bottom

Resolve push notification dependency loop #14702

Merged
merged 9 commits into from
Feb 15, 2023
Merged

Resolve push notification dependency loop #14702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b1044ad
don't bundle notification type in PushNotification export
Julesssss Jan 31, 2023
66f0d8f
use separate import for NotificationType, resolving dependency loop
Julesssss Jan 31, 2023
1e62140
further separate the PushNotification import loop
Julesssss Jan 31, 2023
227de27
remove Push notification dependency loop
Julesssss Feb 1, 2023
4ac8a3d
Merge branch 'main' into jules-resolvePushNotificationDependencyLoop
Julesssss Feb 1, 2023
007f60c
invoke PushNotification.init from outside module
Julesssss Feb 2, 2023
dd81bc2
Merge branch 'main' into jules-resolvePushNotificationDependencyLoop
Julesssss Feb 2, 2023
465282a
revert notification type refactor
Julesssss Feb 2, 2023
3e4d0ec
Merge branch 'main' into jules-resolvePushNotificationDependencyLoop
Julesssss Feb 13, 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/libs/Notification/PushNotification/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import NotificationType from './NotificationType';

// Push notifications are only supported on mobile, so we'll just noop here
export default {
init: () => {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that this is actually called outside of the module, I think it makes sense to export it from the non-native file. But I'm not 100% sure it's necessary

register: () => {},
deregister: () => {},
onReceived: () => {},
Expand Down
8 changes: 0 additions & 8 deletions src/libs/Notification/PushNotification/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import {UrbanAirship, EventType, iOS} from 'urbanairship-react-native';
import lodashGet from 'lodash/get';
import Log from '../../Log';
import NotificationType from './NotificationType';
import PushNotification from '.';
import * as Report from '../../actions/Report';

const notificationEventActionMap = {};

Expand Down Expand Up @@ -109,12 +107,6 @@ function register(accountID) {
// Regardless of the user's opt-in status, we still want to receive silent push notifications.
Log.info(`[PUSH_NOTIFICATIONS] Subscribing to notifications for account ID ${accountID}`);
UrbanAirship.setNamedUser(accountID.toString());

// When the user logged out and then logged in with a different account
// while the app is still in background, we must resubscribe to the report
// push notification in order to render the report click behaviour correctly
Comment on lines -113 to -115
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 attempted to simplify this comment here

PushNotification.init();
Report.subscribeToReportCommentPushNotifications();
}

/**
Expand Down
5 changes: 5 additions & 0 deletions src/libs/actions/Session/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import * as Authentication from '../../Authentication';
import * as Welcome from '../Welcome';
import * as API from '../../API';
import * as NetworkStore from '../../Network/NetworkStore';
import * as Report from '../Report';
import DateUtils from '../../DateUtils';

let credentials = {};
Expand All @@ -40,6 +41,10 @@ Onyx.connect({

if (accountID) {
PushNotification.register(accountID);

// Prevent issue where report linking fails after users switch accounts without closing the app
PushNotification.init();
Report.subscribeToReportCommentPushNotifications();
} else {
PushNotification.deregister();
PushNotification.clearNotifications();
Expand Down