Skip to content

Commit

Permalink
fix(api): Regression on Data Export API + side effects between API te…
Browse files Browse the repository at this point in the history
…sts (#418)

* local optim: run docker:seed just once

* test: "data export api provides profile tracks of given user id"

* Revert "local optim: run docker:seed just once"

This reverts commit 93d9f65.

* test: "provides profile tracks of given user id, as a list of links"

* add same tests by username instead of user_id

* new tests: "provides playlist tracks of given user id"

* same, with username => error because "admin" user handle conflicts with admin routes

* => use DUMMY_USER to prevent conflict between handle and admin routes => "User not found..." error

* fix: don't let the presence of a playlistId preempt the retrieval of user by their handle

* add links to API docs

* fix: use DUMMY_USER instead of ADMIN_USER in post.api.tests.js

* extract data.api.tests.js from user.api.tests.js

* ❌ new test: "provides list of playlists"

* fix: make the /playlists handler return the actual list of playlists

* /playlists route now works in JSON format

* fix tests

* fix test expectation

* normalize formatting conditions

* make "bareFormats" ('json' / 'links') easier to find

* clean up playlist processing logic
  • Loading branch information
adrienjoly committed Dec 19, 2020
1 parent dda5b1c commit f4f1466
Show file tree
Hide file tree
Showing 6 changed files with 185 additions and 33 deletions.
21 changes: 11 additions & 10 deletions app/controllers/LibUser.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,15 +269,14 @@ function fetchAndRenderProfile(options, callback, process) {
options.bodyClass += ' userProfileV2';
options.nbPlaylists = (options.user.pl || []).length;
if (options.showPlaylists) {
var playlists = options.user.pl;
const playlists = options.user.pl;
//userModel.fetchPlaylists(options.user, {}, function(playlists) { // includes number of tracks per pl
options.pageTitle = 'Playlists by ' + options.user.name;
options.tabTitle = 'Playlists';
options.bodyClass += ' userPlaylists';
options.playlists = playlists.reverse();
playlists = playlists.reverse();
options.playlists = [...playlists].reverse(); // clone before reversing
options.showPlaylists = { items: renderPlaylists(options) };
process([]);
process(playlists);
//});
} else if (options.showLikes) {
options.tabTitle = 'Likes';
Expand Down Expand Up @@ -432,12 +431,12 @@ function fetchAndRenderProfile(options, callback, process) {
}
}

var bareFormats = { json: true, links: true };
var bareFormats = new Set(['json', 'links']);

function fetchAndRender(options, callback) {
options.bodyClass = '';

var process = bareFormats[options.format]
var process = bareFormats.has(options.format)
? callback
: function (posts) {
if (!options.format && !options.embedW) {
Expand Down Expand Up @@ -550,7 +549,7 @@ function renderUserLibrary(lib, user) {
safeCallback + '(' + JSON.stringify(feed) + ')',
'application/javascript'
);
} else if (options.format == 'links')
} else if (options.format == 'links') {
lib.renderOther(
feed
.map(function (p) {
Expand All @@ -559,9 +558,11 @@ function renderUserLibrary(lib, user) {
.join('\n'),
'text/text'
);
else if (options.format == 'json') lib.renderJson(feed);
else if (options.after || options.before) lib.render({ html: feed });
else
} else if (options.format == 'json') {
lib.renderJson(feed);
} else if (options.after || options.before) {
lib.render({ html: feed });
} else
lib.renderPage(
user,
null /*sidebarHtml*/,
Expand Down
8 changes: 3 additions & 5 deletions app/controllers/userLibrary.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,6 @@ exports.controller = function (request, reqParams, response) {
});
} else if (path == '/all') {
return renderAllLibrary(lib);
} else if (reqParams.playlistId) {
userModel.fetchByUid(reqParams.id, function (user) {
renderUserLibrary(lib, user);
});
} else if (reqParams.handle)
userModel.fetchByHandle(reqParams.handle, function (user) {
renderUserLibrary(lib, user);
Expand All @@ -201,5 +197,7 @@ exports.controller = function (request, reqParams, response) {
redirectTo(path.replace('/u/' + reqParams.id, '/' + user.handle));
else renderUserLibrary(lib, user);
});
} else response.badRequest();
} else {
response.badRequest();
}
};
139 changes: 139 additions & 0 deletions test/api/data.api.tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/* global describe, it, before */

const assert = require('assert');
const request = require('request');

const { URL_PREFIX, DUMMY_USER, cleanup } = require('../fixtures.js');
const api = require('../api-client.js');

const reqGet = (url) =>
new Promise((resolve, reject) =>
request.get({ url }, (err, response, body) =>
err ? reject(err) : resolve({ response, body })
)
);

const addTrackToPlaylist = (user, plName, post) =>
new Promise((resolve, reject) => {
const postInPlaylist = { ...post, pl: { id: 'create', name: plName } };
api.loginAs(DUMMY_USER, (error, { jar }) => {
api.addPost(jar, postInPlaylist, (error, res) =>
error ? reject(error) : resolve(res)
);
});
});

describe(`Data Export API`, () => {
// API documentation: https://openwhyd.github.io/openwhyd/API.html#openwhyd-data-export-api

before(cleanup); // to prevent side effects between tests

// add a playlist with one track
const user = DUMMY_USER;
const plName = 'my first playlist';
const track = {
name: 'my first track',
eId: '/yt/59MdiE1IsBY',
url: '//youtube.com/watch?v=59MdiE1IsBY',
};
before(function () {
this.timeout(4000);
return addTrackToPlaylist(user, plName, track);
});

describe(`provides profile tracks`, () => {
it(`of given user id, as JSON`, async () => {
const { body } = await reqGet(`${URL_PREFIX}/u/${user.id}?format=json`);
const parsedBody = JSON.parse(body) || {};
assert.strictEqual(parsedBody.error, undefined);
assert.strictEqual(parsedBody.length, 1);
assert.strictEqual(parsedBody[0].name, track.name);
});

it(`of given user id, as a list of links`, async () => {
const { body } = await reqGet(`${URL_PREFIX}/u/${user.id}?format=links`);
assert.strictEqual(body, track.url);
});

it(`of given user handle, as JSON`, async () => {
const { body } = await reqGet(`${URL_PREFIX}/${user.handle}?format=json`);
const parsedBody = JSON.parse(body) || {};
assert.strictEqual(parsedBody.error, undefined);
assert.strictEqual(parsedBody.length, 1);
assert.strictEqual(parsedBody[0].name, track.name);
});

it(`of given user handle, as a list of links`, async () => {
const { body } = await reqGet(
`${URL_PREFIX}/${user.handle}?format=links`
);
assert.strictEqual(body, track.url);
});
});

describe(`provides list of playlists`, () => {
it(`of given user id, as JSON`, async () => {
const plUrl = `${URL_PREFIX}/u/${user.id}/playlists`;
const { body } = await reqGet(`${plUrl}?format=json`);
const parsedBody = JSON.parse(body) || {};
assert.strictEqual(parsedBody.error, undefined);
assert.strictEqual(parsedBody.length, 1);
assert.strictEqual(parsedBody[0].name, plName);
});

it(`of given user id, as list`, async () => {
const expectedRes = `<a href="/u/${user.id}/playlist/0">${plName}</a>`;
const plUrl = `${URL_PREFIX}/u/${user.id}/playlists`;
const { body } = await reqGet(`${plUrl}?format=list`);
assert.notStrictEqual(body.indexOf(expectedRes), -1);
});

it(`of given user handle, as JSON`, async () => {
const plUrl = `${URL_PREFIX}/${user.handle}/playlists`;
const { body } = await reqGet(`${plUrl}?format=json`);
const parsedBody = JSON.parse(body) || {};
assert.strictEqual(parsedBody.error, undefined);
assert.strictEqual(parsedBody.length, 1);
assert.strictEqual(parsedBody[0].name, plName);
});

it(`of given user handle, as list`, async () => {
const expectedRes = `<a href="/u/${user.id}/playlist/0">${plName}</a>`;
const plUrl = `${URL_PREFIX}/${user.handle}/playlists`;
const { body } = await reqGet(`${plUrl}?format=list`);
assert.notStrictEqual(body.indexOf(expectedRes), -1);
});
});

describe(`provides playlist tracks`, () => {
it(`of given user id, as JSON`, async () => {
const plUrl = `${URL_PREFIX}/u/${user.id}/playlist/0`;
const { body } = await reqGet(`${plUrl}?format=json`);
const parsedBody = JSON.parse(body) || {};
assert.strictEqual(parsedBody.error, undefined);
assert.strictEqual(parsedBody.length, 1);
assert.strictEqual(parsedBody[0].name, track.name);
});

it(`of given user id, as a list of links`, async () => {
const plUrl = `${URL_PREFIX}/u/${user.id}/playlist/0`;
const { body } = await reqGet(`${plUrl}?format=links`);
assert.strictEqual(body, track.url);
});

it(`of given user handle, as JSON`, async () => {
const plUrl = `${URL_PREFIX}/${user.handle}/playlist/0`;
const { body } = await reqGet(`${plUrl}?format=json`);
const parsedBody = JSON.parse(body) || {};
assert.strictEqual(parsedBody.error, undefined);
assert.strictEqual(parsedBody.length, 1);
assert.strictEqual(parsedBody[0].name, track.name);
});

it(`of given user handle, as a list of links`, async () => {
const plUrl = `${URL_PREFIX}/u/${user.id}/playlist/0`;
const { body } = await reqGet(`${plUrl}?format=links`);
assert.strictEqual(body, track.url);
});
});
});
20 changes: 10 additions & 10 deletions test/api/post.api.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

var assert = require('assert');

var { ADMIN_USER, cleanup } = require('../fixtures.js');
var { DUMMY_USER, cleanup } = require('../fixtures.js');
var api = require('../api-client.js');

describe(`post api`, function () {
Expand All @@ -15,7 +15,7 @@ describe(`post api`, function () {
};

it(`should allow adding a track`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
api.addPost(jar, post, function (error, { body }) {
assert.ifError(error);
assert.equal(body.eId, post.eId);
Expand All @@ -29,7 +29,7 @@ describe(`post api`, function () {
});

it(`should allow re-adding a track (aka "repost")`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
api.addPost(jar, { pId }, function (error, { body }) {
assert.ifError(error);
assert(body._id);
Expand All @@ -52,7 +52,7 @@ describe(`post api`, function () {
});

it(`should allow adding a track to a playlist`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
api.addPost(jar, postInPlaylist, function (error, { body }) {
assert.ifError(error);
assert(body._id);
Expand All @@ -66,7 +66,7 @@ describe(`post api`, function () {
});

it(`make sure that the playlist was created`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
api.getUser(jar, {}, function (error, { body }) {
assert.equal(body.pl.length, 1);
assert.equal(body.pl[0].id, firstPlaylistIndex);
Expand All @@ -79,7 +79,7 @@ describe(`post api`, function () {
});

it(`should find 1 track in the playlist`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
api.getPlaylist(jar, playlistFullId, function (error, { body }) {
assert.ifError(error);
assert.equal(body.length, 1);
Expand All @@ -92,7 +92,7 @@ describe(`post api`, function () {
});

it(`should return 1 track in the playlist`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
api.getPlaylistTracks(jar, `u/${uId}`, firstPlaylistIndex, function (
error,
{ body }
Expand All @@ -106,7 +106,7 @@ describe(`post api`, function () {
});

it(`should return 1 track in the playlist, with limit=1000`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
const url = `/u/${uId}/playlist/${firstPlaylistIndex}?format=json&limit=1000`;
api.get(jar, url, function (error, { body }) {
assert.equal(body.length, 1);
Expand All @@ -118,7 +118,7 @@ describe(`post api`, function () {
});

it(`should return tracks if two limit parameters are provided`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
const url = `/u/${uId}/playlist/${firstPlaylistIndex}?format=json&limit=1000&limit=20`;
// => the `limit` property will be parsed as ["1000","20"] => causing bug #89
api.get(jar, url, function (error, { body }) {
Expand All @@ -132,7 +132,7 @@ describe(`post api`, function () {
// TODO: delete post

it(`should return the comment data after adding it`, function (done) {
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
const comment = {
pId,
text: 'hello world',
Expand Down
15 changes: 8 additions & 7 deletions test/api/user.api.tests.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
/* global describe, it, before */

var assert = require('assert');
var { ADMIN_USER, cleanup } = require('../fixtures.js');
var api = require('../api-client.js');
const assert = require('assert');

const { DUMMY_USER, cleanup } = require('../fixtures.js');
const api = require('../api-client.js');

describe(`user api -- getting user data`, function () {
before(cleanup); // to prevent side effects between tests

it(`gets user profile data`, function (done) {
const url =
'/api/user?includeSubscr=true&isSubscr=true&countPosts=true&countLikes=true&getVersion=1';
api.loginAs(ADMIN_USER, function (error, { jar }) {
api.loginAs(DUMMY_USER, function (error, { jar }) {
api.get(jar, url, function (err, { body, ...res }) {
assert.ifError(err);
assert.equal(res.response.statusCode, 200);
assert(!body.error);
assert.equal(body.email, ADMIN_USER.email);
assert.equal(body.email, DUMMY_USER.email);
assert(body.openwhydServerVersion);
done();
});
Expand All @@ -27,11 +28,11 @@ describe(`user api -- setting user data`, function () {
before(cleanup); // to prevent side effects between tests

it(`updates the user's name`, function (done) {
api.loginAs(ADMIN_USER, function (error, { body, jar }) {
api.loginAs(DUMMY_USER, function (error, { body, jar }) {
assert.ifError(body.error);
assert(body.redirect);
api.getUser(jar, {}, function (error, { body }) {
assert.equal(body.name, ADMIN_USER.name);
assert.equal(body.name, DUMMY_USER.name);
const newName = 'renamed user';
api.setUser(jar, { name: newName }, function (error, { body }) {
assert.equal(body.name, newName);
Expand Down
15 changes: 14 additions & 1 deletion test/fixtures.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const childProcess = require('child_process');

exports.URL_PREFIX = 'http://localhost:8080';

// inserted by config/initdb_testing.js
exports.ADMIN_USER = {
id: '000000000000000000000001',
email: process.env.WHYD_ADMIN_EMAIL || 'test@openwhyd.org',
Expand All @@ -12,6 +13,17 @@ exports.ADMIN_USER = {
md5: '21232f297a57a5a743894a0e4a801fc3',
};

// inserted by config/initdb_testing.js
exports.DUMMY_USER = {
id: '000000000000000000000002',
email: 'dummy@openwhyd.org',
name: 'dummy',
handle: 'dummy',
password: 'admin',
pwd: 'admin',
md5: '21232f297a57a5a743894a0e4a801fc3',
};

exports.TEST_USER = {
email: 'test-user@openwhyd.org',
name: 'Test User',
Expand All @@ -22,7 +34,8 @@ exports.TEST_USER = {
};

// Call this before each test to prevent side effects between tests
exports.cleanup = (done) => {
exports.cleanup = function (done) {
this.timeout(4000);
console.warn('🧹 Cleaning up test db...');
const process = childProcess.fork('test/reset-test-db.js');
process.on('close', () => done());
Expand Down

0 comments on commit f4f1466

Please sign in to comment.