Merge pull request #4989 from NativeScript/vladimirov/update-feedback-analytics

fix: ensure data is sent to analytics on preuninstall

Author: rosen-vladimirov

lib/common/commands/preuninstall.ts

@@ -27,6 +27,7 @@ export class PreUninstallCommand implements ICommand {
 		}
 
 		this.$fs.deleteFile(path.join(this.$settingsService.getProfileDir(), "KillSwitches", "cli"));
+		await this.$analyticsService.finishTracking();
 	}
 
 	private async handleIntentionalUninstall(): Promise<void> {

lib/common/declarations.d.ts

@@ -207,6 +207,12 @@ declare const enum TrackingTypes {
 	 * Defines that the broker process should get and track the data from preview app to Google Analytics
 	 */
 	PreviewAppData = "PreviewAppData",
+
+	/**
+	 * Defines that the broker process should send all the pending information to Analytics.
+	 * After that the process should send information it has finished tracking and die gracefully.
+	 */
+	FinishTracking = "FinishTracking",
 }
 
 /**
@@ -688,6 +694,7 @@ interface IAnalyticsService {
 	setStatus(settingName: string, enabled: boolean): Promise<void>;
 	getStatusMessage(settingName: string, jsonFormat: boolean, readableSettingName: string): Promise<string>;
 	isEnabled(settingName: string): Promise<boolean>;
+	finishTracking(): Promise<void>;
 
 	/**
 	 * Tracks the answer of question if user allows to be tracked.

lib/common/test/unit-tests/preuninstall.ts

@@ -30,7 +30,8 @@ describe("preuninstall", () => {
 		});
 
 		testInjector.register("analyticsService", {
-			trackEventActionInGoogleAnalytics: async (data: IEventActionData): Promise<void> => undefined
+			trackEventActionInGoogleAnalytics: async (data: IEventActionData): Promise<void> => undefined,
+			finishTracking: async(): Promise<void> => undefined
 		});
 
 		testInjector.registerCommand("dev-preuninstall", PreUninstallCommand);
@@ -83,15 +84,22 @@ describe("preuninstall", () => {
 			trackedData.push(data);
 		};
 
+		let isFinishTrackingCalled = false;
+		analyticsService.finishTracking = async (): Promise<void> => {
+			isFinishTrackingCalled = true;
+		};
+
 		const preUninstallCommand: ICommand = testInjector.resolveCommand("dev-preuninstall");
 		for (const testCase of testData) {
 			helpers.isInteractive = () => testCase.isInteractive;
 			helpers.doesCurrentNpmCommandMatch = () => testCase.isIntentionalUninstall;
+			isFinishTrackingCalled = false;
 			await preUninstallCommand.execute([]);
 			assert.deepEqual(trackedData, [{
 				action: "Uninstall CLI",
 				additionalData: testCase.expecteEventLabelData
 			}]);
+			assert.isTrue(isFinishTrackingCalled, "At the end of the command, finishTracking must be called");
 			trackedData = [];
 		}
 	});

lib/detached-processes/detached-process-enums.d.ts

@@ -5,7 +5,12 @@ declare const enum DetachedProcessMessages {
 	/**
 	 * The detached process is initialized and is ready to receive information for tracking.
 	 */
-	ProcessReadyToReceive = "ProcessReadyToReceive"
+	ProcessReadyToReceive = "ProcessReadyToReceive",
+
+	/**
+	 * The detached process finished its tasks and will now exit.
+	 */
+	ProcessFinishedTasks = "ProcessFinishedTasks"
 }
 
 /**

lib/services/analytics/analytics-broker-process.ts

@@ -29,6 +29,10 @@ const finishTracking = async (data?: ITrackingInformation) => {
 	analyticsLoggingService.logData({ message: `analytics-broker-process finish tracking started` });
 	await trackingQueue;
 	analyticsLoggingService.logData({ message: `analytics-broker-process tracking finished` });
+
+};
+
+const killCurrentProcessGracefully = () => {
 	$injector.dispose();
 	process.exit();
 };
@@ -68,12 +72,27 @@ process.on("message", async (data: ITrackingInformation) => {
 		return;
 	}
 
+	if (data.type === TrackingTypes.FinishTracking) {
+		await finishTracking();
+
+		if (process.connected) {
+			analyticsLoggingService.logData({ message: `analytics-broker-process will send ${DetachedProcessMessages.ProcessFinishedTasks} message` });
+			process.send(DetachedProcessMessages.ProcessFinishedTasks, () => {
+				analyticsLoggingService.logData({ message: `analytics-broker-process sent ${DetachedProcessMessages.ProcessFinishedTasks} message and will exit gracefully now` });
+				killCurrentProcessGracefully();
+			});
+		}
+
+		return;
+	}
+
 	await sendDataForTracking(data);
 });
 
 process.on("disconnect", async () => {
 	analyticsLoggingService.logData({ message: "analytics-broker-process received process.disconnect event" });
 	await finishTracking();
+	killCurrentProcessGracefully();
 });
 
 analyticsLoggingService.logData({ message: `analytics-broker-process will send ${DetachedProcessMessages.ProcessReadyToReceive} message` });

lib/services/analytics/analytics-service.ts

@@ -146,6 +146,34 @@ export class AnalyticsService implements IAnalyticsService, IDisposable {
 		await this.trackInGoogleAnalytics(eventActionData);
 	}
 
+	public async finishTracking(): Promise<void> {
+		return new Promise((resolve, reject) => {
+			if (this.brokerProcess && this.brokerProcess.connected) {
+				let timer: NodeJS.Timer;
+
+				const handler = (data: string) => {
+					if (data === DetachedProcessMessages.ProcessFinishedTasks) {
+						this.brokerProcess.removeListener("message", handler);
+						clearTimeout(timer);
+						resolve();
+					}
+				};
+
+				timer = setTimeout(() => {
+					this.brokerProcess.removeListener("message", handler);
+					resolve();
+				}, 3000);
+
+				this.brokerProcess.on("message", handler);
+
+				const msg = { type: TrackingTypes.FinishTracking };
+				this.brokerProcess.send(msg, (err: Error) => this.$logger.trace(`Error while sending ${JSON.stringify(msg)}`));
+			} else {
+				resolve();
+			}
+		});
+	}
+
 	private forcefullyTrackInGoogleAnalytics(gaSettings: IGoogleAnalyticsData): Promise<void> {
 		gaSettings.customDimensions = gaSettings.customDimensions || {};
 		gaSettings.customDimensions[GoogleAnalyticsCustomDimensions.client] = this.$options.analyticsClient || (isInteractive() ? AnalyticsClients.Cli : AnalyticsClients.Unknown);

preuninstall.js

@@ -3,6 +3,9 @@
 var path = require("path");
 var child_process = require("child_process");
 var commandArgs = [path.join(__dirname, "bin", "tns"), "dev-preuninstall"];
+if (process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE) {
+	commandArgs.push("--analyticsLogFile", process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE);
+}
 
 var childProcess = child_process.spawn(process.execPath, commandArgs, { stdio: "inherit"})
 

test/preuninstall.ts

@@ -6,6 +6,7 @@ const childProcess = require("child_process");
 import { SpawnOptions, ChildProcess } from "child_process";
 import * as path from "path";
 import { EventEmitter } from "events";
+import { readFileSync } from "fs";
 
 describe("preuninstall.js", () => {
 	let isSpawnCalled = false;
@@ -46,10 +47,7 @@ describe("preuninstall.js", () => {
 
 		const expectedPathToCliExecutable = path.join(__dirname, "..", "bin", "tns");
 
-		assert.isTrue(argsPassedToSpawn.indexOf(expectedPathToCliExecutable) !== -1, `The spawned args must contain path to TNS.
-				Expected path is: ${expectedPathToCliExecutable}, current args are: ${argsPassedToSpawn}.`);
-		assert.isTrue(argsPassedToSpawn.indexOf("dev-preuninstall") !== -1, `The spawned args must contain the name of the preuninstall command.
-				Expected path is: ${expectedPathToCliExecutable}, current args are: ${argsPassedToSpawn}.`);
+		assert.deepEqual(argsPassedToSpawn, [expectedPathToCliExecutable, "dev-preuninstall"]);
 		assert.deepEqual(optionsPassedToSpawn, [{ stdio: "inherit" }], "The stdio must be inherit as this way CLI's command can determine correctly if terminal is in interactive mode.");
 		assert.deepEqual(dataPassedToConsoleError, []);
 
@@ -58,6 +56,25 @@ describe("preuninstall.js", () => {
 		assert.deepEqual(dataPassedToConsoleError, [`Failed to complete all pre-uninstall steps. Error is ${errMsg}`]);
 	});
 
+	it("passes --analyticsLogFile option when NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE is set", () => {
+		const content = readFileSync(path.join(__dirname, "..", "preuninstall.js")).toString();
+		const originalEnvValue = process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE;
+		process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE = "value from env analyticsLog.txt";
+		/* tslint:disable:no-eval */
+		eval(content);
+		/* tslint:enable:no-eval */
+		process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE = originalEnvValue;
+		assert.isTrue(isSpawnCalled, "child_process.spawn must be called from preuninstall.js");
+
+		// NOTE: As the script is eval'd, the `__dirname` in it is resolved to current file's location,
+		// so the expectedPathToCliExecutable should be set as it is located in current dir.
+		const expectedPathToCliExecutable = path.join(__dirname, "bin", "tns");
+
+		assert.deepEqual(argsPassedToSpawn, [expectedPathToCliExecutable, "dev-preuninstall", "--analyticsLogFile", "value from env analyticsLog.txt"]);
+		assert.deepEqual(optionsPassedToSpawn, [{ stdio: "inherit" }], "The stdio must be inherit as this way CLI's command can determine correctly if terminal is in interactive mode.");
+		assert.deepEqual(dataPassedToConsoleError, []);
+	});
+
 	it("ensure package.json has correct preuninstall script", () => {
 		const packageJsonData = require("../package.json");
 		assert.equal(packageJsonData.scripts.preuninstall, "node preuninstall.js");