import a folder full of EML files to James by ilkerceng · Pull Request #69 · apache/james-project

ilkerceng · 2017-07-05T07:24:33Z

https://issues.apache.org/jira/browse/JAMES-2081
implemented necessary codes, corrected indentations, added tests.

chibenwa · 2017-07-05T08:58:43Z

We tend to prefix our commit names by tickets:

JAMES-2081  	IMPORTEMLFILETOMAILBOX command

Moreover, you need to review this history. Does your changesets makes sens alone? I would encourage you to keep maximum 2/3 commits for this.

eg: commit 1 : Add capability to MailboxManagement
commit 2 : Add feature to the CLI

One commit is also OK

You can do that with

git rebase

chibenwa · 2017-07-05T09:00:02Z

server/container/cli/src/main/java/org/apache/james/cli/type/CmdType.java

    CREATEMAILBOX("CreateMailbox", "namespace", "user", "name"),
    LISTUSERMAILBOXES("ListUserMailboxes", "user"),
    DELETEMAILBOX("DeleteMailbox", "namespace", "user", "name"),
+    IMPORTEMLFILETOMAILBOX("importemlfiletomailbox", "namespace", "user", "name", "path"),


IMPORTEMLFILETOMAILBOX("importEmlFileToMailbox", "namespace", "user", "name", "path")

Snake case makes it easier to read for end user

chibenwa · 2017-07-05T09:00:51Z

server/container/cli/src/test/java/org/apache/james/cli/ServerCmdTest.java

+        String user = "user@domain";
+        String namespace = "#private";
+        String name = "INBOX.test";
+        String emlpath = ClassLoader.getSystemResource("eml/frnog.eml").getFile();


You don't need a real path here. "my/path/to/file.eml" will also work.

chibenwa · 2017-07-05T09:01:21Z

server/container/cli/src/test/java/org/apache/james/cli/type/CmdTypeTest.java

    }

+    @Test
+    public void lookupImportEmlFileToMailboxShouldReturnEnumValue() {


Add one breakline before isEqualTo

chibenwa · 2017-07-05T09:03:31Z

...er/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxManagementTest.java

+        assertThat(inMemoryMapperFactory.getMessageMapper(session).countMessagesInMailbox(mailbox)).isEqualTo(0);
+        Iterator<MailboxMessage> iterator = inMemoryMapperFactory.getMessageMapper(session).findInMailbox(mailbox,
+                MessageRange.all(), null, 1);
+        assertFalse(iterator.hasNext());


asserThat(iterator.hasNext()).isFalse();

chibenwa

Very good proposition! Congratulation!

chibenwa · 2017-07-06T00:37:40Z

server/container/cli/src/main/java/org/apache/james/cli/ServerCmd.java

            mailboxProbe.deleteMailbox(arguments[1], arguments[2], arguments[3]);
            break;
+        case IMPORTEMLFILETOMAILBOX:
+            mailboxProbe.importEmlFileToMailBox(arguments[1], arguments[2], arguments[3], arguments[4]);


Sorry it's mailbox and not mailBox. Could you please fix this case issue?

i have fixed it as Maibox.

chibenwa · 2017-07-06T00:38:07Z

server/container/cli/src/main/java/org/apache/james/cli/probe/impl/JmxMailboxProbe.java

    }

+    @Override
+    public void importEmlFileToMailBox(String namespace, String user, String name, String emlpath) {


Could you please fix this case issue?

i have fixed it as Maibox.

It should be:

mailboxManagerManagement.importEmlFileToMailbox(namespace, user, name, emlpath);

chibenwa · 2017-07-06T00:43:53Z

server/container/cli/src/test/java/org/apache/james/cli/type/CmdTypeTest.java

    }

+    @Test
+    public void lookupImportEmlFileToMailboxShouldReturnEnumValue() {


Maybe we can get the name of the command shorter... Maybe importEml is enough...

I 'm thinking to poor admins...

hmm, what is your last decision for that :) should i change the name "importEmlFileToMailbox" as importEml for all occurences

chibenwa · 2017-07-06T00:44:46Z

...mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/MailboxManagerManagement.java

+            session = mailboxManager.createSystemSession(user, log);
+            mailboxManager.startProcessingRequest(session);
+            MessageManager messageManager = mailboxManager.getMailbox(new MailboxPath(namespace, user, name), session);
+            InputStream emlFileResourceAsStream = new FileInputStream(emlpath);


emlFileAsStream

i have changed it as emlFileAsStream

chibenwa · 2017-07-06T00:46:21Z

...mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/MailboxManagerManagement.java

+            MessageManager messageManager = mailboxManager.getMailbox(new MailboxPath(namespace, user, name), session);
+            InputStream emlFileResourceAsStream = new FileInputStream(emlpath);
+            ComposedMessageId composedMessageId = messageManager.appendMessage(emlFileResourceAsStream, new Date(),
+                    session, false, new Flags());


What does false means? We tend to extract values to constant to give them a name.

I believe, if it is isRecent then it should be true.

yep it is isRecent, i have changed isRecent as true.

chibenwa · 2017-07-06T00:48:57Z

...er/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxManagementTest.java

+        mailboxManagerManagement.importEmlFileToMailBox(MailboxConstants.USER_NAMESPACE, USER, "name", emlpath);
+
+        assertThat(inMemoryMapperFactory.getMessageMapper(session).countMessagesInMailbox(mailbox)).isEqualTo(1);
+        Iterator<MailboxMessage> iterator = inMemoryMapperFactory.getMessageMapper(session).findInMailbox(mailbox,


Here what is null? Your fetch type? Use FetchType.Full instead.

Extract 1 in a constant named limit

Sorry, named LIMIT

i have changed it as MessageMapper.FetchType.Full, added LIMIT as a constant for 1

chibenwa · 2017-07-06T00:49:21Z

...er/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxManagementTest.java

+        mailboxManagerManagement.importEmlFileToMailBox(MailboxConstants.USER_NAMESPACE, USER, "name", "wrong_path" + emlpath);
+
+        assertThat(inMemoryMapperFactory.getMessageMapper(session).countMessagesInMailbox(mailbox)).isEqualTo(0);
+        Iterator<MailboxMessage> iterator = inMemoryMapperFactory.getMessageMapper(session).findInMailbox(mailbox,


sorry but what you mean with "Idem here"?

I mean I have the same comment than above:

Here what is null? Your fetch type? Use FetchType.Full instead. Extract 1 in a constant named limit

chibenwa · 2017-07-06T08:50:08Z

server/container/cli/src/main/java/org/apache/james/cli/probe/impl/JmxMailboxProbe.java

    }

+    @Override
+    public void importEmlFileToMailBox(String namespace, String user, String name, String emlpath) {


It should be:

mailboxManagerManagement.importEmlFileToMailbox(namespace, user, name, emlpath);

chibenwa · 2017-07-06T08:51:26Z

server/container/cli/src/main/java/org/apache/james/cli/type/CmdType.java

    CREATEMAILBOX("CreateMailbox", "namespace", "user", "name"),
    LISTUSERMAILBOXES("ListUserMailboxes", "user"),
    DELETEMAILBOX("DeleteMailbox", "namespace", "user", "name"),
+    IMPORTEMLFILETOMAILBOX("ImportEmlFileToMailbox", "namespace", "user", "name", "path"),


It should be :

IMPORTEML("ImportEml", "namespace", "user", "name", "path"),

For end user readability

chibenwa · 2017-07-06T08:52:35Z

...er/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxManagementTest.java

+        mailboxManagerManagement.importEmlFileToMailBox(MailboxConstants.USER_NAMESPACE, USER, "name", "wrong_path" + emlpath);
+
+        assertThat(inMemoryMapperFactory.getMessageMapper(session).countMessagesInMailbox(mailbox)).isEqualTo(0);
+        Iterator<MailboxMessage> iterator = inMemoryMapperFactory.getMessageMapper(session).findInMailbox(mailbox,


I mean I have the same comment than above:

Here what is null? Your fetch type? Use FetchType.Full instead. Extract 1 in a constant named limit

ilkerceng · 2017-07-06T08:58:39Z

mr. @chibenwa
you had said before to fix absolute path to relative path in ServerCmdTest.java
i am using ClassLoader.getSystemResource("eml/frnog.eml").getFile() still; in MailboxManagementTest.java

i think i should fix all occurences of ClassLoader.getSystemResource("eml/frnog.eml").getFile() to relative path?
i have realised that i have forgotten pushing "eml/frnog.eml" resource :(

chibenwa · 2017-07-06T11:55:25Z

server/container/guice/mailbox/src/main/java/org/apache/james/modules/MailboxProbeImpl.java

        return subscriptionManager.subscriptions(mailboxSession);
    }
+
+	//TODO: ilker: impelement this


Could you please do it then ?

strokyl · 2017-07-10T07:02:58Z

...mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/MailboxManagerManagement.java

+		checkString(namespace, "mailbox path namespace");
+		checkString(user, "mailbox path user");
+		checkString(name, "mailbox name");
+		checkString(emlpath, "email file path name");


I would remove the checkString private method and do:

Preconditions.checkArgument(!Strings.isNullOrEmpty(namespace), "namespace can't be null or empty");

using Strings from Guava: https://google.github.io/guava/releases/18.0/api/docs/com/google/common/base/Strings.html

It's done in linagora#889

IMPORTEMLFILETOMAILBOX is added to executeCommand function

IMPORTEMLFILETOMAILBOX("importemlfiletomailbox", "namespace", "user", "name", "path"),

lookupImportEmlFileToMailboxShouldReturnEnumValue test function is added

importEmlFileToMailBox function declaration added to MailboxProbe interface

importEmlFileToMailBox is implemented checking if arguments correctly given

indentation has corrected.

indentations has corrected

UnitTest is added to control if file is not found

indentations has corrected

…CmdType to camel case

…mentation

…ath is corrected

…led, LIMIT is added as a constant for the maximal limit of returned

importEmlFileToMailbox functions is added to MailboxProbeImpl.java note: will be implemented.

chibenwa · 2017-07-25T07:29:25Z

This has just been merged. Could you please close the pull request? This will avoid asking this to the Apache INFRA team...

Thanks again for your contribution.

ilkerceng · 2017-07-25T07:38:32Z

I also thank you so much.

chibenwa reviewed Jul 5, 2017

View reviewed changes

chibenwa reviewed Jul 6, 2017

View reviewed changes

chibenwa mentioned this pull request Jul 10, 2017

JAMES-2081 ImportEml CLI command linagora/james-project#889

Merged

strokyl reviewed Jul 10, 2017

View reviewed changes

ilkerceng added 18 commits July 10, 2017 13:17

IMPORTEMLFILETOMAILBOX command

a3a8e86

IMPORTEMLFILETOMAILBOX is added to executeCommand function

IMPORTEMLFILETOMAILBOX added as a CmdType

80cdf89

IMPORTEMLFILETOMAILBOX("importemlfiletomailbox", "namespace", "user", "name", "path"),

IMPORTEMLFILETOMAILBOX command type in CmdTypeTest

47de596

lookupImportEmlFileToMailboxShouldReturnEnumValue test function is added

importEmlFileToMailBox function declaration in MailboxProbe

5779453

importEmlFileToMailBox function declaration added to MailboxProbe interface

importEmlFileToMailBox in MailboxManagerManagement

8d7e12e

importEmlFileToMailBox is implemented checking if arguments correctly given

indentation

bf7d5c4

indentation has corrected.

indentation

2cb8d18

indentations has corrected

Test if file is not found

59cb1db

UnitTest is added to control if file is not found

indentation

b7286ed

indentations has corrected

Convert "importemlfiletomailbox" argument for IMPORTEMLFILETOMAILBOX …

f635ab8

…CmdType to camel case

assertThat instead of using assertFalse

fd62ec6

convert importemlfiletomailbox to camel case

3eccd02

change format lookupImportEmlFileToMailboxShouldReturnEnumValue imple…

9515a6b

…mentation

Absolute path is changed with relative path

da91cd5

importEmlFileToMailBox has changed to importEmlFileToMailbox

286c347

importEmlFileToMailBox has changed to importEmlFileToMailbox and emlp…

5d77fcc

…ath is corrected

the mailbox name has fixed, isRecent is set to true

caa43b3

MessageMapper.FetchType.Full is added to get type of data will be fil…

2d93f08

…led, LIMIT is added as a constant for the maximal limit of returned

ilkerceng added 9 commits July 10, 2017 13:17

Resource is added

7789b59

mailBox is fixed as mailbox

acd7692

variable name is fixed

3a4d9c0

importEmlfileToMailbox in MaiboxProbeImpl

f9eaaf1

importEmlFileToMailbox functions is added to MailboxProbeImpl.java note: will be implemented.

ImportEmlFileToMailbox command changed to ImportEml

3b869bb

added throw not implemented exception

8b25016

importEmlFileToMailbox function is impelemented MailboxProbeImpl

d8e14d6

startProcessingRequest line is added

15aa311

replaced according to inteface placement

ba2c27c

ilkerceng closed this Jul 25, 2017

Conversation

ilkerceng commented Jul 5, 2017

Uh oh!

chibenwa commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chibenwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilkerceng commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chibenwa commented Jul 25, 2017

Uh oh!

ilkerceng commented Jul 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chibenwa commented Jul 5, 2017 •

edited

Loading