Skip to content

Commit 1b0cef7

Browse files
fadi-georgefadi-george
authored
Merge pull request #1327 from OneSignal/fg/login-before-sub-fix
Login Bug Fix
2 parents eb1d8c3 + 5c8d0ed commit 1b0cef7

File tree

6 files changed

+207
-21
lines changed

6 files changed

+207
-21
lines changed

‎src/core/executors/LoginUserOperationExecutor.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { ModelChangeTags } from 'src/core/types/models';
22
import { ExecutionResult, IOperationExecutor } from 'src/core/types/operation';
3-
import User from 'src/onesignal/User';
43
import OneSignalError from 'src/shared/errors/OneSignalError';
54
import Environment from 'src/shared/helpers/Environment';
65
import EventHelper from 'src/shared/helpers/EventHelper';
@@ -60,9 +59,7 @@ export class LoginUserOperationExecutor implements IOperationExecutor {
6059
const startingOp = operations[0];
6160

6261
if (startingOp instanceof LoginUserOperation)
63-
return this.loginUser(startingOp, operations.slice(1)).finally(() => {
64-
User.createOrGetInstance().isCreatingUser = false;
65-
});
62+
return this.loginUser(startingOp, operations.slice(1));
6663

6764
throw new Error(`Unrecognized operation: ${startingOp.name}`);
6865
}

‎src/onesignal/User.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@ import {
1212
import { isObject, isValidEmail, logMethodCall } from '../shared/utils/utils';
1313

1414
export default class User {
15-
isCreatingUser = false;
16-
1715
static singletonInstance?: User = undefined;
1816

1917
/**

‎src/onesignal/UserDirector.ts‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,9 @@ import { LoginUserOperation } from 'src/core/operations/LoginUserOperation';
55
import Log from 'src/shared/libraries/Log';
66
import { IDManager } from 'src/shared/managers/IDManager';
77
import MainHelper from '../shared/helpers/MainHelper';
8-
import User from './User';
98

109
export default class UserDirector {
1110
static async createUserOnServer(): Promise<void> {
12-
const user = User.createOrGetInstance();
13-
if (user.isCreatingUser) return;
14-
1511
const identityModel = OneSignal.coreDirector.getIdentityModel();
1612
const appId = MainHelper.getAppId();
1713

@@ -21,7 +17,6 @@ export default class UserDirector {
2117
pushOp.id = pushOp.id ?? IDManager.createLocalId();
2218
const { id, ...rest } = pushOp.toJSON();
2319

24-
User.createOrGetInstance().isCreatingUser = true;
2520
OneSignal.coreDirector.operationRepo.enqueue(
2621
new LoginUserOperation(
2722
appId,
@@ -39,11 +34,6 @@ export default class UserDirector {
3934
);
4035
}
4136

42-
static resetUserMetaProperties() {
43-
const user = User.createOrGetInstance();
44-
user.isCreatingUser = false;
45-
}
46-
4737
// Resets models similar to Android SDK
4838
// https://github.com/OneSignal/OneSignal-Android-SDK/blob/ed2e87618ea3af81b75f97b0a4cbb8f658c7fc80/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt#L448
4939
static resetUserModels() {

‎src/page/managers/LoginManager.ts‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { LoginUserOperation } from 'src/core/operations/LoginUserOperation';
22
import { TransferSubscriptionOperation } from 'src/core/operations/TransferSubscriptionOperation';
33
import { ModelChangeTags } from 'src/core/types/models';
4-
import User from 'src/onesignal/User';
54
import MainHelper from 'src/shared/helpers/MainHelper';
65
import OneSignal from '../../onesignal/OneSignal';
76
import UserDirector from '../../onesignal/UserDirector';
@@ -46,8 +45,6 @@ export default class LoginManager {
4645
);
4746
const newIdentityOneSignalId = identityModel.onesignalId;
4847

49-
User.createOrGetInstance().isCreatingUser = true;
50-
5148
const appId = MainHelper.getAppId();
5249

5350
const pushOp = await OneSignal.coreDirector.getPushSubscriptionModel();
@@ -81,7 +78,6 @@ export default class LoginManager {
8178
if (!identityModel.externalId)
8279
return Log.debug('Logout: User is not logged in, skipping logout');
8380

84-
UserDirector.resetUserMetaProperties();
8581
UserDirector.resetUserModels();
8682

8783
// create a new anonymous user

‎src/shared/managers/SubscriptionManager.test.ts‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
import { APP_ID, DUMMY_EXTERNAL_ID } from '__test__/support/constants';
2+
import TestContext from '__test__/support/environment/TestContext';
3+
import { TestEnvironment } from '__test__/support/environment/TestEnvironment';
4+
import { setupSubModelStore } from '__test__/support/environment/TestEnvironmentHelpers';
5+
import {
6+
createUserFn,
7+
setCreateUserResponse,
8+
} from '__test__/support/helpers/requests';
9+
import {
10+
getSubscriptionFn,
11+
MockServiceWorker,
12+
} from '__test__/support/mocks/MockServiceWorker';
13+
import ContextSW from '../models/ContextSW';
14+
import { RawPushSubscription } from '../models/RawPushSubscription';
15+
import { IDManager } from './IDManager';
16+
import {
17+
SubscriptionManager,
18+
SubscriptionManagerConfig,
19+
} from './SubscriptionManager';
20+
21+
const subConfig: SubscriptionManagerConfig = {
22+
appId: APP_ID,
23+
vapidPublicKey: '',
24+
onesignalVapidPublicKey: '',
25+
};
26+
27+
const getRawSubscription = (): RawPushSubscription => {
28+
const rawSubscription = new RawPushSubscription();
29+
rawSubscription.w3cAuth = 'auth';
30+
rawSubscription.w3cP256dh = 'p256dh';
31+
rawSubscription.w3cEndpoint = new URL('https://example.com/endpoint');
32+
rawSubscription.safariDeviceToken = 'safariDeviceToken';
33+
return rawSubscription;
34+
};
35+
36+
describe('SubscriptionManager', () => {
37+
beforeEach(async () => {
38+
vi.resetModules();
39+
await TestEnvironment.initialize();
40+
});
41+
42+
describe('updatePushSubscriptionModelWithRawSubscription', () => {
43+
test('should create the push subscription model if it does not exist', async () => {
44+
setCreateUserResponse();
45+
const context = new ContextSW(TestContext.getFakeMergedConfig());
46+
const subscriptionManager = new SubscriptionManager(context, subConfig);
47+
const rawSubscription = getRawSubscription();
48+
49+
let subModels =
50+
await OneSignal.coreDirector.subscriptionModelStore.list();
51+
expect(subModels.length).toBe(0);
52+
53+
// mimicing the event helper checkAndTriggerSubscriptionChanged
54+
await OneSignal.database.setPushToken(
55+
rawSubscription.w3cEndpoint?.toString(),
56+
);
57+
58+
await subscriptionManager._updatePushSubscriptionModelWithRawSubscription(
59+
rawSubscription,
60+
);
61+
62+
subModels = await OneSignal.coreDirector.subscriptionModelStore.list();
63+
expect(subModels.length).toBe(1);
64+
65+
const id = subModels[0].id;
66+
expect(IDManager.isLocalId(id)).toBe(true);
67+
expect(subModels[0].toJSON()).toEqual({
68+
id,
69+
device_model: '',
70+
device_os: 56,
71+
enabled: true,
72+
notification_types: 1,
73+
sdk: '1',
74+
token: rawSubscription.w3cEndpoint?.toString(),
75+
type: 'ChromePush',
76+
web_auth: rawSubscription.w3cAuth,
77+
web_p256: rawSubscription.w3cP256dh,
78+
});
79+
80+
await vi.waitUntil(() => createUserFn.mock.calls.length > 0);
81+
expect(createUserFn).toHaveBeenCalledWith({
82+
identity: {},
83+
properties: {
84+
language: 'en',
85+
timezone_id: 'America/Los_Angeles',
86+
},
87+
refresh_device_metadata: true,
88+
subscriptions: [
89+
{
90+
device_model: '',
91+
device_os: 56,
92+
enabled: true,
93+
notification_types: 1,
94+
sdk: '1',
95+
token: rawSubscription.w3cEndpoint?.toString(),
96+
type: 'ChromePush',
97+
web_auth: rawSubscription.w3cAuth,
98+
web_p256: rawSubscription.w3cP256dh,
99+
},
100+
],
101+
});
102+
});
103+
104+
test('should create user if push subscription model does not have an id', async () => {
105+
const generatePushSubscriptionModelSpy = vi.spyOn(
106+
OneSignal.coreDirector,
107+
'generatePushSubscriptionModel',
108+
);
109+
const rawSubscription = getRawSubscription();
110+
getSubscriptionFn.mockResolvedValue({
111+
endpoint: rawSubscription.w3cEndpoint?.toString(),
112+
});
113+
setCreateUserResponse({
114+
externalId: 'some-external-id',
115+
});
116+
117+
const context = new ContextSW(TestContext.getFakeMergedConfig());
118+
const subscriptionManager = new SubscriptionManager(context, subConfig);
119+
120+
// create push sub with no id
121+
const identityModel = OneSignal.coreDirector.getIdentityModel();
122+
identityModel.onesignalId = IDManager.createLocalId();
123+
identityModel.externalId = 'some-external-id';
124+
125+
await setupSubModelStore({
126+
id: '',
127+
token: rawSubscription.w3cEndpoint?.toString(),
128+
onesignalId: identityModel.onesignalId,
129+
});
130+
131+
await subscriptionManager._updatePushSubscriptionModelWithRawSubscription(
132+
rawSubscription,
133+
);
134+
135+
// should not call generatePushSubscriptionModelSpy
136+
expect(generatePushSubscriptionModelSpy).not.toHaveBeenCalled();
137+
138+
expect(createUserFn).toHaveBeenCalledWith({
139+
identity: {
140+
external_id: 'some-external-id',
141+
},
142+
properties: {
143+
language: 'en',
144+
timezone_id: 'America/Los_Angeles',
145+
},
146+
refresh_device_metadata: true,
147+
subscriptions: [
148+
{
149+
device_model: '',
150+
device_os: 56,
151+
enabled: true,
152+
notification_types: 1,
153+
sdk: '1',
154+
token: rawSubscription.w3cEndpoint?.toString(),
155+
type: 'ChromePush',
156+
},
157+
],
158+
});
159+
});
160+
161+
test('should update the push subscription model if it already exists', async () => {
162+
setCreateUserResponse();
163+
const context = new ContextSW(TestContext.getFakeMergedConfig());
164+
const subscriptionManager = new SubscriptionManager(context, subConfig);
165+
const rawSubscription = getRawSubscription();
166+
167+
await OneSignal.database.setPushToken(
168+
rawSubscription.w3cEndpoint?.toString(),
169+
);
170+
171+
const pushModel = await setupSubModelStore({
172+
id: '123',
173+
token: rawSubscription.w3cEndpoint?.toString(),
174+
onesignalId: DUMMY_EXTERNAL_ID,
175+
});
176+
pushModel.web_auth = 'old-web-auth';
177+
pushModel.web_p256 = 'old-web-p256';
178+
179+
await subscriptionManager._updatePushSubscriptionModelWithRawSubscription(
180+
rawSubscription,
181+
);
182+
183+
const updatedPushModel =
184+
(await OneSignal.coreDirector.getPushSubscriptionModel())!;
185+
expect(updatedPushModel.token).toBe(
186+
rawSubscription.w3cEndpoint?.toString(),
187+
);
188+
expect(updatedPushModel.web_auth).toBe(rawSubscription.w3cAuth);
189+
expect(updatedPushModel.web_p256).toBe(rawSubscription.w3cP256dh);
190+
});
191+
});
192+
});
193+
194+
Object.defineProperty(global.navigator, 'serviceWorker', {
195+
value: new MockServiceWorker(),
196+
writable: true,
197+
});

‎src/shared/managers/SubscriptionManager.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,11 +174,19 @@ export class SubscriptionManager {
174174
rawPushSubscription: RawPushSubscription,
175175
) {
176176
const pushModel = await OneSignal.coreDirector.getPushSubscriptionModel();
177-
177+
// EventHelper checkAndTriggerSubscriptionChanged is called before this function when permission is granted and so
178+
// it will save the push token/id to the database so we don't need to save the token afer generating
178179
if (!pushModel) {
179180
OneSignal.coreDirector.generatePushSubscriptionModel(rawPushSubscription);
180181
return UserDirector.createUserOnServer();
182+
183+
// Bug w/ v160400 release where isCreatingUser was improperly set and never reset
184+
// so a check if pushModel id is needed to recreate the user
181185
}
186+
if (!pushModel.id) {
187+
return UserDirector.createUserOnServer();
188+
}
189+
182190
// resubscribing. update existing push subscription model
183191
const serializedSubscriptionRecord = new FuturePushSubscriptionRecord(
184192
rawPushSubscription,

0 commit comments

Comments
 (0)