Starting to reply to a message while the chat history is cleared gives unexpected behaviour

[nickvergessen]

How to use GitHub

• Please use the 👍 reaction to show that you are affected by the same issue.
• Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
• Subscribe to receive notifications on status change and new comments.

---

Steps to reproduce

  Scenario: Starting to reply to a message while the chat history is cleared
    Given user "participant1" creates room "room" (v4)
      | roomType | 3     |
      | roomName | room |
    And user "participant1" sends message "Message 1" to room "room" with 201
    And user "participant1" deletes chat history for room "room" with 200
    And user "participant1" sends reply "Message 1-1" on message "Message 1" to room "room" with 201

The problem:

    And user "participant1" sends reply "Message 1-1" on message "Message 1" to room "room" with 201 # FeatureContext::userSendsReplyToRoom()
      {"ocs":{"meta":{"status":"failure","statuscode":400,"message":""},"data":[]}}
      Failed asserting that 400 matches expected 201.

The suggested step to solve:

    And user "participant1" sends reply "Message 1-1" on message "Message 1" to room "room" with 400
      """
      {
          "type": "error",
          "error": {
              "code": "invalid_parent_message",
              "message": "The parent message do not exists anymore."
          }
      }
      """

---

Also tagging https://github.com/nextcloud/spreed/labels/feature%3A%20api%20%F0%9F%9B%A0%EF%B8%8F , as there is a chance that the message expires exactly while you post, which will result in a "default" 400 Bad Request. Maybe we should add a message there on the return which can then be shown in the UIs

[nickvergessen]

[vitormattos]

@nickvergessen

I used the following scenario:

  Scenario: Starting to quote a message that then expires before/while submitting
    Given user "participant1" creates room "room" (v4)
      | roomType | 3     |
      | roomName | room |
    When user "participant1" set the message expiration to 3 of room "room" with 200 (v4)
    And user "participant1" sends message "Message 1" to room "room" with 201
    And wait for 3 seconds
    And user "participant1" set the message expiration to 0 of room "room" with 200 (v4)
    And user "participant1" sends reply "Message 1-1" on message "Message 1" to room "room" with 201

And got this result:

I think that this problem already was solved.

[nickvergessen]

The test above does not run the cron job so messages are still available on DB level

[vitormattos]

Yes, only will be deleted by cron job, but when I reproduced the scenario I didn't saw the deleted message, this is the point.

If I don't see and can't retrieve the deleted message, I think that wait by cron job won't a problem.

Could you provide more information about how to reproduce the scenario that you got when took the print screen? I think that this problem already was solved.

[vitormattos]

I think that this PR https://github.com/nextcloud/spreed/pull/8515 solved the problem.

[nickvergessen]

No, it's not solved, you will end up here: https://github.com/nextcloud/spreed/blob/d480f3af4fda882fe01c4ec523eb3ba7d62b5db2/lib/Controller/ChatController.php#L214-L217

Sample test:

  Scenario: Starting to reply to a message while the chat history is cleared
    Given user "participant1" creates room "room" (v4)
      | roomType | 3     |
      | roomName | room |
    And user "participant1" sends message "Message 1" to room "room" with 201
    And user "participant1" deletes chat history for room "room" with 200
    And user "participant1" sends reply "Message 1-1" on message "Message 1" to room "room" with 201

    And user "participant1" sends reply "Message 1-1" on message "Message 1" to room "room" with 201 # FeatureContext::userSendsReplyToRoom()
      {"ocs":{"meta":{"status":"failure","statuscode":400,"message":""},"data":[]}}
      Failed asserting that 400 matches expected 201.

[vitormattos]

This scenario is different from this issue description. Isn't about expire feature, is related as deleted history.

I updated the subject and description.

[nickvergessen]

It's both the very same thing, just easier to create (since you don't need to run cron)

[vitormattos]

Suggested response:

Status code: 400 Body:

{
    "type": "error",
    "error": {
        "code": "invalid_parent_message",
        "message": "The parent message do not exists anymore."
    }
}

But, we need the opinion from client developers.

[nickvergessen]

Could also just drop the "reply"/parent reference and post it as a normal message. In both cases (expiration and purge of history) it's more to what I would expect as a user.

[vitormattos]

Maybe the message will send outside of context if we don't return an error message. I think that block to send would be best. But this is more an UX point.

[vitormattos]

@nickvergessen is ok the acceptance scenario to this issue?

[nickvergessen]

Yeah I guess blocking would also work, but yeah a dedicated error should be returned.

[vitormattos]

Then, following the requirement to return dedicated error, I think the scenario at description of this issue is OK to be implemented because have a specific error message.

I think that will be necessary check with clients developers what will be the impact of this change before start the implementation.

[Antreesy]

Could also just drop the "reply"/parent reference and post it as a normal message.

Can be done for frontend before posting while processing messages (or purging the messages store) with dropping parentIdToReply in chatExtras cc @DorraJaouad

[nickvergessen]

Could still happen that it expires within the network time 🙈

[DorraJaouad]

Could still happen that it expires within the network time

Even if it is expires ( no sys message to process), the reply parent in Quote no longer has id so parentIdToReply can be dropped based on that instead of processing messages.

[Antreesy]

before posting

We could only fix it at frontend untill request was sent. Rest is on API)