RSS service + controller improved for consistency (#9233)

refs #9192, refs #5091 

- Moved all url generation into generate-feed.js, so we can see as much data processing as possible in a single place.
- Refactored the way res.locals were used, to be more like how express uses them prior to rendering
- Removed a bunch of code & tests todo with context for RSS - I can't see any way that'd be used, unless we switched the rendering to use a template.
- moved the RSS rendering to be part of the service, not controller
- updated the tests significantly 

Note: RSS generate-feed has a complete duplication of the code used in the excerpt helper in order to create an item description

core/server/controllers/channel.js

@@ -9,7 +9,7 @@ var _ = require('lodash'),
     renderChannel = require('./frontend/render-channel');
 
 // This here is a controller.
-// The "route" is handled in controllers/channels/router.js
+// The "route" is handled in services/channels/router.js
 // There's both a top-level channelS router, and an individual channel one
 module.exports = function channelController(req, res, next) {
     // Parse the parameters we need from the URL

core/server/controllers/frontend/context.js

@@ -18,7 +18,6 @@ var config = require('../../config'),
     privatePattern = new RegExp('^\\/' + config.get('routeKeywords').private + '\\/'),
     subscribePattern = new RegExp('^\\/' + config.get('routeKeywords').subscribe + '\\/'),
     ampPattern = new RegExp('\\/' + config.get('routeKeywords').amp + '\\/$'),
-    rssPattern = new RegExp('^\\/rss\\/'),
     homePattern = new RegExp('^\\/$');
 
 function setResponseContext(req, res, data) {
@@ -42,11 +41,6 @@ function setResponseContext(req, res, data) {
         res.locals.context.push('home');
     }
 
-    // This is not currently used, as setRequestContext is not called for RSS feeds
-    if (rssPattern.test(res.locals.relativeUrl)) {
-        res.locals.context.push('rss');
-    }
-
     // Add context 'amp' to either post or page, if we have an `*/amp` route
     if (ampPattern.test(res.locals.relativeUrl) && data.post) {
         res.locals.context.push('amp');

core/server/controllers/rss.js

@@ -1,6 +1,5 @@
 var _ = require('lodash'),
     url = require('url'),
-    utils = require('../utils'),
     errors = require('../errors'),
     i18n = require('../i18n'),
     safeString = require('../utils/index').safeString,
@@ -10,7 +9,7 @@ var _ = require('lodash'),
     fetchData = require('./frontend/fetch-data'),
     handleError = require('./frontend/error'),
 
-    rssCache = require('../services/rss'),
+    rssService = require('../services/rss'),
     generate;
 
 // @TODO: is this the right logic? Where should this live?!
@@ -33,59 +32,39 @@ function getData(channelOpts) {
     channelOpts.data = channelOpts.data || {};
 
     return fetchData(channelOpts).then(function formatResult(result) {
-        var response = {};
+        var response = _.pick(result, ['posts', 'meta']);
 
         response.title = getTitle(result.data);
         response.description = settingsCache.get('description');
-        response.results = {
-            posts: result.posts,
-            meta: result.meta
-        };
 
         return response;
     });
 }
 
 // This here is a controller.
-// The "route" is handled in controllers/channels/router.js
+// The "route" is handled in services/channels/router.js
 // We can only generate RSS for channels, so that sorta makes sense, but the location is rubbish
 // @TODO finish refactoring this - it's now a controller
 generate = function generate(req, res, next) {
     // Parse the parameters we need from the URL
     var pageParam = req.params.page !== undefined ? req.params.page : 1,
-        slugParam = req.params.slug ? safeString(req.params.slug) : undefined;
+        slugParam = req.params.slug ? safeString(req.params.slug) : undefined,
+        // Base URL needs to be the URL for the feed without pagination:
+        baseUrl = getBaseUrlForRSSReq(req.originalUrl, pageParam);
 
     // @TODO: fix this, we shouldn't change the channel object!
     // Set page on postOptions for the query made later
     res.locals.channel.postOptions.page = pageParam;
     res.locals.channel.slugParam = slugParam;
 
     return getData(res.locals.channel).then(function handleResult(data) {
-        // Base URL needs to be the URL for the feed without pagination:
-        var baseUrl = getBaseUrlForRSSReq(req.originalUrl, pageParam),
-            maxPage = data.results.meta.pagination.pages;
-
-        // If page is greater than number of pages we have, redirect to last page
-        if (pageParam > maxPage) {
+        // If page is greater than number of pages we have, go straight to 404
+        if (pageParam > data.meta.pagination.pages) {
             return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')}));
         }
 
-        // Renderer begin
-        // Format data
-        data.version = res.locals.safeVersion;
-        data.siteUrl = utils.url.urlFor('home', {secure: req.secure}, true);
-        data.feedUrl = utils.url.urlFor({relativeUrl: baseUrl, secure: req.secure}, true);
-        data.secure = req.secure;
-
-        // No context, no template
-        // @TODO: should we have context? The context file expects it!
-
-        // Render call - to a different renderer
-        // @TODO this is effectively a renderer
-        return rssCache.getXML(baseUrl, data).then(function then(feedXml) {
-            res.set('Content-Type', 'text/xml; charset=UTF-8');
-            res.send(feedXml);
-        });
+        // Render call - to a special RSS renderer
+        return rssService.render(res, baseUrl, data);
     }).catch(handleError(next));
 };
 

core/server/services/rss/cache.js

@@ -2,15 +2,15 @@ var crypto = require('crypto'),
     generateFeed = require('./generate-feed'),
     feedCache = {};
 
-module.exports.getXML = function getFeedXml(path, data) {
+module.exports.getXML = function getFeedXml(baseUrl, data) {
     var dataHash = crypto.createHash('md5').update(JSON.stringify(data)).digest('hex');
-    if (!feedCache[path] || feedCache[path].hash !== dataHash) {
+    if (!feedCache[baseUrl] || feedCache[baseUrl].hash !== dataHash) {
         // We need to regenerate
-        feedCache[path] = {
+        feedCache[baseUrl] = {
             hash: dataHash,
-            xml: generateFeed(data)
+            xml: generateFeed(baseUrl, data)
         };
     }
 
-    return feedCache[path].xml;
+    return feedCache[baseUrl].xml;
 };

core/server/services/rss/generate-feed.js

@@ -5,6 +5,7 @@ var downsize = require('downsize'),
     processUrls = require('../../utils/make-absolute-urls'),
 
     generateFeed,
+    generateItem,
     generateTags;
 
 generateTags = function generateTags(data) {
@@ -20,60 +21,77 @@ generateTags = function generateTags(data) {
     return [];
 };
 
-generateFeed = function generateFeed(data) {
-    var feed = new RSS({
-        title: data.title,
-        description: data.description,
-        generator: 'Ghost ' + data.version,
-        feed_url: data.feedUrl,
-        site_url: data.siteUrl,
-        image_url: utils.url.urlFor({relativeUrl: 'favicon.png'}, true),
-        ttl: '60',
-        custom_namespaces: {
-            content: 'http://purl.org/rss/1.0/modules/content/',
-            media: 'http://search.yahoo.com/mrss/'
-        }
-    });
+generateItem = function generateItem(post, siteUrl, secure) {
+    var itemUrl = utils.url.urlFor('post', {post: post, secure: secure}, true),
+        htmlContent = processUrls(post.html, siteUrl, itemUrl),
+        item = {
+            title: post.title,
+            // @TODO: DRY this up with data/meta/index & other excerpt code
+            description: post.custom_excerpt || post.meta_description || downsize(htmlContent.html(), {words: 50}),
+            guid: post.id,
+            url: itemUrl,
+            date: post.published_at,
+            categories: generateTags(post),
+            author: post.author ? post.author.name : null,
+            custom_elements: []
+        },
+        imageUrl;
 
-    data.results.posts.forEach(function forEach(post) {
-        var itemUrl = utils.url.urlFor('post', {post: post, secure: data.secure}, true),
-            htmlContent = processUrls(post.html, data.siteUrl, itemUrl),
-            item = {
-                title: post.title,
-                description: post.custom_excerpt || post.meta_description || downsize(htmlContent.html(), {words: 50}),
-                guid: post.id,
-                url: itemUrl,
-                date: post.published_at,
-                categories: generateTags(post),
-                author: post.author ? post.author.name : null,
-                custom_elements: []
-            },
-            imageUrl;
+    if (post.feature_image) {
+        imageUrl = utils.url.urlFor('image', {image: post.feature_image, secure: secure}, true);
 
-        if (post.feature_image) {
-            imageUrl = utils.url.urlFor('image', {image: post.feature_image, secure: data.secure}, true);
-
-            // Add a media content tag
-            item.custom_elements.push({
-                'media:content': {
-                    _attr: {
-                        url: imageUrl,
-                        medium: 'image'
-                    }
+        // Add a media content tag
+        item.custom_elements.push({
+            'media:content': {
+                _attr: {
+                    url: imageUrl,
+                    medium: 'image'
                 }
-            });
+            }
+        });
 
-            // Also add the image to the content, because not all readers support media:content
-            htmlContent('p').first().before('<img src="' + imageUrl + '" />');
-            htmlContent('img').attr('alt', post.title);
+        // Also add the image to the content, because not all readers support media:content
+        htmlContent('p').first().before('<img src="' + imageUrl + '" />');
+        htmlContent('img').attr('alt', post.title);
+    }
+
+    item.custom_elements.push({
+        'content:encoded': {
+            _cdata: htmlContent.html()
         }
+    });
 
-        item.custom_elements.push({
-            'content:encoded': {
-                _cdata: htmlContent.html()
+    return item;
+};
+
+/**
+ * Generate Feed
+ *
+ * Data is an object which contains the res.locals + results from fetching a channel, but without related data.
+ *
+ * @param {string} baseUrl
+ * @param {{title, description, safeVersion, secure, posts}} data
+ */
+generateFeed = function generateFeed(baseUrl, data) {
+    var siteUrl = utils.url.urlFor('home', {secure: data.secure}, true),
+        feedUrl = utils.url.urlFor({relativeUrl: baseUrl, secure: data.secure}, true),
+        feed = new RSS({
+            title: data.title,
+            description: data.description,
+            generator: 'Ghost ' + data.safeVersion,
+            feed_url: feedUrl,
+            site_url: siteUrl,
+            image_url: utils.url.urlFor({relativeUrl: 'favicon.png'}, true),
+            ttl: '60',
+            custom_namespaces: {
+                content: 'http://purl.org/rss/1.0/modules/content/',
+                media: 'http://search.yahoo.com/mrss/'
             }
         });
 
+    data.posts.forEach(function forEach(post) {
+        var item = generateItem(post, siteUrl, data.secure);
+
         filters.doFilter('rss.item', item, post).then(function then(item) {
             feed.item(item);
         });

core/server/services/rss/index.js

@@ -1 +1 @@
-module.exports = require('./cache');
+module.exports = require('./renderer');

core/server/services/rss/renderer.js

@@ -0,0 +1,15 @@
+var _ = require('lodash'),
+    rssCache = require('./cache');
+
+module.exports.render = function render(res, baseUrl, data) {
+    // Format data - this is the same as what Express does
+    var rssData = _.merge({}, res.locals, data);
+
+    // Fetch RSS from the cache
+    return rssCache
+        .getXML(baseUrl, rssData)
+        .then(function then(feedXml) {
+            res.set('Content-Type', 'text/xml; charset=UTF-8');
+            res.send(feedXml);
+        });
+};

core/test/unit/controllers/frontend/context_spec.js

@@ -410,48 +410,6 @@ describe('Contexts', function () {
         });
     });
 
-    describe('RSS', function () {
-        // NOTE: this works, but is never used in reality, as setResponseContext isn't called
-        // for RSS feeds at the moment.
-        it('should correctly identify /rss/ as rss', function () {
-            // Setup test
-            setupContext('/rss/');
-
-            // Execute test
-            setResponseContext(req, res, data);
-
-            // Check context
-            should.exist(res.locals.context);
-            res.locals.context.should.be.an.Array().with.lengthOf(1);
-            res.locals.context[0].should.eql('rss');
-        });
-
-        it('will not identify /rss/2/ as rss & paged without page param', function () {
-            // Setup test by setting relativeUrl
-            setupContext('/rss/2/');
-
-            // Execute test
-            setResponseContext(req, res, data);
-
-            // Check context
-            should.exist(res.locals.context);
-            res.locals.context.should.be.an.Array().with.lengthOf(1);
-            res.locals.context[0].should.eql('rss');
-        });
-
-        it('should correctly identify /rss/2/ as rss & paged with page param', function () {
-            // Setup test by setting relativeUrl
-            setupContext('/rss/2/', 2);
-
-            // Execute test
-            setResponseContext(req, res, data);
-
-            should.exist(res.locals.context);
-            res.locals.context.should.be.an.Array().with.lengthOf(2);
-            res.locals.context[0].should.eql('paged');
-            res.locals.context[1].should.eql('rss');
-        });
-    });
     describe('AMP', function () {
         it('should correctly identify an AMP post', function () {
             // Setup test