You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "vttranlina (via GitHub)" <gi...@apache.org> on 2023/03/20 01:17:48 UTC
[GitHub] [james-project] vttranlina opened a new pull request, #1492: JAMES-3885 - Change username - should migrate succeed when new user already exists
vttranlina opened a new pull request, #1492:
URL: https://github.com/apache/james-project/pull/1492
## Why
Script:
- Bob already has an inbox mailbox (no messages)
- Alice change her username to Bob by webadmin
- Get task status, the result is ABORTED
## How to fix it
Keep all old BOB mailbox data (messages, subscriptions...)
- Rename Bob X mailbox -> Bob X-migrating mailbox
- Rename Alice mailbox X -> Bob mailbox X
- Copy messages in Bob X-migrating mailbox to Bob mailbox X, and then delete Bob X-migrating mailbox
--
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 diff in pull request #1492: JAMES-3885 - Change username - should migrate succeed when mailboxes from old user already exist on the new user
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #1492:
URL: https://github.com/apache/james-project/pull/1492#discussion_r1141621609
##########
server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java:
##########
@@ -147,4 +151,51 @@ void shouldTransferSubscriptionToNewUser() throws Exception {
assertThat(subscriptionManager.subscriptions(mailboxManager.createSystemSession(BOB)))
.containsOnly(MailboxPath.forUser(BOB, "subscribed"));
}
+
+ @Test
+ void shouldMigrateMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
+ MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE);
+ MailboxSession bobSession = mailboxManager.createSystemSession(BOB);
+ MailboxPath aliceInbox = MailboxPath.inbox(ALICE);
+ MailboxPath bobInbox = MailboxPath.inbox(BOB);
+ mailboxManager.createMailbox(aliceInbox, MailboxManager.CreateOption.NONE, aliceSession);
+ mailboxManager.createMailbox(bobInbox, MailboxManager.CreateOption.NONE, bobSession);
+
+ mailboxManager.getMailbox(aliceInbox, aliceSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("alice33333", StandardCharsets.UTF_8)
+ .build()), aliceSession);
+
+ mailboxManager.getMailbox(bobInbox, bobSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("bob", StandardCharsets.UTF_8)
+ .build()), bobSession);
+
+ Mono.from(testee.changeUsername(ALICE, BOB)).block();
+
+ assertThat(mailboxManager.getMailbox(bobInbox, bobSession).getMessageCount(bobSession))
+ .isEqualTo(2);
+
+ assertThat(mailboxManager.list(bobSession)).containsOnly(bobInbox);
+ }
+
+ @Test
+ void shouldMigrateSubMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
Review Comment:
Ok I got it :)
How about renaming the test: `shouldMigrateMailboxesWhenNewUserHasAlreadyOherMailboxes` ?
--
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 #1492: JAMES-3885 - Change username - should migrate succeed when mailboxes from old user already exist on the new user
Posted by "vttranlina (via GitHub)" <gi...@apache.org>.
vttranlina commented on PR #1492:
URL: https://github.com/apache/james-project/pull/1492#issuecomment-1475600578
> Rethinking about this, could we just move directly messages from `Alice mailbox X` to `Bob mailbox X` (without a temporary mailbox)?
The rename method not only moves messages
It also subscription, acl, sub mailbox, delegatee...vvv
I believe we have more than one way to do this, but the renameMethod looks complex, so let's make it less code
--
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 diff in pull request #1492: JAMES-3885 - Change username - should migrate succeed when mailboxes from old user already exist on the new user
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #1492:
URL: https://github.com/apache/james-project/pull/1492#discussion_r1141611928
##########
server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java:
##########
@@ -147,4 +151,51 @@ void shouldTransferSubscriptionToNewUser() throws Exception {
assertThat(subscriptionManager.subscriptions(mailboxManager.createSystemSession(BOB)))
.containsOnly(MailboxPath.forUser(BOB, "subscribed"));
}
+
+ @Test
+ void shouldMigrateMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
+ MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE);
+ MailboxSession bobSession = mailboxManager.createSystemSession(BOB);
+ MailboxPath aliceInbox = MailboxPath.inbox(ALICE);
+ MailboxPath bobInbox = MailboxPath.inbox(BOB);
+ mailboxManager.createMailbox(aliceInbox, MailboxManager.CreateOption.NONE, aliceSession);
+ mailboxManager.createMailbox(bobInbox, MailboxManager.CreateOption.NONE, bobSession);
+
+ mailboxManager.getMailbox(aliceInbox, aliceSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("alice33333", StandardCharsets.UTF_8)
+ .build()), aliceSession);
+
+ mailboxManager.getMailbox(bobInbox, bobSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("bob", StandardCharsets.UTF_8)
+ .build()), bobSession);
+
+ Mono.from(testee.changeUsername(ALICE, BOB)).block();
+
+ assertThat(mailboxManager.getMailbox(bobInbox, bobSession).getMessageCount(bobSession))
+ .isEqualTo(2);
+
+ assertThat(mailboxManager.list(bobSession)).containsOnly(bobInbox);
+ }
+
+ @Test
+ void shouldMigrateSubMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
Review Comment:
I thought your change was supposed to be able to do the migration when mailboxes have the same name? I don't get your comment sorry
--
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 pull request #1492: JAMES-3885 - Change username - should migrate succeed when mailboxes from old user already exist on the new user
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on PR #1492:
URL: https://github.com/apache/james-project/pull/1492#issuecomment-1475597128
```
Rename Bob X mailbox -> Bob X-migrating mailbox
Rename Alice mailbox X -> Bob mailbox X
Copy messages in Bob X-migrating mailbox to Bob mailbox X, and then delete Bob X-migrating mailbox
```
Rethinking about this, could we just move directly messages from `Alice mailbox X` to `Bob mailbox X` (without a temporary mailbox)?
--
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 diff in pull request #1492: JAMES-3885 - Change username - should migrate succeed when new user already exists
Posted by "vttranlina (via GitHub)" <gi...@apache.org>.
vttranlina commented on code in PR #1492:
URL: https://github.com/apache/james-project/pull/1492#discussion_r1141599795
##########
server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java:
##########
@@ -147,4 +151,51 @@ void shouldTransferSubscriptionToNewUser() throws Exception {
assertThat(subscriptionManager.subscriptions(mailboxManager.createSystemSession(BOB)))
.containsOnly(MailboxPath.forUser(BOB, "subscribed"));
}
+
+ @Test
+ void shouldMigrateMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
+ MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE);
+ MailboxSession bobSession = mailboxManager.createSystemSession(BOB);
+ MailboxPath aliceInbox = MailboxPath.inbox(ALICE);
+ MailboxPath bobInbox = MailboxPath.inbox(BOB);
+ mailboxManager.createMailbox(aliceInbox, MailboxManager.CreateOption.NONE, aliceSession);
+ mailboxManager.createMailbox(bobInbox, MailboxManager.CreateOption.NONE, bobSession);
+
+ mailboxManager.getMailbox(aliceInbox, aliceSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("alice33333", StandardCharsets.UTF_8)
+ .build()), aliceSession);
+
+ mailboxManager.getMailbox(bobInbox, bobSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("bob", StandardCharsets.UTF_8)
+ .build()), bobSession);
+
+ Mono.from(testee.changeUsername(ALICE, BOB)).block();
+
+ assertThat(mailboxManager.getMailbox(bobInbox, bobSession).getMessageCount(bobSession))
+ .isEqualTo(2);
+
+ assertThat(mailboxManager.list(bobSession)).containsOnly(bobInbox);
+ }
+
+ @Test
+ void shouldMigrateSubMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
Review Comment:
> Bob and Alice don't have the same mailboxes here though,
No, it abort when Bob and Alice have the same mailbox name
--
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 diff in pull request #1492: JAMES-3885 - Change username - should migrate succeed when mailboxes from old user already exist on the new user
Posted by "vttranlina (via GitHub)" <gi...@apache.org>.
vttranlina commented on code in PR #1492:
URL: https://github.com/apache/james-project/pull/1492#discussion_r1141616653
##########
server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java:
##########
@@ -147,4 +151,51 @@ void shouldTransferSubscriptionToNewUser() throws Exception {
assertThat(subscriptionManager.subscriptions(mailboxManager.createSystemSession(BOB)))
.containsOnly(MailboxPath.forUser(BOB, "subscribed"));
}
+
+ @Test
+ void shouldMigrateMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
+ MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE);
+ MailboxSession bobSession = mailboxManager.createSystemSession(BOB);
+ MailboxPath aliceInbox = MailboxPath.inbox(ALICE);
+ MailboxPath bobInbox = MailboxPath.inbox(BOB);
+ mailboxManager.createMailbox(aliceInbox, MailboxManager.CreateOption.NONE, aliceSession);
+ mailboxManager.createMailbox(bobInbox, MailboxManager.CreateOption.NONE, bobSession);
+
+ mailboxManager.getMailbox(aliceInbox, aliceSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("alice33333", StandardCharsets.UTF_8)
+ .build()), aliceSession);
+
+ mailboxManager.getMailbox(bobInbox, bobSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("bob", StandardCharsets.UTF_8)
+ .build()), bobSession);
+
+ Mono.from(testee.changeUsername(ALICE, BOB)).block();
+
+ assertThat(mailboxManager.getMailbox(bobInbox, bobSession).getMessageCount(bobSession))
+ .isEqualTo(2);
+
+ assertThat(mailboxManager.list(bobSession)).containsOnly(bobInbox);
+ }
+
+ @Test
+ void shouldMigrateSubMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
Review Comment:
Ah, I just re-read your comment,
This test case for ensuring when user has mailbox A,
Mailbox A won't be changed when migrate mailbox B
It look like not clear sense, maybe it is unnecessary?
--
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 merged pull request #1492: JAMES-3885 - Change username - should migrate succeed when mailboxes from old user already exist on the new user
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael merged PR #1492:
URL: https://github.com/apache/james-project/pull/1492
--
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 diff in pull request #1492: JAMES-3885 - Change username - should migrate succeed when new user already exists
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #1492:
URL: https://github.com/apache/james-project/pull/1492#discussion_r1141591435
##########
server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStep.java:
##########
@@ -76,11 +77,35 @@ public Publisher<Void> changeUsername(Username oldUsername, Username newUsername
private Mono<Void> migrateMailbox(MailboxSession fromSession, MailboxSession toSession, org.apache.james.mailbox.model.MailboxMetaData mailbox) {
MailboxPath renamedPath = mailbox.getPath().withUser(toSession.getUser());
- return mailboxManager.renameMailboxReactive(mailbox.getPath(), renamedPath,
- MailboxManager.RenameOption.RENAME_SUBSCRIPTIONS,
- fromSession, toSession)
+ Mono<Void> renamePublisher = mailboxManager.renameMailboxReactive(mailbox.getPath(), renamedPath,
Review Comment:
I feel that could be just extracted into a function directly?
##########
server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java:
##########
@@ -147,4 +151,51 @@ void shouldTransferSubscriptionToNewUser() throws Exception {
assertThat(subscriptionManager.subscriptions(mailboxManager.createSystemSession(BOB)))
.containsOnly(MailboxPath.forUser(BOB, "subscribed"));
}
+
+ @Test
+ void shouldMigrateMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
+ MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE);
+ MailboxSession bobSession = mailboxManager.createSystemSession(BOB);
+ MailboxPath aliceInbox = MailboxPath.inbox(ALICE);
+ MailboxPath bobInbox = MailboxPath.inbox(BOB);
+ mailboxManager.createMailbox(aliceInbox, MailboxManager.CreateOption.NONE, aliceSession);
+ mailboxManager.createMailbox(bobInbox, MailboxManager.CreateOption.NONE, bobSession);
+
+ mailboxManager.getMailbox(aliceInbox, aliceSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("alice33333", StandardCharsets.UTF_8)
+ .build()), aliceSession);
+
+ mailboxManager.getMailbox(bobInbox, bobSession)
+ .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of()
+ .setSubject("toto")
+ .setBody("bob", StandardCharsets.UTF_8)
+ .build()), bobSession);
+
+ Mono.from(testee.changeUsername(ALICE, BOB)).block();
+
+ assertThat(mailboxManager.getMailbox(bobInbox, bobSession).getMessageCount(bobSession))
+ .isEqualTo(2);
+
+ assertThat(mailboxManager.list(bobSession)).containsOnly(bobInbox);
+ }
+
+ @Test
+ void shouldMigrateSubMailboxesWhenNewUserHasAlreadyMailbox() throws Exception {
Review Comment:
If I understand correctly, Bob and Alice don't have the same mailboxes here though, so it's not really an already mailbox exists use case?
--
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