Skip to content
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

Merged
merged 7 commits into from Oct 13, 2021

Conversation

@subotic
Copy link
Collaborator

@subotic subotic commented Oct 8, 2021

resolves DEV-119

@subotic subotic self-assigned this Oct 8, 2021
@subotic subotic requested review from irinaschubert and mpro7 Oct 10, 2021
Copy link
Contributor

@irinaschubert irinaschubert left a comment

It's basically just one comment (just added it on several locations). Apart from that I think it looks good.

Loading

/**
* Helper method converting an [[Either]] to a [[Future]].
*/
def toFuture[A](either: Either[Throwable, A]): Future[A] = either.fold(Future.failed, Future.successful)
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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)?

Loading

@@ -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),
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

Copy link
Collaborator Author

@subotic subotic Oct 12, 2021

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.

Loading

@@ -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.")
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

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

Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

@@ -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),
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

@@ -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,
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

@@ -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,
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

@@ -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,
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

@@ -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,
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

@@ -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,
Copy link
Contributor

@irinaschubert irinaschubert Oct 11, 2021

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?

Loading

@subotic subotic requested a review from mpro7 Oct 13, 2021
mpro7
mpro7 approved these changes Oct 13, 2021
Copy link
Contributor

@mpro7 mpro7 left a comment

LGTM, but please answer my comment.

Loading

/**
* Project payload
*/
final case class ProjectCreatePayloadADM(
Copy link
Contributor

@mpro7 mpro7 Oct 13, 2021

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)?

Loading

Copy link
Collaborator Author

@subotic subotic Oct 13, 2021

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.

Loading

Copy link
Collaborator Author

@subotic subotic Oct 13, 2021

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.

Loading

Copy link
Collaborator Author

@subotic subotic Oct 13, 2021

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.

Loading

Copy link
Contributor

@mpro7 mpro7 Oct 13, 2021

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.

Loading

@subotic subotic merged commit 32b9e49 into main Oct 13, 2021
11 checks passed
Loading
@subotic subotic deleted the wip/DEV-119-cleaner-value-objects-usage branch Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants