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(projects): cleaner value objects usage in addProject route (DEV-119) #1920
Conversation
webapi/src/main/scala/org/knora/webapi/routing/admin/ProjectsRouteADM.scala
Outdated
Show resolved
Hide resolved
webapi/src/main/scala/org/knora/webapi/routing/admin/ProjectsRouteADM.scala
Outdated
Show resolved
Hide resolved
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.
It's basically just one comment (just added it on several locations). Apart from that I think it looks good.
/** | ||
* Helper method converting an [[Either]] to a [[Future]]. | ||
*/ | ||
def toFuture[A](either: Either[Throwable, A]): Future[A] = either.fold(Future.failed, Future.successful) |
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 we need this (i.e. do we still want to use Either in the future and not Validation everywhere)?
@@ -145,7 +145,7 @@ class UsersRouteADM(routeData: KnoraRouteData) extends KnoraRoute(routeData) wit | |||
givenName = GivenName.create(apiRequest.givenName).fold(error => throw error, value => value), | |||
familyName = FamilyName.create(apiRequest.familyName).fold(error => throw error, value => value), | |||
password = Password.create(apiRequest.password).fold(error => throw error, value => value), | |||
status = Status.create(apiRequest.status).fold(error => throw error, value => value), | |||
status = Status.make(apiRequest.status).fold(error => throw error.head, value => value), | |||
lang = LanguageCode.create(apiRequest.lang).fold(error => throw error, value => value), |
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 fold
still needed? Can't it be done with Validation.validateWith
as in the ProjectsRouteADM.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.
Validation.validateWith
needs all inputs to be of Validation
type. The change can be done in another PR.
@@ -414,7 +414,7 @@ class UsersRouteADM(routeData: KnoraRouteData) extends KnoraRoute(routeData) wit | |||
} | |||
|
|||
val newStatus = apiRequest.status match { | |||
case Some(status) => Status.create(status).fold(error => throw error, value => value) | |||
case Some(status) => Status.make(status).fold(error => throw error.head, value => value) | |||
case None => throw BadRequestException("The status is missing.") |
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.
See above, is fold
needed?
@@ -464,7 +464,7 @@ class UsersRouteADM(routeData: KnoraRouteData) extends KnoraRoute(routeData) wit | |||
} | |||
|
|||
/* update existing user's status to false */ | |||
val status = Status.create(false).fold(error => throw error, value => value) | |||
val status = Status.make(false).fold(error => throw error.head, value => value) | |||
|
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.
See above, is fold
needed?
@@ -55,7 +55,7 @@ class ValueObjectsADMSpec extends UnitSpec(ValueObjectsADMSpec.config) { | |||
givenName = GivenName.create(createUserApiRequestADM.givenName).fold(error => throw error, value => value), | |||
familyName = FamilyName.create(createUserApiRequestADM.familyName).fold(error => throw error, value => value), | |||
password = Password.create(createUserApiRequestADM.password).fold(error => throw error, value => value), | |||
status = Status.create(createUserApiRequestADM.status).fold(error => throw error, value => value), | |||
status = Status.make(createUserApiRequestADM.status).fold(error => throw error.head, value => value), | |||
lang = LanguageCode.create(createUserApiRequestADM.lang).fold(error => throw error, value => value), |
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.
See above, is fold
needed?
@@ -469,7 +469,7 @@ class UsersResponderADMSpec extends CoreSpec(UsersResponderADMSpec.config) with | |||
"UPDATE the user's status, (deleting) making him inactive " in { | |||
responderManager ! UserChangeStatusRequestADM( | |||
userIri = SharedTestDataADM.normalUser.id, | |||
status = Status.create(false).fold(error => throw error, value => value), | |||
status = Status.make(false).fold(error => throw error.head, value => value), | |||
featureFactoryConfig = defaultFeatureFactoryConfig, |
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.
See above, can't fold
be omitted?
@@ -480,7 +480,7 @@ class UsersResponderADMSpec extends CoreSpec(UsersResponderADMSpec.config) with | |||
|
|||
responderManager ! UserChangeStatusRequestADM( | |||
userIri = SharedTestDataADM.normalUser.id, | |||
status = Status.create(true).fold(error => throw error, value => value), | |||
status = Status.make(true).fold(error => throw error.head, value => value), | |||
featureFactoryConfig = defaultFeatureFactoryConfig, |
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.
See above, can't fold
be omitted?
@@ -557,7 +557,7 @@ class UsersResponderADMSpec extends CoreSpec(UsersResponderADMSpec.config) with | |||
/* Status is updated by other normal user */ | |||
responderManager ! UserChangeStatusRequestADM( | |||
userIri = SharedTestDataADM.superUser.id, | |||
status = Status.create(false).fold(error => throw error, value => value), | |||
status = Status.make(false).fold(error => throw error.head, value => value), | |||
featureFactoryConfig = defaultFeatureFactoryConfig, |
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.
See above, can't fold
be omitted?
@@ -585,7 +585,7 @@ class UsersResponderADMSpec extends CoreSpec(UsersResponderADMSpec.config) with | |||
|
|||
responderManager ! UserChangeStatusRequestADM( | |||
userIri = KnoraSystemInstances.Users.SystemUser.id, | |||
status = Status.create(false).fold(error => throw error, value => value), | |||
status = Status.make(false).fold(error => throw error.head, value => value), | |||
featureFactoryConfig = defaultFeatureFactoryConfig, |
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.
See above, can't fold
be omitted?
@@ -598,7 +598,7 @@ class UsersResponderADMSpec extends CoreSpec(UsersResponderADMSpec.config) with | |||
|
|||
responderManager ! UserChangeStatusRequestADM( | |||
userIri = KnoraSystemInstances.Users.AnonymousUser.id, | |||
status = Status.create(false).fold(error => throw error, value => value), | |||
status = Status.make(false).fold(error => throw error.head, value => value), | |||
featureFactoryConfig = defaultFeatureFactoryConfig, |
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.
See above, can't fold
be omitted?
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.
LGTM, but please answer my comment.
/** | ||
* Project payload | ||
*/ | ||
final case class ProjectCreatePayloadADM( |
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.
You have moved updated class to the new file specific to the create method. Does it mean we should keep things separated like that - one class per file, or actually having all payloads in ROUTEPayloadsXXX
would be better idea (if there would be more)?
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 that one RoutePayloadsADM file would be enough. There are not that many per route. There could also be a RouteValueObjects file, where the route specific value objects are defined.
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.
RoutePayloadsADM => ProjectPayloadsADM.scala, GroupPayloadsADM.scala, etc.
RouteValueObjects => ProjectValueObjectsADM.scala, GroupValueObjectsADM.scala, etc.
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.
also, anything that is shared, should be inside a SharedValueObjectsADM.scala to make it obvious.
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.
That was exactly my plan, but after our latest conversation about the modularisation I'm not sure if we should have shared value objects anymore.
resolves DEV-119