Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Fixed Twitter not rendering in Emails #22015

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ class TwitterOEmbedProvider {
Authorization: `Bearer ${this.dependencies.config.bearerToken}`
}
}).json();

oembedData.tweet_data = body.data;
oembedData.tweet_data.includes = body.includes;
} catch (err) {
Expand Down
131 changes: 131 additions & 0 deletions ghost/core/core/server/services/oembed/XEmbedProvider.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
const {ValidationError} = require('@tryghost/errors');

const TWITTER_PATH_REGEX = /\/status\/(\d+)/;

/**
* @typedef {import('./oembed').ICustomProvider} ICustomProvider
* @typedef {import('./oembed').IExternalRequest} IExternalRequest
*/

/**
* @implements ICustomProvider
*/
class XEmbedProvider {
/**
* @param {object} dependencies
*/
constructor(dependencies) {
this._dependencies = dependencies;
}

/**
* Checks if the provided URL can be supported (i.e., if it is a Twitter or X.com URL).
* @param {URL} url
* @returns {Promise<boolean>}
*/
async canSupportRequest(url) {
return (url.host === 'twitter.com' || url.host === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname);
}
Comment on lines +26 to +28
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use url.hostname instead of url.host for reliable host comparison

In the canSupportRequest method, using url.host may cause host comparison failures when the URL includes a port number (e.g., twitter.com:80). The url.hostname property excludes the port number and should be used instead for accurate host comparisons.

Apply this diff to fix the issue:

-            return (url.host === 'twitter.com' || url.host === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname);
+            return (url.hostname === 'twitter.com' || url.hostname === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async canSupportRequest(url) {
return (url.host === 'twitter.com' || url.host === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname);
}
async canSupportRequest(url) {
return (url.hostname === 'twitter.com' || url.hostname === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't expect a Twitter / X url to ever include a port number.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ronaldlangeveld Understood, I'll retract the suggestion since Twitter/X URLs won't contain port numbers in your use case.


✏️ Learnings added
Learnt from: ronaldlangeveld
PR: TryGhost/Ghost#22015
File: ghost/core/core/server/services/oembed/XEmbedProvider.js:26-28
Timestamp: 2025-01-20T07:04:25.902Z
Learning: In the Ghost codebase, Twitter/X URLs are not expected to include port numbers, so using either `url.host` or `url.hostname` is acceptable when parsing these URLs.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


/**
* Extracts the Tweet ID from the given URL.
* @param {URL} url
* @returns {Promise<string>}
* @throws {ValidationError} If the URL is invalid.
*/
async getTweetId(url) {
const match = TWITTER_PATH_REGEX.exec(url.pathname);

if (!match) {
throw new ValidationError({
message: 'Invalid URL'
});
}

return match[1];
}

// Maps tweet entity in email template compatible format
async mapTweetToTweetData(tweetEntity) {
const urls = (tweetEntity.entities?.urls || []).map((url) => {
return {
url: url,
display_url: url.replace(/(^\w+:|^)\/\//, '') // Remove the protocol
};
});

const mentionedUsers = (tweetEntity.entities?.mentionedUsers || []).map((mention) => {
return {
username: mention
};
});
const tweetData = {
id: tweetEntity.id,
author_id: tweetEntity.tweetBy.id,
public_metrics: {
retweet_count: tweetEntity.retweetCount,
like_count: tweetEntity.likeCount
},
users: [{
id: tweetEntity.tweetBy.id,
name: tweetEntity.tweetBy.fullName,
username: tweetEntity.tweetBy.userName,
profile_image_url: tweetEntity.tweetBy.profileImage
}],
text: tweetEntity.fullText,
created_at: new Date(tweetEntity.createdAt).toISOString(),
entities: {
urls: urls,
hashtags: tweetEntity.entities.hashtags.map((hashtag) => {
return {
tag: hashtag
};
}),
mentions: mentionedUsers
},
Comment on lines +75 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null checks for optional properties to prevent runtime errors

In the mapTweetToTweetData method, accessing tweetEntity.entities.hashtags and tweetEntity.entities.mentionedUsers without checking if entities or their properties are defined may cause runtime errors if they are undefined or null. Consider adding optional chaining or default values to ensure robustness.

Apply this diff to add safe accessors:

-            const urls = (tweetEntity.entities?.urls || []).map((url) => {
-                return {
-                    url: url,
-                    display_url: url.replace(/(^\w+:|^)\/\//, '') // Remove the protocol
-                };
-            });
+            const urls = (tweetEntity.entities?.urls || []).map((url) => ({
+                url: url,
+                display_url: url.replace(/(^\w+:|^)\/\//, '') // Remove the protocol
+            }));

-            const mentionedUsers = (tweetEntity.entities?.mentionedUsers || []).map((mention) => {
-                return {
-                    username: mention
-                };
-            });
+            const mentionedUsers = (tweetEntity.entities?.mentionedUsers || []).map((mention) => ({
+                username: mention
+            }));

-            hashtags: tweetEntity.entities.hashtags.map((hashtag) => {
+            hashtags: (tweetEntity.entities?.hashtags || []).map((hashtag) => {
                 return {
                     tag: hashtag
                 };
             }),

Committable suggestion skipped: line range outside the PR's diff.

attachments: {
...(tweetEntity.media?.length > 0
? {media_keys: tweetEntity.media.map(media => media.url)}
: {})
},
includes: {
media: tweetEntity.media
},
retweetedTweet: tweetEntity.retweetedTweet,
source: 'rettiwt'
};

return tweetData;
}
/**
* @param {URL} url
*/

/**
* Fetches the oEmbed data for the given Twitter URL.
* @param {URL} url
* @returns {Promise<object>}
*/

async getOEmbedData(url) {
if (url.host === 'x.com') { // api is still at twitter.com... also not certain how people are getting x urls because twitter currently redirects every x host to twitter
url = new URL('https://twitter.com' + url.pathname);
}

const [match, tweetId] = url.pathname.match(TWITTER_PATH_REGEX);
if (!match) {
return null;
}
Comment on lines +115 to +118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect destructuring of regex match result

The destructuring assignment in getOEmbedData is incorrect. The match result should be obtained first, checked for validity, and then extract the tweetId. The current code may cause errors if the URL does not match the expected pattern.

Apply this diff to correct the destructuring:

-            const [match, tweetId] = url.pathname.match(TWITTER_PATH_REGEX);
-            if (!match) {
+            const match = url.pathname.match(TWITTER_PATH_REGEX);
+            if (!match) {
                 return null;
             }
+            const tweetId = match[1];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [match, tweetId] = url.pathname.match(TWITTER_PATH_REGEX);
if (!match) {
return null;
}
const match = url.pathname.match(TWITTER_PATH_REGEX);
if (!match) {
return null;
}
const tweetId = match[1];

const {extract} = require('@extractus/oembed-extractor');
/** @type {object} */
const oembedData = await extract(url.href);

const tweetData = await this._dependencies._fetchTweetEntity(tweetId);
oembedData.tweet_data = await this.mapTweetToTweetData(tweetData);
oembedData.type = 'twitter';

return oembedData;
}
}

module.exports = XEmbedProvider;
20 changes: 16 additions & 4 deletions ghost/core/core/server/services/oembed/service.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const config = require('../../../shared/config');
const storage = require('../../adapters/storage');
const externalRequest = require('../../lib/request-external');
const {Rettiwt} = require('rettiwt-api');

const OEmbed = require('@tryghost/oembed-service');
const oembed = new OEmbed({config, externalRequest, storage});
Expand All @@ -12,10 +13,21 @@ const nft = new NFT({
}
});

const Twitter = require('./TwitterOEmbedProvider');
const twitter = new Twitter({
config: {
bearerToken: config.get('twitter').privateReadOnlyToken
//#TODO Clean up

// const Twitter = require('./TwitterOEmbedProvider');
// const twitter = new Twitter({
// config: {
// bearerToken: config.get('twitter').privateReadOnlyToken
// }
// });

const X = require('./XEmbedProvider');
const rettiwt = new Rettiwt();
const twitter = new X({
_fetchTweetEntity: async (tweetId) => {
const response = await rettiwt.tweet.details(tweetId);
return response;
}
});

Expand Down
1 change: 1 addition & 0 deletions ghost/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@
"node-jose": "2.2.0",
"path-match": "1.2.4",
"probe-image-size": "7.2.3",
"rettiwt-api": "4.1.4",
"rss": "1.2.2",
"sanitize-html": "2.14.0",
"semver": "7.6.3",
Expand Down
206 changes: 206 additions & 0 deletions ghost/core/test/unit/server/services/oembed/rettiwtEmbed.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
const assert = require('assert/strict');
const sinon = require('sinon');
const XEmbedProvider = require('../../../../../core/server/services/oembed/XEmbedProvider');
const nock = require('nock');
const {ValidationError} = require('@tryghost/errors');

describe('XEmbedProvider', function () {
let provider;
let mockDependencies;

before(function () {
nock.disableNetConnect();
});

beforeEach(function () {
mockDependencies = {
_fetchTweetEntity: sinon.stub()
};
provider = new XEmbedProvider(mockDependencies);
});

afterEach(function () {
sinon.restore();
nock.cleanAll();
});

describe('canSupportRequest', function () {
it('should support valid Twitter URLs', async function () {
const url = new URL('https://twitter.com/Ghost/status/1630581157568839683');
const result = await provider.canSupportRequest(url);
assert.equal(result, true);
});

it('should support valid X.com URLs', async function () {
const url = new URL('https://x.com/Ghost/status/1630581157568839683');
const result = await provider.canSupportRequest(url);
assert.equal(result, true);
});

it('should reject unsupported URLs', async function () {
const url = new URL('https://example.com/some/path');
const result = await provider.canSupportRequest(url);
assert.equal(result, false);
});
});

describe('getTweetId', function () {
it('should extract Tweet ID from valid URL', async function () {
const url = new URL('https://twitter.com/Ghost/status/1630581157568839683');
const tweetId = await provider.getTweetId(url);
assert.equal(tweetId, '1630581157568839683');
});

it('should throw ValidationError for invalid URL', async function () {
const url = new URL('https://twitter.com/Ghost/some/invalid/path');
await assert.rejects(() => provider.getTweetId(url), ValidationError, 'Invalid URL');
});
});

describe('mapTweetToTweetData', function () {
it('should map tweet entity correctly', async function () {
const tweetEntity = {
id: '12345',
tweetBy: {
id: '67890',
fullName: 'Ghost',
userName: 'GhostOrg',
profileImage: 'https://example.com/profile.jpg'
},
retweetCount: 5,
likeCount: 20,
fullText: 'This is a test tweet.',
createdAt: '2024-01-01T12:00:00Z',
entities: {
urls: ['https://example.com'],
hashtags: ['example'],
mentionedUsers: ['anotherUser']
},
media: [{url: 'https://example.com/media.jpg'}],
retweetedTweet: null
};

const tweetData = await provider.mapTweetToTweetData(tweetEntity);

assert.deepEqual(tweetData, {
id: '12345',
author_id: '67890',
public_metrics: {
retweet_count: 5,
like_count: 20
},
users: [{
id: '67890',
name: 'Ghost',
username: 'GhostOrg',
profile_image_url: 'https://example.com/profile.jpg'
}],
source: 'rettiwt',
text: 'This is a test tweet.',
created_at: '2024-01-01T12:00:00.000Z',
entities: {
urls: [{url: 'https://example.com', display_url: 'example.com'}],
hashtags: [{
tag: 'example'
}],
mentions: [{
username: 'anotherUser'
}]
},
attachments: {
media_keys: ['https://example.com/media.jpg']
},
includes: {
media: [{url: 'https://example.com/media.jpg'}]
},
retweetedTweet: null
});
});
});

describe('getOEmbedData', function () {
function nockOembedRequest() {
nock('https://publish.twitter.com')
.get('/oembed')
.query(true)
.reply(200, {
html: '<blockquote>Test embed</blockquote>'
});
}

it('should fetch and map oEmbed data correctly', async function () {
const tweetURL = new URL('https://twitter.com/Ghost/status/1630581157568839683');

mockDependencies._fetchTweetEntity.resolves({
id: '12345',
tweetBy: {id: '67890', fullName: 'Ghost', userName: 'GhostOrg', profileImage: 'https://example.com/profile.jpg'},
retweetCount: 5,
likeCount: 20,
fullText: 'This is a test tweet.',
createdAt: '2024-01-01T12:00:00Z',
entities: {
urls: ['https://example.com', 'https://example2.com'],
hashtags: ['example'],
mentions: ['anotherUser']
},
media: [{url: 'https://example.com/media.jpg'}],
retweetedTweet: null
});

nockOembedRequest();

nock('https://api.twitter.com')
.get('/2/tweets/1630581157568839683')
.query(true)
.reply(200, {
data: {id: '1630581157568839683'}
});

const oembedData = await provider.getOEmbedData(tweetURL);

assert.equal(oembedData.type, 'twitter');
assert.ok(oembedData.html);
assert.ok(oembedData.tweet_data);
});
Comment on lines +131 to +164
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add test for error handling in getOEmbedData

Currently, there is no test case verifying how getOEmbedData handles errors when fetching oEmbed data or tweet entities fails. Consider adding a test to ensure the method handles such errors gracefully.

Do you want me to draft an additional test case to cover error handling in getOEmbedData?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ronaldlangeveld Here's a test case that covers error handling scenarios for the getOEmbedData method:

it('should handle errors when fetching oEmbed data', async function () {
    const tweetURL = new URL('https://twitter.com/Ghost/status/1630581157568839683');

    // Mock a failed oEmbed request
    nock('https://publish.twitter.com')
        .get('/oembed')
        .query(true)
        .reply(500, 'Internal Server Error');

    await assert.rejects(
        () => provider.getOEmbedData(tweetURL),
        {
            name: 'InternalServerError',
            message: /Failed to fetch oEmbed data/
        }
    );
});

it('should handle errors when fetching tweet entity', async function () {
    const tweetURL = new URL('https://twitter.com/Ghost/status/1630581157568839683');

    // Mock successful oEmbed request
    nock('https://publish.twitter.com')
        .get('/oembed')
        .query(true)
        .reply(200, {
            html: '<blockquote>Test embed</blockquote>'
        });

    // Mock failed tweet entity fetch
    mockDependencies._fetchTweetEntity.rejects(new Error('Failed to fetch tweet'));

    await assert.rejects(
        () => provider.getOEmbedData(tweetURL),
        {
            message: /Failed to fetch tweet/
        }
    );
});

These tests verify that:

  1. The method handles failures in the oEmbed request
  2. The method handles failures in tweet entity fetching
  3. Appropriate errors are thrown with meaningful messages

Would you like me to add any additional error scenarios?


it('should handle errors when fetching oEmbed data', async function () {
const tweetURL = new URL('https://twitter.com/Ghost/status/1630581157568839683');

// Mock a failed oEmbed request
nock('https://publish.twitter.com')
.get('/oembed')
.query(true)
.reply(500, 'Internal Server Error');

await assert.rejects(
() => provider.getOEmbedData(tweetURL),
{
name: 'Error',
message: 'Request failed with error code 500'
}
);
});

it('should handle errors when fetching tweet entity', async function () {
const tweetURL = new URL('https://twitter.com/Ghost/status/1630581157568839683');

// Mock successful oEmbed request
nock('https://publish.twitter.com')
.get('/oembed')
.query(true)
.reply(200, {
html: '<blockquote>Test embed</blockquote>'
});

// Mock failed tweet entity fetch
mockDependencies._fetchTweetEntity.rejects(new Error('Failed to fetch tweet'));

await assert.rejects(
() => provider.getOEmbedData(tweetURL),
{
message: 'Failed to fetch tweet'
}
);
});
});
});
Loading
Loading