-
Notifications
You must be signed in to change notification settings - Fork 24
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
Refactor Backend: Merge UserSQL and User #2913
Changes from 14 commits
f48b2da
e9735e4
e7e8ad5
4afea1f
b209f17
c435b07
c5e47bc
1e889fd
4bbc29f
eee99a5
845d17a
1b7477b
ec7ed69
f120507
4083694
5060d86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,25 @@ | ||
package controllers | ||
|
||
import com.scalableminds.util.tools.Fox | ||
import javax.inject.Inject | ||
|
||
import models.binary.{DataSet, DataSetDAO, DataSetSQLDAO} | ||
import models.configuration.{DataSetConfiguration, UserConfiguration} | ||
import models.user.UserService | ||
import oxalis.security.WebknossosSilhouette.{SecuredAction, SecuredRequest, UserAwareAction, UserAwareRequest} | ||
import models.team.OrganizationSQLDAO | ||
import models.user.{UserDataSetConfigurationSQLDAO, UserService} | ||
import oxalis.security.WebknossosSilhouette.{SecuredAction, UserAwareAction} | ||
import play.api.i18n.{Messages, MessagesApi} | ||
import play.api.libs.concurrent.Execution.Implicits._ | ||
import play.api.libs.json.JsObject | ||
import play.api.libs.json.{JsObject, JsValue} | ||
import play.api.libs.json.Json._ | ||
import play.api.mvc.Action | ||
import play.libs.Json | ||
|
||
/** | ||
* Controller that handles the CRUD api for configurations (mostly settings for the tracing view) | ||
*/ | ||
|
||
class ConfigurationController @Inject()(val messagesApi: MessagesApi) extends Controller { | ||
|
||
def read = UserAwareAction.async { implicit request => | ||
request.identity.toFox.flatMap { user => | ||
UserService.findOneById(user.id, useCache = false) | ||
.map(_.userConfiguration.configurationOrDefaults) | ||
for { | ||
userConfig <- user.userConfigurationStructured | ||
} yield userConfig.configurationOrDefaults | ||
} | ||
.getOrElse(UserConfiguration.default.configuration) | ||
.map(configuration => Ok(toJson(configuration))) | ||
|
@@ -39,8 +37,9 @@ class ConfigurationController @Inject()(val messagesApi: MessagesApi) extends Co | |
|
||
def readDataSet(dataSetName: String) = UserAwareAction.async { implicit request => | ||
request.identity.toFox.flatMap { user => | ||
UserService.findOneById(user.id, useCache = false) | ||
.flatMap(_.dataSetConfigurations.get(dataSetName)) | ||
for { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be more than a refactoring. If I understand it correctly, this used to be cached in the And for my understanding: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct observation, I did not consider that this is now bypassing the Cache. However, I believe that it is not problematic. This configuration is requested via a separate route (not part of the user json) and that route is requested far less often. Also, the UserCache’s cache invalidation should provide that there is not actually a change in behavior (albeit maybe a tiny one in performance) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine for me 👍 |
||
configurationJson: JsValue <- UserDataSetConfigurationSQLDAO.findOneForUserAndDataset(user._id, dataSetName) | ||
} yield DataSetConfiguration(configurationJson.validate[Map[String, JsValue]].getOrElse(Map.empty)) | ||
} | ||
.orElse(DataSetDAO.findOneBySourceName(dataSetName).flatMap(_.defaultConfiguration)) | ||
.getOrElse(DataSetConfiguration.constructInitialDefault(List())) | ||
|
@@ -68,7 +67,8 @@ class ConfigurationController @Inject()(val messagesApi: MessagesApi) extends Co | |
def updateDataSetDefault(dataSetName: String) = SecuredAction.async(parse.json(maxLength = 20480)) { implicit request => | ||
for { | ||
dataset <- DataSetDAO.findOneBySourceName(dataSetName) ?~> Messages("dataset.notFound") | ||
_ <- (request.identity.isAdminOf(dataset.owningOrganization) || request.identity.isTeamManagerInOrg(dataset.owningOrganization)) ?~> Messages("notAllowed") | ||
organization <- OrganizationSQLDAO.findOneByName(dataset.owningOrganization) | ||
_ <- Fox.assertTrue(request.identity.isTeamManagerOrAdminOfOrg(organization._id)) ?~> Messages("notAllowed") | ||
jsConfiguration <- request.body.asOpt[JsObject] ?~> Messages("user.configuration.dataset.invalid") | ||
conf = jsConfiguration.fields.toMap | ||
_ <- DataSetSQLDAO.updateDefaultConfigurationByName(dataSetName, DataSetConfiguration(conf)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,13 @@ | ||
package controllers | ||
|
||
import javax.inject.Inject | ||
|
||
import com.scalableminds.util.geometry.Point3D | ||
import com.scalableminds.util.accesscontext.GlobalAccessContext | ||
import com.scalableminds.util.tools.DefaultConverters._ | ||
import com.scalableminds.util.tools.{Fox, JsonHelper} | ||
import models.binary._ | ||
import models.team.TeamDAO | ||
import models.user.{User, UserService} | ||
import models.user.UserService | ||
import oxalis.ndstore.{ND2WK, NDServerConnection} | ||
import oxalis.security.URLSharing | ||
import oxalis.security.WebknossosSilhouette.{SecuredAction, SecuredRequest, UserAwareAction} | ||
|
@@ -20,6 +19,7 @@ import play.api.libs.json._ | |
import reactivemongo.bson.BSONObjectID | ||
import reactivemongo.play.json.BSONFormats._ | ||
import com.scalableminds.util.tools.Math | ||
import utils.ObjectId | ||
|
||
import scala.concurrent.ExecutionContext.Implicits._ | ||
import scala.concurrent.Future | ||
|
@@ -78,14 +78,15 @@ class DataSetController @Inject()(val messagesApi: MessagesApi) extends Controll | |
def list = UserAwareAction.async { implicit request => | ||
UsingFilters( | ||
Filter("isEditable", (value: Boolean, el: DataSet) => | ||
el.isEditableBy(request.identity) && value || !el.isEditableBy(request.identity) && !value), | ||
for {isEditable <- el.isEditableBy(request.identity)} yield {isEditable && value || !isEditable && !value}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also here, why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isEditableBy is now also returning a Fox, so the alternative to the |
||
Filter("isActive", (value: Boolean, el: DataSet) => | ||
el.isActive == value) | ||
Fox.successful(el.isActive == value)) | ||
) { filter => | ||
DataSetDAO.findAll.flatMap { | ||
dataSets => | ||
for { | ||
js <- Fox.serialCombined(filter.applyOn(dataSets))(d => DataSet.dataSetPublicWrites(d, request.identity)) | ||
filtered <- filter.applyOn(dataSets) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❤️ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
js <- Fox.serialCombined(filtered)(d => DataSet.dataSetPublicWrites(d, request.identity)) | ||
} yield { | ||
Ok(Json.toJson(js)) | ||
} | ||
|
@@ -96,9 +97,10 @@ class DataSetController @Inject()(val messagesApi: MessagesApi) extends Controll | |
def accessList(dataSetName: String) = SecuredAction.async { implicit request => | ||
for { | ||
dataSet <- DataSetDAO.findOneBySourceName(dataSetName) ?~> Messages("dataSet.notFound", dataSetName) | ||
users <- UserService.findByTeams(dataSet.allowedTeams) | ||
users <- UserService.findByTeams(dataSet.allowedTeams.map(ObjectId.fromBsonId(_))) | ||
usersJs <- Fox.serialCombined(users.distinct)(_.compactWrites) | ||
} yield { | ||
Ok(Writes.list(User.userCompactWrites).writes(users.distinct)) | ||
Ok(Json.toJson(usersJs)) | ||
} | ||
} | ||
|
||
|
@@ -176,7 +178,7 @@ class DataSetController @Inject()(val messagesApi: MessagesApi) extends Controll | |
case (server, name, token, team) => | ||
for { | ||
_ <- DataSetService.checkIfNewDataSetName(name) ?~> Messages("dataSet.name.alreadyTaken") | ||
_ <- ensureTeamAdministration(request.identity, team) | ||
_ <- ensureTeamAdministration(request.identity, ObjectId.fromBsonId(team)) ?~> Messages("team.admin.notAllowed") | ||
ndProject <- NDServerConnection.requestProjectInformationFromNDStore(server, name, token) | ||
dataSet <- ND2WK.dataSetFromNDProject(ndProject, team) | ||
_ <- DataSetDAO.insert(dataSet)(GlobalAccessContext) | ||
|
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.
just out of curiosity: why did you switch here to the
for
syntax?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.
userConfigurationStructured now returns a Fox instead of the content directly (this is a common change in this PR). I tend to get confused with nested
flatMaps
, which would have now been necessary 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.
got it, thanks 👍