From 0a7adfa391f2a9695a5a6b8c877ba2cd020fbed4 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Mon, 26 Aug 2019 17:59:17 +0300 Subject: [PATCH 1/8] feat: add tracking for companies Add tracking with information about the company from which CLI is being used. Use the insights endpoint to get information about the company and track it as custom dimensions on each hit. >NOTE: Information will be tracked only when CLI's usage-reporting is enabled. In case you do not want to have this information tracked, execute `tns usage-reporting disable`. --- config/config.json | 1 + lib/bootstrap.ts | 1 + .../google-analytics-custom-dimensions.d.ts | 7 +- lib/config.ts | 1 + lib/declarations.d.ts | 2 +- lib/definitions/company-insights-service.d.ts | 59 ++++++ .../analytics/google-analytics-provider.ts | 10 + lib/services/company-insights-service.ts | 33 ++++ test/services/company-insights-service.ts | 177 ++++++++++++++++++ 9 files changed, 289 insertions(+), 2 deletions(-) create mode 100644 lib/definitions/company-insights-service.d.ts create mode 100644 lib/services/company-insights-service.ts create mode 100644 test/services/company-insights-service.ts diff --git a/config/config.json b/config/config.json index eee0ce45e1..c5ef90c67a 100644 --- a/config/config.json +++ b/config/config.json @@ -6,6 +6,7 @@ "DISABLE_HOOKS": false, "UPLOAD_PLAYGROUND_FILES_ENDPOINT": "https://play.nativescript.org/api/files", "SHORTEN_URL_ENDPOINT": "https://play.nativescript.org/api/shortenurl?longUrl=%s", + "INSIGHTS_URL_ENDPOINT": "https://play.nativescript.org/api/insights", "PREVIEW_APP_ENVIRONMENT": "live", "GA_TRACKING_ID": "UA-111455-51" } \ No newline at end of file diff --git a/lib/bootstrap.ts b/lib/bootstrap.ts index 2035630fc5..44d12e0bdf 100644 --- a/lib/bootstrap.ts +++ b/lib/bootstrap.ts @@ -67,6 +67,7 @@ $injector.require("userSettingsService", "./services/user-settings-service"); $injector.requirePublic("analyticsSettingsService", "./services/analytics-settings-service"); $injector.require("analyticsService", "./services/analytics/analytics-service"); $injector.require("googleAnalyticsProvider", "./services/analytics/google-analytics-provider"); +$injector.requirePublicClass("companyInsightsService", "./services/company-insights-service"); $injector.require("platformCommandParameter", "./platform-command-param"); $injector.requireCommand("create", "./commands/create-project"); diff --git a/lib/common/services/analytics/google-analytics-custom-dimensions.d.ts b/lib/common/services/analytics/google-analytics-custom-dimensions.d.ts index 3559a5778d..a7f8332ebb 100644 --- a/lib/common/services/analytics/google-analytics-custom-dimensions.d.ts +++ b/lib/common/services/analytics/google-analytics-custom-dimensions.d.ts @@ -7,5 +7,10 @@ declare const enum GoogleAnalyticsCustomDimensions { nodeVersion = "cd6", playgroundId = "cd7", usedTutorial = "cd8", - isShared = "cd9" + isShared = "cd9", + companyName = "cd10", + companyCountry = "cd11", + companyRevenue = "cd12", + companyIndustries = "cd13", + companyEmployeeCount = "cd14", } \ No newline at end of file diff --git a/lib/config.ts b/lib/config.ts index 6d1f8acdce..955726a0ac 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -8,6 +8,7 @@ export class Configuration implements IConfiguration { // User specific config USE_POD_SANDBOX: boolean = false; UPLOAD_PLAYGROUND_FILES_ENDPOINT: string = null; SHORTEN_URL_ENDPOINT: string = null; + INSIGHTS_URL_ENDPOINT: string = null; PREVIEW_APP_ENVIRONMENT: string = null; GA_TRACKING_ID: string = null; DISABLE_HOOKS: boolean = false; diff --git a/lib/declarations.d.ts b/lib/declarations.d.ts index 18bb7c2e39..f940044f9d 100644 --- a/lib/declarations.d.ts +++ b/lib/declarations.d.ts @@ -431,6 +431,7 @@ interface IConfiguration extends Config.IConfig { USE_POD_SANDBOX: boolean; UPLOAD_PLAYGROUND_FILES_ENDPOINT: string; SHORTEN_URL_ENDPOINT: string; + INSIGHTS_URL_ENDPOINT: string; PREVIEW_APP_ENVIRONMENT: string; GA_TRACKING_ID: string; } @@ -617,7 +618,6 @@ interface IITMSData { user: IApplePortalUserDetail; applicationSpecificPassword: string; - /** * Path to a .ipa file which will be uploaded. * @type {string} diff --git a/lib/definitions/company-insights-service.d.ts b/lib/definitions/company-insights-service.d.ts new file mode 100644 index 0000000000..511b5fa53a --- /dev/null +++ b/lib/definitions/company-insights-service.d.ts @@ -0,0 +1,59 @@ +/** + * Describes the information for a company. + */ +interface ICompanyData { + /** + * The name of the company. + */ + name: string; + + /** + * The country where the company is located. + */ + country: string; + + /** + * The revenue (stringified) of the company. + */ + revenue: string; + + /** + * The industries in which the company is determined to work. + * NOTE: The single value contains multiple industries separated with __ + */ + industries: string; + + /** + * Number of employees in the company (stringified). + */ + employeeCount: string; +} + +/** + * Describes information about the company returned by the Playground's /api/insights endpoint. + */ +interface IPlaygroundInsightsCompanyData { + name: string; + country: string; + revenue: string; + industries: string[]; + employeeCount: string; +} + +/** + * Describes the information returned by the Playground's /api/insights endpoint. + */ +interface IPlaygroundInsightsEndpointData { + company: IPlaygroundInsightsCompanyData; +} + +/** + * Describes the service that can be used to get insights about the company using the CLI. + */ +interface ICompanyInsightsService { + /** + * Describes information about the company. + * @returns {Promise} + */ + getCompanyData(): Promise; +} diff --git a/lib/services/analytics/google-analytics-provider.ts b/lib/services/analytics/google-analytics-provider.ts index 9173318793..d1f73c2509 100644 --- a/lib/services/analytics/google-analytics-provider.ts +++ b/lib/services/analytics/google-analytics-provider.ts @@ -12,6 +12,7 @@ export class GoogleAnalyticsProvider implements IGoogleAnalyticsProvider { private $logger: ILogger, private $proxyService: IProxyService, private $config: IConfiguration, + private $companyInsightsService: ICompanyInsightsService, private analyticsLoggingService: IFileLogService) { } @@ -80,6 +81,15 @@ export class GoogleAnalyticsProvider implements IGoogleAnalyticsProvider { defaultValues[GoogleAnalyticsCustomDimensions.usedTutorial] = playgrounInfo.usedTutorial.toString(); } + const companyData = await this.$companyInsightsService.getCompanyData(); + if (companyData) { + defaultValues[GoogleAnalyticsCustomDimensions.companyName] = companyData.name; + defaultValues[GoogleAnalyticsCustomDimensions.companyCountry] = companyData.country; + defaultValues[GoogleAnalyticsCustomDimensions.companyRevenue] = companyData.revenue; + defaultValues[GoogleAnalyticsCustomDimensions.companyIndustries] = companyData.industries; + defaultValues[GoogleAnalyticsCustomDimensions.companyEmployeeCount] = companyData.employeeCount; + } + customDimensions = _.merge(defaultValues, customDimensions); _.each(customDimensions, (value, key) => { diff --git a/lib/services/company-insights-service.ts b/lib/services/company-insights-service.ts new file mode 100644 index 0000000000..d0f20308d6 --- /dev/null +++ b/lib/services/company-insights-service.ts @@ -0,0 +1,33 @@ +import { AnalyticsEventLabelDelimiter } from "../constants"; +import { cache } from "../common/decorators"; + +export class CompanyInsightsService implements ICompanyInsightsService { + constructor(private $config: IConfiguration, + private $httpClient: Server.IHttpClient, + private $logger: ILogger) { } + + @cache() + public async getCompanyData(): Promise { + let companyData: ICompanyData = null; + try { + const response = await this.$httpClient.httpRequest(this.$config.INSIGHTS_URL_ENDPOINT); + const data = (JSON.parse(response.body)); + if (data.company) { + const industries = _.isArray(data.company.industries) ? data.company.industries.join(AnalyticsEventLabelDelimiter) : null; + companyData = { + name: data.company.name, + country: data.company.country, + revenue: data.company.revenue, + employeeCount: data.company.employeeCount, + industries + }; + } + } catch (err) { + this.$logger.trace(`Unable to get data for company. Error is: ${err}`); + } + + return companyData; + } +} + +$injector.register("companyInsightsService", CompanyInsightsService); diff --git a/test/services/company-insights-service.ts b/test/services/company-insights-service.ts new file mode 100644 index 0000000000..96400d203a --- /dev/null +++ b/test/services/company-insights-service.ts @@ -0,0 +1,177 @@ +import { assert } from "chai"; +import { Yok } from "../../lib/common/yok"; +import { LoggerStub } from "../stubs"; +import { CompanyInsightsService } from "../../lib/services/company-insights-service"; + +describe("companyInsightsService", () => { + const insightsUrlEndpoint = "/api/insights"; + const createTestInjector = (): IInjector => { + const testInjector = new Yok(); + testInjector.register("config", { + INSIGHTS_URL_ENDPOINT: insightsUrlEndpoint + }); + testInjector.register("httpClient", {}); + testInjector.register("logger", LoggerStub); + testInjector.register("companyInsightsService", CompanyInsightsService); + return testInjector; + }; + + describe("getCompanyData", () => { + describe("returns null when", () => { + it("the http client fails to get data", async () => { + const testInjector = createTestInjector(); + const companyInsightsService = testInjector.resolve("companyInsightsService"); + const httpClient = testInjector.resolve("httpClient"); + const errMsg = "custom error"; + httpClient.httpRequest = async () => { + throw new Error(errMsg); + }; + + const companyData = await companyInsightsService.getCompanyData(); + assert.isNull(companyData); + const logger = testInjector.resolve("logger"); + assert.isTrue(logger.traceOutput.indexOf(errMsg) !== -1); + }); + + it("the body of the response is not a valid JSON", async () => { + const testInjector = createTestInjector(); + const companyInsightsService = testInjector.resolve("companyInsightsService"); + const httpClient = testInjector.resolve("httpClient"); + httpClient.httpRequest = async (): Promise => { + return { + body: "invalid JSON" + }; + }; + + const companyData = await companyInsightsService.getCompanyData(); + assert.isNull(companyData); + const logger = testInjector.resolve("logger"); + assert.isTrue(logger.traceOutput.indexOf("SyntaxError: Unexpected token") !== -1); + }); + + it("response does not contain company property", async () => { + const httpResultData = { + foo: "bar" + }; + + const testInjector = createTestInjector(); + const companyInsightsService = testInjector.resolve("companyInsightsService"); + const httpClient = testInjector.resolve("httpClient"); + httpClient.httpRequest = async (): Promise => { + return { + body: JSON.stringify(httpResultData) + }; + }; + + const companyData = await companyInsightsService.getCompanyData(); + assert.deepEqual(companyData, null); + }); + }); + + describe("returns correct data when", () => { + it("response contains company property", async () => { + const httpResultData = { + company: { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + industries: [ + "Software", + "Software 2" + ], + employeeCount: "500" + } + }; + + const testInjector = createTestInjector(); + const companyInsightsService = testInjector.resolve("companyInsightsService"); + const httpClient = testInjector.resolve("httpClient"); + httpClient.httpRequest = async (): Promise => { + return { + body: JSON.stringify(httpResultData) + }; + }; + + const companyData = await companyInsightsService.getCompanyData(); + assert.deepEqual(companyData, { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + industries: "Software__Software 2", + employeeCount: "500" + }); + }); + + it("response contains company property and industries in it are not populated", async () => { + const httpResultData = { + company: { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + employeeCount: "500" + } + }; + + const testInjector = createTestInjector(); + const companyInsightsService = testInjector.resolve("companyInsightsService"); + const httpClient = testInjector.resolve("httpClient"); + httpClient.httpRequest = async (): Promise => { + return { + body: JSON.stringify(httpResultData) + }; + }; + + const companyData = await companyInsightsService.getCompanyData(); + assert.deepEqual(companyData, { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + industries: null, + employeeCount: "500" + }); + }); + }); + + it("is called only once per process", async () => { + const httpResultData = { + company: { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + industries: [ + "Software", + "Software 2" + ], + employeeCount: "500" + } + }; + + const testInjector = createTestInjector(); + const companyInsightsService = testInjector.resolve("companyInsightsService"); + const httpClient = testInjector.resolve("httpClient"); + let httpRequestCounter = 0; + httpClient.httpRequest = async (): Promise => { + httpRequestCounter++; + return { + body: JSON.stringify(httpResultData) + }; + }; + + const expectedData = { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + industries: "Software__Software 2", + employeeCount: "500" + }; + const companyData = await companyInsightsService.getCompanyData(); + assert.deepEqual(companyData, expectedData); + assert.equal(httpRequestCounter, 1); + + const companyDataSecondCall = await companyInsightsService.getCompanyData(); + assert.deepEqual(companyDataSecondCall, expectedData); + + assert.equal(httpRequestCounter, 1); + }); + }); +}); From 545b3a53d09994cf90fe6e0ea70218c8991b1fe1 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Tue, 27 Aug 2019 00:21:32 +0300 Subject: [PATCH 2/8] feat: add ipService to get current ip address Add a new ipService to get the current ip address. As this information is critical for us, use three different ways to determine the ip address, so even if one of them fails, use the result from the next one. Use the `play.nativescript.org/api/whoami` endpoint to get data for the public ip address. --- config/config.json | 1 + lib/bootstrap.ts | 1 + lib/config.ts | 1 + lib/declarations.d.ts | 1 + lib/definitions/ip-service.d.ts | 10 +++ lib/services/ip-service.ts | 58 +++++++++++++ npm-shrinkwrap.json | 2 +- test/services/ip-service.ts | 139 ++++++++++++++++++++++++++++++++ 8 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 lib/definitions/ip-service.d.ts create mode 100644 lib/services/ip-service.ts create mode 100644 test/services/ip-service.ts diff --git a/config/config.json b/config/config.json index c5ef90c67a..953ccf8bdf 100644 --- a/config/config.json +++ b/config/config.json @@ -7,6 +7,7 @@ "UPLOAD_PLAYGROUND_FILES_ENDPOINT": "https://play.nativescript.org/api/files", "SHORTEN_URL_ENDPOINT": "https://play.nativescript.org/api/shortenurl?longUrl=%s", "INSIGHTS_URL_ENDPOINT": "https://play.nativescript.org/api/insights", + "WHOAMI_URL_ENDPOINT": "https://play.nativescript.org/api/whoami", "PREVIEW_APP_ENVIRONMENT": "live", "GA_TRACKING_ID": "UA-111455-51" } \ No newline at end of file diff --git a/lib/bootstrap.ts b/lib/bootstrap.ts index 44d12e0bdf..8c22720c4f 100644 --- a/lib/bootstrap.ts +++ b/lib/bootstrap.ts @@ -229,3 +229,4 @@ $injector.require("watchIgnoreListService", "./services/watch-ignore-list-servic $injector.requirePublicClass("initializeService", "./services/initialize-service"); $injector.require("npmConfigService", "./services/npm-config-service"); +$injector.require("ipService", "./services/ip-service"); diff --git a/lib/config.ts b/lib/config.ts index 955726a0ac..702b102022 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -9,6 +9,7 @@ export class Configuration implements IConfiguration { // User specific config UPLOAD_PLAYGROUND_FILES_ENDPOINT: string = null; SHORTEN_URL_ENDPOINT: string = null; INSIGHTS_URL_ENDPOINT: string = null; + WHOAMI_URL_ENDPOINT: string = null; PREVIEW_APP_ENVIRONMENT: string = null; GA_TRACKING_ID: string = null; DISABLE_HOOKS: boolean = false; diff --git a/lib/declarations.d.ts b/lib/declarations.d.ts index f940044f9d..d2110fac42 100644 --- a/lib/declarations.d.ts +++ b/lib/declarations.d.ts @@ -432,6 +432,7 @@ interface IConfiguration extends Config.IConfig { UPLOAD_PLAYGROUND_FILES_ENDPOINT: string; SHORTEN_URL_ENDPOINT: string; INSIGHTS_URL_ENDPOINT: string; + WHOAMI_URL_ENDPOINT: string; PREVIEW_APP_ENVIRONMENT: string; GA_TRACKING_ID: string; } diff --git a/lib/definitions/ip-service.d.ts b/lib/definitions/ip-service.d.ts new file mode 100644 index 0000000000..8db6f5f976 --- /dev/null +++ b/lib/definitions/ip-service.d.ts @@ -0,0 +1,10 @@ +/** + * Describes the service used to get information for the current IP Address. + */ +interface IIPService { + /** + * Gives information about the current public IPv4 address. + * @returns {Promise} The IP address or null in case unable to find the current IP. + */ + getCurrentIPv4Address(): Promise; +} diff --git a/lib/services/ip-service.ts b/lib/services/ip-service.ts new file mode 100644 index 0000000000..d1a5d9e17d --- /dev/null +++ b/lib/services/ip-service.ts @@ -0,0 +1,58 @@ +export class IPService implements IIPService { + private static GET_IP_TIMEOUT = 1000; + constructor(private $config: IConfiguration, + private $httpClient: Server.IHttpClient, + private $logger: ILogger) { } + + public async getCurrentIPv4Address(): Promise { + const ipAddress = await this.getIPAddressFromServiceReturningJSONWithIPProperty(this.$config.WHOAMI_URL_ENDPOINT) || + await this.getIPAddressFromServiceReturningJSONWithIPProperty("https://api.myip.com") || + await this.getIPAddressFromIpifyOrgAPI() || + null; + + return ipAddress; + } + + private async getIPAddressFromServiceReturningJSONWithIPProperty(apiEndpoint: string): Promise { + let ipAddress: string = null; + try { + const response = await this.$httpClient.httpRequest({ + method: "GET", + url: apiEndpoint, + timeout: IPService.GET_IP_TIMEOUT + }); + + this.$logger.trace(`${apiEndpoint} returns ${response.body}`); + + const jsonData = JSON.parse(response.body); + ipAddress = jsonData.ip; + } catch (err) { + this.$logger.trace(`Unable to get information about current IP Address from ${apiEndpoint} Error is:`, err); + } + + return ipAddress; + } + + private async getIPAddressFromIpifyOrgAPI(): Promise { + // https://www.ipify.org/ + const ipifyOrgAPIEndpoint = "https://api.ipify.org"; + let ipAddress: string = null; + try { + const response = await this.$httpClient.httpRequest({ + method: "GET", + url: ipifyOrgAPIEndpoint, + timeout: IPService.GET_IP_TIMEOUT + }); + + this.$logger.trace(`${ipifyOrgAPIEndpoint} returns ${response.body}`); + + ipAddress = (response.body || '').toString(); + } catch (err) { + this.$logger.trace(`Unable to get information about current IP Address from ${ipifyOrgAPIEndpoint} Error is:`, err); + } + + return ipAddress; + } +} + +$injector.register("ipService", IPService); diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index ed0aebd4f8..f8ee065a88 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -8497,4 +8497,4 @@ "integrity": "sha512-99p+ohUBZ2Es0AXrw/tpazMcJ0/acpdQXr0UPrVWF0p7i8XiOYvjiXTdwXUVCTPopBGCSDtWBzOoYNPtF3z/8w==" } } -} +} \ No newline at end of file diff --git a/test/services/ip-service.ts b/test/services/ip-service.ts new file mode 100644 index 0000000000..0d91ebe0d5 --- /dev/null +++ b/test/services/ip-service.ts @@ -0,0 +1,139 @@ +import { Yok } from "../../lib/common/yok"; +import { LoggerStub } from "../stubs"; +import { IPService } from "../../lib/services/ip-service"; +import { assert } from "chai"; + +describe("ipService", () => { + const ip = "8.8.8.8"; + const whoamiDefaultEndpoint = "https://who.am.i/api/whoami"; + const errorMsgForDefaultEndpoint = `Unable to get data from ${whoamiDefaultEndpoint}`; + const errMsgForMyipCom = "Unable to get data from myip.com"; + const errMsgForIpifyOrg = "Unable to get data from ipify.org"; + + const createTestInjector = (): IInjector => { + const testInjector = new Yok(); + testInjector.register("httpClient", { + httpRequest: async (options: any, proxySettings?: IProxySettings): Promise => ({}) + }); + + testInjector.register("logger", LoggerStub); + testInjector.register("ipService", IPService); + testInjector.register("config", { + WHOAMI_URL_ENDPOINT: whoamiDefaultEndpoint + }); + return testInjector; + }; + + describe("getCurrentIPv4Address", () => { + it("returns result from default service (play.nativescript.org) when it succeeds", async () => { + const testInjector = createTestInjector(); + const httpClient = testInjector.resolve("httpClient"); + const httpRequestPassedOptions: any[] = []; + httpClient.httpRequest = async (options: any, proxySettings?: IProxySettings): Promise => { + httpRequestPassedOptions.push(options); + return { body: JSON.stringify({ ip }) }; + }; + + const ipService = testInjector.resolve("ipService"); + const ipAddress = await ipService.getCurrentIPv4Address(); + + assert.equal(ipAddress, ip); + assert.deepEqual(httpRequestPassedOptions, [{ method: "GET", url: whoamiDefaultEndpoint, timeout: 1000 }]); + }); + + it("returns result from myip.com when the default endpoint fails", async () => { + const testInjector = createTestInjector(); + const httpClient = testInjector.resolve("httpClient"); + const httpRequestPassedOptions: any[] = []; + httpClient.httpRequest = async (options: any, proxySettings?: IProxySettings): Promise => { + httpRequestPassedOptions.push(options); + if (options.url === whoamiDefaultEndpoint) { + throw new Error(errorMsgForDefaultEndpoint); + } + return { body: JSON.stringify({ ip }) }; + }; + + const ipService = testInjector.resolve("ipService"); + const ipAddress = await ipService.getCurrentIPv4Address(); + + assert.equal(ipAddress, ip); + assert.deepEqual(httpRequestPassedOptions, [ + { method: "GET", url: whoamiDefaultEndpoint, timeout: 1000 }, + { method: "GET", url: "https://api.myip.com", timeout: 1000 } + ]); + + const logger = testInjector.resolve("logger"); + assert.isTrue(logger.traceOutput.indexOf(errorMsgForDefaultEndpoint) !== -1, `Trace output\n'${logger.traceOutput}'\ndoes not contain expected message:\n${errorMsgForDefaultEndpoint}`); + + }); + + it("returns result from ipify.com when it default endpoint and myip.comm both fail", async () => { + const testInjector = createTestInjector(); + const httpClient = testInjector.resolve("httpClient"); + const httpRequestPassedOptions: any[] = []; + httpClient.httpRequest = async (options: any, proxySettings?: IProxySettings): Promise => { + httpRequestPassedOptions.push(options); + if (options.url === whoamiDefaultEndpoint) { + throw new Error(errorMsgForDefaultEndpoint); + } + + if (options.url === "https://api.myip.com") { + throw new Error(errMsgForMyipCom); + } + + return { body: ip }; + }; + + const ipService = testInjector.resolve("ipService"); + const ipAddress = await ipService.getCurrentIPv4Address(); + + assert.equal(ipAddress, ip); + assert.deepEqual(httpRequestPassedOptions, [ + { method: "GET", url: whoamiDefaultEndpoint, timeout: 1000 }, + { method: "GET", url: "https://api.myip.com", timeout: 1000 }, + { method: "GET", url: "https://api.ipify.org", timeout: 1000 } + ]); + + const logger = testInjector.resolve("logger"); + assert.isTrue(logger.traceOutput.indexOf(errorMsgForDefaultEndpoint) !== -1, `Trace output\n'${logger.traceOutput}'\ndoes not contain expected message:\n${errorMsgForDefaultEndpoint}`); + assert.isTrue(logger.traceOutput.indexOf(errMsgForMyipCom) !== -1, `Trace output\n'${logger.traceOutput}'\ndoes not contain expected message:\n${errMsgForMyipCom}`); + }); + + it("returns null when all endpoints fail", async () => { + const testInjector = createTestInjector(); + const httpClient = testInjector.resolve("httpClient"); + const httpRequestPassedOptions: any[] = []; + httpClient.httpRequest = async (options: any, proxySettings?: IProxySettings): Promise => { + httpRequestPassedOptions.push(options); + if (options.url === whoamiDefaultEndpoint) { + throw new Error(errorMsgForDefaultEndpoint); + } + + if (options.url === "https://api.myip.com") { + throw new Error(errMsgForMyipCom); + } + + if (options.url === "https://api.ipify.org") { + throw new Error(errMsgForIpifyOrg); + } + + return { body: ip }; + }; + + const ipService = testInjector.resolve("ipService"); + const ipAddress = await ipService.getCurrentIPv4Address(); + + assert.isNull(ipAddress); + assert.deepEqual(httpRequestPassedOptions, [ + { method: "GET", url: whoamiDefaultEndpoint, timeout: 1000 }, + { method: "GET", url: "https://api.myip.com", timeout: 1000 }, + { method: "GET", url: "https://api.ipify.org", timeout: 1000 } + ]); + + const logger = testInjector.resolve("logger"); + assert.isTrue(logger.traceOutput.indexOf(errorMsgForDefaultEndpoint) !== -1, `Trace output\n'${logger.traceOutput}'\ndoes not contain expected message:\n${errorMsgForDefaultEndpoint}`); + assert.isTrue(logger.traceOutput.indexOf(errMsgForMyipCom) !== -1, `Trace output\n'${logger.traceOutput}'\ndoes not contain expected message:\n${errMsgForMyipCom}`); + assert.isTrue(logger.traceOutput.indexOf(errMsgForIpifyOrg) !== -1, `Trace output\n'${logger.traceOutput}'\ndoes not contain expected message:\n${errMsgForMyipCom}`); + }); + }); +}); From 478e56b2289555348c7b008f1ab3db9e61b6e2e2 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Tue, 27 Aug 2019 00:55:01 +0300 Subject: [PATCH 3/8] fix: handle correctly non-shared instances in injector Currently `$injector` can have non-shared instances by setting the third argument of the `register` method to false, i.e.: ``` $injector.register("myClass", MyClass, false); ``` However, in this case, the injector caches only the last created instance and this way it will not call the dispose method of all created instances. Fix this by caching all instances in an array and iterrate over them when disposing them. This way we can have correct resolving of instances by name when constructor has non-injectable dependencies: ``` class A { constructor(private stringArg: string) { } } $injector.register("a", A, false); // in another file: $injector.resolve("a", { stringArg: "stringValue" }); //returns new instance; $injector.resolve("a", { stringArg: "different stringValue" }); //returns new instance; ``` --- lib/common/test/unit-tests/yok.ts | 24 ++++++++++++++++++++++ lib/common/yok.ts | 33 +++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/lib/common/test/unit-tests/yok.ts b/lib/common/test/unit-tests/yok.ts index 762ac2832c..d6e28ae233 100644 --- a/lib/common/test/unit-tests/yok.ts +++ b/lib/common/test/unit-tests/yok.ts @@ -227,6 +227,30 @@ describe("yok", () => { assert.isTrue(thing.disposed); }); + it("disposes all instances", () => { + const injector = new Yok(); + + function Thing(arg: string) { this.arg = arg; /* intentionally left blank */ } + + Thing.prototype.dispose = function () { + this.disposed = true; + }; + + injector.register("thing", Thing, false); + const thing1 = injector.resolve("thing", { arg: "thing1"}); + const thing2 = injector.resolve("thing", { arg: "thing2"}); + const thing3 = injector.resolve("thing", { arg: "thing3"}); + + assert.equal(thing1.arg, "thing1"); + assert.equal(thing2.arg, "thing2"); + assert.equal(thing3.arg, "thing3"); + injector.dispose(); + + assert.isTrue(thing1.disposed); + assert.isTrue(thing2.disposed); + assert.isTrue(thing3.disposed); + }); + }); describe("classes", () => { diff --git a/lib/common/yok.ts b/lib/common/yok.ts index efcb5e0878..87e749103e 100644 --- a/lib/common/yok.ts +++ b/lib/common/yok.ts @@ -40,7 +40,7 @@ export function register(...rest: any[]) { export interface IDependency { require?: string; resolver?: () => any; - instance?: any; + instances?: any[]; shared?: boolean; } @@ -128,7 +128,7 @@ export class Yok implements IInjector { } private resolveInstance(name: string): any { - let classInstance = this.modules[name].instance; + let classInstance = _.first(this.modules[name].instances); if (!classInstance) { classInstance = this.resolve(name); } @@ -282,7 +282,12 @@ export class Yok implements IInjector { if (_.isFunction(resolver)) { dependency.resolver = resolver; } else { - dependency.instance = resolver; + dependency.instances = dependency.instances || []; + if (shared) { + dependency.instances[0] = resolver; + } else { + dependency.instances.push(resolver); + } } this.modules[name] = dependency; @@ -370,6 +375,7 @@ export class Yok implements IInjector { pushIndent(); let dependency: IDependency; + let instance: any; try { dependency = this.resolveDependency(name); @@ -377,19 +383,24 @@ export class Yok implements IInjector { throw new Error("unable to resolve " + name); } - if (!dependency.instance || !dependency.shared) { + if (!dependency.instances || !dependency.instances.length || !dependency.shared) { if (!dependency.resolver) { throw new Error("no resolver registered for " + name); } - dependency.instance = this.resolveConstructor(dependency.resolver, ctorArguments); + dependency.instances = dependency.instances || []; + + instance = this.resolveConstructor(dependency.resolver, ctorArguments); + dependency.instances.push(instance); + } else { + instance = _.first(dependency.instances); } } finally { popIndent(); delete this.resolutionProgress[name]; } - return dependency.instance; + return instance; } private resolveDependency(name: string): IDependency { @@ -424,10 +435,12 @@ export class Yok implements IInjector { public dispose(): void { Object.keys(this.modules).forEach((moduleName) => { - const instance = this.modules[moduleName].instance; - if (instance && instance.dispose && instance !== this) { - instance.dispose(); - } + const instances = this.modules[moduleName].instances; + _.forEach(instances, instance => { + if (instance && instance.dispose && instance !== this) { + instance.dispose(); + } + }); }); } } From e8ce13b98790d92466ecf223f8f3e5d1a983f2bc Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Wed, 28 Aug 2019 00:44:24 +0300 Subject: [PATCH 4/8] feat: introduce jsonFileSettingsService to work with JSON files Intorduce a new service - `jsonFileSettingsService` to work with JSON files in which you need to persist specific settings. The read/write operation of the file are locked, which makes the service usable in multiple cases. Having a separate service allows us to use multiple files to store data instead of writing everything in the user-settings.json file. To create the service reuse the logic from the old UserSettingsServiceBase and make it available through `$injector`. Add logic in the service to allow writing/reading data within specific cache timeout. In case you want to save data with cache information, just pass this setting to saveSetting method and the service will automatically add required information in the file. When you read information from the file, the service will check if you have passed cacheTimeout and will respect it when checking the value. In case you've passed cacheTimeout, but the value in the file does not have cache information, the service will return null as it is unable to determine if the cache is valid. In case there's cache information, the service will return the value in case the cache had not expired and null otherwise. In case the value in the file has cache information, but the readSetting is called without cacheTimeout, the value is returned immediately without checking the cache. The service is marked as non-shared, so for each `$injector.resolve("jsonFileSettingsService", { jsonFileSettingsPath: "myPath.json" })`, CLI will create a new instance of the service. As all actions are locked via lockfile, this shouldn't be a problem. --- lib/bootstrap.ts | 1 + lib/common/declarations.d.ts | 5 +- .../json-file-settings-service.d.ts | 15 + lib/common/definitions/user-settings.d.ts | 7 - .../services/json-file-settings-service.ts | 132 +++++++ lib/common/services/user-settings-service.ts | 107 ----- .../services/json-file-settings-service.ts | 372 ++++++++++++++++++ lib/services/analytics-settings-service.ts | 2 +- lib/services/user-settings-service.ts | 35 +- 9 files changed, 548 insertions(+), 128 deletions(-) create mode 100644 lib/common/definitions/json-file-settings-service.d.ts delete mode 100644 lib/common/definitions/user-settings.d.ts create mode 100644 lib/common/services/json-file-settings-service.ts delete mode 100644 lib/common/services/user-settings-service.ts create mode 100644 lib/common/test/unit-tests/services/json-file-settings-service.ts diff --git a/lib/bootstrap.ts b/lib/bootstrap.ts index 8c22720c4f..29173a1df5 100644 --- a/lib/bootstrap.ts +++ b/lib/bootstrap.ts @@ -230,3 +230,4 @@ $injector.requirePublicClass("initializeService", "./services/initialize-service $injector.require("npmConfigService", "./services/npm-config-service"); $injector.require("ipService", "./services/ip-service"); +$injector.require("jsonFileSettingsService", "./common/services/json-file-settings-service"); diff --git a/lib/common/declarations.d.ts b/lib/common/declarations.d.ts index 93d14faedb..11f42187a3 100644 --- a/lib/common/declarations.d.ts +++ b/lib/common/declarations.d.ts @@ -1229,9 +1229,8 @@ interface IPlistParser { parseFileSync(plistFilePath: string): any; } -interface IUserSettingsService extends UserSettings.IUserSettingsService { - loadUserSettingsFile(): Promise; - saveSettings(data: IDictionary<{}>): Promise; +interface IUserSettingsService extends IJsonFileSettingsService { + // keep for backwards compatibility } /** diff --git a/lib/common/definitions/json-file-settings-service.d.ts b/lib/common/definitions/json-file-settings-service.d.ts new file mode 100644 index 0000000000..53f6c5cebd --- /dev/null +++ b/lib/common/definitions/json-file-settings-service.d.ts @@ -0,0 +1,15 @@ +interface ICacheTimeoutOpts { + cacheTimeout: number; +} + +interface IUseCacheOpts { + useCaching: boolean; +} + +interface IJsonFileSettingsService { + getSettingValue(settingName: string, cacheOpts?: ICacheTimeoutOpts): Promise; + saveSetting(key: string, value: T, cacheOpts?: IUseCacheOpts): Promise; + removeSetting(key: string): Promise; + loadUserSettingsFile(): Promise; + saveSettings(data: IDictionary<{}>, cacheOpts?: IUseCacheOpts): Promise; +} diff --git a/lib/common/definitions/user-settings.d.ts b/lib/common/definitions/user-settings.d.ts deleted file mode 100644 index e1214a1e9b..0000000000 --- a/lib/common/definitions/user-settings.d.ts +++ /dev/null @@ -1,7 +0,0 @@ -declare module UserSettings { - interface IUserSettingsService { - getSettingValue(settingName: string): Promise; - saveSetting(key: string, value: T): Promise; - removeSetting(key: string): Promise; - } -} \ No newline at end of file diff --git a/lib/common/services/json-file-settings-service.ts b/lib/common/services/json-file-settings-service.ts new file mode 100644 index 0000000000..23e3e8c0c6 --- /dev/null +++ b/lib/common/services/json-file-settings-service.ts @@ -0,0 +1,132 @@ +import * as path from "path"; +import { parseJson } from "../helpers"; + +export class JsonFileSettingsService implements IJsonFileSettingsService { + private jsonSettingsFilePath: string = null; + protected jsonSettingsData: any = null; + private get lockFilePath(): string { + return `${this.jsonSettingsFilePath}.lock`; + } + + constructor(jsonFileSettingsPath: string, + private $fs: IFileSystem, + private $lockService: ILockService, + private $logger: ILogger) { + this.jsonSettingsFilePath = jsonFileSettingsPath; + } + + public async getSettingValue(settingName: string, cacheOpts?: { cacheTimeout: number }): Promise { + const action = async (): Promise => { + await this.loadUserSettingsFile(); + + if (this.jsonSettingsData && _.has(this.jsonSettingsData, settingName)) { + const data = this.jsonSettingsData[settingName]; + const dataToReturn = data.modifiedByCacheMechanism ? data.value : data; + if (cacheOpts && cacheOpts.cacheTimeout) { + if (!data.modifiedByCacheMechanism) { + // If data has no cache, but we want to check the timeout, consider the data as outdated. + // this should be a really rare case + return null; + } + + const currentTime = Date.now(); + if ((currentTime - data.time) > cacheOpts.cacheTimeout) { + return null; + } + } + + return dataToReturn; + } + + return null; + }; + + return this.$lockService.executeActionWithLock(action, this.lockFilePath); + } + + public async saveSetting(key: string, value: T, cacheOpts?: { useCaching: boolean }): Promise { + const settingObject: any = {}; + settingObject[key] = value; + + return this.saveSettings(settingObject, cacheOpts); + } + + public async removeSetting(key: string): Promise { + const action = async (): Promise => { + await this.loadUserSettingsFile(); + + delete this.jsonSettingsData[key]; + await this.saveSettings(); + }; + + return this.$lockService.executeActionWithLock(action, this.lockFilePath); + } + + public saveSettings(data?: any, cacheOpts?: { useCaching: boolean }): Promise { + const action = async (): Promise => { + await this.loadUserSettingsFile(); + this.jsonSettingsData = this.jsonSettingsData || {}; + + _(data) + .keys() + .each(propertyName => { + this.jsonSettingsData[propertyName] = cacheOpts && cacheOpts.useCaching && !data[propertyName].modifiedByCacheMechanism ? { + time: Date.now(), + value: data[propertyName], + modifiedByCacheMechanism: true + } : data[propertyName]; + }); + + this.$fs.writeJson(this.jsonSettingsFilePath, this.jsonSettingsData); + }; + + return this.$lockService.executeActionWithLock(action, this.lockFilePath); + } + + public async loadUserSettingsFile(): Promise { + if (!this.jsonSettingsData) { + await this.loadUserSettingsData(); + } + } + + private async loadUserSettingsData(): Promise { + if (!this.$fs.exists(this.jsonSettingsFilePath)) { + const unexistingDirs = this.getUnexistingDirectories(this.jsonSettingsFilePath); + + this.$fs.writeFile(this.jsonSettingsFilePath, null); + + // when running under 'sudo' we create the /.local/share/.nativescript-cli dir with root as owner + // and other Applications cannot access this directory anymore. (bower/heroku/etc) + if (process.env.SUDO_USER) { + for (const dir of unexistingDirs) { + await this.$fs.setCurrentUserAsOwner(dir, process.env.SUDO_USER); + } + } + } + + const data = this.$fs.readText(this.jsonSettingsFilePath); + + try { + this.jsonSettingsData = parseJson(data); + } catch (err) { + this.$logger.trace(`Error while trying to parseJson ${data} data from ${this.jsonSettingsFilePath} file. Err is: ${err}`); + this.$fs.deleteFile(this.jsonSettingsFilePath); + } + } + + private getUnexistingDirectories(filePath: string): Array { + const unexistingDirs: Array = []; + let currentDir = path.join(filePath, ".."); + while (true) { + // this directory won't be created. + if (this.$fs.exists(currentDir)) { + break; + } + unexistingDirs.push(currentDir); + currentDir = path.join(currentDir, ".."); + } + return unexistingDirs; + } +} + +$injector.register("jsonFileSettingsService", JsonFileSettingsService, false); diff --git a/lib/common/services/user-settings-service.ts b/lib/common/services/user-settings-service.ts deleted file mode 100644 index d10954b616..0000000000 --- a/lib/common/services/user-settings-service.ts +++ /dev/null @@ -1,107 +0,0 @@ -import * as path from "path"; -import { parseJson } from "../helpers"; - -export class UserSettingsServiceBase implements IUserSettingsService { - private userSettingsFilePath: string = null; - protected userSettingsData: any = null; - private get lockFilePath(): string { - return `user-settings.lock`; - } - - constructor(userSettingsFilePath: string, - protected $fs: IFileSystem, - protected $lockService: ILockService, - private $logger: ILogger) { - this.userSettingsFilePath = userSettingsFilePath; - } - - public async getSettingValue(settingName: string): Promise { - const action = async (): Promise => { - await this.loadUserSettingsFile(); - return this.userSettingsData ? this.userSettingsData[settingName] : null; - }; - - return this.$lockService.executeActionWithLock(action, this.lockFilePath); - } - - public async saveSetting(key: string, value: T): Promise { - const settingObject: any = {}; - settingObject[key] = value; - - return this.saveSettings(settingObject); - } - - public async removeSetting(key: string): Promise { - const action = async (): Promise => { - await this.loadUserSettingsFile(); - - delete this.userSettingsData[key]; - await this.saveSettings(); - }; - - return this.$lockService.executeActionWithLock(action, this.lockFilePath); - } - - public saveSettings(data?: any): Promise { - const action = async (): Promise => { - await this.loadUserSettingsFile(); - this.userSettingsData = this.userSettingsData || {}; - - _(data) - .keys() - .each(propertyName => { - this.userSettingsData[propertyName] = data[propertyName]; - }); - - this.$fs.writeJson(this.userSettingsFilePath, this.userSettingsData); - }; - - return this.$lockService.executeActionWithLock(action, this.lockFilePath); - } - - // TODO: Remove Promise, reason: writeFile - blocked as other implementation of the interface has async operation. - public async loadUserSettingsFile(): Promise { - if (!this.userSettingsData) { - await this.loadUserSettingsData(); - } - } - - protected async loadUserSettingsData(): Promise { - if (!this.$fs.exists(this.userSettingsFilePath)) { - const unexistingDirs = this.getUnexistingDirectories(this.userSettingsFilePath); - - this.$fs.writeFile(this.userSettingsFilePath, null); - - // when running under 'sudo' we create the /.local/share/.nativescript-cli dir with root as owner - // and other Applications cannot access this directory anymore. (bower/heroku/etc) - if (process.env.SUDO_USER) { - for (const dir of unexistingDirs) { - await this.$fs.setCurrentUserAsOwner(dir, process.env.SUDO_USER); - } - } - } - - const data = this.$fs.readText(this.userSettingsFilePath); - - try { - this.userSettingsData = parseJson(data); - } catch (err) { - this.$logger.trace(`Error while trying to parseJson ${data} data from ${this.userSettingsFilePath} file. Err is: ${err}`); - this.$fs.deleteFile(this.userSettingsFilePath); - } - } - - private getUnexistingDirectories(filePath: string): Array { - const unexistingDirs: Array = []; - let currentDir = path.join(filePath, ".."); - while (true) { - // this directory won't be created. - if (this.$fs.exists(currentDir)) { - break; - } - unexistingDirs.push(currentDir); - currentDir = path.join(currentDir, ".."); - } - return unexistingDirs; - } -} diff --git a/lib/common/test/unit-tests/services/json-file-settings-service.ts b/lib/common/test/unit-tests/services/json-file-settings-service.ts new file mode 100644 index 0000000000..53330cad2e --- /dev/null +++ b/lib/common/test/unit-tests/services/json-file-settings-service.ts @@ -0,0 +1,372 @@ +import { Yok } from "../../../yok"; +import { assert } from "chai"; +import { CommonLoggerStub } from "../stubs"; +import { JsonFileSettingsService } from "../../../services/json-file-settings-service"; + +const originalDateNow = Date.now; + +describe("jsonFileSettingsService", () => { + const jsonFileSettingsPath = "abc.json"; + let dataPassedToWriteFile: { filename: string, data: string }[] = []; + let dataPassedToWriteJson: { filename: string, data: any }[] = []; + let dataInFile: IDictionary = {}; + let deletedFiles: string[] = []; + + const createTestInjector = (): IInjector => { + const testInjector = new Yok(); + /** + * constructor(jsonFileSettingsPath: string, } + */ + testInjector.register("fs", { + exists: (filePath: string): boolean => true, + writeFile: (filename: string, data: string, encoding?: string): void => { + dataPassedToWriteFile.push({ filename, data }); + }, + setCurrentUserAsOwner: async (path: string, owner: string): Promise => undefined, + readText: (filename: string, encoding?: IReadFileOptions | string): string => JSON.stringify(dataInFile[filename]), + deleteFile: (filePath: string): void => { + deletedFiles.push(filePath); + }, + writeJson: (filename: string, data: any, space?: string, encoding?: string): void => { + dataPassedToWriteJson.push({ filename, data }); + } + }); + testInjector.register("lockService", { + executeActionWithLock: async (action: () => Promise, lockFilePath?: string, lockOpts?: ILockOptions): Promise => { + return action(); + } + }); + testInjector.register("logger", CommonLoggerStub); + testInjector.register("jsonFileSettingsService", JsonFileSettingsService); + return testInjector; + }; + + beforeEach(() => { + dataPassedToWriteFile = []; + dataPassedToWriteJson = []; + dataInFile = {}; + deletedFiles = []; + Date.now = originalDateNow; + }); + + describe("getSettingValue", () => { + it("returns correct data without cache", async () => { + dataInFile = { [jsonFileSettingsPath]: { prop1: 1 } }; + + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const result = await jsonFileSettingsService.getSettingValue("prop1"); + assert.equal(result, 1); + }); + + it("returns null when file does not contain the required property", async () => { + dataInFile = { [jsonFileSettingsPath]: { prop1: 1 } }; + + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const result = await jsonFileSettingsService.getSettingValue("prop2"); + assert.equal(result, null); + }); + + it("returns result when data in file has cache information and the call does not require cache options to be checked", async () => { + const currentTime = Date.now(); + dataInFile = { + [jsonFileSettingsPath]: { + prop1: { + time: currentTime, + modifiedByCacheMechanism: true, + value: 1 + } + } + }; + + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const result = await jsonFileSettingsService.getSettingValue("prop1"); + assert.equal(result, 1); + }); + + it("returns null when data in file does not have caching information and request is to use cacheOptions", async () => { + dataInFile = { [jsonFileSettingsPath]: { prop1: 1 } }; + + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const result = await jsonFileSettingsService.getSettingValue("prop1", { cacheTimeout: 10000 }); + assert.equal(result, null); + }); + + it("returns result when data in file has cache information and the cache is not outdated", async () => { + const currentTime = Date.now(); + dataInFile = { + [jsonFileSettingsPath]: { + prop1: { + time: currentTime, + modifiedByCacheMechanism: true, + value: 1 + } + } + }; + + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const result = await jsonFileSettingsService.getSettingValue("prop1", { cacheTimeout: 100000 }); + assert.equal(result, 1); + }); + + it("returns null when data in file has cache information and the cache is outdated", async () => { + const currentTime = Date.now(); + dataInFile = { + [jsonFileSettingsPath]: { + prop1: { + time: currentTime, + modifiedByCacheMechanism: true, + value: 1 + } + } + }; + + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + + const result = await new Promise((resolve, reject) => { + setTimeout(() => { + jsonFileSettingsService.getSettingValue("prop1", { cacheTimeout: 1 }).then(resolve, reject); + }, 2); + }); + + assert.equal(result, null); + }); + }); + + describe("saveSettings", () => { + it("writes passed data without cache when cache data is not passed", async () => { + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const settingsToSave: any = { + prop1: { + innerProp1: 1 + }, + prop2: "value2" + }; + + await jsonFileSettingsService.saveSettings(settingsToSave); + assert.deepEqual(dataPassedToWriteJson, [{ filename: jsonFileSettingsPath, data: settingsToSave }]); + }); + + it("writes full file data and modifies only properties included in the passed object", async () => { + dataInFile = { + [jsonFileSettingsPath]: { + prop0: { + innerProp1: 1 + }, + prop1: "value2" + } + }; + + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const settingsToSave: any = { + prop1: { + innerProp1: 1 + }, + prop2: "value2" + }; + + await jsonFileSettingsService.saveSettings(settingsToSave); + assert.deepEqual(dataPassedToWriteJson, [{ + filename: jsonFileSettingsPath, + data: { + prop0: { + innerProp1: 1 + }, + prop1: { + innerProp1: 1 + }, + prop2: "value2" + } + }]); + }); + + it("writes passed data with cache when useCaching is passed", async () => { + const time = 1234; + Date.now = () => time; + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const settingsToSave: any = { + prop1: { + innerProp1: 1 + }, + prop2: "value2" + }; + + await jsonFileSettingsService.saveSettings(settingsToSave, { useCaching: true }); + + assert.deepEqual(dataPassedToWriteJson, [{ + filename: jsonFileSettingsPath, + data: { + prop1: { + time, + modifiedByCacheMechanism: true, + value: { + innerProp1: 1 + } + }, + prop2: { + time, + modifiedByCacheMechanism: true, + value: "value2" + } + } + }]); + }); + + it("writes passed data with cache when useCaching is passed and does not add additional cache data to a property in case it already has such (passed directly to the method)", async () => { + const time = 1234; + const timeForPassedData = 123; + Date.now = () => time; + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + const settingsToSave: any = { + prop1: { + time: timeForPassedData, + modifiedByCacheMechanism: true, + value: { + innerProp1: 1 + } + }, + prop2: "value2" + }; + + await jsonFileSettingsService.saveSettings(settingsToSave, { useCaching: true }); + + assert.deepEqual(dataPassedToWriteJson, [{ + filename: jsonFileSettingsPath, + data: { + prop1: { + time: timeForPassedData, + modifiedByCacheMechanism: true, + value: { + innerProp1: 1 + } + }, + prop2: { + time, + modifiedByCacheMechanism: true, + value: "value2" + } + } + }]); + }); + }); + + describe("saveSetting", () => { + it("writes passed data without cache when cache data is not passed", async () => { + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + + await jsonFileSettingsService.saveSetting("prop1", { + innerProp1: 1 + }); + + assert.deepEqual(dataPassedToWriteJson, [{ filename: jsonFileSettingsPath, data: { prop1: { innerProp1: 1 } } }]); + }); + + it("writes passed data with cache when useCaching is passed", async () => { + const time = 1234; + Date.now = () => time; + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + + await jsonFileSettingsService.saveSetting("prop1", { innerProp1: 1 }, { useCaching: true }); + + assert.deepEqual(dataPassedToWriteJson, [{ + filename: jsonFileSettingsPath, + data: { + prop1: { + time, + modifiedByCacheMechanism: true, + value: { + innerProp1: 1 + } + } + } + }]); + }); + + it("writes passed data with cache when useCaching is passed and does not add additional cache data to a property in case it already has such (passed directly to the method)", async () => { + const time = 1234; + const timeForPassedData = 123; + Date.now = () => time; + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + + await jsonFileSettingsService.saveSetting("prop1", { + time: timeForPassedData, + modifiedByCacheMechanism: true, + value: { + innerProp1: 1 + } + }, { useCaching: true }); + + assert.deepEqual(dataPassedToWriteJson, [{ + filename: jsonFileSettingsPath, + data: { + prop1: { + time: timeForPassedData, + modifiedByCacheMechanism: true, + value: { + innerProp1: 1 + } + } + } + }]); + }); + }); + + describe("removeSetting", () => { + it("removes existing property from file when fileData does not have caching information", async () => { + dataInFile = { [jsonFileSettingsPath]: { prop1: 1, prop2: { innerProp1: 2 } } }; + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + await jsonFileSettingsService.removeSetting("prop2"); + assert.deepEqual(dataPassedToWriteJson, [{ + filename: jsonFileSettingsPath, + data: { + prop1: 1 + } + }]); + }); + + it("removes existing property from file when fileData does not have caching information", async () => { + const currentTime = "12454"; + dataInFile = { + [jsonFileSettingsPath]: { + prop1: { + time: currentTime, + modifiedByCacheMechanism: true, + value: 1 + }, + prop2: { + time: currentTime, + modifiedByCacheMechanism: true, + value: { innerProp1: "val" } + } + } + }; + + const testInjector = createTestInjector(); + const jsonFileSettingsService = testInjector.resolve("jsonFileSettingsService", { jsonFileSettingsPath }); + await jsonFileSettingsService.removeSetting("prop2"); + assert.deepEqual(dataPassedToWriteJson, [{ + filename: jsonFileSettingsPath, + data: { + prop1: { + time: currentTime, + modifiedByCacheMechanism: true, + value: 1 + } + } + }]); + }); + }); +}); diff --git a/lib/services/analytics-settings-service.ts b/lib/services/analytics-settings-service.ts index 4a0a85d4e2..7cd16655de 100644 --- a/lib/services/analytics-settings-service.ts +++ b/lib/services/analytics-settings-service.ts @@ -4,7 +4,7 @@ import { exported } from "../common/decorators"; class AnalyticsSettingsService implements IAnalyticsSettingsService { private static SESSIONS_STARTED_KEY_PREFIX = "SESSIONS_STARTED_"; - constructor(private $userSettingsService: UserSettings.IUserSettingsService, + constructor(private $userSettingsService: IUserSettingsService, private $staticConfig: IStaticConfig, private $hostInfo: IHostInfo, private $osInfo: IOsInfo, diff --git a/lib/services/user-settings-service.ts b/lib/services/user-settings-service.ts index 6e20579c3d..c3d39f654e 100644 --- a/lib/services/user-settings-service.ts +++ b/lib/services/user-settings-service.ts @@ -1,17 +1,32 @@ import * as path from "path"; -import * as userSettingsServiceBaseLib from "../common/services/user-settings-service"; -export class UserSettingsService extends userSettingsServiceBaseLib.UserSettingsServiceBase { - constructor($fs: IFileSystem, - $settingsService: ISettingsService, - $lockService: ILockService, - $logger: ILogger) { - const userSettingsFilePath = path.join($settingsService.getProfileDir(), "user-settings.json"); - super(userSettingsFilePath, $fs, $lockService, $logger); +export class UserSettingsService implements IUserSettingsService { + private get $jsonFileSettingsService(): IJsonFileSettingsService { + const userSettingsFilePath = path.join(this.$settingsService.getProfileDir(), "user-settings.json"); + return this.$injector.resolve("jsonFileSettingsService", { jsonFileSettingsPath: userSettingsFilePath }); + } + constructor(private $injector: IInjector, + private $settingsService: ISettingsService) { + } + + public getSettingValue(settingName: string, cacheOpts?: ICacheTimeoutOpts): Promise { + return this.$jsonFileSettingsService.getSettingValue(settingName, cacheOpts); + } + + public saveSetting(key: string, value: T, cacheOpts?: IUseCacheOpts): Promise { + return this.saveSetting(key, value, cacheOpts); + } + + public saveSettings(data: IDictionary<{}>, cacheOpts?: IUseCacheOpts): Promise { + return this.$jsonFileSettingsService.saveSettings(data, cacheOpts); + } + + public removeSetting(key: string): Promise { + return this.removeSetting(key); } - public async loadUserSettingsFile(): Promise { - await this.loadUserSettingsData(); + public loadUserSettingsFile(): Promise { + return this.$jsonFileSettingsService.loadUserSettingsFile(); } } $injector.register("userSettingsService", UserSettingsService); From 7c1987efea8823e1ebe4dfe3581aab2c5089c093 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Wed, 28 Aug 2019 11:18:44 +0300 Subject: [PATCH 5/8] feat: add caching for getting company information Add local caching in a file when getting the company information. This allows us to minimize the number of calls to external services. As the companyInsightsService now uses multiple other services and is also exposed publically, rename it to companyInsightsController. Set the expiration of the cache to 2 days. --- lib/bootstrap.ts | 2 +- .../company-insights-controller.ts | 89 +++++++ ....d.ts => company-insights-controller.d.ts} | 2 +- lib/services/analytics/analytics-service.ts | 2 +- .../analytics/google-analytics-provider.ts | 4 +- lib/services/company-insights-service.ts | 33 --- .../company-insights-controller.ts | 226 ++++++++++++++++++ test/nativescript-cli-lib.ts | 3 + test/services/company-insights-service.ts | 177 -------------- 9 files changed, 323 insertions(+), 215 deletions(-) create mode 100644 lib/controllers/company-insights-controller.ts rename lib/definitions/{company-insights-service.d.ts => company-insights-controller.d.ts} (96%) delete mode 100644 lib/services/company-insights-service.ts create mode 100644 test/controllers/company-insights-controller.ts delete mode 100644 test/services/company-insights-service.ts diff --git a/lib/bootstrap.ts b/lib/bootstrap.ts index 29173a1df5..56d552ba6e 100644 --- a/lib/bootstrap.ts +++ b/lib/bootstrap.ts @@ -67,7 +67,7 @@ $injector.require("userSettingsService", "./services/user-settings-service"); $injector.requirePublic("analyticsSettingsService", "./services/analytics-settings-service"); $injector.require("analyticsService", "./services/analytics/analytics-service"); $injector.require("googleAnalyticsProvider", "./services/analytics/google-analytics-provider"); -$injector.requirePublicClass("companyInsightsService", "./services/company-insights-service"); +$injector.requirePublicClass("companyInsightsController", "./controllers/company-insights-controller"); $injector.require("platformCommandParameter", "./platform-command-param"); $injector.requireCommand("create", "./commands/create-project"); diff --git a/lib/controllers/company-insights-controller.ts b/lib/controllers/company-insights-controller.ts new file mode 100644 index 0000000000..8c7fb4c4e5 --- /dev/null +++ b/lib/controllers/company-insights-controller.ts @@ -0,0 +1,89 @@ +import { AnalyticsEventLabelDelimiter } from "../constants"; +import { cache } from "../common/decorators"; +import * as path from "path"; + +export class CompanyInsightsController implements ICompanyInsightsController { + private static CACHE_TIMEOUT = 48 * 60 * 60 * 1000; // 2 days in milliseconds + private get $jsonFileSettingsService(): IJsonFileSettingsService { + return this.$injector.resolve("jsonFileSettingsService", { + jsonFileSettingsPath: path.join(this.$settingsService.getProfileDir(), "company-insights-data.json") + }); + } + + constructor(private $config: IConfiguration, + private $httpClient: Server.IHttpClient, + private $injector: IInjector, + private $ipService: IIPService, + private $logger: ILogger, + private $settingsService: ISettingsService) { } + + public async getCompanyData(): Promise { + let companyData: ICompanyData = null; + const { currentPublicIP, cacheKey } = await this.getIPInfo(); + + companyData = await this.getCompanyDataFromCache(cacheKey); + + if (!companyData) { + companyData = await this.getCompanyDataFromPlaygroundInsightsEndpoint(); + if (companyData && currentPublicIP) { + await this.$jsonFileSettingsService.saveSetting(cacheKey, companyData, { useCaching: true }); + } + } + + return companyData; + } + + private async getIPInfo(): Promise<{ currentPublicIP: string, cacheKey: string }> { + let currentPublicIP: string = null; + let keyInJsonFile: string = null; + + try { + currentPublicIP = await this.$ipService.getCurrentIPv4Address(); + keyInJsonFile = `companyInformation_${currentPublicIP}`; + } catch (err) { + this.$logger.trace(`Unable to get current public ip address. Error is: `, err); + } + + return { currentPublicIP, cacheKey: keyInJsonFile }; + } + + private async getCompanyDataFromCache(keyInJsonFile: string): Promise { + let companyData: ICompanyData = null; + + try { + if (keyInJsonFile) { + companyData = await this.$jsonFileSettingsService.getSettingValue(keyInJsonFile, { cacheTimeout: CompanyInsightsController.CACHE_TIMEOUT }); + } + } catch (err) { + this.$logger.trace(`Unable to get data from file, error is:`, err); + } + + return companyData; + } + + @cache() + private async getCompanyDataFromPlaygroundInsightsEndpoint(): Promise { + let companyData: ICompanyData = null; + + try { + const response = await this.$httpClient.httpRequest(this.$config.INSIGHTS_URL_ENDPOINT); + const data = (JSON.parse(response.body)); + if (data.company) { + const industries = _.isArray(data.company.industries) ? data.company.industries.join(AnalyticsEventLabelDelimiter) : null; + companyData = { + name: data.company.name, + country: data.company.country, + revenue: data.company.revenue, + employeeCount: data.company.employeeCount, + industries + }; + } + } catch (err) { + this.$logger.trace(`Unable to get data for company. Error is: ${err}`); + } + + return companyData; + } +} + +$injector.register("companyInsightsController", CompanyInsightsController); diff --git a/lib/definitions/company-insights-service.d.ts b/lib/definitions/company-insights-controller.d.ts similarity index 96% rename from lib/definitions/company-insights-service.d.ts rename to lib/definitions/company-insights-controller.d.ts index 511b5fa53a..65e6ced520 100644 --- a/lib/definitions/company-insights-service.d.ts +++ b/lib/definitions/company-insights-controller.d.ts @@ -50,7 +50,7 @@ interface IPlaygroundInsightsEndpointData { /** * Describes the service that can be used to get insights about the company using the CLI. */ -interface ICompanyInsightsService { +interface ICompanyInsightsController { /** * Describes information about the company. * @returns {Promise} diff --git a/lib/services/analytics/analytics-service.ts b/lib/services/analytics/analytics-service.ts index cdcbbda5be..02b4741a99 100644 --- a/lib/services/analytics/analytics-service.ts +++ b/lib/services/analytics/analytics-service.ts @@ -15,7 +15,7 @@ export class AnalyticsService implements IAnalyticsService, IDisposable { private $options: IOptions, private $staticConfig: Config.IStaticConfig, private $prompter: IPrompter, - private $userSettingsService: UserSettings.IUserSettingsService, + private $userSettingsService: IUserSettingsService, private $analyticsSettingsService: IAnalyticsSettingsService, private $childProcess: IChildProcess, private $projectDataService: IProjectDataService, diff --git a/lib/services/analytics/google-analytics-provider.ts b/lib/services/analytics/google-analytics-provider.ts index d1f73c2509..edca114bdc 100644 --- a/lib/services/analytics/google-analytics-provider.ts +++ b/lib/services/analytics/google-analytics-provider.ts @@ -12,7 +12,7 @@ export class GoogleAnalyticsProvider implements IGoogleAnalyticsProvider { private $logger: ILogger, private $proxyService: IProxyService, private $config: IConfiguration, - private $companyInsightsService: ICompanyInsightsService, + private $companyInsightsController: ICompanyInsightsController, private analyticsLoggingService: IFileLogService) { } @@ -81,7 +81,7 @@ export class GoogleAnalyticsProvider implements IGoogleAnalyticsProvider { defaultValues[GoogleAnalyticsCustomDimensions.usedTutorial] = playgrounInfo.usedTutorial.toString(); } - const companyData = await this.$companyInsightsService.getCompanyData(); + const companyData = await this.$companyInsightsController.getCompanyData(); if (companyData) { defaultValues[GoogleAnalyticsCustomDimensions.companyName] = companyData.name; defaultValues[GoogleAnalyticsCustomDimensions.companyCountry] = companyData.country; diff --git a/lib/services/company-insights-service.ts b/lib/services/company-insights-service.ts deleted file mode 100644 index d0f20308d6..0000000000 --- a/lib/services/company-insights-service.ts +++ /dev/null @@ -1,33 +0,0 @@ -import { AnalyticsEventLabelDelimiter } from "../constants"; -import { cache } from "../common/decorators"; - -export class CompanyInsightsService implements ICompanyInsightsService { - constructor(private $config: IConfiguration, - private $httpClient: Server.IHttpClient, - private $logger: ILogger) { } - - @cache() - public async getCompanyData(): Promise { - let companyData: ICompanyData = null; - try { - const response = await this.$httpClient.httpRequest(this.$config.INSIGHTS_URL_ENDPOINT); - const data = (JSON.parse(response.body)); - if (data.company) { - const industries = _.isArray(data.company.industries) ? data.company.industries.join(AnalyticsEventLabelDelimiter) : null; - companyData = { - name: data.company.name, - country: data.company.country, - revenue: data.company.revenue, - employeeCount: data.company.employeeCount, - industries - }; - } - } catch (err) { - this.$logger.trace(`Unable to get data for company. Error is: ${err}`); - } - - return companyData; - } -} - -$injector.register("companyInsightsService", CompanyInsightsService); diff --git a/test/controllers/company-insights-controller.ts b/test/controllers/company-insights-controller.ts new file mode 100644 index 0000000000..09ca16fafc --- /dev/null +++ b/test/controllers/company-insights-controller.ts @@ -0,0 +1,226 @@ +import { assert } from "chai"; +import { Yok } from "../../lib/common/yok"; +import { LoggerStub } from "../stubs"; +import { CompanyInsightsController } from "../../lib/controllers/company-insights-controller"; + +describe("companyInsightsController", () => { + const insightsUrlEndpoint = "/api/insights"; + const currentIp = "8.8.8.8"; + const profileDir = "profileDir"; + const cacheTimeout = 48 * 60 * 60 * 1000; // 2 days in milliseconds + const defaultCompanyData = { + company: { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + industries: [ + "Software", + "Software 2" + ], + employeeCount: "500" + } + }; + + const defaultExpectedCompanyData: ICompanyData = { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + industries: "Software__Software 2", + employeeCount: "500" + }; + + const defaultExpectedDataPassedToGetSetting: any[] = [{ settingName: `companyInformation_${currentIp}`, cacheOpts: { cacheTimeout } }]; + const defaultExpectedDataPassedToSaveSetting: any[] = [ + { + cacheOpts: { + useCaching: true + }, + key: "companyInformation_8.8.8.8", + value: { + country: "Bulgaria", + employeeCount: "500", + industries: "Software__Software 2", + name: "Progress", + revenue: "123131" + } + } + ]; + + let dataPassedToGetSettingValue: { settingName: string, cacheOpts?: ICacheTimeoutOpts }[] = []; + let dataPassedToSaveSettingValue: { key: string, value: any, cacheOpts?: IUseCacheOpts }[] = []; + let getSettingValueResult: IDictionary = null; + let httpRequestCounter = 0; + let httpRequestResult: any = null; + let testInjector: IInjector = null; + let companyInsightsController: ICompanyInsightsController = null; + + const createTestInjector = (): IInjector => { + const injector = new Yok(); + injector.register("config", { + INSIGHTS_URL_ENDPOINT: insightsUrlEndpoint + }); + + injector.register("httpClient", { + httpRequest: async (options: any, proxySettings?: IProxySettings): Promise => { + httpRequestCounter++; + return { body: JSON.stringify(httpRequestResult) }; + } + }); + + injector.register("logger", LoggerStub); + injector.register("injector", injector); + injector.register("ipService", { + getCurrentIPv4Address: async (): Promise => currentIp + }); + + injector.register("settingsService", { + getProfileDir: (): string => profileDir + }); + + injector.register("jsonFileSettingsService", { + getSettingValue: async (settingName: string, cacheOpts?: ICacheTimeoutOpts): Promise => { + dataPassedToGetSettingValue.push({ settingName, cacheOpts }); + return getSettingValueResult; + }, + + saveSetting: async (key: string, value: any, cacheOpts?: IUseCacheOpts): Promise => { + dataPassedToSaveSettingValue.push({ key, value, cacheOpts }); + } + }); + + injector.register("companyInsightsController", CompanyInsightsController); + + return injector; + }; + + beforeEach(() => { + dataPassedToGetSettingValue = []; + dataPassedToSaveSettingValue = []; + getSettingValueResult = null; + httpRequestCounter = 0; + httpRequestResult = defaultCompanyData; + testInjector = createTestInjector(); + companyInsightsController = testInjector.resolve("companyInsightsController"); + }); + + describe("getCompanyData", () => { + describe("returns null when data does not exist in the cache and", () => { + it("the http client fails to get data", async () => { + const httpClient = testInjector.resolve("httpClient"); + const errMsg = "custom error"; + httpClient.httpRequest = async () => { + throw new Error(errMsg); + }; + + const companyData = await companyInsightsController.getCompanyData(); + assert.isNull(companyData); + const logger = testInjector.resolve("logger"); + assert.isTrue(logger.traceOutput.indexOf(errMsg) !== -1); + }); + + it("the body of the response is not a valid JSON", async () => { + const httpClient = testInjector.resolve("httpClient"); + httpClient.httpRequest = async (): Promise => { + return { body: "invalid JSON" }; + }; + + const companyData = await companyInsightsController.getCompanyData(); + assert.isNull(companyData); + const logger = testInjector.resolve("logger"); + assert.isTrue(logger.traceOutput.indexOf("SyntaxError: Unexpected token") !== -1); + }); + + it("response does not contain company property", async () => { + httpRequestResult = { + foo: "bar" + }; + + const companyData = await companyInsightsController.getCompanyData(); + assert.deepEqual(companyData, null); + }); + }); + + describe("returns correct data when", () => { + it("data for current ip exist in the cache", async () => { + httpRequestResult = null; + + getSettingValueResult = defaultExpectedCompanyData; // data in the file should be in the already parsed format + const companyData = await companyInsightsController.getCompanyData(); + assert.deepEqual(companyData, defaultExpectedCompanyData); + + assert.equal(httpRequestCounter, 0, "In case we have data for the company in our cache, we should not make any http requests"); + assert.deepEqual(dataPassedToGetSettingValue, defaultExpectedDataPassedToGetSetting); + assert.deepEqual(dataPassedToSaveSettingValue, []); + }); + + describe("data for current ip does not exist in the cache and", () => { + + it("unable to get current ip address, but still can get company information", async () => { + const ipService = testInjector.resolve("ipService"); + ipService.getCurrentIPv4Address = async (): Promise => { throw new Error("Unable to get current ip addreess"); }; + + const companyData = await companyInsightsController.getCompanyData(); + assert.deepEqual(companyData, defaultExpectedCompanyData); + assert.equal(httpRequestCounter, 1, "We should have only one http request"); + assert.deepEqual(dataPassedToGetSettingValue, [], "When we are unable to get IP, we should not try to get value from the cache."); + assert.deepEqual(dataPassedToSaveSettingValue, [], "When we are unable to get IP, we should not persist anything."); + }); + + it("response contains company property", async () => { + const companyData = await companyInsightsController.getCompanyData(); + assert.deepEqual(companyData, defaultExpectedCompanyData); + assert.deepEqual(dataPassedToGetSettingValue, defaultExpectedDataPassedToGetSetting); + assert.deepEqual(dataPassedToSaveSettingValue, defaultExpectedDataPassedToSaveSetting); + }); + + it("response contains company property and industries in it are not populated", async () => { + httpRequestResult = { + company: { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + employeeCount: "500" + } + }; + + const companyData = await companyInsightsController.getCompanyData(); + assert.deepEqual(companyData, { + name: "Progress", + country: "Bulgaria", + revenue: "123131", + industries: null, + employeeCount: "500" + }); + + assert.deepEqual(dataPassedToGetSettingValue, defaultExpectedDataPassedToGetSetting); + assert.deepEqual(dataPassedToSaveSettingValue, [ + { + cacheOpts: { + useCaching: true + }, + key: "companyInformation_8.8.8.8", + value: { + country: "Bulgaria", + employeeCount: "500", + industries: null, + name: "Progress", + revenue: "123131" + } + } + ]); + }); + + }); + }); + + it("is called only once per process", async () => { + const companyData = await companyInsightsController.getCompanyData(); + assert.deepEqual(companyData, defaultExpectedCompanyData); + assert.equal(httpRequestCounter, 1); + + const companyDataSecondCall = await companyInsightsController.getCompanyData(); + assert.deepEqual(companyDataSecondCall, defaultExpectedCompanyData); + assert.equal(httpRequestCounter, 1); + }); + }); +}); diff --git a/test/nativescript-cli-lib.ts b/test/nativescript-cli-lib.ts index d51dc9cfa7..e2fe965ee5 100644 --- a/test/nativescript-cli-lib.ts +++ b/test/nativescript-cli-lib.ts @@ -71,6 +71,9 @@ describe("nativescript-cli-lib", () => { ], initializeService: [ "initialize" + ], + companyInsightsController: [ + "getCompanyData" ] }; diff --git a/test/services/company-insights-service.ts b/test/services/company-insights-service.ts deleted file mode 100644 index 96400d203a..0000000000 --- a/test/services/company-insights-service.ts +++ /dev/null @@ -1,177 +0,0 @@ -import { assert } from "chai"; -import { Yok } from "../../lib/common/yok"; -import { LoggerStub } from "../stubs"; -import { CompanyInsightsService } from "../../lib/services/company-insights-service"; - -describe("companyInsightsService", () => { - const insightsUrlEndpoint = "/api/insights"; - const createTestInjector = (): IInjector => { - const testInjector = new Yok(); - testInjector.register("config", { - INSIGHTS_URL_ENDPOINT: insightsUrlEndpoint - }); - testInjector.register("httpClient", {}); - testInjector.register("logger", LoggerStub); - testInjector.register("companyInsightsService", CompanyInsightsService); - return testInjector; - }; - - describe("getCompanyData", () => { - describe("returns null when", () => { - it("the http client fails to get data", async () => { - const testInjector = createTestInjector(); - const companyInsightsService = testInjector.resolve("companyInsightsService"); - const httpClient = testInjector.resolve("httpClient"); - const errMsg = "custom error"; - httpClient.httpRequest = async () => { - throw new Error(errMsg); - }; - - const companyData = await companyInsightsService.getCompanyData(); - assert.isNull(companyData); - const logger = testInjector.resolve("logger"); - assert.isTrue(logger.traceOutput.indexOf(errMsg) !== -1); - }); - - it("the body of the response is not a valid JSON", async () => { - const testInjector = createTestInjector(); - const companyInsightsService = testInjector.resolve("companyInsightsService"); - const httpClient = testInjector.resolve("httpClient"); - httpClient.httpRequest = async (): Promise => { - return { - body: "invalid JSON" - }; - }; - - const companyData = await companyInsightsService.getCompanyData(); - assert.isNull(companyData); - const logger = testInjector.resolve("logger"); - assert.isTrue(logger.traceOutput.indexOf("SyntaxError: Unexpected token") !== -1); - }); - - it("response does not contain company property", async () => { - const httpResultData = { - foo: "bar" - }; - - const testInjector = createTestInjector(); - const companyInsightsService = testInjector.resolve("companyInsightsService"); - const httpClient = testInjector.resolve("httpClient"); - httpClient.httpRequest = async (): Promise => { - return { - body: JSON.stringify(httpResultData) - }; - }; - - const companyData = await companyInsightsService.getCompanyData(); - assert.deepEqual(companyData, null); - }); - }); - - describe("returns correct data when", () => { - it("response contains company property", async () => { - const httpResultData = { - company: { - name: "Progress", - country: "Bulgaria", - revenue: "123131", - industries: [ - "Software", - "Software 2" - ], - employeeCount: "500" - } - }; - - const testInjector = createTestInjector(); - const companyInsightsService = testInjector.resolve("companyInsightsService"); - const httpClient = testInjector.resolve("httpClient"); - httpClient.httpRequest = async (): Promise => { - return { - body: JSON.stringify(httpResultData) - }; - }; - - const companyData = await companyInsightsService.getCompanyData(); - assert.deepEqual(companyData, { - name: "Progress", - country: "Bulgaria", - revenue: "123131", - industries: "Software__Software 2", - employeeCount: "500" - }); - }); - - it("response contains company property and industries in it are not populated", async () => { - const httpResultData = { - company: { - name: "Progress", - country: "Bulgaria", - revenue: "123131", - employeeCount: "500" - } - }; - - const testInjector = createTestInjector(); - const companyInsightsService = testInjector.resolve("companyInsightsService"); - const httpClient = testInjector.resolve("httpClient"); - httpClient.httpRequest = async (): Promise => { - return { - body: JSON.stringify(httpResultData) - }; - }; - - const companyData = await companyInsightsService.getCompanyData(); - assert.deepEqual(companyData, { - name: "Progress", - country: "Bulgaria", - revenue: "123131", - industries: null, - employeeCount: "500" - }); - }); - }); - - it("is called only once per process", async () => { - const httpResultData = { - company: { - name: "Progress", - country: "Bulgaria", - revenue: "123131", - industries: [ - "Software", - "Software 2" - ], - employeeCount: "500" - } - }; - - const testInjector = createTestInjector(); - const companyInsightsService = testInjector.resolve("companyInsightsService"); - const httpClient = testInjector.resolve("httpClient"); - let httpRequestCounter = 0; - httpClient.httpRequest = async (): Promise => { - httpRequestCounter++; - return { - body: JSON.stringify(httpResultData) - }; - }; - - const expectedData = { - name: "Progress", - country: "Bulgaria", - revenue: "123131", - industries: "Software__Software 2", - employeeCount: "500" - }; - const companyData = await companyInsightsService.getCompanyData(); - assert.deepEqual(companyData, expectedData); - assert.equal(httpRequestCounter, 1); - - const companyDataSecondCall = await companyInsightsService.getCompanyData(); - assert.deepEqual(companyDataSecondCall, expectedData); - - assert.equal(httpRequestCounter, 1); - }); - }); -}); From d3f1e76a9ab038e2fa77df4bf107d38b1f5d426c Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Wed, 28 Aug 2019 17:14:23 +0300 Subject: [PATCH 6/8] chore: remove commented code --- .../test/unit-tests/services/json-file-settings-service.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/common/test/unit-tests/services/json-file-settings-service.ts b/lib/common/test/unit-tests/services/json-file-settings-service.ts index 53330cad2e..2ca75d297f 100644 --- a/lib/common/test/unit-tests/services/json-file-settings-service.ts +++ b/lib/common/test/unit-tests/services/json-file-settings-service.ts @@ -14,9 +14,7 @@ describe("jsonFileSettingsService", () => { const createTestInjector = (): IInjector => { const testInjector = new Yok(); - /** - * constructor(jsonFileSettingsPath: string, } - */ + testInjector.register("fs", { exists: (filePath: string): boolean => true, writeFile: (filename: string, data: string, encoding?: string): void => { From 6312c2f84f5d02889058bb16c1d8c03649f7e667 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 29 Aug 2019 14:50:00 +0300 Subject: [PATCH 7/8] fix: pass ip as query parameter --- config/config.json | 2 +- .../company-insights-controller.ts | 13 ++++++---- .../company-insights-controller.ts | 26 +++++++++---------- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/config/config.json b/config/config.json index 953ccf8bdf..5adda54814 100644 --- a/config/config.json +++ b/config/config.json @@ -6,7 +6,7 @@ "DISABLE_HOOKS": false, "UPLOAD_PLAYGROUND_FILES_ENDPOINT": "https://play.nativescript.org/api/files", "SHORTEN_URL_ENDPOINT": "https://play.nativescript.org/api/shortenurl?longUrl=%s", - "INSIGHTS_URL_ENDPOINT": "https://play.nativescript.org/api/insights", + "INSIGHTS_URL_ENDPOINT": "https://play.nativescript.org/api/insights?ipAddress=%s", "WHOAMI_URL_ENDPOINT": "https://play.nativescript.org/api/whoami", "PREVIEW_APP_ENVIRONMENT": "live", "GA_TRACKING_ID": "UA-111455-51" diff --git a/lib/controllers/company-insights-controller.ts b/lib/controllers/company-insights-controller.ts index 8c7fb4c4e5..8d0e70c0df 100644 --- a/lib/controllers/company-insights-controller.ts +++ b/lib/controllers/company-insights-controller.ts @@ -1,9 +1,11 @@ + import { AnalyticsEventLabelDelimiter } from "../constants"; import { cache } from "../common/decorators"; import * as path from "path"; +import * as util from "util"; export class CompanyInsightsController implements ICompanyInsightsController { - private static CACHE_TIMEOUT = 48 * 60 * 60 * 1000; // 2 days in milliseconds + private static CACHE_TIMEOUT = 30 * 24 * 60 * 60 * 1000; // 30 days in milliseconds private get $jsonFileSettingsService(): IJsonFileSettingsService { return this.$injector.resolve("jsonFileSettingsService", { jsonFileSettingsPath: path.join(this.$settingsService.getProfileDir(), "company-insights-data.json") @@ -23,8 +25,8 @@ export class CompanyInsightsController implements ICompanyInsightsController { companyData = await this.getCompanyDataFromCache(cacheKey); - if (!companyData) { - companyData = await this.getCompanyDataFromPlaygroundInsightsEndpoint(); + if (!companyData && currentPublicIP) { + companyData = await this.getCompanyDataFromPlaygroundInsightsEndpoint(currentPublicIP); if (companyData && currentPublicIP) { await this.$jsonFileSettingsService.saveSetting(cacheKey, companyData, { useCaching: true }); } @@ -62,11 +64,12 @@ export class CompanyInsightsController implements ICompanyInsightsController { } @cache() - private async getCompanyDataFromPlaygroundInsightsEndpoint(): Promise { + private async getCompanyDataFromPlaygroundInsightsEndpoint(ipAddress: string): Promise { let companyData: ICompanyData = null; try { - const response = await this.$httpClient.httpRequest(this.$config.INSIGHTS_URL_ENDPOINT); + const url = util.format(this.$config.INSIGHTS_URL_ENDPOINT, encodeURIComponent(ipAddress)); + const response = await this.$httpClient.httpRequest(url); const data = (JSON.parse(response.body)); if (data.company) { const industries = _.isArray(data.company.industries) ? data.company.industries.join(AnalyticsEventLabelDelimiter) : null; diff --git a/test/controllers/company-insights-controller.ts b/test/controllers/company-insights-controller.ts index 09ca16fafc..072d60a36b 100644 --- a/test/controllers/company-insights-controller.ts +++ b/test/controllers/company-insights-controller.ts @@ -4,10 +4,10 @@ import { LoggerStub } from "../stubs"; import { CompanyInsightsController } from "../../lib/controllers/company-insights-controller"; describe("companyInsightsController", () => { - const insightsUrlEndpoint = "/api/insights"; + const insightsUrlEndpoint = "/api/insights?ipAddress=%s"; const currentIp = "8.8.8.8"; const profileDir = "profileDir"; - const cacheTimeout = 48 * 60 * 60 * 1000; // 2 days in milliseconds + const cacheTimeout = 30 * 24 * 60 * 60 * 1000; // 2 days in milliseconds const defaultCompanyData = { company: { name: "Progress", @@ -138,6 +138,17 @@ describe("companyInsightsController", () => { const companyData = await companyInsightsController.getCompanyData(); assert.deepEqual(companyData, null); }); + + it("unable to get current ip address", async () => { + const ipService = testInjector.resolve("ipService"); + ipService.getCurrentIPv4Address = async (): Promise => { throw new Error("Unable to get current ip addreess"); }; + + const companyData = await companyInsightsController.getCompanyData(); + assert.deepEqual(companyData, null); + assert.equal(httpRequestCounter, 0, "We should not have any http request"); + assert.deepEqual(dataPassedToGetSettingValue, [], "When we are unable to get IP, we should not try to get value from the cache."); + assert.deepEqual(dataPassedToSaveSettingValue, [], "When we are unable to get IP, we should not persist anything."); + }); }); describe("returns correct data when", () => { @@ -155,17 +166,6 @@ describe("companyInsightsController", () => { describe("data for current ip does not exist in the cache and", () => { - it("unable to get current ip address, but still can get company information", async () => { - const ipService = testInjector.resolve("ipService"); - ipService.getCurrentIPv4Address = async (): Promise => { throw new Error("Unable to get current ip addreess"); }; - - const companyData = await companyInsightsController.getCompanyData(); - assert.deepEqual(companyData, defaultExpectedCompanyData); - assert.equal(httpRequestCounter, 1, "We should have only one http request"); - assert.deepEqual(dataPassedToGetSettingValue, [], "When we are unable to get IP, we should not try to get value from the cache."); - assert.deepEqual(dataPassedToSaveSettingValue, [], "When we are unable to get IP, we should not persist anything."); - }); - it("response contains company property", async () => { const companyData = await companyInsightsController.getCompanyData(); assert.deepEqual(companyData, defaultExpectedCompanyData); From 54a579f871b9ac46797899a1a98c012239168bc3 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 29 Aug 2019 19:20:27 +0300 Subject: [PATCH 8/8] fix: use correct URL for insights endpoint --- config/config.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/config.json b/config/config.json index 5adda54814..7803a32f33 100644 --- a/config/config.json +++ b/config/config.json @@ -6,7 +6,7 @@ "DISABLE_HOOKS": false, "UPLOAD_PLAYGROUND_FILES_ENDPOINT": "https://play.nativescript.org/api/files", "SHORTEN_URL_ENDPOINT": "https://play.nativescript.org/api/shortenurl?longUrl=%s", - "INSIGHTS_URL_ENDPOINT": "https://play.nativescript.org/api/insights?ipAddress=%s", + "INSIGHTS_URL_ENDPOINT": "https://play-server.nativescript.org/api/insights?ipAddress=%s", "WHOAMI_URL_ENDPOINT": "https://play.nativescript.org/api/whoami", "PREVIEW_APP_ENVIRONMENT": "live", "GA_TRACKING_ID": "UA-111455-51"