odoo icon indicating copy to clipboard operation
odoo copied to clipboard

[IMP] mail: improve new message separator visibility

Open tsm-odoo opened this issue 1 year ago • 5 comments

The new message separator is displayed on discuss channels to keep track of what the user has read.

Until now, this separator was removed as soon as the composer was focused, thus making it hard to see.

This commit improves its visibility by keeping it visible until:

  • The user posts a message
  • The user accesses the thread twice

part of task-2628317

NOTE - some tests were removed/replaced:

  • focusing a chat window of a chat should make the new message separator disappear: Now irrelevant, as the new message separator behavior is no longer linked to focus and is consistent between discuss and chat windows since it is handled by the thread service. The responsibility of testing the disappearance behavior now lies with keep until user goes back to the thread. The responsibility for testing the mark as read part now lies with mark as read when opening chat window.
  • no new message separator on posting message (no message history): This test is weak since it only includes negative assertions. It has been merged with no new messages separator on posting message (some message history) and reworked to include a positive assertion. The test now responsible for testing this behavior is keep until current user sends a message, which accurately reflects the intention of these tests.
  • new message separator is shown in a chat window of a chat on receiving new message if there is a history of conversation renamed to show when message is received in chat window: shorter since the suite itself provide additional context, match its counterpart below.
  • new message separator is shown in chat window of chat on receiving new message when there was no history renamed: see https://github.com/odoo/odoo/pull/159352#discussion_r1565561728.

tsm-odoo avatar Mar 26 '24 15:03 tsm-odoo

Pull request status dashboard.

robodoo avatar Mar 26 '24 15:03 robodoo

Small issue I noticed when testing:

  • going from chat window to discuss keeps the separator, but going back to chat window loses it
  • (in discuss app, when the thread is already focused while receiving messages) when going back and forth to thread once, it clears the unread counter, but keeps the separator. Shouldn't both be synced? Shouldn't the new message separator be reset after the first coming back too?

Both issues are fixed.

tsm-odoo avatar Apr 03 '24 10:04 tsm-odoo

@alexkuhn

Regarding https://github.com/odoo/odoo/pull/159352#discussion_r1570925664,

There is no markAsRead() split everywhere in the code; it's as simple as that: when users actively open a thread, we mark the thread as read. Yes, it is added in two points since our patch of thread.open hijacks the original method, totally bypassing the super call when opening a chat window. This is bad; patches should not ignore the original method. We could fix the patch of the open method and keep a single entry point, as you suggest which would be thread.open. Would that be acceptable to you?

tsm-odoo avatar Apr 19 '24 08:04 tsm-odoo

@alexkuhn Changes applied. Two unresolved comments:

  • As I said I don't think I can rely on the component without breaking the whole feature. We already explored this path and it's fare less reliable than model state.
  • Fix was staged 10mn ago so should be there soon.

Le me know what you think :)

tsm-odoo avatar May 15 '24 12:05 tsm-odoo

@robodoo r+

Approved by PO

tsm-odoo avatar May 16 '24 13:05 tsm-odoo

@robodoo r+

fixed small mistake that caused red runbot :)

tsm-odoo avatar May 16 '24 14:05 tsm-odoo

Merge method set to rebase and fast-forward.

robodoo avatar May 16 '24 16:05 robodoo