Skip to content

Commit

Permalink
fix(track-content): fix issue of not downloading when button is click…
Browse files Browse the repository at this point in the history 
…ed before track info is fetched
  • Loading branch information
@xtangle
xtangle committed May 18, 2018
1 parent 2055eac commit eb81f32
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 51 deletions.
4 changes: 2 additions & 2 deletions karma.conf.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module.exports = function (config) {
'test/ts/**/*.ts': ['karma-typescript']
},
client: {
captureConsole: true,
captureConsole: false,
mocha: {
opts: 'mocha.opts'
}
Expand Down Expand Up @@ -44,7 +44,7 @@ module.exports = function (config) {
},
port: 9876,
colors: true,
logLevel: config.LOG_DISABLE,
logLevel: config.LOG_INFO,
autoWatch: true,
browsers: ['ChromeHeadless'],
singleRun: true,
Expand Down
10 changes: 5 additions & 5 deletions src/ts/download/metadata/id3-metadata-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ export const ID3MetadataService: IID3MetadataService = {
switchMap(writeMetadata$.bind(null, metadata)),
map(ID3WriterService.getURL),
map((url: string) => ({...downloadOptions, url})),
timeout(30000),
timeout(60000),
catchError((err: any) => {
logger.error(err);
logger.error('Unable to fetch metadata', err);
return of(downloadOptions);
})
);
Expand Down Expand Up @@ -66,9 +66,9 @@ function withCoverArt$(metadata: ITrackMetadata, writer: IID3Writer): Observable
useUnicodeEncoding: false
})
),
timeout(10000),
catchError((err: any) => {
logger.error(err);
timeout(20000),
catchError((err) => {
logger.error('Unable to fetch cover art', err);
return of(writer);
})
);
Expand Down
28 changes: 20 additions & 8 deletions src/ts/page/track-content-page.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {logger} from '@src/util/logger';
import {UrlService} from '@src/util/url-service';
import * as $ from 'jquery';
import {BehaviorSubject, fromEvent, merge, Subscription} from 'rxjs';
import {map, throttleTime} from 'rxjs/operators';
import {first, skipWhile, switchMap, takeWhile, throttleTime, timeout} from 'rxjs/operators';

export const ZC_TRACK_DL_BUTTON_ID = 'zcTrackDlButton';

Expand Down Expand Up @@ -98,16 +98,28 @@ export class TrackContentPage implements IContentPage {
.attr('id', ZC_TRACK_DL_BUTTON_ID)
.prop('title', 'Download this track')
.html('Download');
const downloadClick$ = fromEvent(dlButton[0], 'click').pipe(throttleTime(3000));
this.addDlButtonBehavior(dlButton);
return dlButton;
}

private addDlButtonBehavior(dlButton: JQuery<HTMLElement>) {
const downloadClick$ = fromEvent(dlButton, 'click').pipe(throttleTime(3000));
const toFirstTrackInfo$ = () => this.trackInfo$.pipe(
skipWhile((trackInfo) => trackInfo === null),
takeWhile((trackInfo) => trackInfo !== null),
first(),
timeout(30000)
);
this.subscriptions.add(
downloadClick$.pipe(map(() => this.trackInfo$.getValue()))
.subscribe((trackInfo: ITrackInfo) => {
if (trackInfo) {
downloadClick$.pipe(switchMap(toFirstTrackInfo$))
.subscribe(
(trackInfo) => {
logger.debug('Downloading track', trackInfo.id);
ContentPageMessenger.sendToExtension(new RequestTrackDownloadMessage(trackInfo));
}
})
},
(err) => logger.error('Timeout when fetching track info', err)
)
);
return dlButton;
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/ts/download/metadata/id3-metadata-service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('id3 metadata service', () => {
it('should not add metadata if fetching the song data times out', () => {
stubGetArrayBuffer$.withArgs(downloadOptions.url).returns(NEVER);
rx.subscribeTo(fixture.addID3V2Metadata$(metadata, downloadOptions));
fakeTimer.tick(31000);
fakeTimer.tick(60001);

verifyDownloadOptionsEmittedWithUrl(downloadOptions.url);
});
Expand Down Expand Up @@ -125,14 +125,14 @@ describe('id3 metadata service', () => {
it('should not add cover art metadata if fetching cover art data times out', () => {
stubGetArrayBuffer$.withArgs(metadata.cover_url).returns(NEVER);
rx.subscribeTo(fixture.addID3V2Metadata$(metadata, downloadOptions));
fakeTimer.tick(11000);
fakeTimer.tick(20001);

expect(stubSetFrame).to.not.have.been.calledWith(writer, 'APIC', match.any);
verifyDownloadOptionsEmittedWithUrl(metadataAddedURL);
});

it('should not add cover art metadata if there is an error while fetching cover art', () => {
stubGetArrayBuffer$.withArgs(metadata.cover_url).returns(throwError('error fetching cover art'));
stubGetArrayBuffer$.withArgs(metadata.cover_url).returns(throwError('Some error'));
rx.subscribeTo(fixture.addID3V2Metadata$(metadata, downloadOptions));
fakeTimer.next();

Expand Down
34 changes: 17 additions & 17 deletions test/ts/page/track-content-page.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {ContentPageMessenger} from '@src/messaging/page/content-page-messenger';
import {RequestTrackDownloadMessage} from '@src/messaging/page/request-track-download.message';
import {TrackContentPage, ZC_TRACK_DL_BUTTON_ID} from '@src/page/track-content-page';
import {UrlService} from '@src/util/url-service';
import {useSinonChai, useSinonChrome} from '@test/test-initializers';
import {useFakeTimer, useSinonChai, useSinonChrome} from '@test/test-initializers';
import {tick} from '@test/test-utils';
import * as $ from 'jquery';
import {BehaviorSubject, Subject, Subscription} from 'rxjs';
Expand Down Expand Up @@ -208,19 +208,13 @@ describe('track content page', () => {
});

describe('the download button behavior', () => {
const sinon = require('sinon');
let fakeTimer: SinonFakeTimers;
const cw = useFakeTimer();

beforeEach('ensure download button is injected', () => {
fakeTimer = sinon.useFakeTimers();
document.body.innerHTML = testHtml;
fixture.load();
});

afterEach(() => {
fakeTimer.restore();
});

it('should have the correct classes', () => {
const selector = '.sc-button.sc-button-medium.sc-button-responsive' +
`.${ZC_DL_BUTTON_ICON_CLASS}.${ZC_DL_BUTTON_CLASS}`;
Expand Down Expand Up @@ -252,27 +246,33 @@ describe('track content page', () => {
describe('the behavior when clicked', () => {
let stubSendToExtension: SinonStub;

before(() => {
beforeEach(() => {
stubSendToExtension = stub(ContentPageMessenger, 'sendToExtension');
});

afterEach(() => {
stubSendToExtension.resetHistory();
});

after(() => {
stubSendToExtension.restore();
});

it('should send a request download message if track info is not null', () => {
it('should send a request download message if there is track info', () => {
fakeTrackInfo$.next(fakeTrackInfo);
getDlButton().trigger('click');
expect(stubSendToExtension).to.have.been.calledOnce
.calledWithExactly(new RequestTrackDownloadMessage(fakeTrackInfo));
});

it('should not send a request download message if there is no track info', () => {
it('should send a request download message if track info is received within 30s', () => {
getDlButton().trigger('click');
cw.clock.tick(29999);
fakeTrackInfo$.next(fakeTrackInfo);
expect(stubSendToExtension).to.have.been.calledOnce
.calledWithExactly(new RequestTrackDownloadMessage(fakeTrackInfo));
});

it('should not send a request download message if track info is not received within 30s', () => {
getDlButton().trigger('click');
cw.clock.tick(30001);
fakeTrackInfo$.next(fakeTrackInfo);
expect(stubSendToExtension).to.not.have.been.called;
});

Expand All @@ -284,11 +284,11 @@ describe('track content page', () => {
it('should throttle clicks that are within 3s of each other', () => {
fakeTrackInfo$.next(fakeTrackInfo);
getDlButton().trigger('click');
fakeTimer.tick(2900);
cw.clock.tick(2900);
getDlButton().trigger('click');
expect(stubSendToExtension).to.have.been.calledOnce;

fakeTimer.tick(101);
cw.clock.tick(101);
getDlButton().trigger('click');
expect(stubSendToExtension).to.have.been.calledTwice;
});
Expand Down
30 changes: 14 additions & 16 deletions test/ts/util/logger.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,39 +9,37 @@ describe('logger', () => {
let stubDebug: SinonStub;
let stubError: SinonStub;

before(() => {
beforeEach(() => {
stubDebug = stub(console, 'debug');
stubError = stub(console, 'error');
});

afterEach(() => {
stubDebug.resetHistory();
stubError.resetHistory();
});

after(() => {
process.env.NODE_ENV = prevNodeEnv;
stubDebug.restore();
stubError.restore();
process.env.NODE_ENV = prevNodeEnv;
});

it('should print debug message to console if in development mode', () => {
it('should print debug message to console in development mode', () => {
process.env.NODE_ENV = 'development';
logger.debug('some message', 1, 'arg-two');
expect(stubDebug).to.have.been.calledOnce
.calledWithExactly(`ZC: some message`, 1, 'arg-two');
expect(stubDebug).to.have.been.calledOnce.calledWithExactly(`ZC: some message`, 1, 'arg-two');
});

it('should not print debug message to console if not in development mode', () => {
it('should not print debug message to console in production mode', () => {
process.env.NODE_ENV = 'production';
logger.debug('some message');
expect(stubDebug).to.not.have.been.called;
});

it('should print error message to console', () => {
const ev = new ErrorEvent('some error type');
logger.error('some error message', ev);
expect(stubError).to.have.been.calledOnce
.calledWithExactly(`ZC: some error message`, ev);
it('should print just error message to console', () => {
logger.error('Some error message');
expect(stubError).to.have.been.calledOnce.calledWithExactly(`ZC: Some error message`);
});

it('should print error message along with error to console', () => {
const ev = new ErrorEvent('Some error');
logger.error('Some error message', ev);
expect(stubError).to.have.been.calledOnce.calledWithExactly(`ZC: Some error message`, ev);
});
});

0 comments on commit eb81f32

Please sign in to comment.