add scribe #467

Reettaphant · 2017-05-31T08:11:32Z

add a scribe editor for adding links and lists to video atom description and trail text
when updating youtube description, strip links and html tags off the description. If a published video page exists, add a link to the bottom of the description pointing to it

mbarton · 2017-05-31T08:52:17Z

app/model/commands/PublishAtomCommand.scala

+      val usages: JsValue = (capi.capiQuery("/atom/media/" + atomId + "/usage", true) \ "response" \ "results").get
+      val usagesList = usages.as[List[String]]
+
+      val composerPage = usagesList.find(usage => ".*/video/.*".r.pattern.matcher(usage).matches())


Regexing on the ID feels brittle. Perhaps we could hit CAPI for each usage and find the first one where type is video?

Yes, I will do that.

mbarton · 2017-05-31T09:03:05Z

app/model/commands/PublishAtomCommand.scala

@@ -141,11 +147,40 @@ case class PublishAtomCommand(id: String, override val stores: DataStores, youTu
    }
  }

+  private def parseDescriptionForYoutube(description: String): String = {
+      val html = Jsoup.parse(description)


Just out of interest, what happens if we send HTML in a description to YouTube? I'm wondering if they clean it or if we just end up with garbage

We just end up with an error from the youtube api so we can't even update it.

YT accept all characters except < and > (https://developers.google.com/youtube/v3/docs/videos#snippet.description)

mbarton · 2017-05-31T09:07:02Z

app/model/commands/PublishAtomCommand.scala

+      }
+    }
+    catch {
+      case _ => ""


When would we hit this case?

mbarton · 2017-05-31T09:12:59Z

public/video-ui/src/services/VideosApi.js

@@ -121,12 +121,15 @@ export default {

    function updateArticleField(stage, data, composerUrl, pageId) {
      if (data.value || data.belongsTo === 'settings') {
+        const value = data.isHtml
+          ? data.value.split('"').join('\\"')


Does this work if we put quotes in the text?

Yes, although mentioning this made me realise that having double quotes in any text fields (e.g. title) would break syncing back to composer so I'm fixing this to do it for all the fields wit text input.

akash1810

A few minor comments, but this looks great! Can we add a couple of screenshots/gifs too?

akash1810 · 2017-05-31T13:43:07Z

app/model/commands/PublishAtomCommand.scala

@@ -141,11 +147,40 @@ case class PublishAtomCommand(id: String, override val stores: DataStores, youTu
    }
  }

+  private def parseDescriptionForYoutube(description: String): String = {
+      val html = Jsoup.parse(description)


YT accept all characters except < and > (https://developers.google.com/youtube/v3/docs/videos#snippet.description)

akash1810 · 2017-05-31T13:48:13Z

app/model/commands/PublishAtomCommand.scala

@@ -141,11 +147,41 @@ case class PublishAtomCommand(id: String, override val stores: DataStores, youTu
    }
  }

+  private def parseDescriptionForYoutube(description: String): String = {
+      val html = Jsoup.parse(description)
+      html.select("a").remove()


Should we also strip off <ul>s? e.g. this video https://www.theguardian.com/environment/video/2017/may/31/faceless-fish-rediscovered-in-australian-waters-video-report

All the html tags are stripped in the line below (html.text()) so all the list tags will be stripped away. This line removes all links with their content so in the example you shared, all the contents on the list would be removed. If the list contained text instead of a link, the text inside would remain but the list tags would be removed. I guess it would be simpler to always remove all lists, not sure if there is a case when lists are used without links.

akash1810 · 2017-05-31T13:52:31Z

app/model/commands/PublishAtomCommand.scala

  private def updateYoutubeMetadata(previewAtom: MediaAtom, asset: Asset) = {
+
+    val description = previewAtom.description.map(description => {


this could be moved here so we can use the builder pattern like this

As we discussed, this becomes difficult because getting the youtube description requires querying capi so I'm just going to leave this here.

akash1810 · 2017-05-31T13:54:37Z

package.json

@@ -74,6 +74,11 @@
    "redux": "^3.6.0",
    "redux-thunk": "^2.1.0",
    "reqwest": "^2.0.5",
+    "scribe": "guardian/scribe#fcf4e16",


why are we using scribe on this commit rather than a release?

Because scribe hasn't been released for a while so newer versions are not available on npm. I could look into releasing it later today.

akash1810 · 2017-05-31T13:57:00Z

public/video-ui/src/components/FormFields/ScribeEditor.js

+      .trim()
+      .replace(/<(?:.|\n)*?>/gm, '')
+      .split(/\s+/)
+      .filter(_ => _.length !== 0);


Ooh using _ in js! Controversial!

akash1810 · 2017-05-31T14:40:56Z

public/video-ui/src/components/FormFields/ScribeEditor.js

+    this.scribe.on('content-changed', this.onContentChange);
+    this.refs.editor.innerHTML = this.props.value;
+  }
+  //this.props.copiedValue


is the linter happy with this?

akash1810 · 2017-05-31T14:43:12Z

public/video-ui/src/services/VideosApi.js

@@ -121,12 +121,15 @@ export default {

    function updateArticleField(stage, data, composerUrl, pageId) {
      if (data.value || data.belongsTo === 'settings') {
+        const value = data.isFreeText
+          ? data.value.split('"').join('\\"')


this looks like a fancy .replace('"', '\"')?

Replace only replaces one character so this seemed like the simples way of doing it.

.replace(/"/g, '\\"') should work, but its a minor point, so feel free to ignore 😄

jennysivapalan · 2017-06-01T16:28:53Z

app/model/commands/PublishAtomCommand.scala

@@ -141,11 +147,41 @@ case class PublishAtomCommand(id: String, override val stores: DataStores, youTu
    }
  }

+  private def parseDescriptionForYoutube(description: String): String = {


Maybe the method could show more intent so, removeHtmlTagsForYouTubeDescription. That might help us in the future to remember why we do this?

prout-bot · 2017-06-02T13:23:34Z

Seen on PROD (merged by @Reettaphant 42 minutes and 40 seconds ago) Please check your changes!

Reettaphant force-pushed the rv-add-scribe branch from e477402 to 1119653 Compare May 31, 2017 08:40

mbarton reviewed May 31, 2017

View reviewed changes

akash1810 reviewed May 31, 2017

View reviewed changes

Reettaphant added 12 commits June 1, 2017 08:10

add scribe depencies

f8af858

rudimentary scribe editor

07af6c0

display html when not in edit mode

bafc5a4

get word count working

72d24ef

fix error display

6089e6c

correctly display placeholder

c27603c

fix trail functionality

2e5186e

remove text areas

9e83161

add sensible word limits

a1d5a05

fix styling

6fb0658

send the correct description to youtube on server

c74c838

escape double quotes in link tags

e77e053

Reettaphant force-pushed the rv-add-scribe branch from 15ab286 to 0dd8bcb Compare June 1, 2017 08:14

Reettaphant added 2 commits June 1, 2017 09:16

query capi to find video type

afbfd88

escape quotes in all text fields

c14d11b

jennysivapalan reviewed Jun 1, 2017

View reviewed changes

more descriptive name for description parsing function

6f9f81c

Reettaphant force-pushed the rv-add-scribe branch from 0dd8bcb to 6f9f81c Compare June 2, 2017 07:09

akash1810 approved these changes Jun 2, 2017

View reviewed changes

Reettaphant merged commit c3f3ba5 into master Jun 2, 2017

Reettaphant deleted the rv-add-scribe branch June 2, 2017 12:40

prout-bot added Pending-on-PROD Seen-on-PROD and removed Pending-on-PROD labels Jun 2, 2017

rhystmills mentioned this pull request Oct 25, 2023

Get minimum ProseMirror editor working guardian/prosemirror-editor#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add scribe #467

add scribe #467

Reettaphant commented May 31, 2017 •

edited

Loading

mbarton May 31, 2017

Reettaphant May 31, 2017

mbarton May 31, 2017

Reettaphant May 31, 2017

akash1810 May 31, 2017

mbarton May 31, 2017

mbarton May 31, 2017

Reettaphant May 31, 2017

akash1810 left a comment

akash1810 May 31, 2017

akash1810 May 31, 2017

Reettaphant Jun 1, 2017

akash1810 May 31, 2017

Reettaphant Jun 2, 2017

akash1810 May 31, 2017

Reettaphant Jun 2, 2017

akash1810 May 31, 2017

akash1810 May 31, 2017

akash1810 May 31, 2017

Reettaphant Jun 1, 2017

akash1810 Jun 1, 2017 •

edited

Loading

jennysivapalan Jun 1, 2017

prout-bot commented Jun 2, 2017

		private def updateYoutubeMetadata(previewAtom: MediaAtom, asset: Asset) = {

		val description = previewAtom.description.map(description => {

add scribe #467

add scribe #467

Conversation

Reettaphant commented May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash1810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash1810 Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prout-bot commented Jun 2, 2017

Reettaphant commented May 31, 2017 •

edited

Loading

akash1810 Jun 1, 2017 •

edited

Loading