Address PR Comments

Author: alexs-mparticle

src/cookieSyncManager.ts

@@ -14,6 +14,10 @@ const hasFrequencyCapExpired = (
     frequencyCap: number,
     lastSyncDate?: number,
 ): boolean => {
+    if (!lastSyncDate) {
+        return true;
+    }
+
     return (
         new Date().getTime() >
         new Date(lastSyncDate).getTime() + frequencyCap * DAYS_IN_MILLISECONDS
@@ -61,7 +65,6 @@ export default function CookieSyncManager(
             // Url for cookie sync pixel
             const pixelUrl = replaceAmpWithAmpersand(pixelSettings.pixelUrl);
 
-            // TODO: Document requirements for redirectUrl
             const redirectUrl = pixelSettings.redirectUrl
                 ? replaceAmpWithAmpersand(pixelSettings.redirectUrl)
                 : null;
@@ -72,8 +75,6 @@ export default function CookieSyncManager(
                 redirectUrl
             );
 
-            // TODO: Is there a historic reason for checking for previousMPID?
-            //       it does not appear to be passed in anywhere
             if (previousMPID && previousMPID !== mpid) {
                 if (persistence && persistence[mpid]) {
                     if (!persistence[mpid].csd) {

test/jest/cookieSyncManager.spec.ts

@@ -218,7 +218,6 @@ describe('CookieSyncManager', () => {
             );
         });
 
-        // QUESTION: What is the purpose of this code path?
         it('should call performCookieSync with mpid if previousMpid and mpid match', () => {
             const mockMPInstance = ({
                 _Store: {
@@ -247,6 +246,33 @@ describe('CookieSyncManager', () => {
             ); 
         });
 
+        it('should perform a cookie sync if lastSyncDateForModule is null', () => {
+            const mockMPInstance = ({
+                _Store: {
+                    webviewBridgeEnabled: false,
+                    pixelConfigurations: [pixelSettings],
+                },
+                _Persistence: {
+                    getPersistence: () => ({testMPID: {}}),
+                },
+            } as unknown) as MParticleWebSDK;
+
+            const cookieSyncManager = new CookieSyncManager(mockMPInstance);
+            cookieSyncManager.performCookieSync = jest.fn();
+
+            cookieSyncManager.attemptCookieSync(null, testMPID, true);
+
+            expect(cookieSyncManager.performCookieSync).toHaveBeenCalledWith(
+                '',
+                '5',
+                testMPID,
+                {},
+                undefined,
+                true,
+                false, 
+            );
+        });
+
         it('should perform a cookie sync if lastSyncDateForModule has passed the frequency cap', () => {
             const now = new Date().getTime();
 
@@ -331,6 +357,25 @@ describe('CookieSyncManager', () => {
             expect(mockMPInstance._Store.pixelConfigurations.length).toBe(1);
             expect(mockMPInstance._Store.pixelConfigurations[0].moduleId).toBe(5);
         });
+
+        it('should not perform a cookie sync if persistence is empty', () => {
+            const mockMPInstance = ({
+                _Store: {
+                    webviewBridgeEnabled: false,
+                    pixelConfigurations: [pixelSettings],
+                },
+                _Persistence: {
+                    getPersistence: () => ({}),
+                },
+            } as unknown) as MParticleWebSDK;
+
+            const cookieSyncManager = new CookieSyncManager(mockMPInstance);
+            cookieSyncManager.performCookieSync = jest.fn();
+
+            cookieSyncManager.attemptCookieSync(null, testMPID, true);
+
+            expect(cookieSyncManager.performCookieSync).not.toHaveBeenCalled();
+        });
     });
 
     describe('#performCookieSync', () => {
@@ -560,13 +605,16 @@ describe('CookieSyncManager', () => {
 
             const cookieSyncManager = new CookieSyncManager(mockMPInstance);
 
+            // Note: We expect that the input of the pixelUrl will include a `?`
+            // (ideally at the end of the string)
+            // so that the redirectUrl can be processed by the backend correctly
             const result = cookieSyncManager.combineUrlWithRedirect(
                 '1234',
-                'https://test.com/some/path',
+                'https://test.com/some/path?',
                 'https://redirect.mparticle.com/v1/sync?esid=1234&MPID=%%mpid%%&ID=$UID&Key=testMPID&env=2'
             );
 
-            expect(result).toBe('https://test.com/some/pathhttps%3A%2F%2Fredirect.mparticle.com%2Fv1%2Fsync%3Fesid%3D1234%26amp%3BMPID%3D1234%26amp%3BID%3D%24UID%26amp%3BKey%3DtestMPID%26amp%3Benv%3D2');
+            expect(result).toBe('https://test.com/some/path?https%3A%2F%2Fredirect.mparticle.com%2Fv1%2Fsync%3Fesid%3D1234%26amp%3BMPID%3D1234%26amp%3BID%3D%24UID%26amp%3BKey%3DtestMPID%26amp%3Benv%3D2');
         });
 
         it('should return the pixelUrl if no redirectUrl is defined', () => {