0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-01-20 22:42:53 -05:00

Fixed member attribution for subdirectories (#15277)

fixes https://github.com/TryGhost/Team/issues/1829

- Remove the subdirectories when creating the Attribution instances
- URLs are now always stored relative to the subdirectory instead of the root directory (makes changing the subdirectory easier)
- Fixed returning absolute urls
- Added tests
This commit is contained in:
Simon Backx 2022-08-22 17:16:18 +02:00 committed by GitHub
parent adaecde430
commit fe3430202a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 450 additions and 130 deletions

View file

@ -1,6 +1,7 @@
const urlService = require('../url');
const labsService = require('../../../shared/labs');
const DomainEvents = require('@tryghost/domain-events');
const urlUtils = require('../../../shared/url-utils');
class MemberAttributionServiceWrapper {
init() {
@ -15,6 +16,7 @@ class MemberAttributionServiceWrapper {
const urlTranslator = new UrlTranslator({
urlService,
urlUtils,
models: {
Post: models.Post,
User: models.User,

View file

@ -1,9 +1,9 @@
const {agentProvider, fixtureManager} = require('../../utils/e2e-framework');
const {agentProvider, fixtureManager, configUtils} = require('../../utils/e2e-framework');
const should = require('should');
const nock = require('nock');
const models = require('../../../core/server/models');
const urlService = require('../../../core/server/services/url');
const memberAttributionService = require('../../../core/server/services/member-attribution');
const urlUtils = require('../../../core/shared/url-utils');
describe('Member Attribution Service', function () {
before(async function () {
@ -11,122 +11,368 @@ describe('Member Attribution Service', function () {
await fixtureManager.init('posts');
});
afterEach(function () {
nock.cleanAll();
});
/**
* Test that getAttribution correctly resolves all model types that are supported
*/
describe('getAttribution for models', function () {
it('resolves posts', async function () {
const id = fixtureManager.get('posts', 0).id;
const post = await models.Post.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(post.id, {absolute: false});
describe('without subdirectory', function () {
it('resolves urls', async function () {
const subdomainRelative = '/my-static-page/';
const url = urlUtils.createUrl(subdomainRelative, false);
const absoluteUrl = urlUtils.createUrl(subdomainRelative, true);
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: post.id,
url,
type: 'post'
}));
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: null,
url: subdomainRelative,
type: 'url'
}));
(await attribution.getResource()).should.match(({
id: null,
url: absoluteUrl,
type: 'url',
title: subdomainRelative
}));
});
const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true});
it('resolves posts', async function () {
const id = fixtureManager.get('posts', 0).id;
const post = await models.Post.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: post.id,
url,
type: 'post'
}));
const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true});
(await attribution.getResource()).should.match(({
id: post.id,
url: absoluteUrl,
type: 'post',
title: post.get('title')
}));
});
(await attribution.getResource()).should.match(({
id: post.id,
url: absoluteUrl,
type: 'post',
title: post.get('title')
}));
it('resolves removed resources', async function () {
const id = fixtureManager.get('posts', 0).id;
const post = await models.Post.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true});
const urlWithoutSubdirectory = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: false});
const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
// Without subdirectory
attribution.should.match(({
id: post.id,
url: urlWithoutSubdirectory,
type: 'post'
}));
// Unpublish this post
await models.Post.edit({status: 'draft'}, {id});
(await attribution.getResource()).should.match(({
id: null,
url: absoluteUrl,
type: 'url',
title: urlWithoutSubdirectory
}));
await models.Post.edit({status: 'published'}, {id});
});
it('resolves pages', async function () {
const id = fixtureManager.get('posts', 5).id;
const post = await models.Post.where('id', id).fetch({require: true});
should(post.get('type')).eql('page');
const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: post.id,
url,
type: 'page'
}));
const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true});
(await attribution.getResource()).should.match(({
id: post.id,
url: absoluteUrl,
type: 'page',
title: post.get('title')
}));
});
it('resolves tags', async function () {
const id = fixtureManager.get('tags', 0).id;
const tag = await models.Tag.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(tag.id, {absolute: false, withSubdirectory: true});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: tag.id,
url,
type: 'tag'
}));
const absoluteUrl = urlService.getUrlByResourceId(tag.id, {absolute: true, withSubdirectory: true});
(await attribution.getResource()).should.match(({
id: tag.id,
url: absoluteUrl,
type: 'tag',
title: tag.get('name')
}));
});
it('resolves authors', async function () {
const id = fixtureManager.get('users', 0).id;
const author = await models.User.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(author.id, {absolute: false, withSubdirectory: true});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: author.id,
url,
type: 'author'
}));
const absoluteUrl = urlService.getUrlByResourceId(author.id, {absolute: true, withSubdirectory: true});
(await attribution.getResource()).should.match(({
id: author.id,
url: absoluteUrl,
type: 'author',
title: author.get('name')
}));
});
});
it('resolves pages', async function () {
const id = fixtureManager.get('posts', 5).id;
const post = await models.Post.where('id', id).fetch({require: true});
should(post.get('type')).eql('page');
describe('with subdirectory', function () {
beforeEach(function () {
configUtils.set('url', 'https://siteurl.com/subdirectory/');
});
afterEach(function () {
configUtils.restore();
});
const url = urlService.getUrlByResourceId(post.id, {absolute: false});
it('resolves urls', async function () {
const subdomainRelative = '/my-static-page/';
const url = urlUtils.createUrl(subdomainRelative, false);
const absoluteUrl = urlUtils.createUrl(subdomainRelative, true);
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: post.id,
url,
type: 'page'
}));
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: null,
url: subdomainRelative,
type: 'url'
}));
(await attribution.getResource()).should.match(({
id: null,
url: absoluteUrl,
type: 'url',
title: subdomainRelative
}));
});
const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true});
it('resolves posts', async function () {
const id = fixtureManager.get('posts', 0).id;
const post = await models.Post.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true});
const urlWithoutSubdirectory = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: false});
(await attribution.getResource()).should.match(({
id: post.id,
url: absoluteUrl,
type: 'page',
title: post.get('title')
}));
});
// Check if we are actually testing with subdirectories
should(url).startWith('/subdirectory/');
it('resolves tags', async function () {
const id = fixtureManager.get('tags', 0).id;
const tag = await models.Tag.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(tag.id, {absolute: false});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: tag.id,
url,
type: 'tag'
}));
// Without subdirectory
attribution.should.match(({
id: post.id,
url: urlWithoutSubdirectory,
type: 'post'
}));
const absoluteUrl = urlService.getUrlByResourceId(tag.id, {absolute: true});
const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true});
(await attribution.getResource()).should.match(({
id: tag.id,
url: absoluteUrl,
type: 'tag',
title: tag.get('name')
}));
});
(await attribution.getResource()).should.match(({
id: post.id,
url: absoluteUrl,
type: 'post',
title: post.get('title')
}));
});
it('resolves authors', async function () {
const id = fixtureManager.get('users', 0).id;
const author = await models.User.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(author.id, {absolute: false});
it('resolves removed resources', async function () {
const id = fixtureManager.get('posts', 0).id;
const post = await models.Post.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true});
const urlWithoutSubdirectory = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: false});
const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: author.id,
url,
type: 'author'
}));
// Check if we are actually testing with subdirectories
should(url).startWith('/subdirectory/');
const absoluteUrl = urlService.getUrlByResourceId(author.id, {absolute: true});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
(await attribution.getResource()).should.match(({
id: author.id,
url: absoluteUrl,
type: 'author',
title: author.get('name')
}));
// Without subdirectory
attribution.should.match(({
id: post.id,
url: urlWithoutSubdirectory,
type: 'post'
}));
// Unpublish this post
await models.Post.edit({status: 'draft'}, {id});
(await attribution.getResource()).should.match(({
id: null,
url: absoluteUrl,
type: 'url',
title: urlWithoutSubdirectory
}));
});
it('resolves pages', async function () {
const id = fixtureManager.get('posts', 5).id;
const post = await models.Post.where('id', id).fetch({require: true});
should(post.get('type')).eql('page');
const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true});
const urlWithoutSubdirectory = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: false});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: post.id,
url: urlWithoutSubdirectory,
type: 'page'
}));
const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true});
(await attribution.getResource()).should.match(({
id: post.id,
url: absoluteUrl,
type: 'page',
title: post.get('title')
}));
});
it('resolves tags', async function () {
const id = fixtureManager.get('tags', 0).id;
const tag = await models.Tag.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(tag.id, {absolute: false, withSubdirectory: true});
const urlWithoutSubdirectory = urlService.getUrlByResourceId(tag.id, {absolute: false, withSubdirectory: false});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: tag.id,
url: urlWithoutSubdirectory,
type: 'tag'
}));
const absoluteUrl = urlService.getUrlByResourceId(tag.id, {absolute: true, withSubdirectory: true});
(await attribution.getResource()).should.match(({
id: tag.id,
url: absoluteUrl,
type: 'tag',
title: tag.get('name')
}));
});
it('resolves authors', async function () {
const id = fixtureManager.get('users', 0).id;
const author = await models.User.where('id', id).fetch({require: true});
const url = urlService.getUrlByResourceId(author.id, {absolute: false, withSubdirectory: true});
const urlWithoutSubdirectory = urlService.getUrlByResourceId(author.id, {absolute: false, withSubdirectory: false});
const attribution = memberAttributionService.service.getAttribution([
{
path: url,
time: 123
}
]);
attribution.should.match(({
id: author.id,
url: urlWithoutSubdirectory,
type: 'author'
}));
const absoluteUrl = urlService.getUrlByResourceId(author.id, {absolute: true, withSubdirectory: true});
(await attribution.getResource()).should.match(({
id: author.id,
url: absoluteUrl,
type: 'author',
title: author.get('name')
}));
});
});
});
});

View file

@ -1,7 +1,7 @@
/**
* @typedef {object} AttributionResource
* @prop {string|null} id
* @prop {string|null} url
* @prop {string|null} url (absolute URL)
* @prop {'page'|'post'|'author'|'tag'|'url'} type
* @prop {string|null} title
*/
@ -12,7 +12,7 @@ class Attribution {
/**
* @param {object} data
* @param {string|null} [data.id]
* @param {string|null} [data.url]
* @param {string|null} [data.url] Relative to subdirectory
* @param {'page'|'post'|'author'|'tag'|'url'} [data.type]
*/
constructor({id, url, type}, {urlTranslator}) {
@ -39,8 +39,7 @@ class Attribution {
return {
id: null,
type: 'url',
// TODO: make url absolute
url: this.url,
url: this.#urlTranslator.relativeToAbsolute(this.url),
title: this.url
};
}
@ -54,8 +53,7 @@ class Attribution {
return {
id: null,
type: 'url',
// TODO: make url absolute
url: this.url,
url: this.#urlTranslator.relativeToAbsolute(this.url),
title: this.url
};
}
@ -96,12 +94,22 @@ class AttributionBuilder {
});
}
// Convert history to subdirectory relative (instead of root relative)
// Note: this is ordered from recent to oldest!
const subdirectoryRelativeHistory = [];
for (const item of history) {
subdirectoryRelativeHistory.push({
...item,
path: this.urlTranslator.stripSubdirectoryFromPath(item.path)
});
}
// TODO: if something is wrong with the attribution script, and it isn't loading
// we might get out of date URLs
// so we need to check the time of each item and ignore items that are older than 24u here!
// Start at the end. Return the first post we find
for (const item of history) {
for (const item of subdirectoryRelativeHistory) {
const typeId = this.urlTranslator.getTypeAndId(item.path);
if (typeId && typeId.type === 'post') {
@ -114,7 +122,7 @@ class AttributionBuilder {
// No post found?
// Try page or tag or author
for (const item of history) {
for (const item of subdirectoryRelativeHistory) {
const typeId = this.urlTranslator.getTypeAndId(item.path);
if (typeId) {
@ -129,7 +137,7 @@ class AttributionBuilder {
// In the future we might decide to exclude certain URLs, that can happen here
return this.build({
id: null,
url: history.last.path,
url: subdirectoryRelativeHistory[0].path,
type: 'url'
});
}

View file

@ -20,13 +20,6 @@ class UrlHistory {
return this.history.length;
}
get last() {
if (this.length === 0) {
return undefined;
}
return this.history[this.history.length - 1];
}
/**
* Iterate from latest item to newest item (reversed!)
*/

View file

@ -5,7 +5,6 @@ class MemberAttributionService {
*
* @param {Object} deps
* @param {Object} deps.attributionBuilder
* @param {Object} deps.labsService
* @param {Object} deps.models
* @param {Object} deps.models.MemberCreatedEvent
* @param {Object} deps.models.SubscriptionCreatedEvent

View file

@ -14,16 +14,38 @@ class UrlTranslator {
*
* @param {Object} deps
* @param {UrlService} deps.urlService
* @param {Object} deps.urlUtils
* @param {Object} deps.models
* @param {Object} deps.models.Post
* @param {Object} deps.models.Tag
* @param {Object} deps.models.User
*/
constructor({urlService, models}) {
constructor({urlService, urlUtils, models}) {
this.urlService = urlService;
this.urlUtils = urlUtils;
this.models = models;
}
/**
* Convert root relative URLs to subdirectory relative URLs
*/
stripSubdirectoryFromPath(path) {
// Bit weird, but only way to do it with the urlUtils atm
// First convert path to an absolute path
const absolute = this.urlUtils.relativeToAbsolute(path);
// Then convert it to a relative path, but without subdirectory
return this.urlUtils.absoluteToRelative(absolute, {withoutSubdirectory: true});
}
/**
* Convert root relative URLs to subdirectory relative URLs
*/
relativeToAbsolute(path) {
return this.urlUtils.relativeToAbsolute(path);
}
getTypeAndId(url) {
const resource = this.urlService.getResource(url);
if (!resource) {

View file

@ -32,9 +32,18 @@ describe('AttributionBuilder', function () {
return {
id,
type,
url: '/path',
url: 'https://absolute/dir/path',
title: 'Title'
};
},
relativeToAbsolute(path) {
return 'https://absolute/dir' + path;
},
stripSubdirectoryFromPath(path) {
if (path.startsWith('/dir/')) {
return path.substring('/dir/'.length - 1);
}
return path;
}
}
});
@ -46,33 +55,33 @@ describe('AttributionBuilder', function () {
});
it('Returns last url', function () {
const history = new UrlHistory([{path: '/not-last', time: 123}, {path: '/test', time: 123}]);
should(attributionBuilder.getAttribution(history)).match({type: 'url', id: null, url: '/test'});
const history = new UrlHistory([{path: '/dir/not-last', time: 123}, {path: '/dir/test/', time: 123}]);
should(attributionBuilder.getAttribution(history)).match({type: 'url', id: null, url: '/test/'});
});
it('Returns last post', function () {
const history = new UrlHistory([
{path: '/my-post', time: 123},
{path: '/test', time: 124},
{path: '/unknown-page', time: 125}
{path: '/dir/my-post', time: 123},
{path: '/dir/test', time: 124},
{path: '/dir/unknown-page', time: 125}
]);
should(attributionBuilder.getAttribution(history)).match({type: 'post', id: 123, url: '/my-post'});
});
it('Returns last post even when it found pages', function () {
const history = new UrlHistory([
{path: '/my-post', time: 123},
{path: '/my-page', time: 124},
{path: '/unknown-page', time: 125}
{path: '/dir/my-post', time: 123},
{path: '/dir/my-page', time: 124},
{path: '/dir/unknown-page', time: 125}
]);
should(attributionBuilder.getAttribution(history)).match({type: 'post', id: 123, url: '/my-post'});
});
it('Returns last page if no posts', function () {
const history = new UrlHistory([
{path: '/other', time: 123},
{path: '/my-page', time: 124},
{path: '/unknown-page', time: 125}
{path: '/dir/other', time: 123},
{path: '/dir/my-page', time: 124},
{path: '/dir/unknown-page', time: 125}
]);
should(attributionBuilder.getAttribution(history)).match({type: 'page', id: 845, url: '/my-page'});
});
@ -99,7 +108,7 @@ describe('AttributionBuilder', function () {
should(await attributionBuilder.build({type: 'post', id: '123', url: '/post'}).getResource()).match({
type: 'post',
id: '123',
url: '/path',
url: 'https://absolute/dir/path',
title: 'Title'
});
});
@ -108,7 +117,7 @@ describe('AttributionBuilder', function () {
should(await attributionBuilder.build({type: 'url', id: null, url: '/url'}).getResource()).match({
type: 'url',
id: null,
url: '/url',
url: 'https://absolute/dir/url',
title: '/url'
});
});
@ -117,7 +126,7 @@ describe('AttributionBuilder', function () {
should(await attributionBuilder.build({type: 'post', id: 'invalid', url: '/post'}).getResource()).match({
type: 'url',
id: null,
url: '/post',
url: 'https://absolute/dir/post',
title: '/post'
});
});

View file

@ -166,4 +166,45 @@ describe('UrlTranslator', function () {
should(await translator.getResourceById('invalid', 'tag')).eql(null);
});
});
describe('relativeToAbsolute', function () {
let translator;
before(function () {
translator = new UrlTranslator({
urlUtils: {
relativeToAbsolute: (t) => {
return 'absolute/' + t;
}
}
});
});
it('passes relativeToAbsolute to urlUtils', async function () {
should(translator.relativeToAbsolute('relative')).eql('absolute/relative');
});
});
describe('stripSubdirectoryFromPath', function () {
let translator;
before(function () {
translator = new UrlTranslator({
urlUtils: {
relativeToAbsolute: (t) => {
return 'absolute' + t;
},
absoluteToRelative: (t) => {
const prefix = 'absolute/dir/';
if (t.startsWith(prefix)) {
return t.substring(prefix.length - 1);
}
return t;
}
}
});
});
it('passes calls to urlUtils', async function () {
should(translator.stripSubdirectoryFromPath('/dir/relative')).eql('/relative');
});
});
});