From cddaaa2c4a6ea0d4c6d10f51d92cb2c01e39a7fa Mon Sep 17 00:00:00 2001 From: Chris Williams Date: Wed, 17 Oct 2018 12:53:42 -0700 Subject: [PATCH 1/2] [superset-client][logger] replace ajax with SupersetClient --- .../assets/spec/javascripts/logger_spec.js | 88 +++++++++++-------- superset/assets/src/logger.js | 20 ++--- 2 files changed, 56 insertions(+), 52 deletions(-) diff --git a/superset/assets/spec/javascripts/logger_spec.js b/superset/assets/spec/javascripts/logger_spec.js index 4b857593c2fac..147e3c7d7f676 100644 --- a/superset/assets/spec/javascripts/logger_spec.js +++ b/superset/assets/spec/javascripts/logger_spec.js @@ -1,5 +1,4 @@ -import $ from 'jquery'; -import sinon from 'sinon'; +import fetchMock from 'fetch-mock'; import { Logger, ActionLog } from '../../src/logger'; @@ -47,6 +46,10 @@ describe('ActionLog', () => { }); describe('Logger', () => { + const logEndpoint = 'glob:*/superset/log/*'; + fetchMock.post(logEndpoint, 'success'); + afterEach(fetchMock.reset); + it('should add events when .append(eventName, eventBody) is called', () => { const eventName = 'testEvent'; const eventBody = { test: 'event' }; @@ -59,13 +62,6 @@ describe('Logger', () => { }); describe('.send()', () => { - beforeEach(() => { - sinon.spy($, 'ajax'); - }); - afterEach(() => { - $.ajax.restore(); - }); - const eventNames = ['test']; function setup(overrides = {}) { @@ -73,16 +69,17 @@ describe('Logger', () => { return log; } - it('should POST an event to /superset/log/ when called', () => { + it('should POST an event to /superset/log/ when called', (done) => { const log = setup(); Logger.start(log); Logger.append(eventNames[0], { test: 'event' }); expect(log.events[eventNames[0]]).toHaveLength(1); Logger.end(log); - expect($.ajax.calledOnce).toBe(true); - const args = $.ajax.getCall(0).args[0]; - expect(args.url).toBe('/superset/log/'); - expect(args.method).toBe('POST'); + + setTimeout(() => { + expect(fetchMock.calls(logEndpoint)).toHaveLength(1); + done(); + }); }); it("should flush the log's events", () => { @@ -97,7 +94,7 @@ describe('Logger', () => { it( 'should include ts, start_offset, event_name, impression_id, source, and source_id in every event', - () => { + (done) => { const config = { eventNames: ['event1', 'event2'], impressionId: 'impress_me', @@ -111,39 +108,52 @@ describe('Logger', () => { Logger.append('event2', { foo: 'bar' }); Logger.end(log); - const args = $.ajax.getCall(0).args[0]; - const events = JSON.parse(args.data.events); - - expect(events).toHaveLength(2); - expect(events[0]).toMatchObject({ - key: 'value', - event_name: 'event1', - impression_id: config.impressionId, - source: config.source, - source_id: config.sourceId, - }); - expect(events[1]).toMatchObject({ - foo: 'bar', - event_name: 'event2', - impression_id: config.impressionId, - source: config.source, - source_id: config.sourceId, + setTimeout(() => { + const calls = fetchMock.calls(logEndpoint); + expect(calls).toHaveLength(1); + const options = calls[0][1]; + const events = JSON.parse(options.body.get('events')); + + expect(events).toHaveLength(2); + + expect(events[0]).toMatchObject({ + key: 'value', + event_name: 'event1', + impression_id: config.impressionId, + source: config.source, + source_id: config.sourceId, + }); + + expect(events[1]).toMatchObject({ + foo: 'bar', + event_name: 'event2', + impression_id: config.impressionId, + source: config.source, + source_id: config.sourceId, + }); + + expect(typeof events[0].ts).toBe('number'); + expect(typeof events[1].ts).toBe('number'); + expect(typeof events[0].start_offset).toBe('number'); + expect(typeof events[1].start_offset).toBe('number'); + + done(); }); - expect(typeof events[0].ts).toBe('number'); - expect(typeof events[1].ts).toBe('number'); - expect(typeof events[0].start_offset).toBe('number'); - expect(typeof events[1].start_offset).toBe('number'); }, ); it( 'should send() a log immediately if .append() is called with sendNow=true', - () => { + (done) => { const log = setup(); Logger.start(log); Logger.append(eventNames[0], { test: 'event' }, true); - expect($.ajax.calledOnce).toBe(true); - Logger.end(log); + + setTimeout(() => { + expect(fetchMock.calls(logEndpoint)).toHaveLength(1); + Logger.end(log); // flush logs + done(); + }, 0); }, ); }); diff --git a/superset/assets/src/logger.js b/superset/assets/src/logger.js index afd33d8ea0d6d..7378520daeada 100644 --- a/superset/assets/src/logger.js +++ b/superset/assets/src/logger.js @@ -1,6 +1,5 @@ /* eslint no-console: 0 */ - -import $ from 'jquery'; +import { SupersetClient } from '@superset-ui/core'; // This creates an association between an eventName and the ActionLog instance so that // Logger.append calls do not have to know about the appropriate ActionLog instance @@ -41,13 +40,13 @@ export const Logger = { send(log) { const { impressionId, source, sourceId, events } = log; - let url = '/superset/log/'; + let endpoint = '/superset/log/?explode=events'; // backend logs treat these request params as first-class citizens if (source === 'dashboard') { - url += `?dashboard_id=${sourceId}`; + endpoint += `&dashboard_id=${sourceId}`; } else if (source === 'slice') { - url += `?slice_id=${sourceId}`; + endpoint += `&slice_id=${sourceId}`; } const eventData = []; @@ -63,14 +62,9 @@ export const Logger = { }); } - $.ajax({ - url, - method: 'POST', - dataType: 'json', - data: { - explode: 'events', - events: JSON.stringify(eventData), - }, + SupersetClient.post({ + endpoint, + postPayload: { events: eventData }, }); // flush events for this logger From 2fdf01441e28ccf787ca9aeab3beb58097c1dfd6 Mon Sep 17 00:00:00 2001 From: Chris Williams Date: Wed, 17 Oct 2018 15:03:06 -0700 Subject: [PATCH 2/2] [superset-client][logger] don't parse log response --- superset/assets/spec/javascripts/logger_spec.js | 7 ++++--- superset/assets/src/logger.js | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/superset/assets/spec/javascripts/logger_spec.js b/superset/assets/spec/javascripts/logger_spec.js index 147e3c7d7f676..36e292398a856 100644 --- a/superset/assets/spec/javascripts/logger_spec.js +++ b/superset/assets/spec/javascripts/logger_spec.js @@ -42,13 +42,14 @@ describe('ActionLog', () => { log.addEvent(eventName, eventBody); expect(log.events[eventName]).toHaveLength(1); expect(log.events[eventName][0]).toMatchObject(eventBody); + Logger.end(log); }); }); describe('Logger', () => { const logEndpoint = 'glob:*/superset/log/*'; fetchMock.post(logEndpoint, 'success'); - afterEach(fetchMock.reset); + afterEach(fetchMock.resetHistory); it('should add events when .append(eventName, eventBody) is called', () => { const eventName = 'testEvent'; @@ -79,7 +80,7 @@ describe('Logger', () => { setTimeout(() => { expect(fetchMock.calls(logEndpoint)).toHaveLength(1); done(); - }); + }, 0); }); it("should flush the log's events", () => { @@ -138,7 +139,7 @@ describe('Logger', () => { expect(typeof events[1].start_offset).toBe('number'); done(); - }); + }, 0); }, ); diff --git a/superset/assets/src/logger.js b/superset/assets/src/logger.js index 7378520daeada..abc6339a11140 100644 --- a/superset/assets/src/logger.js +++ b/superset/assets/src/logger.js @@ -65,6 +65,7 @@ export const Logger = { SupersetClient.post({ endpoint, postPayload: { events: eventData }, + parseMethod: null, }); // flush events for this logger @@ -82,7 +83,7 @@ export class ActionLog { this.impressionId = impressionId; this.source = source; this.sourceId = sourceId; - this.eventNames = eventNames; + this.eventNames = eventNames || []; this.sendNow = sendNow || false; this.events = {};