[GitHub] [james-project] chibenwa commented on a change in pull request #385: JAMES-3520 MDN/send

[GitBox <gi...@apache.org>]

chibenwa commented on a change in pull request #385:
URL: https://github.com/apache/james-project/pull/385#discussion_r615750464



##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MDNSendMethodContract.scala
##########
@@ -1275,6 +1338,95 @@ trait MDNSendMethodContract {
                     |}""".stripMargin)
   }
 
+  @Test
+  def mdnSendShouldReturnNotFoundWhenIdentityIdIsNotExits(guiceJamesServer: GuiceJamesServer): Unit = {

Review comment:
       WhenIdentityDoesNotExist

##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MDNSendMethodContract.scala
##########
@@ -1275,6 +1338,95 @@ trait MDNSendMethodContract {
                     |}""".stripMargin)
   }
 
+  @Test
+  def mdnSendShouldReturnNotFoundWhenIdentityIdIsNotExits(guiceJamesServer: GuiceJamesServer): Unit = {
+    val mailboxProbe: MailboxProbeImpl = guiceJamesServer.getProbe(classOf[MailboxProbeImpl])
+
+    val bobMailBoxPath: MailboxPath = MailboxPath.inbox(BOB)
+    mailboxProbe.createMailbox(bobMailBoxPath)
+
+    val andreMailBoxPath: MailboxPath = MailboxPath.inbox(ANDRE)
+    mailboxProbe.createMailbox(andreMailBoxPath)
+
+    val message: Message = Message.Builder
+      .of
+      .setSubject("test")
+      .setSender(BOB.asString)
+      .setFrom(BOB.asString)
+      .setTo(ANDRE.asString)
+      .setBody("Body of mail, that mdn related", StandardCharsets.UTF_8)
+      .build
+
+    val emailIdRelated: MessageId = mailboxProbe

Review comment:
       relatedEmailId (here and in other places?)

##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MDNSendMethodContract.scala
##########
@@ -130,7 +130,7 @@ trait MDNSendMethodContract {
          |      "MDN/send",
          |      {
          |        "accountId": "$ANDRE_ACCOUNT_ID",
-         |        "identityId": "I64588216",
+         |        "identityId": "$ANDRE_ACCOUNT_ID",

Review comment:
       You do put an accountId in an identityId? These are not the same...
   
   Edit accountId and identityId happens to share the same format in our implementation.
   
   However we need 2 separate constant in our tests:
   
   ```
     val ANDRE_ACCOUNT_ID: String = "1e8584548eca20f26faf6becc1704a0f352839f12c208a47fbd486d60f491f7c"
       val ANDRE_IDENTITY_ID: String = "1e8584548eca20f26faf6becc1704a0f352839f12c208a47fbd486d60f491f7c"
   ```
   
   And use
   
   ```
   "accountId": "$ANDRE_ACCOUNT_ID",
   "identityId": "$ANDRE_IDENTITY_ID",
   ````
   
   here.

##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/IdentityGetMethod.scala
##########
@@ -66,3 +67,11 @@ class IdentityGetMethod @Inject() (identityFactory: IdentityFactory,
     SMono.fromCallable(() => identityFactory.listIdentities(mailboxSession))
       .map(request.computeResponse)
 }
+
+case class IdentifyStore @Inject()(identityFactory: IdentityFactory) {

Review comment:
       I would prefer `IdentifyResolver`

##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MDNSendMethod.scala
##########
@@ -209,15 +212,27 @@ class MDNSendMethod @Inject()(serializer: MDNSerializer,
       .map(mailbox => mailbox.getAddress)
       .toRight(MDNSendNotFoundException("Invalid \"Disposition-Notification-To\" header field."))
 
-  private def buildMDN(requestEntry: MDNSendCreateRequest, originalMessage: Message, originalRecipientAddress: String): MDN = {
+  private def getMDNFinalRecipient(requestEntry: MDNSendCreateRequest, identity: Identity): Either[MDNSendForbiddenFromException, FinalRecipient] = {

Review comment:
       With a real mastery of Option and EIther monads, not a single i is required in this method. Can you do the refactoring?




-- 
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.

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