Skip to content

[UBERF-41] Chat notification fixes#3630

Merged
haiodo merged 1 commit into
mainfrom
uberf-41-chat-notif-fixes
Aug 25, 2023
Merged

[UBERF-41] Chat notification fixes#3630
haiodo merged 1 commit into
mainfrom
uberf-41-chat-notif-fixes

Conversation

@sol-0

@sol-0 sol-0 commented Aug 25, 2023

Copy link
Copy Markdown
Contributor

Contribution checklist

Brief description

https://front.hc.engineering/workbench/platform/tracker/UBERF-41

Checklist

  • - UI test added to added/changed functionality?
  • - Screenshot is added to PR if applicable ?
  • - Does the code work? Check whether function and logic are correct.
  • - Does Changelog.md is updated with changes?
  • - Does the translations are up to date?
  • - Does it well tested?
  • - Tested for Chrome.
  • - Tested for Safari.
  • - Go through the changed code looking for typos, TODOs, commented LOCs, debugging pieces of code, etc.
  • - Rebase your branch onto master and upstream branch
  • - Is there any redundant or duplicate code?
  • - Are required links are linked to PR?
  • - Does new code is well documented ?

Related issues

A list of closed updated issues

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>
@sol-0
sol-0 requested a review from BykhovDenis August 25, 2023 03:23
@sol-0

sol-0 commented Aug 25, 2023

Copy link
Copy Markdown
Contributor Author

@BykhovDenis While searching for the root cause for this issue, I noticed that the logic in couple of places assumed that NotificationClientImpl.docUpdatesStore stores only new updates.
I've adjusted the logic to properly show chat notifications, but haven't changed the store logic itself. So, the questions is: shall I change the logic of populating of the store instead? (store only updated spaces and only new txes for those spaces)
In my opinion, having all the updates in the store is more flexible, but we might need to adjust the code in other places also in case there are any similar issues with notifications.

@BykhovDenis

Copy link
Copy Markdown
Member

@BykhovDenis While searching for the root cause for this issue, I noticed that the logic in couple of places assumed that NotificationClientImpl.docUpdatesStore stores only new updates.
I've adjusted the logic to properly show chat notifications, but haven't changed the store logic itself. So, the questions is: shall I change the logic of populating of the store instead? (store only updated spaces and only new txes for those spaces)
In my opinion, having all the updates in the store is more flexible, but we might need to adjust the code in other places also in case there are any similar issues with notifications.

It was old implementation, we added all ties instead only new for have easy way to show people mode, because it show not only new changes. I think we shouldn't currently change store logic.

@sol-0
sol-0 marked this pull request as ready for review August 25, 2023 07:40
@sol-0
sol-0 requested a review from haiodo August 25, 2023 07:40
@haiodo
haiodo merged commit 3be2e0e into main Aug 25, 2023
@haiodo
haiodo deleted the uberf-41-chat-notif-fixes branch August 25, 2023 07:53
MichaelUray pushed a commit to MichaelUray/huly-platform that referenced this pull request Jul 9, 2026
Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>
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.

3 participants