You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "Arsnael (via GitHub)" <gi...@apache.org> on 2023/03/20 10:20:39 UTC
[GitHub] [james-project] Arsnael opened a new pull request, #1495: [FIX] Shared mailbox revocation rights should be visible in Mailbox/C…
Arsnael opened a new pull request, #1495:
URL: https://github.com/apache/james-project/pull/1495
…hanges
--
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 #1495: [FIX] Shared mailbox revocation rights should be visible in Mailbox/C…
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #1495:
URL: https://github.com/apache/james-project/pull/1495#issuecomment-1477193441
Rebase (build was green)
--
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 #1495: [FIX] Shared mailbox revocation rights should be visible in Mailbox/C…
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #1495:
URL: https://github.com/apache/james-project/pull/1495#discussion_r1142832779
##########
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/MailboxChange.java:
##########
@@ -226,7 +226,21 @@ public List<JmapChange> fromMailboxACLUpdated(MailboxACLUpdated mailboxACLUpdate
.delegated()
.build());
- return Stream.concat(Stream.of(ownerChange), shareeChanges)
+ Stream<MailboxChange> deletionChanges = mailboxACLUpdated.getAclDiff()
+ .removedEntries()
+ .filter(entry -> entry.getKey().getNameType().equals(MailboxACL.NameType.user))
+ .filter(entry -> !entry.getKey().isNegative())
+ .map(entry -> MailboxChange.builder()
+ .accountId(AccountId.fromString(entry.getKey().getName()))
+ .state(stateFactory.generate())
+ .date(now)
+ .isCountChange(false)
+ .destroyed(ImmutableList.of(mailboxACLUpdated.getMailboxId()))
Review Comment:
Are you sure that with just updated though the client will understand that the user is not linked to the shared mailbox anymore?
--
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 #1495: [FIX] Shared mailbox revocation rights should be visible in Mailbox/C…
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #1495:
URL: https://github.com/apache/james-project/pull/1495#discussion_r1143007919
##########
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/MailboxChange.java:
##########
@@ -226,7 +226,21 @@ public List<JmapChange> fromMailboxACLUpdated(MailboxACLUpdated mailboxACLUpdate
.delegated()
.build());
- return Stream.concat(Stream.of(ownerChange), shareeChanges)
+ Stream<MailboxChange> deletionChanges = mailboxACLUpdated.getAclDiff()
+ .removedEntries()
+ .filter(entry -> entry.getKey().getNameType().equals(MailboxACL.NameType.user))
+ .filter(entry -> !entry.getKey().isNegative())
+ .map(entry -> MailboxChange.builder()
+ .accountId(AccountId.fromString(entry.getKey().getName()))
+ .state(stateFactory.generate())
+ .date(now)
+ .isCountChange(false)
+ .destroyed(ImmutableList.of(mailboxACLUpdated.getMailboxId()))
Review Comment:
Indeed I managed to reproduce your case, now I get it.
Made the change to updated and added your case as a test as well
--
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 #1495: [FIX] Shared mailbox revocation rights should be visible in Mailbox/C…
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael merged PR #1495:
URL: https://github.com/apache/james-project/pull/1495
--
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 diff in pull request #1495: [FIX] Shared mailbox revocation rights should be visible in Mailbox/C…
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #1495:
URL: https://github.com/apache/james-project/pull/1495#discussion_r1142926648
##########
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/MailboxChange.java:
##########
@@ -226,7 +226,21 @@ public List<JmapChange> fromMailboxACLUpdated(MailboxACLUpdated mailboxACLUpdate
.delegated()
.build());
- return Stream.concat(Stream.of(ownerChange), shareeChanges)
+ Stream<MailboxChange> deletionChanges = mailboxACLUpdated.getAclDiff()
+ .removedEntries()
+ .filter(entry -> entry.getKey().getNameType().equals(MailboxACL.NameType.user))
+ .filter(entry -> !entry.getKey().isNegative())
+ .map(entry -> MailboxChange.builder()
+ .accountId(AccountId.fromString(entry.getKey().getName()))
+ .state(stateFactory.generate())
+ .date(now)
+ .isCountChange(false)
+ .destroyed(ImmutableList.of(mailboxACLUpdated.getMailboxId()))
Review Comment:
Client will fetch the id to get the changes and get "not found".
If it is re-shared then it will again appear as "updated" and thus will eventually be synchronised.
With "destroyed" the client will drop the item. If I re-share it it might appear as still destroyed because we fold changes. So never synchronised.
If you do not believe me, write this test:
```
Given bob mailbox in state A
AND bob revokes ritghts on marketting@linagora.com team mailbox -> his mailboxes are in state B
AND alice re shares marketting@linagora.com -> his mailboxes are in state C
WHEN bob requests changes with state A
THEN marketting@linagora.com team mailbox appears as updated => synced
```
If it appears as destroyed it would not get synced ;-)
--
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 diff in pull request #1495: [FIX] Shared mailbox revocation rights should be visible in Mailbox/C…
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #1495:
URL: https://github.com/apache/james-project/pull/1495#discussion_r1142195485
##########
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/MailboxChange.java:
##########
@@ -226,7 +226,21 @@ public List<JmapChange> fromMailboxACLUpdated(MailboxACLUpdated mailboxACLUpdate
.delegated()
.build());
- return Stream.concat(Stream.of(ownerChange), shareeChanges)
+ Stream<MailboxChange> deletionChanges = mailboxACLUpdated.getAclDiff()
+ .removedEntries()
+ .filter(entry -> entry.getKey().getNameType().equals(MailboxACL.NameType.user))
+ .filter(entry -> !entry.getKey().isNegative())
+ .map(entry -> MailboxChange.builder()
+ .accountId(AccountId.fromString(entry.getKey().getName()))
+ .state(stateFactory.generate())
+ .date(now)
+ .isCountChange(false)
+ .destroyed(ImmutableList.of(mailboxACLUpdated.getMailboxId()))
Review Comment:
I think let's call it updated because otherwise if I revoke then re-invite this will not synchronize well
--
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