Refactor Backend: Merge UserSQL and User #2913

fm3 · 2018-07-18T10:25:29Z

URL of deployed dev instance (used for testing):

https://refactorbackenduser.webknossos.xyz

Steps to test:

set up wK with multiple user accounts (try registration form also)
check that rights/team management still works
click around, general testing
user viewport/dataset settings should work (be saved, loaded again after refresh)
create some tasks, trace some, download project

Issues:

contributes to Reduce redundant layers of indirection in Backend (SQLDAO / DAO etc) #2521

~~[ ] Updated changelog~~
~~[ ] Updated migration guide if applicable~~
Ready for review

…nossos into refactor-backend-user

jstriebel · 2018-07-19T16:04:07Z

app/controllers/AnnotationIOController.scala

    for {
      projectIdValidated <- ObjectId.parse(projectId)
      project <- ProjectSQLDAO.findOne(projectIdValidated) ?~> Messages("project.notFound", projectId)
-      teamIdBson <- project._team.toBSONObjectId.toFox
-      _ <- user.assertTeamManagerOrAdminOf(teamIdBson)
+      _ <- Fox.assertBoolean(user.isTeamManagerOrAdminOf(project._team))


This purely waits for the future and checks that it is not None, right? Seems to be the correct refactoring, still wondering what this line does.

This is about functions that return Fox[Boolean]. At several locations, we want to check such booleans which we can only get packed in a Fox. One previous method to deal with that by creating helper functions for each, such as assertTeamManagerOrAdminOf, which unpacks the Fox returned by isTeamManagerOrAdminOf, and then checks the content of that boolean, by casting it to a new Fox via bool2fox. In this PR the number of these helper functions would have increased, so I refactored this into the helper function Fox.assertBoolean. It returns Fox[Unit], which is successful only when the original function returns `Fox.successful(true)´.

Ah, ok. How about renaming it to Fox.assertTrue?

jstriebel · 2018-07-19T16:09:13Z

app/controllers/ConfigurationController.scala

 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 {


just out of curiosity: why did you switch here to the for syntax?

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.

got it, thanks 👍

jstriebel

Nice 👍

I just reviewed the first files (in the github order), until DataSetController.scala. Let's clarify those things first :-)

jstriebel · 2018-07-19T16:11:43Z

app/controllers/ConfigurationController.scala

@@ -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 {


This seems to be more than a refactoring. If I understand it correctly, this used to be cached in the UserCache, not sure if it was used from there.

And for my understanding: findOneForUserAndDataset comes from slick now, right?

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)

Fine for me 👍

jstriebel · 2018-07-19T16:14:53Z

app/controllers/Controller.scala

-      _ <- ensureTeamAdministration(user, teamIdBson)
-    } yield ()
+  def ensureTeamAdministration(user: UserSQL, teamId: ObjectId): Fox[Unit] =
+    Fox.assertBoolean(user.isTeamManagerOrAdminOf(teamId)) ?~> Messages("team.admin.notAllowed")


Does assertBoolean actually assert it's true? It looks like it should, but it doesn't sound like it does.

I sure hope it does (see above)

jstriebel · 2018-07-19T16:15:21Z

app/controllers/ConfigurationController.scala

@@ -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.assertBoolean(request.identity.isTeamManagerOrAdminOfOrg(organization._id)) ?~> Messages("notAllowed")


Could we just use ensureTeamAdministration instead?

Yes, will do.

or not, since this is not team-specific (kind of a special case, all teammanagers are allowed to do this, no matter which team they are in). But I did this replacement in another instance.

jstriebel · 2018-07-19T16:20:23Z

app/controllers/DataSetController.scala

@@ -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}),


also here, why the for syntax?

isEditableBy is now also returning a Fox, so the alternative to the for-yield would have been a map of some kind, and I find the for easier to read

jstriebel · 2018-07-19T16:21:21Z

app/controllers/DataSetController.scala

    ) { filter =>
      DataSetDAO.findAll.flatMap {
        dataSets =>
          for {
-            js <- Fox.serialCombined(filter.applyOn(dataSets))(d => DataSet.dataSetPublicWrites(d, request.identity))
+            filtered <- filter.applyOn(dataSets)


filter.applyOn is now also asynchronous so the result has to be unpacked, which is what this line does :)

jstriebel · 2018-07-19T16:24:47Z

app/controllers/DataSetController.scala

@@ -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)
+          _ <- Fox.assertBoolean(request.identity.isTeamManagerOrAdminOf(ObjectId.fromBsonId(team))) ?~> Messages("team.admin.notAllowed")


Why not stay with the new ensureTeamAdministration? Seems to be equivalent.

Yes, will do.
(the explanation is that I first wanted to remove the ensureTeamAdministration entirely because I found it a little superfluos and the name a little confusing, but then I saw that it was used in so many places that I stopped removing it. Also, I guess does help reduce clutter a little bit)

jstriebel · 2018-07-20T09:57:05Z

app/controllers/UserController.scala

-  def current = SecuredAction { implicit request =>
-    Ok(Json.toJson(request.identity)(User.userPublicWrites(request.identity)))
+  def current = SecuredAction.async { implicit request =>
+    for {


I think we have this pattern quite often, should we extract this at some point? (Not part of this PR, though.)

implicit request => for { … js <- something.publicWrites(request.identity) } yield Ok(userJs)

jstriebel

Still not done with the review, but have some more questions and proposals.

jstriebel · 2018-07-20T10:04:52Z

app/controllers/UserController.scala

@@ -61,8 +63,9 @@ class UserController @Inject()(val messagesApi: MessagesApi)

  def userLoggedTime(userId: String) = SecuredAction.async { implicit request =>
    for {
-      user <- UserDAO.findOneById(userId) ?~> Messages("user.notFound")
-      _ <- user.isEditableBy(request.identity) ?~> Messages("notAllowed")


How was isEditableBy actually checked? Did this happen via the ?~>? I thought that was only checking that the option is defined, but not the packed boolean.

good catch, needs Fox.assertTrue. The ?~> just adds the failure message

This is now done via the assertBoolean, right? We had the same thing in multiple places, so probably users were able to do actually to much. So we might now forbid things that were working before (wrongly). Let's maybe discuss that later in person.

previously, isEditableBy just returned a boolean (which could be checked without the assertBoolean) – but now the signature has changed, so the way we check it needs to change too, in order to preserve the behavior

I wasn't aware of the implicit conversion, see #2941.

Let's double check all the changes where user.isEditableBy was used before, if they have assertBoolean/assertTrue.

jstriebel · 2018-07-20T10:08:49Z

app/controllers/UserController.scala

-                  )
-              }
-            )
+  def usersLoggedTime = SecuredAction.async(validateJson[TimeSpanRequest]) { implicit request =>


+1 for extracting validateJson

jstriebel · 2018-07-20T10:13:40Z

app/controllers/UserController.scala

@@ -149,15 +147,15 @@ class UserController @Inject()(val messagesApi: MessagesApi)
  // REST API
  def list = SecuredAction.async { implicit request =>
    UsingFilters(
-      Filter("includeAnonymous", (value: Boolean, el: User) => value || !el.isAnonymous, default = Some("false")),


Why is the includeAnonymous Filter removed?

the backend does not support anonymous users anymore (was for amazon mturk users).

jstriebel · 2018-07-20T10:23:29Z

app/controllers/UserController.scala

@@ -190,20 +192,23 @@ class UserController @Inject()(val messagesApi: MessagesApi)
    withJsonBodyUsing(userUpdateReader) {
      case (firstName, lastName, email, isActive, isAdmin, assignedMemberships, experiences) =>
        for {
-          user <- UserDAO.findOneById(userId) ?~> Messages("user.notFound")


Why not keep findOneById? I think we use it quite often.

Also unpacking with the Messages("user.notFound") should be possible in an abstract findOneById, maybe having a match expression switching the message-identifier.

Not sure I understand you here. UserSQLDAO replaces UserDAO here. The findOne methods everywhere expect an ObjectId, which is why there is no explicit “byId” in the name. The “user.notFound” failure message is added here since the SQLDAOs have more specific database-related failure messages

👍 , see #2940

jstriebel · 2018-07-20T12:09:45Z

app/models/annotation/AnnotationService.scala

+  private def selectSuitableTeam(user: UserSQL, dataSet: DataSet)(implicit ctx: DBAccessContext): Fox[ObjectId] = {
+    (for {
+      userTeamIds <- user.teamIds
+    } yield {


What exactly is flattened here? Could we use async to circumvent those (for {} yield { for {} yield x }).flatten patterns?

the outer for-yield returns a Fox, with the content taken from the yield block.
that content, in this case, is itself a Fox.
Hence, the Fox[Fox[ObjectId]] is flattened to Fox[ObjectId].
I haven’t yet tried scala-async, and I’m a bit sceptical of it, especially in combination with Foxes, but maybe that is something we can explore in the future

jstriebel · 2018-07-20T12:34:51Z

app/models/team/Team.scala

-    user.organization == organization && user.isAdmin
+  def assertCouldBeAdministratedBy(user: UserSQL) =
+    for {
+      asBoolean <- couldBeAdministratedBy(user)


This syntax is not very intuitive to me. Having something like the following would be awesome (in my opinion, of course):

async def assertCouldBeAdministratedBy(user: UserSQL) = if (await couldBeAdministratedBy(user)) Success() else { // not sure with the syntax here, but something like throw Messages("notAllowed") Failure() }

a) What is your opinion about this, @fm3 ?
b) Is something similar possible?

(having the bool2fox conversion explicitely would help to make it clearer, but I still find an if statement appropriate)

should be Fox.assertTrue(team.couldBeAdministratedBy(user)) ?~> Messages("notAllowed"). will change.

jstriebel · 2018-07-20T12:51:53Z

app/models/user/UserService.scala

-        result
-    }
+    for {
+      _ <- UserSQLDAO.updateValues(user._id, firstName, lastName, email, isAdmin, !activated)


Why now !activated? Probably the attribute name would help here.

jstriebel · 2018-07-20T13:29:01Z

app/oxalis/thirdparty/BrainTracing.scala

@@ -96,57 +97,59 @@ object BrainTracing extends LazyLogging with FoxImplicits {
        a.id))
  }

-  def logTime(user: User, time: Long, annotation: Option[AnnotationSQL])(implicit ctx: DBAccessContext): Future[Boolean] = {
+  def logTimeIfNeeded(user: UserSQL, time: Long, annotation: Option[AnnotationSQL])(implicit ctx: DBAccessContext): Fox[Boolean] = {


I guess you forgot to exchange all calls to logTime to logTimeIfNeeded. This would also be fine as an inner function, I guess.

actually, there are no calls to logTime or to logTimeIfNeeded. We don’t log time to BrainTracing anymore, haven’t been doing that for severall months (instead they fetch time via our API now). I’m removing that functionality in #2939

jstriebel · 2018-07-20T13:30:43Z

app/oxalis/thirdparty/BrainTracing.scala

-      brainTracingRequest.onFailure{
-        case e: Exception =>
-          logger.error(s"Failed to register user '${user.email}' in brain tracing db. Exception: ${e.getMessage}")
+  def registerIfNeeded(user: UserSQL): Fox[String] =


@fm3 Did you double check to change all register calls to registerIfNeeded? Also here, maybe just use an inner function, if the new register should actually not be called.

I made register private, forbidding external calls to it

True, didn't notice this!

jstriebel · 2018-07-20T14:03:44Z

app/models/user/User.scala

+    }
+
+  def isAdminOf(otherUser: UserSQL): Boolean =
+    this._organization == otherUser._organization && this.isAdmin


isAdminOf(otherUser._organization) would be the same and more precise

jstriebel · 2018-07-20T14:05:05Z

app/models/user/User.scala

+  def isTeamManagerOrAdminOfOrg(_organization: ObjectId): Fox[Boolean] =
+    for {
+      teamManagerTeamIds <- teamManagerTeamIds
+    } yield teamManagerTeamIds.nonEmpty || this.isAdmin && this._organization == _organization


yield teamManagerTeamIds.nonEmpty || isAdminOf(_organization)

Also, use isTeamManagerInOrg(_organization) should be used, so I guess it's just

isTeamManagerInOrg(_organization) || isAdminOf(_organization)

jstriebel · 2018-07-20T14:11:39Z

app/models/user/User.scala

-      organization <- OrganizationSQLDAO.findOneByName(user.organization)
+      team <- TeamSQLDAO.findOne(_team)(GlobalAccessContext)
+      teamManagerTeamIds <- teamManagerTeamIds
+    } yield (teamManagerTeamIds.contains(_team) || this.isAdmin && this._organization == team._organization)


… || isAdminOf(team._organization)

jstriebel

Awesome PR! I went through it now once. I list the requested changes here just to have an overview:

Fox.assertBoolean -> Fox.assertTrue
use ensureTeamAdministration when possible
double check old user.isEditableBy usages for assertBoolean/assertTrue
use Fox.assertTrue(team.couldBeAdministratedBy(user)) ?~> Messages("notAllowed") around line 152 of Team.scala
merge in removal of logTimeIfNeeded/logTime from organization-specific newUserMailingList and overTimeMailingList #2939
refactor isManager/… in User.scala

The only open question from my side is the comment

Why now !activated? Probably the attribute name would help here.

👓

fm3 · 2018-07-20T14:41:20Z

Thanks a lot! I will incorporate your feedback!
the negation in !activated was previously done within the UserDAO (which is now gone) – the field name is isDeactivated – I will explicitly add the name in the call.

jstriebel · 2018-07-20T15:29:44Z

@fm3 : I think the change in Team.scala is missing, but besides that LGTM! Let's merge #2939 (on Monday?) first and with that remove logTime/logTimeIfNeeded. Then this is ready 🚢

jstriebel

🚢

WIP: merge user and usersql

f48b2da

fm3 added backend refactoring labels Jul 18, 2018

fm3 self-assigned this Jul 18, 2018

fm3 and others added 9 commits July 18, 2018 14:16

use UserSQL in more places

e9735e4

use UserSQL in more places

e7e8ad5

use UserSQL in more places

4afea1f

WIP: reconstruct user dataset configurations

b209f17

fix UserSQL compilation errors

c435b07

code cleanup

c5e47bc

Merge branch 'master' into refactor-backend-user

1e889fd

fix permission issue, initial data

4bbc29f

Merge branch 'refactor-backend-user' of github.com:scalableminds/webk…

eee99a5

…nossos into refactor-backend-user

fm3 changed the title ~~[WIP]: Refactor Backend: Merge UserSQL and User~~ Refactor Backend: Merge UserSQL and User Jul 19, 2018

fm3 requested a review from jstriebel July 19, 2018 11:13

fm3 mentioned this pull request Jul 19, 2018

Reduce redundant layers of indirection in Backend (SQLDAO / DAO etc) #2521

Closed

13 tasks

philippotto added this to the Sprint 25a milestone Jul 19, 2018

jstriebel reviewed Jul 19, 2018

View reviewed changes

jstriebel reviewed Jul 20, 2018

View reviewed changes

jstriebel suggested changes Jul 20, 2018

View reviewed changes

fm3 and others added 2 commits July 20, 2018 17:10

implement PR feedback

845d17a

Merge branch 'master' into refactor-backend-user

1b7477b

fm3 added 2 commits July 21, 2018 12:11

remove superfluous function

ec7ed69

merge master into refactor-backend-user

f120507

jstriebel approved these changes Jul 23, 2018

View reviewed changes

fm3 added 2 commits July 24, 2018 08:43

Merge branch 'master' into refactor-backend-user

4083694

Merge branch 'master' into refactor-backend-user

5060d86

fm3 merged commit 7807252 into master Jul 24, 2018

fm3 deleted the refactor-backend-user branch July 24, 2018 08:47

jstriebel mentioned this pull request Jul 24, 2018

scala: explore async-await (instead of for-yield) #2952

Closed

Refactor Backend: Merge UserSQL and User #2913

Refactor Backend: Merge UserSQL and User #2913

Conversation

fm3 commented Jul 18, 2018 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

Choose a reason for hiding this comment

fm3 Jul 20, 2018 • 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

jstriebel 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

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

jstriebel 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

Choose a reason for hiding this comment

jstriebel Jul 20, 2018 • 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

fm3 Jul 20, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstriebel Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstriebel left a comment • edited by fm3 Loading

Choose a reason for hiding this comment

fm3 commented Jul 20, 2018 • edited Loading

jstriebel commented Jul 20, 2018

jstriebel left a comment

Choose a reason for hiding this comment

fm3 commented Jul 18, 2018 •

edited

Loading

fm3 Jul 20, 2018 •

edited

Loading

jstriebel Jul 20, 2018 •

edited

Loading

fm3 Jul 20, 2018 •

edited

Loading

jstriebel Jul 20, 2018 •

edited

Loading

jstriebel left a comment •

edited by fm3

Loading

fm3 commented Jul 20, 2018 •

edited

Loading