New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove awaiting Future and more Improvements #501
Conversation
@Faithcaio this PR has conflicts |
I'd appreciate better commit messages since we're not squashing. I can fix the conflicts tonight if you're busy. There are definitely a lot of great improvements in here that will greatly improve the code quality and performance. 🎉 🎉 |
This includes the Play 2.6 updates. Signed-off-by: Jadon Fowler <jadonflower@gmail.com>
I fixed the conflicts. 👍 |
Maybe use |
I know. Was a bit rusty with my scala in the beginning. |
introduces HeaderData/ProjectData/VersionData case classes for use in templates and caching
app/controllers/Application.scala
Outdated
} | ||
} | ||
fullList map { list => | ||
val lists = list.partition(_._3 == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deconstruct tuple here so both lists can be named?
app/controllers/Application.scala
Outdated
case Some(id) => | ||
val reviews = this.service.access[Review](classOf[Review]) | ||
.filter(_.userId === id) | ||
.map(_.take(20).map(review => { review -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curly braces for map
val userTable = TableQuery[UserTable] | ||
|
||
// Query Orga Members | ||
val q1: Query[(Rep[Int], Rep[Option[RoleType]]), (Int, Option[RoleType]), scala.Seq] = for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for scala.Seq here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably copied the infered type from intellij or similar. I'm wondering why the explicit type at all. Did it cause problems with the union down below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes IntelliJ was confused with the types so I helped it
} | ||
|
||
// Query version author | ||
val q2: Query[(Rep[Int], Rep[Option[RoleType]]), (Int, Option[RoleType]), scala.Seq] = for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for scala.Seq here?
app/controllers/Reviews.scala
Outdated
this.service.insert(review) | ||
Redirect(routes.Reviews.showReviews(author, slug, versionString)) | ||
val closeOldReview = version.mostRecentUnfinishedReview.flatMap { | ||
case Some(oldreview) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing handling of None here.
} | ||
|
||
Try(this.forums.await(this.forums.createProjectTopic(newProject))) | ||
// Create the project and it's settings | ||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use two for comprehensions here?
app/db/impl/access/ProjectBase.scala
Outdated
} | ||
|
||
val channelCleanup = for { | ||
_ <- rcUpdate | ||
proj <- rcUpdate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use two for comprehensions here?
app/ore/rest/OreRestfulApi.scala
Outdated
"tags" -> tags.map(toJson(_)), | ||
"downloads" -> v.downloadCount | ||
) | ||
author.map(a => json.+(("author", JsString(a)))).getOrElse(json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for not using infix +
here? Also, fold is probably clearer here.
app/util/PendingAction.scala
Outdated
@@ -10,11 +10,11 @@ trait PendingAction[R] { | |||
/** | |||
* Completes the action. | |||
*/ | |||
def complete(): Try[R] | |||
def complete(implicit ec: ExecutionContext): Future[R] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a side-effecting method, so reintroducing a the standard parenthesis would be nice.
app/util/PendingAction.scala
Outdated
|
||
/** | ||
* Cancels the action. | ||
*/ | ||
def cancel() | ||
def cancel(implicit ec: ExecutionContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a side-effecting method, so reintroducing a the standard parenthesis would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also remove the execution context from trait methods, I'd rather inject it into the implementations. Also: public methods should have explicit return types.
remove database access from templates
@@ -107,7 +109,7 @@ class Versions @Inject()(stats: StatTracker, | |||
VersionEditAction(author, slug).async { implicit request => | |||
implicit val project = request.project | |||
withVersion(versionString) { version => | |||
project.recommendedVersion = version | |||
project.setRecommendedVersion( version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove space after parenthesis here
app/ore/rest/OreRestfulApi.scala
Outdated
} sortBy { case (p, v, c) => | ||
ordering | ||
//categories.map(_.toSeq).map { cats => | ||
val filtered = cats.map { ca => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fold might be clearer here
app/ore/rest/OreRestfulApi.scala
Outdated
userList.map(ul => toJson(ul.map(writeUser))) | ||
} | ||
|
||
def writeUser(user: User): JsObject = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that signature correct, and if it is, why not use a Writes?
app/util/DataHelper.scala
Outdated
@@ -34,14 +36,17 @@ final class DataHelper @Inject()(config: OreConfig, | |||
/** | |||
* Resets the application to factory defaults. | |||
*/ | |||
def reset(): Unit = { | |||
def reset(implicit ec: ExecutionContext): Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not pure, and as such should have an extra set of parenthesis.
remove more db access from templates
and fixing more compile errors
and more fixing compile errors
it compiles btw
full slug needs parent page
app/controllers/Application.scala
Outdated
val currentUserId = request.data.currentUser.flatMap(_.id).getOrElse(-1) | ||
|
||
// TODO platform filter is not implemented | ||
val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use collectFirst here instead of find and map for single traversal
app/controllers/Application.scala
Outdated
|
||
// TODO platform filter is not implemented | ||
val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform])) | ||
val platformFilter = pform.map(actions.platformFilter).getOrElse(ModelFilter.Empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fold?
app/controllers/Application.scala
Outdated
// TODO platform filter is not implemented | ||
val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform])) | ||
val platformFilter = pform.map(actions.platformFilter).getOrElse(ModelFilter.Empty) | ||
var categoryList: Seq[Category] = categories.map(Categories.fromString).map(_.toSeq).getOrElse(Seq.empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fold?
app/controllers/Application.scala
Outdated
val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform])) | ||
val platformFilter = pform.map(actions.platformFilter).getOrElse(ModelFilter.Empty) | ||
var categoryList: Seq[Category] = categories.map(Categories.fromString).map(_.toSeq).getOrElse(Seq.empty) | ||
val q = query.map(queryString => s"%${queryString.toLowerCase}%").getOrElse("%") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fold?
app/controllers/Application.scala
Outdated
(p.description.toLowerCase like q) || | ||
(p.ownerName.toLowerCase like q) || | ||
(p.pluginId.toLowerCase like q) | ||
} drop offset take pageSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if queries have it, but slice often does the job of both drop and take in one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesnt look like it does
|
||
projects.queryProjectPages(project.p) flatMap { pages => | ||
val pageCount = pages.size + pages.values.map(_.size).sum | ||
this.stats.projectViewed(request => Ok(views.pages.view(project, pages, project.p.homePage, None, pageCount)))(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason request is no longer marked as implicit, but instead passed in explicitly here?
app/models/project/Page.scala
Outdated
@@ -135,7 +135,7 @@ case class Page(override val id: Option[Int] = None, | |||
* | |||
* @return String | |||
*/ | |||
def fullSlug: String = if (parentPage.isDefined) { s"${parentPage.get.slug}/${slug}" } else { slug } | |||
def fullSlug(parentPage: Option[Page]): String = if (parentPage.isDefined) { s"${parentPage.get.slug}/$slug" } else { slug } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fold
app/controllers/Application.scala
Outdated
} | ||
|
||
val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform])) | ||
val platformFilter = pform.map(actions.platformFilter).getOrElse(ModelFilter.Empty) | ||
if (categoryList != null && Categories.visible.toSet.equals(categoryList.toSet)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fairly certain that categoryList can't be null, only empty.
versions <- futureVersions | ||
r <- this.stats.projectViewed { request => | ||
Ok(views.list(project, allChannels, versions, visibleNames, p)) | ||
}(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason request is no longer marked as implicit, but instead passed in explicitly here?
|
||
def of[A](request: Request[A])(implicit cache: AsyncCacheApi, db: JdbcBackend#DatabaseDef, ec: ExecutionContext, | ||
service: ModelService): Future[HeaderData] = { | ||
request.cookies.get("_oretoken") match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flatMap
# Conflicts: # app/ore/permission/PermissionPredicate.scala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do a favor and run optimize imports on changed files? I counted a lot of unused imports that you've added when I went through everything.
For the stuff that can be parallelized. If you want I can add them together with EitherT and OptionT instead.
I should also say that I haven't looked at the views in any way. Someone else should probably do that.
case None => Future.successful(NotFound) | ||
case Some(channel) => | ||
for { | ||
nonEmpty <- channel.versions.nonEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These futures can be run in parallel
app/controllers/project/Pages.scala
Outdated
} else { | ||
(result, false) | ||
project.pages.find((ModelFilter[Page](_.slug === parts(0)) +&& ModelFilter[Page](_.parentId === -1)).fn).flatMap { | ||
case Some(r) => Future { (Some(r), false) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future.successfull
app/controllers/project/Pages.scala
Outdated
implicit val r = request.request | ||
|
||
withPage(data.project, page).flatMap { | ||
case (None, b) => Future.successful(notFound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the b is not used here, mark it as ignored
app/controllers/project/Pages.scala
Outdated
case (Some(p), b) => | ||
projects.queryProjectPages(data.project) flatMap { pages => | ||
val pageCount = pages.size + pages.map(_._2.size).sum | ||
val parentPage = if (pages.contains(p)) None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing different types.
app/controllers/project/Pages.scala
Outdated
val pageCount = pages.size + pages.map(_._2.size).sum | ||
val parentPage = if (pages.contains(p)) None | ||
else pages.collectFirst { case (pp, page) if page.contains(p) => pp } | ||
this.stats.projectViewed(_ => Ok(views.view(data, request.scoped, pages, p, parentPage, pageCount, b)))(cache, request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request should be marked as implicit again here
newVersion.addTag(tag) | ||
} | ||
} | ||
val newVersion = channel.flatMap { channel => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do this in the for comprehension
addTags(ForgeId, "Forge", TagColors.Forge) | ||
for { | ||
channel <- channel | ||
newVersion <- newVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collapse this for comprehension and the previous one
for { | ||
channel <- channel | ||
newVersion <- newVersion | ||
spongeTag <- addTags(newVersion, SpongeApiId, "Sponge", TagColors.Sponge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be parallelized
app/ore/rest/OreRestfulApi.scala
Outdated
} | ||
//categories.map(_.toSeq).map { cats => | ||
val filtered = cats.map { ca => | ||
query.filter { case (p, v, c) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark v and c as ignored
@@ -9,7 +9,7 @@ import play.api.libs.functional.syntax._ | |||
import play.api.libs.json.Reads._ | |||
import util.WSUtils.parseJson | |||
import play.api.libs.json._ | |||
import play.api.libs.ws.{WSClient, WSResponse} | |||
import play.api.libs.ws.{WSClient, WSRequest, WSResponse} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import. Also remove Await further down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If you change the project visibility in your branch the visiblity is changed but the view is not updated. I've to reload, I think in master the view changes automatically
- Remove filter button is always visible also if no categories are selected
- Adding members to a project is not working (always outputs user not found)
- Adding comments to a review is not working
- Flag history is broken. It displays there is one entry in the dropdown but opening the history returns a blank one.
- Child pages are wrongly displayed in the side nav
- Slugs of child pages are wrong
project avatar seems to be broken on ore live too |
file issues please for future work that needs to be done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've tested mostly everything and I couldn't find more bugs.
phew thats a big one..
This PR aims to
In more detail:
TODO: Review and Cleanup
I left out caching for a Future PR.
Also the queries can still be optimized in a lot of places. (looking at Future.sequence usages and other sequential selects)
nice to have maybe in a future PR?
...?