Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISSUE-3189 front end support for subaddressing #3241

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

@florentos17
Copy link
Collaborator Author

florentos17 commented Oct 28, 2024

my plan in the following: rights is an attribute for mailboxes which indicates a list of rights for various identifiers, and in particular, the identifier anyone.

subaddressing will be considered allowed if the identifier anyone has the p right (posting). this p right does not exist just yet (as you can see in the doc) but i will soon open a PR to add it. meanwhile, i try to set the r right.

thus, clicking on the allow/disallow subaddressing menu option should create a JMAP request to set or remove the p right for the identifier anyone, but it does not work... i am able to effectively modify an other attribute (for example, isSubscribed), but it does not work for the attribute rights...

the interesting file is mailbox_api.dart, and in particular line 427

and here are the logs. as you can see, not much details...

you can see the jmap request generated and sent, but then it results in a failure

image

what do you think ? any comment welcome ! :)

@chibenwa
Copy link
Member

Thanks for putting together that work!

@florentos17
Copy link
Collaborator Author

florentos17 commented Oct 29, 2024

the JMAP request generated by clicking the button still does not work. by looking into the james-project repo files MailboxSet.scala and MailboxSetMethodContract.scala, i understood that the rights attribute is not meant to be modified directly, but rather using the sharedWith/ keyword.

i managed to get the the server response when sending such a request:

  • with an incorrect right:
{"sessionState":"2c9f1b12-b35a-43e6-9af2-0106fb53a943","methodResponses":[["Mailbox/set",
{"accountId":"548575abf99bc986f5f2318a4ab2ca22790513380dd5beb32daf004934e1635d","oldState
":"a40c9441-9607-11ef-87be-c1d0831d0b43","newState":"a40c9441-9607-11ef-87be-c1d0831d0b43
","notUpdated":{"58fb3d50-8f84-11ef-87be-c1d0831d0b43":{"type":"invalidArguments","descri
ption":"Specified value do not match the expected JSON format:
List((/anyone(0),List(JsonValidationError(List(Unknown right
'y'),List()))))","properties":["sharedWith"]}}},"c0"]]}
  • with a correct right:
{"sessionState":"2c9f1b12-b35a-43e6-9af2-0106fb53a943","methodResponses":[["Mailbox/set",{"accountId":"548575abf99bc986f5f2318a4ab2ca22790513380dd5beb32daf004934e1635d","oldState":"a40c9441-9607-11ef-87be-c1d0831d0b43","newState
":"a40c9441-9607-11ef-87be-c1d0831d0b43","notUpdated":{"58fb3d50-8f84-11ef-87be-c1d0831d0b43":{"type":"serverFail","description":null}}},"c0"]]}

i am stuck with this "serverFail" error... :/

@chibenwa
Copy link
Member

What is the corresponding JMAP request being done?

@florentos17
Copy link
Collaborator Author

florentos17 commented Oct 30, 2024

What is the corresponding JMAP request being done?

{using: [urn:ietf:params:jmap:mail, urn:ietf:params:jmap:core, urn:apache:james:params:jmap:mail:shares],
methodCalls: [[Mailbox/set, {accountId: 548575abf99bc986f5f2318a4ab2ca22790513380dd5beb32daf004934e1635d,
update: {58fb3d50-8f84-11ef-87be-c1d0831d0b43: {sharedWith: {anyone: [r]}}}}, c0]]}

@chibenwa
Copy link
Member

Personal bet:

  • the issue comes from the fact that anyone isn't supported by the back
  • there is likely also something with non proper error handling going on

@florentos17
Copy link
Collaborator Author

i tried with other identifiers, including user01, testuser01, bob, owner, i always get the same errorFail. when i try with an empty identifier, i get "username should not be null or empty after being trimmed".

so indeed the error handling on the server side seems non exhaustive... is there any way i could access the logs of the server at https://jmap.lin-saas.dev to try to get more info ?

@florentos17
Copy link
Collaborator Author

benoit confirms that the issue comes from the server side, so no need to look further ! :)

@florentos17
Copy link
Collaborator Author

@dab246 i think all of your comments were addressed, can i rebase and clean the commit history ?

@florentos17
Copy link
Collaborator Author

florentos17 commented Nov 29, 2024

  • how to test sub-addressing (tmail.linagora can not work when sending email to subaddress email)

you need to connect the frontend to a server that has the latest changes, including this PR, and this PR. when running my own instance of the server, with the image linagora/tmail-backend-distributed, it works.

note that subaddressing features only show if the server advertizes that it supports it. but currently that advertizement is not merged on the server side (cf this PR) so you'll have to manually change the default value to true in lib/features/mailbox/presentation/mixin/mailbox_widget_mixin.dart line 290 before running the front.

i will post a video testing all cases

done

  • analyze

i feel like i have a test failing, but i can't find which one:

Run ./scripts/test.sh
+ [[ default == \d\e\f\a\u\l\t ]]
+ flutter test -r json
Error: Process completed with exit code 1.

@hoangdat
Copy link
Member

hoangdat commented Dec 2, 2024

  • error in test
Screenshot 2024-12-02 at 09 10 07

@hoangdat
Copy link
Member

hoangdat commented Dec 2, 2024

  • Please let me know how I can verify your completion and corrections. Maybe using dev (mai.lin-saas.dev) would be okay.

@hoangdat
Copy link
Member

hoangdat commented Dec 2, 2024

@florentos17 you can come here to see the detail test result: https://github.com/linagora/tmail-flutter/pull/3241/checks?check_run_id=33790426200

@florentos17
Copy link
Collaborator Author

i will post a video testing all cases

here is a little demo video testing several edge cases:

subaddressing-demo.mp4

Future<void> clearMailboxCache() {
return Future.wait([
_mailboxCacheClient.clearAllData(),
if (PlatformInfo.isMobile) clearAllFileInStorage(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove it, we only need remove mailbox cache and state of mailbox

Copy link
Collaborator Author

@florentos17 florentos17 Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so just this ?

Future<void> clearMailboxCache() {
  return Future.wait([
    _mailboxCacheClient.clearAllData(),
    _stateCacheClient.clearAllData()
  ], eagerError: true);
}

Copy link
Member

@hoangdat hoangdat Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stateCache store the data via tupleKey. So we only to clear state of mailbox. Please try with

_stateCacheClient.deleteItem(StateType.mailbox.getTupleKeyStored(accountId, session.username))

Copy link
Collaborator Author

@florentos17 florentos17 Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clearMailboxCache is called from /lib/features/base/upgradeable/upgrade_hive_database_steps_v13.dart, there is no accountId or session.
do you want to call it directly from mailbox_controller somehow ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okie so please

Future<void> clearMailboxCache() {
  return Future.wait([
    _mailboxCacheClient.clearAllData(),
  ], eagerError: true);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants