Fix some errors, add capability to return multiple errors in results,…

… and some more cleanup

app/controllers/project/Channels.scala

@@ -84,8 +84,8 @@ class Channels @Inject()(forms: OreForms,
     implicit val project: Project = request.data.project
 
     val res = for {
-      channelData <- bindFormEitherT[Future](this.forms.ChannelEdit)(hasErrors => Redirect(self.showList(author, slug)).withError(hasErrors.errors.head.message))
-      _ <- channelData.saveTo(channelName).toLeft(()).leftMap(error => Redirect(self.showList(author, slug)).withError(error))
+      channelData <- bindFormEitherT[Future](this.forms.ChannelEdit)(hasErrors => Redirect(self.showList(author, slug)).withErrors(hasErrors.errors.flatMap(_.messages)))
+      _ <- channelData.saveTo(channelName).leftMap(errors => Redirect(self.showList(author, slug)).withErrors(errors))
     } yield Redirect(self.showList(author, slug))
 
     res.merge
@@ -103,30 +103,26 @@ class Channels @Inject()(forms: OreForms,
   def delete(author: String, slug: String, channelName: String) = ChannelEditAction(author, slug).async { implicit request =>
     implicit val data = request.data
     EitherT.right[Status](data.project.channels.all)
-      .filterOrElse(_.size == 1, Redirect(self.showList(author, slug)).withError("error.channel.last"))
+      .filterOrElse(_.size != 1, Redirect(self.showList(author, slug)).withError("error.channel.last"))
       .flatMap { channels =>
-        EitherT.fromEither[Future](channels.find(c => c.name.equals(channelName)).toRight(NotFound))
+        EitherT.fromEither[Future](channels.find(_.name == channelName).toRight(NotFound))
           .semiFlatMap { channel =>
-            (channel.versions.nonEmpty, Future.sequence(channels.map(_.versions.nonEmpty)).map(l => l.count(_ == true)))
+            (channel.versions.isEmpty, Future.traverse(channels.toSeq)(_.versions.nonEmpty).map(_.count(identity)))
               .parTupled
               .tupleRight(channel)
           }
           .filterOrElse(
-            { case ((nonEmpty, channelCount), _) => nonEmpty && channelCount == 1},
+            { case ((emptyChannel, nonEmptyChannelCount), _) =>
+              emptyChannel || nonEmptyChannelCount > 1},
             Redirect(self.showList(author, slug)).withError("error.channel.lastNonEmpty")
           )
           .map(_._2)
           .filterOrElse(
-            channel => {
-              val reviewedChannels = channels.filter(!_.isNonReviewed)
-              !channel.isNonReviewed && reviewedChannels.size <= 1 && reviewedChannels.contains(channel)
-            },
+            channel => channel.isNonReviewed || channels.count(_.isReviewed) > 1,
             Redirect(self.showList(author, slug)).withError("error.channel.lastReviewed")
           )
-          .map { channel =>
-            this.projects.deleteChannel(channel)
-            Redirect(self.showList(author, slug))
-          }
+          .semiFlatMap(channel => this.projects.deleteChannel(channel))
+          .map(_ => Redirect(self.showList(author, slug)))
       }.merge
   }
 }

app/controllers/project/Versions.scala

@@ -420,26 +420,24 @@ class Versions @Inject()(stats: StatTracker,
                                (implicit req: ProjectRequest[_]): Future[Boolean] = {
     if (version.isReviewed)
       return Future.successful(true)
+
     // check for confirmation
-    req.cookies.get(DownloadWarning.COOKIE).map(_.value).orElse(token) match {
-      case None =>
-        // unconfirmed
-        Future.successful(false)
-      case Some(tkn) =>
+    OptionT
+      .fromOption[Future](req.cookies.get(DownloadWarning.COOKIE).map(_.value).orElse(token))
+      .flatMap { tkn =>
         this.warnings.find { warn =>
           (warn.token === tkn) &&
             (warn.versionId === version.id.get) &&
             (warn.address === InetString(StatTracker.remoteAddress)) &&
             warn.isConfirmed
-        } exists {
-          warn =>
-            if (!warn.hasExpired) true
-            else {
-              warn.remove()
-              false
-            }
         }
-    }
+      }.exists { warn =>
+        if (!warn.hasExpired) true
+        else {
+          warn.remove()
+          false
+        }
+      }
   }
 
   private def _sendVersion(project: Project, version: Version)(implicit req: ProjectRequest[_]): Future[Result] = {
@@ -472,7 +470,7 @@ class Versions @Inject()(stats: StatTracker,
       implicit val r = request.request
       val project = request.data.project
       getVersion(project, target)
-        .filterOrElse(_.isReviewed, Redirect(ShowProject(author, slug)))
+        .filterOrElse(v => !v.isReviewed, Redirect(ShowProject(author, slug)))
         .semiFlatMap { version =>
           // generate a unique "warning" object to ensure the user has landed
           // on the warning before downloading
@@ -530,7 +528,7 @@ class Versions @Inject()(stats: StatTracker,
     ProjectAction(author, slug) async { request =>
       implicit val r: OreRequest[_] = request.request
       getVersion(request.data.project, target)
-        .filterOrElse(_.isReviewed, Redirect(ShowProject(author, slug)))
+        .filterOrElse(v => !v.isReviewed, Redirect(ShowProject(author, slug)))
         .flatMap(version => confirmDownload0(version.id.get, downloadType, token).toRight(Redirect(ShowProject(author, slug))))
         .map { dl =>
           dl.downloadType match {

app/controllers/sugar/ActionHelpers.scala

@@ -1,7 +1,9 @@
 package controllers.sugar
 
 import com.google.common.base.Preconditions.{checkArgument, checkNotNull}
+
 import play.api.data.Form
+import play.api.i18n.Messages
 import play.api.mvc.Results.Redirect
 import play.api.mvc.{Call, Result}
 
@@ -36,6 +38,20 @@ trait ActionHelpers {
       */
     def withError(error: String) = result.flashing("error" -> error)
 
+    /**
+      * Adds one or more errors messages to the result.
+      *
+      * @param errors  Error messages
+      * @return        Result with errors
+      */
+    //TODO: Use NEL[String] if we get the type
+    def withErrors(errors: Seq[String])(implicit messages: Messages): Result = errors match {
+      case Seq()       => result
+      case Seq(single) => withError(messages(single))
+      case multiple    =>
+        result.flashing("error" -> multiple.map(s => messages(s)).mkString("• ", "<br>• ", ""), "error-israw" -> "true")
+    }
+
     /**
       * Adds a success message to the result.
       *

app/db/impl/access/ProjectBase.scala

@@ -158,7 +158,7 @@ class ProjectBase(override val service: ModelService,
       _ = checkArgument(project.id.get == channel.projectId, "invalid project id", "")
       channels <- project.channels.all
       noVersion <- channel.versions.isEmpty
-      nonEmptyChannels <- Future.sequence(channels.map(_.versions.nonEmpty)).map(_.count(_ == true))
+      nonEmptyChannels <- Future.traverse(channels.toSeq)(_.versions.nonEmpty).map(_.count(identity))
       _ = checkArgument(channels.size > 1, "only one channel", "")
       _ = checkArgument(noVersion || nonEmptyChannels > 1, "last non-empty channel", "")
       reviewedChannels = channels.filter(!_.isNonReviewed)

app/form/project/TChannelData.scala

@@ -6,6 +6,8 @@ import ore.OreConfig
 import ore.project.factory.ProjectFactory
 import util.functional.{EitherT, OptionT}
 import util.instances.future._
+import util.syntax._
+import util.StringUtils._
 
 import scala.concurrent.{ExecutionContext, Future}
 
@@ -37,9 +39,9 @@ trait TChannelData {
     */
   def addTo(project: Project)(implicit ec: ExecutionContext): EitherT[Future, String, Channel] = {
     EitherT.liftF(project.channels.all)
-      .filterOrElse(_.size >= config.projects.get[Int]("max-channels"), "A project may only have up to five channels.")
-      .filterOrElse(_.exists(_.name.equalsIgnoreCase(this.channelName)), "A channel with that name already exists.")
-      .filterOrElse(_.exists(_.color == this.color), "A channel with that color already exists.")
+      .filterOrElse(_.size <= config.projects.get[Int]("max-channels"), "A project may only have up to five channels.")
+      .filterOrElse(_.forall(ch => !ch.name.equalsIgnoreCase(this.channelName)), "A channel with that name already exists.")
+      .filterOrElse(_.forall(_.color != this.color), "A channel with that color already exists.")
       .semiFlatMap(_ => this.factory.createChannel(project, this.channelName, this.color, this.nonReviewed))
   }
 
@@ -51,33 +53,27 @@ trait TChannelData {
     * @param project  Project of channel
     * @return         Error, if any
     */
-  def saveTo(oldName: String)(implicit project: Project, ec: ExecutionContext): OptionT[Future, String] = {
-    OptionT(
-      project.channels.all.map { channels =>
-        val channel = channels.find(_.name.equalsIgnoreCase(oldName)).get
-        val colorChan = channels.find(_.color.equals(this.color))
-        val colorTaken = colorChan.exists(_ != channel)
-        if (colorTaken) {
-          Some("A channel with that color already exists.")
-        } else {
-          val nameChan = channels.find(_.name.equalsIgnoreCase(this.channelName))
-          val nameTaken = nameChan.exists(_ != channel)
-          if (nameTaken) {
-            Some("A channel with that name already exists.")
-          } else {
-            val reviewedChannels = channels.filter(!_.isNonReviewed)
-            if (this.nonReviewed && reviewedChannels.size <= 1 && reviewedChannels.contains(channel)) {
-              Some("There must be at least one reviewed channel.")
-            } else {
-              channel.setName(this.channelName)
-              channel.setColor(this.color)
-              channel.setNonReviewed(this.nonReviewed)
-              None
-            }
-          }
-        }
+  //TODO: Return NEL[String] if we get the type
+  def saveTo(oldName: String)(implicit project: Project, ec: ExecutionContext): EitherT[Future, List[String], Unit] = {
+    EitherT.liftF(project.channels.all).flatMap { allChannels =>
+      val (channelChangeSet, channels) = allChannels.partition(_.name.equalsIgnoreCase(oldName))
+      val channel = channelChangeSet.toSeq.head
+      //TODO: Rewrite this nicer if we ever get a Validated/Validation type
+      val e1 = if(channels.exists(_.color == this.color)) List("error.channel.duplicateColor") else Nil
+      val e2 = if(channels.exists(_.name.equalsIgnoreCase(this.channelName))) List("error.channel.duplicateName") else Nil
+      val e3 = if(nonReviewed && channels.count(_.isReviewed) < 1) List("error.channel.minOneReviewed") else Nil
+      val errors = e1 ::: e2 ::: e3
+
+      if(errors.nonEmpty) {
+        EitherT.leftT[Future, Unit](errors)
+      }
+      else {
+        val effects = channel.setName(this.channelName) *>
+          channel.setColor(this.color) *>
+          channel.setNonReviewed(this.nonReviewed)
+        EitherT.right[List[String]](effects).map(_ => ())
       }
-    )
+    }
   }
 
 }

app/models/project/Channel.scala

@@ -2,6 +2,8 @@ package models.project
 
 import java.sql.Timestamp
 
+import scala.concurrent.Future
+
 import com.google.common.base.Preconditions._
 import db.Named
 import db.impl.ChannelTable
@@ -76,12 +78,15 @@ case class Channel(override val id: Option[Int] = None,
     update(ModelKeys.Color)
   }
 
+  def isReviewed: Boolean = !this._isNonReviewed
+
   def isNonReviewed: Boolean = this._isNonReviewed
 
   def setNonReviewed(isNonReviewed: Boolean) = {
     this._isNonReviewed = isNonReviewed
     if (isDefined)
       update(IsNonReviewed)
+    else Future.unit
   }
 
   /**

app/views/utils/alert.scala.html

@@ -13,6 +13,11 @@
         <button type="button" class="close" data-dismiss="alert" aria-label="@messages("general.close")">
             <span aria-hidden="true">&times;</span>
         </button>
-        @(messages(message))
+
+        @if(flash.get(s"$alertType-israw").contains("true")) {
+            @Html(message)
+        } else {
+            @(messages(message))
+        }
     </div>
 }

conf/messages

@@ -80,6 +80,9 @@ error.project.invalidPluginFile =   Invalid plugin file.
 error.channel.last              =   You cannot delete your only channel.
 error.channel.lastNonEmpty      =   You cannot delete your only non-empty channel.
 error.channel.lastReviewed      =   You cannot delete your only reviewed channel.
+error.channel.duplicateColor    =   A channel with that color already exists.
+error.channel.duplicateName     =   A channel with that name already exists.
+error.channel.minOneReviewed    =   There must be at least one reviewed channel.
 error.tagline.tooLong           =   Tagline is too long (max {0}).
 error.org.disabled              =   Apologies, creation of Organizations is temporarily disabled.
 error.org.createLimit           =   You may only create up to {0} organizations!