You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/11/22 10:58:57 UTC
[GitHub] [james-project] vttranlina opened a new pull request #757: [WIP] JAMES-3534 Identity/set update should work on existing server-set Identities
vttranlina opened a new pull request #757:
URL: https://github.com/apache/james-project/pull/757
Jira https://issues.apache.org/jira/browse/JAMES-3534
resolve https://github.com/linagora/james-project/issues/4427
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756546791
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +132,55 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[IdentityOriginWrapper] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(IdentityOriginWrapper.fromCustom)
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[IdentityOriginWrapper] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(IdentityOriginWrapper.fromServerSet)
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_.identity.id)
+ .flatMap(_.reduce(IdentityOriginWrapper.merge))
+ .map(_.identity)
+ }
+
+ def update(user: Username, identityId: IdentityId, identityUpdateRequest: IdentityUpdateRequest): Publisher[Unit] =
+ SMono.fromPublisher(customIdentityDao.update(user, identityId, identityUpdateRequest))
+ .onErrorResume {
+ case error: IdentityNotFoundException =>
+ SFlux.fromIterable(identityFactory.listIdentities(user))
+ .filter(identity => identity.id.equals(identityId))
+ .next()
+ .flatMap(identity => SMono.fromPublisher(customIdentityDao.save(user, identityId, identityUpdateRequest.asCreationRequest(identity.email))))
+ .switchIfEmpty(SMono.error(error))
+ .`then`()
+ }
def delete(username: Username, ids: Seq[IdentityId]): Publisher[Unit] = customIdentityDao.delete(username, ids)
}
-case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
\ No newline at end of file
+case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
+
+case class IdentityOriginWrapper(origin: IdentityOrigin, identity: Identity)
+
+object IdentityOriginWrapper {
+ sealed trait IdentityOrigin
+
+ case class CustomIdentityOrigin() extends IdentityOrigin
+
+ case class ServerSetIdentityOrigin() extends IdentityOrigin
Review comment:
case objects
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756547969
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +132,55 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[IdentityOriginWrapper] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(IdentityOriginWrapper.fromCustom)
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[IdentityOriginWrapper] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(IdentityOriginWrapper.fromServerSet)
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_.identity.id)
+ .flatMap(_.reduce(IdentityOriginWrapper.merge))
+ .map(_.identity)
+ }
+
+ def update(user: Username, identityId: IdentityId, identityUpdateRequest: IdentityUpdateRequest): Publisher[Unit] =
+ SMono.fromPublisher(customIdentityDao.update(user, identityId, identityUpdateRequest))
+ .onErrorResume {
+ case error: IdentityNotFoundException =>
+ SFlux.fromIterable(identityFactory.listIdentities(user))
+ .filter(identity => identity.id.equals(identityId))
+ .next()
+ .flatMap(identity => SMono.fromPublisher(customIdentityDao.save(user, identityId, identityUpdateRequest.asCreationRequest(identity.email))))
+ .switchIfEmpty(SMono.error(error))
+ .`then`()
+ }
def delete(username: Username, ids: Seq[IdentityId]): Publisher[Unit] = customIdentityDao.delete(username, ids)
}
-case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
\ No newline at end of file
+case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
+
+case class IdentityOriginWrapper(origin: IdentityOrigin, identity: Identity)
+
+object IdentityOriginWrapper {
+ sealed trait IdentityOrigin
+
+ case class CustomIdentityOrigin() extends IdentityOrigin
+
+ case class ServerSetIdentityOrigin() extends IdentityOrigin
+
+ def merge(identityWrap1: IdentityOriginWrapper, identityWrap2: IdentityOriginWrapper): IdentityOriginWrapper =
+ if (identityWrap1.origin.isInstanceOf[CustomIdentityOrigin]) {
+ identityWrap1
+ } else {
+ identityWrap2
+ }
+
+ def fromCustom(identity: Identity): IdentityOriginWrapper = IdentityOriginWrapper(CustomIdentityOrigin(), identity)
+
+ def fromServerSet(identity: Identity): IdentityOriginWrapper = IdentityOriginWrapper(ServerSetIdentityOrigin(), identity)
+}
Review comment:
`isInstanceOf` is a Scala anti pattern.
I have omething like this in mind...
```
sealed trait IdentityWithOrigin {
def identity: Identity
def merge(other: IdentityWithOrigin): IdentityWithOrigin
}
case class CustomIdentity(identity: Identity) {
def merge(other: IdentityWithOrigin): IdentityWithOrigin = this
}
case class ServerSetIdentity(identity: Identity) {
def merge(other: IdentityWithOrigin): IdentityWithOrigin = other
}
```
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +132,55 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[IdentityOriginWrapper] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(IdentityOriginWrapper.fromCustom)
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[IdentityOriginWrapper] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(IdentityOriginWrapper.fromServerSet)
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_.identity.id)
+ .flatMap(_.reduce(IdentityOriginWrapper.merge))
+ .map(_.identity)
+ }
+
+ def update(user: Username, identityId: IdentityId, identityUpdateRequest: IdentityUpdateRequest): Publisher[Unit] =
+ SMono.fromPublisher(customIdentityDao.update(user, identityId, identityUpdateRequest))
+ .onErrorResume {
+ case error: IdentityNotFoundException =>
+ SFlux.fromIterable(identityFactory.listIdentities(user))
+ .filter(identity => identity.id.equals(identityId))
+ .next()
+ .flatMap(identity => SMono.fromPublisher(customIdentityDao.save(user, identityId, identityUpdateRequest.asCreationRequest(identity.email))))
+ .switchIfEmpty(SMono.error(error))
+ .`then`()
+ }
def delete(username: Username, ids: Seq[IdentityId]): Publisher[Unit] = customIdentityDao.delete(username, ids)
}
-case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
\ No newline at end of file
+case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
+
+case class IdentityOriginWrapper(origin: IdentityOrigin, identity: Identity)
+
+object IdentityOriginWrapper {
+ sealed trait IdentityOrigin
+
+ case class CustomIdentityOrigin() extends IdentityOrigin
+
+ case class ServerSetIdentityOrigin() extends IdentityOrigin
+
+ def merge(identityWrap1: IdentityOriginWrapper, identityWrap2: IdentityOriginWrapper): IdentityOriginWrapper =
+ if (identityWrap1.origin.isInstanceOf[CustomIdentityOrigin]) {
+ identityWrap1
+ } else {
+ identityWrap2
+ }
+
+ def fromCustom(identity: Identity): IdentityOriginWrapper = IdentityOriginWrapper(CustomIdentityOrigin(), identity)
+
+ def fromServerSet(identity: Identity): IdentityOriginWrapper = IdentityOriginWrapper(ServerSetIdentityOrigin(), identity)
+}
Review comment:
`isInstanceOf` is a Scala anti pattern.
I have something like this in mind...
```
sealed trait IdentityWithOrigin {
def identity: Identity
def merge(other: IdentityWithOrigin): IdentityWithOrigin
}
case class CustomIdentity(identity: Identity) {
def merge(other: IdentityWithOrigin): IdentityWithOrigin = this
}
case class ServerSetIdentity(identity: Identity) {
def merge(other: IdentityWithOrigin): IdentityWithOrigin = other
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a change in pull request #757: [WIP] JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
vttranlina commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r754773525
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +131,42 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[(String, Identity)] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(e=> (IdentityOrigin.CUSTOM, e))
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[(String, Identity)] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(e=> (IdentityOrigin.SERVER_SET, e))
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_._2.id)
+ .flatMap(_.reduce((id1, id2) => id1._1 match {
+ case IdentityOrigin.CUSTOM => id1
+ case _ => id2
+ }))
Review comment:
We have no field in Identity for priority checking. IMO, we shouldn't wrap Identity or add a new field for only this logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: [WIP] JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r754172505
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +131,42 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[(String, Identity)] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(e=> (IdentityOrigin.CUSTOM, e))
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[(String, Identity)] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(e=> (IdentityOrigin.SERVER_SET, e))
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_._2.id)
+ .flatMap(_.reduce((id1, id2) => id1._1 match {
+ case IdentityOrigin.CUSTOM => id1
+ case _ => id2
+ }))
+ .map(_._2)
+ }
+
+ def update(user: Username, identityId: IdentityId, identityUpdateRequest: IdentityUpdateRequest): Publisher[Unit] =
+ SMono.fromPublisher(customIdentityDao.update(user, identityId, identityUpdateRequest))
+ .onErrorResume {
+ case error: IdentityNotFoundException =>
+ SFlux.fromIterable(identityFactory.listIdentities(user))
+ .filter(identity => identity.id.equals(identityId))
+ .next()
+ .flatMap(identity => SMono.fromPublisher(customIdentityDao.save(user, identityId, identityUpdateRequest.asCreationRequest(identity.email))))
+ .switchIfEmpty(SMono.error(error))
+ .`then`()
+ }
def delete(username: Username, ids: Seq[IdentityId]): Publisher[Unit] = customIdentityDao.delete(username, ids)
}
-case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
\ No newline at end of file
+case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
+
+object IdentityOrigin {
+ val CUSTOM: String = "custom"
+ val SERVER_SET: String = "server_set"
+}
Review comment:
Use a case object?
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +131,42 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[(String, Identity)] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(e=> (IdentityOrigin.CUSTOM, e))
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[(String, Identity)] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(e=> (IdentityOrigin.SERVER_SET, e))
Review comment:
Missing a space between `e` and `=>`
Also we can likely find a better name than `e` and if we can't we could use `_`.
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +131,42 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[(String, Identity)] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(e=> (IdentityOrigin.CUSTOM, e))
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[(String, Identity)] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(e=> (IdentityOrigin.SERVER_SET, e))
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_._2.id)
Review comment:
groupBy is fine.
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +131,42 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[(String, Identity)] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(e=> (IdentityOrigin.CUSTOM, e))
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[(String, Identity)] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(e=> (IdentityOrigin.SERVER_SET, e))
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_._2.id)
+ .flatMap(_.reduce((id1, id2) => id1._1 match {
+ case IdentityOrigin.CUSTOM => id1
+ case _ => id2
+ }))
Review comment:
IMO we should have an `identity.merge(otherIdentity)` method.
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +131,42 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[(String, Identity)] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(e=> (IdentityOrigin.CUSTOM, e))
Review comment:
Missing a space between `e` and `=>`
Also we can likely find a better name than `e` and if we can't we could use `_`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756664233
##########
File path: server/data/data-jmap/src/test/scala/org/apache/james/jmap/api/identity/IdentityRepositoryTest.scala
##########
@@ -135,11 +134,47 @@ class IdentityRepositoryTest {
@Test
def updateShouldSuccessWhenCustomExists(): Unit = {
when(identityFactory.listIdentities(BOB)).thenReturn(List())
- SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
- val identity: Identity = SFlux(customIdentityDAO.list(BOB)).blockFirst().get
+ val customIdentity: Identity = SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
- assertThatCode(() => SMono.fromPublisher(testee.update(BOB, identity.id, UPDATE_REQUEST)).block())
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, customIdentity.id, UPDATE_REQUEST)).block())
.doesNotThrowAnyException()
}
+ @Test
+ def updateShouldModifiedEntry(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List())
+ val customIdentity: Identity = SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
+
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, customIdentity.id, UPDATE_REQUEST)).block())
+ .doesNotThrowAnyException()
+
+ assertThat(SFlux(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(Identity(id = customIdentity.id,
+ name = IdentityName("Bob (new name)"),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))),
+ textSignature = TextSignature("text 2 signature"),
+ htmlSignature = HtmlSignature("html 2 signature"),
+ mayDelete = MayDeleteIdentity(true)))
+ }
+
+ @Test
+ def updateShouldSuccessWhenMultiUpdateServerSetId(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List(IDENTITY1))
+ SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, UPDATE_REQUEST)).block()
+
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, UPDATE_REQUEST.copy(name = Some(IdentityNameUpdate(IdentityName("Bob (3)")))))).block())
+ .doesNotThrowAnyException()
+
+ assertThat(SFlux(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(Identity(id = IDENTITY1.id,
+ name = IdentityName("Bob (3)"),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))),
+ textSignature = TextSignature("text 2 signature"),
+ htmlSignature = HtmlSignature("html 2 signature"),
+ mayDelete = MayDeleteIdentity(true)))
+ }
Review comment:
Ok now one more test with the second update request updating just one field (like the identity name) and leaves all the other fields untouched.
(If you update all the fields getting the double update right is trivial but partial updates are trickier...)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756691081
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/IdentitySetContract.scala
##########
@@ -1191,4 +1191,134 @@ trait IdentitySetContract {
.extract()
.jsonPath()
.get(s"methodResponses[0][1].created.$clientId.id")
+
+ @Test
+ def updateShouldAcceptServerSetId(): Unit = {
+ val identityId: String = `given`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(
+ s"""{
+ | "using": ["urn:ietf:params:jmap:core", "urn:ietf:params:jmap:submission"],
+ | "methodCalls": [
+ | ["Identity/get",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "ids": null
+ | }, "c2"
+ | ]
+ |
+ | ]
+ |}""".stripMargin)
+ .when
+ .post
+ .`then`
+ .statusCode(SC_OK)
+ .extract()
+ .jsonPath()
+ .get("methodResponses[0][1].list[0].id")
Review comment:
I don't think you can use a back ref on an update? Not sure to get the point here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756735923
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/IdentitySetContract.scala
##########
@@ -1191,4 +1191,134 @@ trait IdentitySetContract {
.extract()
.jsonPath()
.get(s"methodResponses[0][1].created.$clientId.id")
+
+ @Test
+ def updateShouldAcceptServerSetId(): Unit = {
+ val identityId: String = `given`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(
+ s"""{
+ | "using": ["urn:ietf:params:jmap:core", "urn:ietf:params:jmap:submission"],
+ | "methodCalls": [
+ | ["Identity/get",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "ids": null
+ | }, "c2"
+ | ]
+ |
+ | ]
+ |}""".stripMargin)
+ .when
+ .post
+ .`then`
+ .statusCode(SC_OK)
+ .extract()
+ .jsonPath()
+ .get("methodResponses[0][1].list[0].id")
Review comment:
```scala
val identityId = UUID.nameUUIDFromBytes("bob@domain.tld".getBytes(StandardCharsets.UTF_8))
```
to easily get the default server set id
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a change in pull request #757: [WIP] JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
vttranlina commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r754164824
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +131,42 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[(String, Identity)] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(e=> (IdentityOrigin.CUSTOM, e))
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[(String, Identity)] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(e=> (IdentityOrigin.SERVER_SET, e))
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_._2.id)
Review comment:
The `groupBy` will block reactive. But the total number of identities for each account will not too large.
Should it ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: [WIP] JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r754775599
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +131,42 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[(String, Identity)] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(e=> (IdentityOrigin.CUSTOM, e))
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[(String, Identity)] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(e=> (IdentityOrigin.SERVER_SET, e))
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_._2.id)
+ .flatMap(_.reduce((id1, id2) => id1._1 match {
+ case IdentityOrigin.CUSTOM => id1
+ case _ => id2
+ }))
Review comment:
This code block is hard to read and would benefit from being extracted. Or simplified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa merged pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #757:
URL: https://github.com/apache/james-project/pull/757
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756731336
##########
File path: server/data/data-jmap/src/test/scala/org/apache/james/jmap/api/identity/IdentityRepositoryTest.scala
##########
@@ -135,11 +134,47 @@ class IdentityRepositoryTest {
@Test
def updateShouldSuccessWhenCustomExists(): Unit = {
when(identityFactory.listIdentities(BOB)).thenReturn(List())
- SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
- val identity: Identity = SFlux(customIdentityDAO.list(BOB)).blockFirst().get
+ val customIdentity: Identity = SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
- assertThatCode(() => SMono.fromPublisher(testee.update(BOB, identity.id, UPDATE_REQUEST)).block())
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, customIdentity.id, UPDATE_REQUEST)).block())
.doesNotThrowAnyException()
}
+ @Test
+ def updateShouldModifiedEntry(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List())
+ val customIdentity: Identity = SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
+
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, customIdentity.id, UPDATE_REQUEST)).block())
+ .doesNotThrowAnyException()
+
+ assertThat(SFlux(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(Identity(id = customIdentity.id,
+ name = IdentityName("Bob (new name)"),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))),
+ textSignature = TextSignature("text 2 signature"),
+ htmlSignature = HtmlSignature("html 2 signature"),
+ mayDelete = MayDeleteIdentity(true)))
+ }
+
+ @Test
+ def updateShouldSuccessWhenMultiUpdateServerSetId(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List(IDENTITY1))
+ SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, UPDATE_REQUEST)).block()
+
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, UPDATE_REQUEST.copy(name = Some(IdentityNameUpdate(IdentityName("Bob (3)")))))).block())
+ .doesNotThrowAnyException()
+
+ assertThat(SFlux(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(Identity(id = IDENTITY1.id,
+ name = IdentityName("Bob (3)"),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))),
+ textSignature = TextSignature("text 2 signature"),
+ htmlSignature = HtmlSignature("html 2 signature"),
+ mayDelete = MayDeleteIdentity(true)))
+ }
Review comment:
Maybe this is what Benoit suggests?
```scala
@Test
def updateShouldSuccessWhenSecondPartialUpdateServerSetId(): Unit = {
when(identityFactory.listIdentities(BOB)).thenReturn(List(IDENTITY1))
SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, UPDATE_REQUEST)).block()
val secondUpdateWithName = IdentityUpdateRequest(name = Some(IdentityNameUpdate(IdentityName("Bob (3)"))), None, None,None, None)
assertThatCode(() => SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, secondUpdateWithName)).block())
.doesNotThrowAnyException()
assertThat(SFlux(testee.list(BOB)).collectSeq().block().asJava)
.containsExactlyInAnyOrder(Identity(id = IDENTITY1.id,
name = IdentityName("Bob (3)"),
email = BOB.asMailAddress(),
replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))),
bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))),
textSignature = TextSignature("text 2 signature"),
htmlSignature = HtmlSignature("html 2 signature"),
mayDelete = MayDeleteIdentity(true)))
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756547969
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +132,55 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[IdentityOriginWrapper] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(IdentityOriginWrapper.fromCustom)
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[IdentityOriginWrapper] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(IdentityOriginWrapper.fromServerSet)
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_.identity.id)
+ .flatMap(_.reduce(IdentityOriginWrapper.merge))
+ .map(_.identity)
+ }
+
+ def update(user: Username, identityId: IdentityId, identityUpdateRequest: IdentityUpdateRequest): Publisher[Unit] =
+ SMono.fromPublisher(customIdentityDao.update(user, identityId, identityUpdateRequest))
+ .onErrorResume {
+ case error: IdentityNotFoundException =>
+ SFlux.fromIterable(identityFactory.listIdentities(user))
+ .filter(identity => identity.id.equals(identityId))
+ .next()
+ .flatMap(identity => SMono.fromPublisher(customIdentityDao.save(user, identityId, identityUpdateRequest.asCreationRequest(identity.email))))
+ .switchIfEmpty(SMono.error(error))
+ .`then`()
+ }
def delete(username: Username, ids: Seq[IdentityId]): Publisher[Unit] = customIdentityDao.delete(username, ids)
}
-case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
\ No newline at end of file
+case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
+
+case class IdentityOriginWrapper(origin: IdentityOrigin, identity: Identity)
+
+object IdentityOriginWrapper {
+ sealed trait IdentityOrigin
+
+ case class CustomIdentityOrigin() extends IdentityOrigin
+
+ case class ServerSetIdentityOrigin() extends IdentityOrigin
+
+ def merge(identityWrap1: IdentityOriginWrapper, identityWrap2: IdentityOriginWrapper): IdentityOriginWrapper =
+ if (identityWrap1.origin.isInstanceOf[CustomIdentityOrigin]) {
+ identityWrap1
+ } else {
+ identityWrap2
+ }
+
+ def fromCustom(identity: Identity): IdentityOriginWrapper = IdentityOriginWrapper(CustomIdentityOrigin(), identity)
+
+ def fromServerSet(identity: Identity): IdentityOriginWrapper = IdentityOriginWrapper(ServerSetIdentityOrigin(), identity)
+}
Review comment:
`isInstanceOf` is a Scala anti pattern.
How about pattern matching?
I have omething like this in mind...
```
sealed trait IdentityWithOrigin {
def identity: Identity
}
object IdentityWithOrigin {
def merge(identity1: IdentityWithOrigin, identity2: IdentityWithOrigin): IdentityWithOrigin =
identity1 match {
case CustomIdentity => identity1
case _ => identity2
}
}
case class CustomIdentity(identity: Identity)
case class ServerSetIdentity(identity: Identity)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756549897
##########
File path: server/data/data-jmap/src/test/scala/org/apache/james/jmap/api/identity/IdentityRepositoryTest.scala
##########
@@ -0,0 +1,145 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.jmap.api.identity
+
+import org.apache.james.core.{MailAddress, Username}
+import org.apache.james.jmap.api.identity.IdentityRepositoryTest.{BOB, CREATION_REQUEST, IDENTITY1, UPDATE_REQUEST}
+import org.apache.james.jmap.api.model.{EmailAddress, EmailerName, ForbiddenSendFromException, HtmlSignature, Identity, IdentityId, IdentityName, MayDeleteIdentity, TextSignature}
+import org.apache.james.jmap.memory.identity.MemoryCustomIdentityDAO
+import org.assertj.core.api.Assertions.{assertThat, assertThatCode, assertThatThrownBy}
+import org.junit.jupiter.api.{BeforeEach, Test}
+import org.mockito.ArgumentMatchers.any
+import org.mockito.Mockito.{mock, when}
+import reactor.core.scala.publisher.{SFlux, SMono}
+
+import scala.jdk.CollectionConverters._
+
+object IdentityRepositoryTest {
+ val BOB: Username = Username.of("bob@domain.tld")
+
+ val CREATION_REQUEST: IdentityCreationRequest = IdentityCreationRequest(name = Some(IdentityName("Bob (custom address)")),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss")), new MailAddress("boss@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2")), new MailAddress("boss2@domain.tld")))),
+ textSignature = Some(TextSignature("text signature")),
+ htmlSignature = Some(HtmlSignature("html signature")))
+
+ val UPDATE_REQUEST: IdentityUpdateRequest = IdentityUpdateRequest(
+ name = Some(IdentityNameUpdate(IdentityName("Bob (new name)"))),
+ replyTo = Some(IdentityReplyToUpdate(Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))))),
+ bcc = Some(IdentityBccUpdate(Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))))),
+ textSignature = Some(IdentityTextSignatureUpdate(TextSignature("text 2 signature"))),
+ htmlSignature = Some(IdentityHtmlSignatureUpdate(HtmlSignature("html 2 signature"))))
+
+ val IDENTITY1: Identity = Identity(id = IdentityId.generate,
+ name = IdentityName(""),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss 1")), new MailAddress("boss1@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss bcc 1")), new MailAddress("boss_bcc_1@domain.tld")))),
+ textSignature = TextSignature(""),
+ htmlSignature = HtmlSignature(""),
+ mayDelete = MayDeleteIdentity(false))
+
+ val IDENTITY2: Identity = Identity(id = IdentityId.generate,
+ name = IdentityName(""),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss")), new MailAddress("boss@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2")), new MailAddress("boss2@domain.tld")))),
+ textSignature = TextSignature("text signature"),
+ htmlSignature = HtmlSignature("html signature"),
+ mayDelete = MayDeleteIdentity(true))
+
+}
+
+class IdentityRepositoryTest {
+
+ var testee: IdentityRepository = _
+ var identityFactory: DefaultIdentitySupplier = _
+ var customIdentityDAO: CustomIdentityDAO = _
+
+
+ @BeforeEach
+ def setUp(): Unit = {
+ customIdentityDAO = new MemoryCustomIdentityDAO()
+ identityFactory = mock(classOf[DefaultIdentitySupplier])
+ testee = new IdentityRepository(customIdentityDAO, identityFactory)
+ }
+
+ @Test
+ def saveShouldSuccess(): Unit = {
+ when(identityFactory.userCanSendFrom(any(), any())).thenReturn(true)
+ assertThatCode(() => SMono.fromPublisher(testee.save(BOB, CREATION_REQUEST)).block())
+ .doesNotThrowAnyException()
+ }
+
+ @Test
+ def saveShouldFailWhenUserCanNotSendFrom(): Unit = {
+ when(identityFactory.userCanSendFrom(any(), any())).thenReturn(false)
+ assertThatThrownBy(() => SMono.fromPublisher(testee.save(BOB, CREATION_REQUEST)).block())
+ .isInstanceOf(classOf[ForbiddenSendFromException])
+ }
+
+ @Test
+ def listShouldReturnCustomAndServerSetEntries(): Unit = {
+ SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
+ when(identityFactory.listIdentities(BOB)).thenReturn(List(IDENTITY1))
+
+ assertThat(SFlux.fromPublisher(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(IDENTITY1, SFlux(customIdentityDAO.list(BOB)).blockFirst().get)
+ }
+
+ @Test
+ def listShouldReturnCustomEntryWhenIdExistsInBothCustomAndServerSet(): Unit = {
+ SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
+ val identity: Identity = SFlux(customIdentityDAO.list(BOB)).blockFirst().get
+ val differentIdentityWithSameId: Identity = identity.copy(name = IdentityName("different name"))
+ when(identityFactory.listIdentities(BOB)).thenReturn(List(differentIdentityWithSameId))
+
+ assertThat(SFlux.fromPublisher(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(identity)
+ }
+
+ @Test
+ def updateShouldFailWhenIdNotFoundInBothCustomAndServerSetDAO(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List())
+
+ assertThatThrownBy(() => SMono.fromPublisher(testee.update(BOB, IdentityId.generate, UPDATE_REQUEST)).block())
+ .isInstanceOf(classOf[IdentityNotFoundException])
+ }
+
+ @Test
+ def updateShouldSuccessWhenCustomNotFoundAndServerSetExists(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List(IDENTITY1))
+
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, UPDATE_REQUEST)).block())
+ .doesNotThrowAnyException()
+ }
+
+ @Test
+ def updateShouldSuccessWhenCustomExists(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List())
+ SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
+ val identity: Identity = SFlux(customIdentityDAO.list(BOB)).blockFirst().get
+
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, identity.id, UPDATE_REQUEST)).block())
+ .doesNotThrowAnyException()
+ }
Review comment:
What are the results of these updates?
Can I update a server set identity twice?
##########
File path: server/data/data-jmap/src/test/scala/org/apache/james/jmap/api/identity/IdentityRepositoryTest.scala
##########
@@ -0,0 +1,145 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.jmap.api.identity
+
+import org.apache.james.core.{MailAddress, Username}
+import org.apache.james.jmap.api.identity.IdentityRepositoryTest.{BOB, CREATION_REQUEST, IDENTITY1, UPDATE_REQUEST}
+import org.apache.james.jmap.api.model.{EmailAddress, EmailerName, ForbiddenSendFromException, HtmlSignature, Identity, IdentityId, IdentityName, MayDeleteIdentity, TextSignature}
+import org.apache.james.jmap.memory.identity.MemoryCustomIdentityDAO
+import org.assertj.core.api.Assertions.{assertThat, assertThatCode, assertThatThrownBy}
+import org.junit.jupiter.api.{BeforeEach, Test}
+import org.mockito.ArgumentMatchers.any
+import org.mockito.Mockito.{mock, when}
+import reactor.core.scala.publisher.{SFlux, SMono}
+
+import scala.jdk.CollectionConverters._
+
+object IdentityRepositoryTest {
+ val BOB: Username = Username.of("bob@domain.tld")
+
+ val CREATION_REQUEST: IdentityCreationRequest = IdentityCreationRequest(name = Some(IdentityName("Bob (custom address)")),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss")), new MailAddress("boss@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2")), new MailAddress("boss2@domain.tld")))),
+ textSignature = Some(TextSignature("text signature")),
+ htmlSignature = Some(HtmlSignature("html signature")))
+
+ val UPDATE_REQUEST: IdentityUpdateRequest = IdentityUpdateRequest(
+ name = Some(IdentityNameUpdate(IdentityName("Bob (new name)"))),
+ replyTo = Some(IdentityReplyToUpdate(Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))))),
+ bcc = Some(IdentityBccUpdate(Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))))),
+ textSignature = Some(IdentityTextSignatureUpdate(TextSignature("text 2 signature"))),
+ htmlSignature = Some(IdentityHtmlSignatureUpdate(HtmlSignature("html 2 signature"))))
+
+ val IDENTITY1: Identity = Identity(id = IdentityId.generate,
+ name = IdentityName(""),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss 1")), new MailAddress("boss1@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss bcc 1")), new MailAddress("boss_bcc_1@domain.tld")))),
+ textSignature = TextSignature(""),
+ htmlSignature = HtmlSignature(""),
+ mayDelete = MayDeleteIdentity(false))
+
+ val IDENTITY2: Identity = Identity(id = IdentityId.generate,
+ name = IdentityName(""),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss")), new MailAddress("boss@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2")), new MailAddress("boss2@domain.tld")))),
+ textSignature = TextSignature("text signature"),
+ htmlSignature = HtmlSignature("html signature"),
+ mayDelete = MayDeleteIdentity(true))
+
+}
+
+class IdentityRepositoryTest {
+
+ var testee: IdentityRepository = _
+ var identityFactory: DefaultIdentitySupplier = _
+ var customIdentityDAO: CustomIdentityDAO = _
+
+
+ @BeforeEach
+ def setUp(): Unit = {
+ customIdentityDAO = new MemoryCustomIdentityDAO()
+ identityFactory = mock(classOf[DefaultIdentitySupplier])
+ testee = new IdentityRepository(customIdentityDAO, identityFactory)
+ }
+
+ @Test
+ def saveShouldSuccess(): Unit = {
+ when(identityFactory.userCanSendFrom(any(), any())).thenReturn(true)
+ assertThatCode(() => SMono.fromPublisher(testee.save(BOB, CREATION_REQUEST)).block())
+ .doesNotThrowAnyException()
+ }
+
+ @Test
+ def saveShouldFailWhenUserCanNotSendFrom(): Unit = {
+ when(identityFactory.userCanSendFrom(any(), any())).thenReturn(false)
+ assertThatThrownBy(() => SMono.fromPublisher(testee.save(BOB, CREATION_REQUEST)).block())
+ .isInstanceOf(classOf[ForbiddenSendFromException])
+ }
+
+ @Test
+ def listShouldReturnCustomAndServerSetEntries(): Unit = {
+ SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
+ when(identityFactory.listIdentities(BOB)).thenReturn(List(IDENTITY1))
+
+ assertThat(SFlux.fromPublisher(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(IDENTITY1, SFlux(customIdentityDAO.list(BOB)).blockFirst().get)
Review comment:
```suggestion
val customIdentity = SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
when(identityFactory.listIdentities(BOB)).thenReturn(List(IDENTITY1))
assertThat(SFlux.fromPublisher(testee.list(BOB)).collectSeq().block().asJava)
.containsExactlyInAnyOrder(IDENTITY1, customIdentity)
```
##########
File path: server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/identity/CustomIdentityDAO.scala
##########
@@ -120,15 +132,55 @@ class IdentityRepository @Inject()(customIdentityDao: CustomIdentityDAO, identit
SMono.error(ForbiddenSendFromException(creationRequest.email))
}
- def list(user: Username): Publisher[Identity] = SFlux.merge(Seq(
- customIdentityDao.list(user),
- SMono.fromCallable(() => identityFactory.listIdentities(user))
- .subscribeOn(Schedulers.elastic())
- .flatMapMany(SFlux.fromIterable)))
+ def list(user: Username): Publisher[Identity] = {
+ val customIdentities: SFlux[IdentityOriginWrapper] = SFlux.fromPublisher(customIdentityDao.list(user))
+ .map(IdentityOriginWrapper.fromCustom)
- def update(user: Username, identityId: IdentityId, identityUpdate: IdentityUpdate): Publisher[Unit] = customIdentityDao.update(user, identityId, identityUpdate)
+ val serverSetIdentities: SFlux[IdentityOriginWrapper] = SMono.fromCallable(() => identityFactory.listIdentities(user))
+ .subscribeOn(Schedulers.elastic())
+ .flatMapMany(SFlux.fromIterable)
+ .map(IdentityOriginWrapper.fromServerSet)
+
+ SFlux.merge(Seq(customIdentities, serverSetIdentities))
+ .groupBy(_.identity.id)
+ .flatMap(_.reduce(IdentityOriginWrapper.merge))
+ .map(_.identity)
+ }
+
+ def update(user: Username, identityId: IdentityId, identityUpdateRequest: IdentityUpdateRequest): Publisher[Unit] =
+ SMono.fromPublisher(customIdentityDao.update(user, identityId, identityUpdateRequest))
+ .onErrorResume {
+ case error: IdentityNotFoundException =>
+ SFlux.fromIterable(identityFactory.listIdentities(user))
+ .filter(identity => identity.id.equals(identityId))
+ .next()
+ .flatMap(identity => SMono.fromPublisher(customIdentityDao.save(user, identityId, identityUpdateRequest.asCreationRequest(identity.email))))
+ .switchIfEmpty(SMono.error(error))
+ .`then`()
+ }
def delete(username: Username, ids: Seq[IdentityId]): Publisher[Unit] = customIdentityDao.delete(username, ids)
}
-case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
\ No newline at end of file
+case class IdentityNotFoundException(id: IdentityId) extends RuntimeException(s"$id could not be found")
+
+case class IdentityOriginWrapper(origin: IdentityOrigin, identity: Identity)
+
+object IdentityOriginWrapper {
+ sealed trait IdentityOrigin
+
+ case class CustomIdentityOrigin() extends IdentityOrigin
+
+ case class ServerSetIdentityOrigin() extends IdentityOrigin
+
+ def merge(identityWrap1: IdentityOriginWrapper, identityWrap2: IdentityOriginWrapper): IdentityOriginWrapper =
+ if (identityWrap1.origin.isInstanceOf[CustomIdentityOrigin]) {
+ identityWrap1
+ } else {
+ identityWrap2
+ }
+
+ def fromCustom(identity: Identity): IdentityOriginWrapper = IdentityOriginWrapper(CustomIdentityOrigin(), identity)
+
+ def fromServerSet(identity: Identity): IdentityOriginWrapper = IdentityOriginWrapper(ServerSetIdentityOrigin(), identity)
+}
Review comment:
`isInstanceOf` is a Scala anti pattern.
How about pattern matching?
```
sealed trait IdentityWithOrigin {
def identity: Identity
}
object IdentityWithOrigin {
def merge(identity1: IdentityWithOrigin, identity2: IdentityWithOrigin): IdentityWithOrigin =
identity1 match {
case CustomIdentity => identity1
case _ => identity2
}
}
case class CustomIdentity(identity: Identity)
case class ServerSetIdentity(identity: Identity)
```
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/IdentitySetContract.scala
##########
@@ -1191,4 +1191,134 @@ trait IdentitySetContract {
.extract()
.jsonPath()
.get(s"methodResponses[0][1].created.$clientId.id")
+
+ @Test
+ def updateShouldAcceptServerSetId(): Unit = {
+ val identityId: String = `given`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(
+ s"""{
+ | "using": ["urn:ietf:params:jmap:core", "urn:ietf:params:jmap:submission"],
+ | "methodCalls": [
+ | ["Identity/get",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "ids": null
+ | }, "c2"
+ | ]
+ |
+ | ]
+ |}""".stripMargin)
+ .when
+ .post
+ .`then`
+ .statusCode(SC_OK)
+ .extract()
+ .jsonPath()
+ .get("methodResponses[0][1].list[0].id")
Review comment:
Please use a JMAP back reference to only do a single JMAP call :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael edited a comment on pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
Arsnael edited a comment on pull request #757:
URL: https://github.com/apache/james-project/pull/757#issuecomment-979691235
Can you rebase and squash your commits please @vttranlina ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
vttranlina commented on pull request #757:
URL: https://github.com/apache/james-project/pull/757#issuecomment-979693702
rebase master & squash fixup
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756729809
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/IdentitySetContract.scala
##########
@@ -1191,4 +1191,134 @@ trait IdentitySetContract {
.extract()
.jsonPath()
.get(s"methodResponses[0][1].created.$clientId.id")
+
+ @Test
+ def updateShouldAcceptServerSetId(): Unit = {
+ val identityId: String = `given`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(
+ s"""{
+ | "using": ["urn:ietf:params:jmap:core", "urn:ietf:params:jmap:submission"],
+ | "methodCalls": [
+ | ["Identity/get",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "ids": null
+ | }, "c2"
+ | ]
+ |
+ | ]
+ |}""".stripMargin)
+ .when
+ .post
+ .`then`
+ .statusCode(SC_OK)
+ .extract()
+ .jsonPath()
+ .get("methodResponses[0][1].list[0].id")
Review comment:
Yes maybe not doable...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756734781
##########
File path: server/data/data-jmap/src/test/scala/org/apache/james/jmap/api/identity/IdentityRepositoryTest.scala
##########
@@ -135,11 +134,47 @@ class IdentityRepositoryTest {
@Test
def updateShouldSuccessWhenCustomExists(): Unit = {
when(identityFactory.listIdentities(BOB)).thenReturn(List())
- SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
- val identity: Identity = SFlux(customIdentityDAO.list(BOB)).blockFirst().get
+ val customIdentity: Identity = SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
- assertThatCode(() => SMono.fromPublisher(testee.update(BOB, identity.id, UPDATE_REQUEST)).block())
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, customIdentity.id, UPDATE_REQUEST)).block())
.doesNotThrowAnyException()
}
+ @Test
+ def updateShouldModifiedEntry(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List())
+ val customIdentity: Identity = SMono.fromPublisher(customIdentityDAO.save(BOB, CREATION_REQUEST)).block()
+
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, customIdentity.id, UPDATE_REQUEST)).block())
+ .doesNotThrowAnyException()
+
+ assertThat(SFlux(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(Identity(id = customIdentity.id,
+ name = IdentityName("Bob (new name)"),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))),
+ textSignature = TextSignature("text 2 signature"),
+ htmlSignature = HtmlSignature("html 2 signature"),
+ mayDelete = MayDeleteIdentity(true)))
+ }
+
+ @Test
+ def updateShouldSuccessWhenMultiUpdateServerSetId(): Unit = {
+ when(identityFactory.listIdentities(BOB)).thenReturn(List(IDENTITY1))
+ SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, UPDATE_REQUEST)).block()
+
+ assertThatCode(() => SMono.fromPublisher(testee.update(BOB, IDENTITY1.id, UPDATE_REQUEST.copy(name = Some(IdentityNameUpdate(IdentityName("Bob (3)")))))).block())
+ .doesNotThrowAnyException()
+
+ assertThat(SFlux(testee.list(BOB)).collectSeq().block().asJava)
+ .containsExactlyInAnyOrder(Identity(id = IDENTITY1.id,
+ name = IdentityName("Bob (3)"),
+ email = BOB.asMailAddress(),
+ replyTo = Some(List(EmailAddress(Some(EmailerName("My Boss (updated)")), new MailAddress("boss-updated@domain.tld")))),
+ bcc = Some(List(EmailAddress(Some(EmailerName("My Boss 2 (updated)")), new MailAddress("boss-updated-2@domain.tld")))),
+ textSignature = TextSignature("text 2 signature"),
+ htmlSignature = HtmlSignature("html 2 signature"),
+ mayDelete = MayDeleteIdentity(true)))
+ }
Review comment:
Yes that's it.
Except that we shall not require the `, None, None,None, None` : this boiler plate should be removed by the use of default values...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #757:
URL: https://github.com/apache/james-project/pull/757#discussion_r756737399
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/IdentitySetContract.scala
##########
@@ -1191,4 +1191,134 @@ trait IdentitySetContract {
.extract()
.jsonPath()
.get(s"methodResponses[0][1].created.$clientId.id")
+
+ @Test
+ def updateShouldAcceptServerSetId(): Unit = {
+ val identityId: String = `given`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(
+ s"""{
+ | "using": ["urn:ietf:params:jmap:core", "urn:ietf:params:jmap:submission"],
+ | "methodCalls": [
+ | ["Identity/get",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "ids": null
+ | }, "c2"
+ | ]
+ |
+ | ]
+ |}""".stripMargin)
+ .when
+ .post
+ .`then`
+ .statusCode(SC_OK)
+ .extract()
+ .jsonPath()
+ .get("methodResponses[0][1].list[0].id")
Review comment:
Very true!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #757: JAMES-3534 Identity/set update should work on existing server-set Identities
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #757:
URL: https://github.com/apache/james-project/pull/757#issuecomment-979691235
Can you rebase and squash your commit please @vttranlina ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org