Merge pull request #1432 from guardian/an/fp/add-new-ui-for-video-player-format

Add new UI to select Video Player Format

Author: Fweddi

app/controllers/UploadController.scala

@@ -99,29 +99,23 @@ class UploadController(
     latestAssets.sortBy(ClientAsset.getVersion).reverse
   }
 
-  def create: Action[AnyContent] = LookupPermissions { implicit raw =>
+  def create: Action[AnyContent] = APIAuthAction { implicit raw =>
     parse(raw) { req: UploadRequest =>
-      if (req.selfHost && !raw.permissions.addSelfHostedAsset) {
-        Unauthorized(
-          s"User ${raw.user.email} is not authorised with permissions to upload self-hosted asset"
-        )
-      } else {
-        log.info(
-          s"Request for upload under atom ${req.atomId}. filename=${req.filename}. size=${req.size}, selfHosted=${req.selfHost}"
-        )
+      log.info(
+        s"Request for upload under atom ${req.atomId}. filename=${req.filename}. size=${req.size}, selfHosted=${req.selfHost}"
+      )
 
-        val thriftAtom = getPreviewAtom(req.atomId)
-        val atom = MediaAtom.fromThrift(thriftAtom)
-        val assetVersion =
-          MediaAtomHelpers.getNextAssetVersion(thriftAtom.tdata)
+      val thriftAtom = getPreviewAtom(req.atomId)
+      val atom = MediaAtom.fromThrift(thriftAtom)
+      val assetVersion =
+        MediaAtomHelpers.getNextAssetVersion(thriftAtom.tdata)
 
-        val upload = start(atom, raw.user.email, req, assetVersion)
+      val upload = start(atom, raw.user.email, req, assetVersion)
 
-        log.info(
-          s"Upload created under atom ${req.atomId}. upload=${upload.id}. parts=${upload.parts.size}, selfHosted=${upload.metadata.selfHost}"
-        )
-        Ok(Json.toJson(upload))
-      }
+      log.info(
+        s"Upload created under atom ${req.atomId}. upload=${upload.id}. parts=${upload.parts.size}, selfHosted=${upload.metadata.selfHost}"
+      )
+      Ok(Json.toJson(upload))
     }
   }
 

app/data/AtomListStore.scala

@@ -2,7 +2,15 @@ package data
 
 import com.gu.atom.data.PreviewDynamoDataStore
 import com.gu.media.CapiAccess
-import com.gu.media.model.{ContentChangeDetails, Image, MediaAtom}
+import com.gu.media.model.Platform.{Url, Youtube}
+import com.gu.media.model.VideoPlayerFormat.Loop
+import com.gu.media.model.{
+  ContentChangeDetails,
+  Image,
+  MediaAtom,
+  Platform,
+  VideoPlayerFormat
+}
 import com.gu.media.util.TestFilters
 import model.commands.CommandExceptions.AtomDataStoreError
 import model.{MediaAtomList, MediaAtomSummary}
@@ -14,7 +22,7 @@ trait AtomListStore {
       search: Option[String],
       limit: Option[Int],
       shouldUseCreatedDateForSort: Boolean,
-      mediaPlatform: Option[String],
+      platformFilter: Option[String],
       orderByOldest: Boolean
   ): MediaAtomList
 }
@@ -30,7 +38,7 @@ class CapiBackedAtomListStore(capi: CapiAccess)
       search: Option[String],
       limit: Option[Int],
       shouldUseCreatedDateForSort: Boolean,
-      mediaPlatform: Option[String],
+      platformFilter: Option[String],
       orderByOldest: Boolean
   ): MediaAtomList = {
     val pagination = Pagination.option(CapiMaxPageSize, limit)
@@ -40,7 +48,7 @@ class CapiBackedAtomListStore(capi: CapiAccess)
       case false => Map.empty
     }
 
-    val mediaPlatformFilter = mediaPlatform match {
+    val mediaPlatformFilter = platformFilter match {
       case Some(mPlatform) => Map("media-platform" -> mPlatform)
       case _               => Map.empty
     }
@@ -142,38 +150,42 @@ class CapiBackedAtomListStore(capi: CapiAccess)
         (asset \ "version").as[Long]
       }
 
-      val mediaPlatforms = (atom \ "assets")
-        .as[JsArray]
-        .value
-        .flatMap { asset =>
-          (asset \ "platform").asOpt[String].map(_.toLowerCase)
-        }
-        .toList
-        .distinct
+      val atomPlatform =
+        (atom \ "platform").asOpt[Platform]
 
-      val currentAsset = (atom \ "assets").as[JsArray].value.find { asset =>
+      val activeAsset = (atom \ "assets").as[JsArray].value.find { asset =>
         val assetVersion = (asset \ "version").as[Long]
         activeVersion.contains(assetVersion)
       }
 
-      val currentMediaPlatform = currentAsset.flatMap { asset =>
-        (asset \ "platform").asOpt[String].map(_.toLowerCase)
+      val activeAssetPlatform = activeAsset.map { asset =>
+        (asset \ "platform").as[Platform]
       }
 
-      // sort media platforms so the current one is first
-      val sortedMediaPlatforms = currentMediaPlatform match {
-        case Some(current) => current :: mediaPlatforms.filter(_ != current)
-        case None          => mediaPlatforms
-      }
+      val firstAssetPlatform =
+        (atom \ "assets").as[JsArray].value.headOption.map { asset =>
+          (asset \ "platform").as[Platform]
+        }
+
+      val platform = Platform.getPlatform(
+        atomPlatform,
+        activeAssetPlatform,
+        firstAssetPlatform
+      )
+
+      val videoPlayerFormat =
+        (atom \ "metadata" \ "selfHost" \ "videoPlayerFormat")
+          .asOpt[VideoPlayerFormat]
+          .orElse(if (platform == Url) Some(Loop) else None)
 
       Some(
         MediaAtomSummary(
           id,
           title,
           posterImage,
           contentChangeDetails,
-          sortedMediaPlatforms,
-          currentMediaPlatform
+          platform,
+          videoPlayerFormat
         )
       )
     }
@@ -242,27 +254,13 @@ class DynamoBackedAtomListStore(store: PreviewDynamoDataStore)
   }
 
   private def fromAtom(atom: MediaAtom): MediaAtomSummary = {
-    val versions = atom.assets.map(_.version).toSet
-    val currentAsset = atom.assets.find(asset =>
-      asset.version == atom.activeVersion.getOrElse(versions.max)
-    )
-    val mediaPlatforms = atom.assets.map(_.platform.name.toLowerCase).distinct
-    val currentMediaPlatform =
-      currentAsset.map(_.platform.name).map(_.toLowerCase)
-
-    // sort media platforms so the current one is first
-    val sortedMediaPlatforms = currentMediaPlatform match {
-      case Some(current) => current :: mediaPlatforms.filter(_ != current)
-      case None          => mediaPlatforms
-    }
-
     MediaAtomSummary(
       atom.id,
       atom.title,
       atom.posterImage,
       atom.contentChangeDetails,
-      sortedMediaPlatforms,
-      currentMediaPlatform
+      atom.platform.getOrElse(Youtube),
+      atom.videoPlayerFormat
     )
   }
 }

app/model/MediaAtomSummary.scala

@@ -1,6 +1,11 @@
 package model
 
-import com.gu.media.model.{Image, ContentChangeDetails}
+import com.gu.media.model.{
+  ContentChangeDetails,
+  Image,
+  Platform,
+  VideoPlayerFormat
+}
 import com.gu.ai.x.play.json.Encoders._
 import com.gu.ai.x.play.json.Jsonx
 import play.api.libs.json.Format
@@ -12,8 +17,8 @@ case class MediaAtomSummary(
     title: String,
     posterImage: Option[Image],
     contentChangeDetails: ContentChangeDetails,
-    mediaPlatforms: List[String],
-    currentMediaPlatform: Option[String]
+    platform: Platform,
+    videoPlayerFormat: Option[VideoPlayerFormat]
 )
 
 object MediaAtomList {

common/src/main/scala/com/gu/media/Permissions.scala

@@ -10,22 +10,18 @@ import com.gu.permissions.PermissionDefinition
 
 case class Permissions(
     deleteAtom: Boolean = false,
-    addSelfHostedAsset: Boolean = false,
     setVideosOnAllChannelsPublic: Boolean = false,
-    pinboard: Boolean = false,
-    videoPlayerFormat: Boolean = false
+    pinboard: Boolean = false
 )
 object Permissions {
   implicit val format: Format[Permissions] = Jsonx.formatCaseClass[Permissions]
 
   val app = "atom-maker"
   val basicAccess = PermissionDefinition("media_atom_maker_access", app)
   val deleteAtom = PermissionDefinition("delete_atom", app)
-  val addSelfHostedAsset = PermissionDefinition("add_self_hosted_asset", app)
   val setVideosOnAllChannelsPublic =
     PermissionDefinition("set_videos_on_all_channels_public", app)
   val pinboard = PermissionDefinition("pinboard", "pinboard")
-  val videoPlayerFormat = PermissionDefinition("video_player_format", app)
 }
 
 class MediaAtomMakerPermissionsProvider(
@@ -41,11 +37,9 @@ class MediaAtomMakerPermissionsProvider(
 
   def getAll(user: PandaUser): Permissions = Permissions(
     deleteAtom = hasPermission(deleteAtom, user),
-    addSelfHostedAsset = hasPermission(addSelfHostedAsset, user),
     setVideosOnAllChannelsPublic =
       hasPermission(setVideosOnAllChannelsPublic, user),
-    pinboard = hasPermission(pinboard, user),
-    videoPlayerFormat = hasPermission(videoPlayerFormat, user)
+    pinboard = hasPermission(pinboard, user)
   )
 
   def getStatusPermissions(user: PandaUser): Permissions =

common/src/main/scala/com/gu/media/model/MediaAtom.scala

@@ -15,6 +15,8 @@ import com.gu.contentatom.thrift.{
   AtomType => ThriftAtomType,
   Flags => ThriftFlags
 }
+import com.gu.media.model.Platform.Url
+import com.gu.media.model.VideoPlayerFormat.Loop
 import com.gu.media.util.MediaAtomImplicits
 import com.gu.media.youtube.{
   MediaAtomYoutubeDescriptionHandler,
@@ -278,16 +280,19 @@ object MediaAtom extends MediaAtomImplicits {
   def fromThrift(atom: ThriftAtom) = {
     val data = atom.tdata
 
+    val assets = data.assets.map(Asset.fromThrift).toList
+    val activeVersion = data.activeVersion
     val youtubeDescription: Option[String] =
       MediaAtomYoutubeDescriptionHandler.getYoutubeDescription(data)
+    val platform = getPlatform(data, activeVersion, assets)
 
     MediaAtom(
       id = atom.id,
       labels = atom.labels.toList,
       contentChangeDetails =
         ContentChangeDetails.fromThrift(atom.contentChangeDetails),
-      assets = data.assets.map(Asset.fromThrift).toList,
-      activeVersion = data.activeVersion,
+      assets = assets,
+      activeVersion = activeVersion,
       title = data.title,
       category = Category.fromThrift(data.category),
       plutoData = data.metadata.flatMap(_.pluto).map(PlutoData.fromThrift),
@@ -319,11 +324,46 @@ object MediaAtom extends MediaAtomImplicits {
       youtubeTitle =
         data.metadata.flatMap(_.youtube).map(_.title).getOrElse(data.title),
       youtubeDescription = youtubeDescription,
-      videoPlayerFormat = data.metadata
-        .flatMap(_.selfHost)
-        .flatMap(_.videoPlayerFormat)
-        .map(VideoPlayerFormat.fromThrift),
-      platform = data.platform.map(Platform.fromThrift)
+      videoPlayerFormat = getVideoPlayerFormat(data.metadata, platform),
+      platform = platform
+    )
+  }
+
+  /** will derive videoPlayerFormat if it is missing
+    * @param metadata
+    * @param platform
+    * @return
+    */
+  def getVideoPlayerFormat(
+      metadata: Option[ThriftMetadata],
+      platform: Option[Platform]
+  ): Option[VideoPlayerFormat] = {
+    metadata
+      .flatMap(_.selfHost)
+      .flatMap(_.videoPlayerFormat)
+      .map(VideoPlayerFormat.fromThrift)
+      .orElse(if (platform.contains(Url)) Some(Loop) else None)
+  }
+
+  /** will derive atom-level platform if it is missing
+    * @param data
+    * @param activeVersion
+    * @param assets
+    * @return
+    */
+  def getPlatform(
+      data: ThriftMediaAtom,
+      activeVersion: Option[Long],
+      assets: List[Asset]
+  ): Option[Platform] = {
+    Option(
+      Platform.getPlatform(
+        data.platform.map(Platform.fromThrift),
+        activeVersion
+          .flatMap(activeVersion => assets.find(_.version == activeVersion))
+          .map(_.platform),
+        assets.headOption.map(_.platform)
+      )
     )
   }
 }

common/src/main/scala/com/gu/media/model/Platform.scala

@@ -16,12 +16,12 @@ object Platform {
   case object Url extends Platform { val name = "Url" }
 
   val platformReads = Reads[Platform](json => {
-    json.as[String] match {
-      case "Youtube"     => JsSuccess(Youtube)
-      case "Facebook"    => JsSuccess(Facebook)
-      case "Dailymotion" => JsSuccess(Dailymotion)
-      case "Mainstream"  => JsSuccess(Mainstream)
-      case "Url"         => JsSuccess(Url)
+    json.as[String].toLowerCase match {
+      case "youtube"     => JsSuccess(Youtube)
+      case "facebook"    => JsSuccess(Facebook)
+      case "dailymotion" => JsSuccess(Dailymotion)
+      case "mainstream"  => JsSuccess(Mainstream)
+      case "url"         => JsSuccess(Url)
     }
   })
 
@@ -35,4 +35,30 @@ object Platform {
   private val types = List(Youtube, Facebook, Dailymotion, Mainstream, Url)
 
   def fromThrift(p: ThriftPlatform) = types.find(_.name == p.name).get
+
+  /** The atom-level platform field tells us authoritatively if atom is for
+    * Youtube or Self-hosted video.
+    *
+    * To derive an atom-level platform field from models with potentially
+    * missing data:
+    *   - use atom level platform if provided
+    *   - otherwise use the platform field of the active asset
+    *   - otherwise use the platform field of the first asset, if there are any
+    *     assets
+    *   - otherwise default to Youtube
+    * @param atomPlatform
+    * @param activeAssetPlatform
+    * @param firstAssetPlatform
+    * @return
+    */
+  def getPlatform(
+      atomPlatform: Option[Platform],
+      activeAssetPlatform: Option[Platform],
+      firstAssetPlatform: Option[Platform]
+  ): Platform =
+    atomPlatform
+      .orElse(activeAssetPlatform)
+      .orElse(firstAssetPlatform)
+      .getOrElse(Youtube)
+
 }

common/src/main/scala/com/gu/media/model/VideoPlayerFormat.scala

@@ -16,10 +16,10 @@ object VideoPlayerFormat {
   case object Cinemagraph extends VideoPlayerFormat { val name = "Cinemagraph" }
 
   val videoPlayerFormatReads = Reads[VideoPlayerFormat](json => {
-    json.as[String] match {
-      case "Default"     => JsSuccess(Default)
-      case "Loop"        => JsSuccess(Loop)
-      case "Cinemagraph" => JsSuccess(Cinemagraph)
+    json.as[String].toLowerCase match {
+      case "default"     => JsSuccess(Default)
+      case "loop"        => JsSuccess(Loop)
+      case "cinemagraph" => JsSuccess(Cinemagraph)
     }
   })