Filter/set: Wrong response with multiple actions

hieutbui commented 1 year ago

Description of the bug

When send a request with multiple actions, the response is wrong:

Request:

"action": {
"appendIn": {
"mailboxIds": [
  "aa162850-32cf-11eb-995c-a3ae66e9f96a"
]
},
"markAsSeen": true,
"markAsImportant": true,
"reject": false,
"withKeywords": ["abc", "def"]
}

Reponse

"action": {
"appendIn": {
"mailboxIds": [
  "aa162850-32cf-11eb-995c-a3ae66e9f96a"
]
},
"markAsSeen": false,
"markAsImportant": false,
"reject": false,
"withKeywords": []
}

Reproduction Steps

Send a request includes multiple actions with some optional actions set to true

Expected result

"action": {
  "appendIn": {
    "mailboxIds": [
      "aa162850-32cf-11eb-995c-a3ae66e9f96a"
    ]
  },
  "markAsSeen": true,
  "markAsImportant": true,
  "reject": false,
  "withKeywords": ["abc", "def"]
}

Context

OS: Window
Preprod Server

Additional information

Full request and response file: MultiAction.md
Example:

https://github.com/linagora/tmail-backend/assets/80142234/cb864603-52b9-4c6b-b91c-567324181372

hieutbui commented 1 year ago

@chibenwa @Arsnael I have some problems with the Filter/set. Please check it!

Arsnael commented 1 year ago

Let me have a look...

Arsnael commented 1 year ago

I can confirm with postman it happens on preprod

What I don't get is that we have integration tests covering the case you mention and they pass... so it's hard to get what's going on

Will study the matter, thanks for the report

Arsnael commented 1 year ago

Talked too fast... working for memory, not for distributed and we dont play all distributed tests on the CI. So yes there is an issue

quantranhong1999 commented 1 year ago

not for distributed and we dont play all distributed tests on the CI

BTW should we enable CI to run distributed JMAP tests? at least for TMail? Not the first time we've spotted this and this time leads to a bug.

Arsnael commented 1 year ago

We could... I spotted the issue though, it's from James and the tests in james don't really cover this case as well. Fix here:

Arsnael commented 1 year ago

@hieutbui Fix has been merged on James project.

Now I'm working getting that merged on tmail-backend with proper updates. When it's merged on tmail-backend, I will deploy the master branch image containing the fix on preprod so you can test.

Does that sound good to you?

hieutbui commented 1 year ago

@hieutbui Fix has been merged on James project.

Now I'm working getting that merged on tmail-backend with proper updates. When it's merged on tmail-backend, I will deploy the master branch image containing the fix on preprod so you can test.

Does that sound good to you?

Glad to hear that. It will get my work back on track, please let me know when it's done. Thanks a lot!

Arsnael commented 1 year ago

Unfortunately a work tierce got merged on james meanwhile that trashes the build on tmail-backend... I pushed a fix but it will make the process longer

hieutbui commented 1 year ago

I see! A fix will take time to be good. I'll be waiting. Btw, thank you for your update real quick!

Arsnael commented 1 year ago

@hieutbui Fix deployed on preprod. I tested again with postman and this time I get the correct behavior

By the way the conf might show up alright already, as the filter actions were successfully saved on the backend, it's just we forgot to serialize them when serving the api (showing thus default values)

Let me know if it's fine for you

hieutbui commented 1 year ago

@Arsnael I tested it and everything works good.

Just 1 question, can we control the order of actions? Cause now user can define which action is selected first. For example, If i send request with "markAsSeen" and "markAsImportant" before "appendIn", can response be the same:

"action": {
  "markAsSeen": true,
  "markAsImportant": true,
  "appendIn": {
    "mailboxIds": [
      "aa162850-32cf-11eb-995c-a3ae66e9f96a"
    ]
  },
  "reject": false,
  "withKeywords": ["abc", "def"]
}

Arsnael commented 1 year ago

@hieutbui No there is no orders of actions atm Is this really necessary though, as they are all not really dependent of others? I mean that you append a mail in a mailbox and mark it as seen and important, in one order or the other, end result is the same logically, right?

hieutbui commented 1 year ago

@Arsnael Yes, I know that logically they are the same. I just consider if it can be ordered according to user selections.

Arsnael commented 1 year ago

Not on the backend side, the rendering in the response will be always ordered the same. It would add complexity for not much tbh

linagora / tmail-backend