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

Refactored statuses #2807

Merged
merged 1 commit into from
Jan 4, 2018
Merged

Conversation

janherich
Copy link
Contributor

@janherich janherich commented Dec 24, 2017

Further refactoring building on #1875, message send/delivery status handling is significantly simplified (don't be fooled by only ~40 LOC removed, it's more like ~80 lines of runtime code less then before when you subtract the realm schema update + migration).

Additionally fixes #2659

Situation before PR

Our logic related to message send/delivery statuses was little bit cluttered.
In each message data object, we had :message-status and :user-statuses and logic updating and displaying those had to take both those places into consideration.
Additionally, logic for updating was super bloated, spread in multiple places in
protocol/handlers ns and I believe it was even inconsistent, which is no surprise
when you have like 4 places where you do the same checks based on the status
change & current status information.

Situation after PR

Everything related to message send/delivery statuses is held in the :user-statuses
only.
For incoming message:

{current-public-key      :received|:seen
 participant-public-key  :delivered|:seen}

Under current public key we have either received (user didn't yet opened the chat with new message) or already seen (chat opened, message displayed) status.
For each other participant, we are informed if the message is delivered to him or seen by him.

For outgoing message:

{current-public-key     :pending|:sent
 participant-public-key :delivered|:seen}

It's essentially the same as above, only under current-public key, the status signalises that the message is queued to be sent (:pending) or already sent (:sent)

I would also like to emphasise that I removed big, complicated effect ::save-message-status! and
made effect ::pending-messages-save just dumb facade for realm call without any logic.
And that how we should write all our effects (and re-use them), just doing the absolute bare minimum (isolating IO calls), even having simple logic like (when (#{:message :group-message} type) ...) in them is bad.
The reason why is that we can't really unit test them and whenever we want to reuse the logic with different effects (like we are doing in status-desktop) or we would want to have different effect implementation (let's say we are moving from realm to something else), we just have more to code to port (and more possibilities to make mistakes).

Additional changes include:

  • functions get-message-content-by-id and get-last-outgoing of the message data-store ns removed, because they are not used anymore
  • status-im.chat.views.input.suggestions sanitised (proper prefixes, no referring, etc), this was done because I had to touch the ns anyway because of the get-message-content-by-id removal

Steps to test:

  • Test behavior of message send/delivery statuses in all 3 types of chat
  • Test that after upgrade, nothing is borked :)

status: ready

Copy link
Contributor

@jeluard jeluard left a comment

Choose a reason for hiding this comment

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

Please motivate this refactoring and add unit tests.

message (get-stored-message message-identifier)]
;; proceed with updating status if chat is active, message is in realm and existing
;; status is not already "final" = `:seen` by the user sending the update
(when (and chats-is-active? message (not= :seen (get message :user-statuses from-id)))
Copy link
Contributor

Choose a reason for hiding this comment

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

@janherich can you extract message (not= :seen (get message :user-statuses from-id)) so that in result condition will be

(when (and chats-is-active? (not (seen? message from-id))) ..

@janherich janherich force-pushed the feature/refactored-message-statuses-#1875 branch from 87522e6 to 7172523 Compare December 25, 2017 00:15
@janherich
Copy link
Contributor Author

@rasom done

@janherich janherich force-pushed the feature/refactored-message-statuses-#1875 branch from 7172523 to 6ad62d7 Compare December 25, 2017 01:47
@janherich janherich force-pushed the feature/refactored-message-statuses-#1875 branch from 6ad62d7 to 9df640e Compare December 25, 2017 22:47
@janherich janherich force-pushed the feature/refactored-message-statuses-#1875 branch from 9df640e to 3111c63 Compare December 28, 2017 12:19
@Serhy Serhy self-assigned this Dec 28, 2017
@janherich janherich force-pushed the feature/refactored-message-statuses-#1875 branch from 3111c63 to 8553f43 Compare December 29, 2017 09:43
@Serhy
Copy link
Contributor

Serhy commented Dec 29, 2017

The first issue noticed:

In Group chat when contact sends any of /location /request /send and another contact seen the messages - it does not indicated on sender side that someone seen any of three. For text messages/emojii all is fine and it’s indicated on sender side that chat participant seen the message.

To reproduce:

  1. Open Status on Device1
  2. Open Status on Device2
  3. Create UserA on Device1
  4. Create UserB on Device2
  5. UserA adds UserB in contacts.
  6. UserA create a Group chat: (I used Jarrad and UserB)
  7. UserB navigates to created group chat screen
  8. UserA sends text/emojii in Group chat. UserB seen the message and on UserA screen small avatar of UserB visible below sent message. Good!
  9. UserA sends /location, /request, /send messages in Group Chat. UserB seen the message but on UserA screen there is no indication (no small avatar of UserB) visible below sent message.

TF session representing UserA: https://app.testfairy.com/projects/4803590-status/builds/7396791/sessions/7/?accessToken=wKYxYVkg-xGR9bU/zZg4BkV0EE8 (01:30-03:20)
TF session representing UserB: https://app.testfairy.com/projects/4803622-status/builds/7392267/sessions/38/?accessToken=k0bBG9afSvTdkUo-HjDh7nnccqU (02:30-4:20)

Android 7.0 (Galaxy J7 real device, Status version 0.9.10-494-g3111c630) and iOS (iPhone X, Status version 0.9.11d494)

@Serhy
Copy link
Contributor

Serhy commented Dec 29, 2017

Second issue noticed:
When I delete 1-1 chat(s) with previously read messages I had with another contacts (so no Unread message counter displayed on Chat screen), after I log out and log back in - all deleted chats reappear and display all messages I had with the contacts that were deleted, like they were Unread.

https://app.testfairy.com/projects/4803622-status/builds/7392267/sessions/24/?accessToken=IFcF1Epo0dBaEz2royLWr5qBAS4 - from 02:23

Steps to reproduce:

  1. Open Status on Device1
  2. Open Status on Device2
  3. Create UserA on Device1
  4. Create UserB on Device2
  5. UserA adds UserB in contacts.
  6. UserA sends message to UserB
  7. UserB navigate to 1-1 Chat with UserA to read message.
  8. UserB Navigate back to Chats list. Confirm no unread messages counter present.
  9. UserB deletes the chat with UserA
    10 UserB logged out
  10. UserB logs back in and stays on Chats list.

Current result: UserA appeared in Chats list of UserB with the counter of unread messages (1 unread message following above steps).
Expected result: No deleted chats appear (with all messages in Unread state) if to log out and log back in.

@janherich janherich force-pushed the feature/refactored-message-statuses-#1875 branch from 8553f43 to 06a294a Compare December 29, 2017 23:23
@janherich
Copy link
Contributor Author

Hi @Serhy, the first issue is already fixed, regarding the second one, are you sure it's a regression and it's not happening on develop as well ?
I was able to simulate it (although not very consistently), but it seems to me that it's not really related to this PR.

@oskarth
Copy link
Contributor

oskarth commented Jan 2, 2018

Moved issue to what seems like right place. Small housekeeping requests:

@Serhy Can you please move issue to "PENDING CONTRIBUTOR" (Jan) in future?

@janherich Can you please move to "QA TODO" if you request action from QA in future?

@Serhy
Copy link
Contributor

Serhy commented Jan 3, 2018

The send/deliver message status looks good in this PR. #2659 fixed as well.

However, we are failing on account log in attempt after upgrade, if there was a group chat created in release version.

It doesn't happen if only 1-1 chat existed in release version prior upgrade.
Bug is not reproduced in latest develop (note: issue #2602 reproduced as well for this PR, but it was present before in develop).

Both iOS and Android affected.

Upgrade from release (0.9.12b) made on 0.9.10-498-g06a294a9+ version

TF session with logs and video after upgrade: https://app.testfairy.com/projects/4803622-status/builds/7422116/sessions/25/?accessToken=5900J3aUCqRSfUZgmSynm42W99M

Steps to reproduce:

  1. Install Status release version (from play store or testflight for iOS)
  2. Create an account (i've used password 'aaaaaa' for my case)
  3. Add new contact (scan QR code or paste public key)
  4. Create Group chat with Jarrad and newly added contact (I named group chat as 'qwerty')
  5. Send any text message in newly created group chat
  6. Upgrade to latest build of this branch
  7. Log in with account created in release version

Actual result: Primary key property 'user-status.message-id' has duplicate values after migration. error appears, user not logged in.
Expected result: User logged in, all chats used in release version are present.

@janherich janherich force-pushed the feature/refactored-message-statuses-#1875 branch from 06a294a to 6594bba Compare January 3, 2018 16:12
@janherich
Copy link
Contributor Author

Hi @Serhy, I fixed the migration issue mentioned.

@Serhy
Copy link
Contributor

Serhy commented Jan 4, 2018

These all looks good: tested on Samsung J7 and iPhone X.
All above bugs also fixed!
Thanks, @janherich !

@janherich janherich force-pushed the feature/refactored-message-statuses-#1875 branch from 6594bba to 52ddccc Compare January 4, 2018 12:49
@janherich janherich merged commit 52ddccc into develop Jan 4, 2018
@janherich janherich deleted the feature/refactored-message-statuses-#1875 branch January 4, 2018 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unviewed count notification is not persisted between app restarts
6 participants