ACC-622 Enhance response to include info about Graphic syndication #235
Conversation
server/lib/enrich/article.js
Outdated
| const countOfGraphics = content.contentStats && content.contentStats.graphics && content.contentStats.graphics; | ||
| const countOfSharableGraphics = content.embeds ? content.embeds.filter(embed => embed && embed.type.endsWith('Graphic')).filter(embed => embed.canBeSyndicated === 'yes').length : 0; | ||
|
|
||
| content.canAllGraphicsBeSyndicated = countOfGraphics > 0 ? countOfSharableGraphics < countOfGraphics : false; |
There was a problem hiding this comment.
Decided to go with false rather than null because that way I can have expect a boolean in tests.
There was a problem hiding this comment.
Wouldn't the following achieve the same purpose in a simpler way 🤔 ?
| content.canAllGraphicsBeSyndicated = countOfGraphics > 0 ? countOfSharableGraphics < countOfGraphics : false; | |
| const canAllGraphicsBeSyndicated = countOfGraphics === countOfSharableGraphics; |
server/lib/syndicate-content.js
Outdated
| @@ -70,5 +70,10 @@ function tidy(item, includeBody) { | |||
| delete item.fileName; | |||
| } | |||
|
|
|||
| if (item.hasGraphics === undefined){ | |||
There was a problem hiding this comment.
This bit removes the keys and their undefined values from non-article types.
server/lib/enrich/article.js
Outdated
| const countOfGraphics = content.contentStats && content.contentStats.graphics && content.contentStats.graphics; | ||
| const countOfSharableGraphics = content.embeds ? content.embeds.filter(embed => embed && embed.type.endsWith('Graphic')).filter(embed => embed.canBeSyndicated === 'yes').length : 0; | ||
|
|
||
| content.canAllGraphicsBeSyndicated = countOfGraphics > 0 ? countOfSharableGraphics < countOfGraphics : false; | ||
|
|
There was a problem hiding this comment.
The embeds array contains all the images and graphics in all their different sizes. However, only the original (default size) images are used for syndication.
Should the graphics included in the content document be the source of truth for graphics in the article instead of the embeds array?
the embeds array would then be used as a map to check whether the graphics can be syndicated or not.
There was a problem hiding this comment.
But that's what's there, with content.hasGraphics relying on contentStats.graphics count? (line 34)?
Then you have countOfGraphics (once again based on the contentStats). And countOfSharableGraphics which runs through the embeds to see how many can be syndicated.
It feels like you know something I don't - I'm just using an example ES article to figure this out 😅
There was a problem hiding this comment.
contentStats.graphics is gotten from counting the number of graphics in the content document. embeds contains all graphics and their different sizes.
For example, the count for contentStats.graphics can be 1 and the count of graphics object in embeds could be 2 (graphics object for normal screens, graphics object for mobile screens).
Therefore, using that to count the number of sharable graphics can be inaccurate.
As we do not have any identifier for graphics object for normal screen (which is the graphics size we are using for syndication) in the embeds array.
We can
- Add a way to identify graphics objects for normal screens in the embeds array. That would require work on next-es-interface
- Get the graphics included in the content document, and look up the graphics object in the embeds array to check the
canBeSyndicatedproperty.
server/lib/enrich/article.js
Outdated
| const countOfGraphics = content.contentStats && content.contentStats.graphics && content.contentStats.graphics; | ||
| const countOfSharableGraphics = content.embeds ? content.embeds.filter(embed => embed && embed.type.endsWith('Graphic')).filter(embed => embed.canBeSyndicated === 'yes').length : 0; | ||
|
|
||
| content.canAllGraphicsBeSyndicated = countOfGraphics > 0 ? countOfSharableGraphics < countOfGraphics : false; |
There was a problem hiding this comment.
Wouldn't the following achieve the same purpose in a simpler way 🤔 ?
| content.canAllGraphicsBeSyndicated = countOfGraphics > 0 ? countOfSharableGraphics < countOfGraphics : false; | |
| const canAllGraphicsBeSyndicated = countOfGraphics === countOfSharableGraphics; |
Ticket
https://financialtimes.atlassian.net/browse/ACC-622
Things done in this PR
hasGraphicsboolean andcanAllGraphicsBeSyndicatedboolean for article onlyShortfalls
Currently we don't have an article that has
canAllGraphicsBeSyndicatedtrue, and I'm not sure what's the practicality of having a test for something that is different in prod.