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.

Sign up for GitHub

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

Jump to bottom

Allow to fetch more history in public chats (up to 30 days) #8033

Merged
merged 1 commit into from Apr 26, 2019
Merged

Allow to fetch more history in public chats (up to 30 days) #8033

merged 1 commit into from Apr 26, 2019

Conversation

rasom
Copy link
Member

@rasom rasom commented Apr 24, 2019 •
edited by StatusWrike

Please review only the last commit, merge after #8025

status: ready

@status-github-bot status-github-bot bot added this to REVIEW in Pipeline for QA Apr 24, 2019
@rasom rasom self-assigned this Apr 24, 2019
@rasom rasom changed the title Allow to fetch more history in chats (up to 30 days) [WIP] Allow to fetch more history in chats (up to 30 days) Apr 24, 2019
@status-im-auto
Copy link
Member

status-im-auto commented Apr 24, 2019 •
edited

Jenkins Builds

Click to see older builds (29)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
❌ 412b162 #2 2019-04-24 19:53:57 ~12 min ios 📄 log
✔️ 412b162 #2 2019-04-24 20:05:54 ~24 min macos 📦 dmg
✔️ 412b162 #2 2019-04-24 20:09:24 ~28 min android 📦 apk
✔️ 412b162 #2 2019-04-24 20:18:39 ~37 min linux 📦 App
✔️ 412b162 #2 2019-04-24 20:19:56 ~38 min windows 📦 exe
✔️ 057e803 #3 2019-04-25 06:09:19 ~16 min macos 📦 dmg
✔️ 057e803 #3 2019-04-25 06:14:58 ~22 min linux 📦 App
✔️ 057e803 #3 2019-04-25 06:16:12 ~23 min android 📦 apk
✔️ 057e803 #3 2019-04-25 06:18:23 ~25 min android-e2e 📦 apk
✔️ 057e803 #3 2019-04-25 06:19:22 ~26 min ios 📦 ipa
✔️ 057e803 #3 2019-04-25 06:19:29 ~26 min windows 📦 exe
✔️ e1b686c #5 2019-04-25 08:58:49 ~14 min macos 📦 dmg
✔️ e1b686c #5 2019-04-25 09:00:28 ~15 min linux 📦 App
✔️ e1b686c #5 2019-04-25 09:00:36 ~16 min android-e2e 📦 apk
✔️ e1b686c #5 2019-04-25 09:01:00 ~16 min android 📦 apk
✔️ e1b686c #5 2019-04-25 09:08:44 ~24 min ios 📦 ipa
✔️ e1b686c #5 2019-04-25 09:09:30 ~24 min windows 📦 exe
✔️ 3e107e6 #6 2019-04-25 18:37:53 ~15 min macos 📦 dmg
✔️ 3e107e6 #6 2019-04-25 18:38:22 ~15 min linux 📦 App
✔️ 3e107e6 #6 2019-04-25 18:38:56 ~16 min android-e2e 📦 apk
✔️ 3e107e6 #6 2019-04-25 18:39:02 ~16 min android 📦 apk
✔️ 3e107e6 #6 2019-04-25 18:45:06 ~22 min windows 📦 exe
✔️ 3e107e6 #6 2019-04-25 18:46:13 ~23 min ios 📦 ipa
✔️ 23cf56a #7 2019-04-26 06:35:21 ~15 min macos 📦 dmg
✔️ 23cf56a #7 2019-04-26 06:35:49 ~15 min linux 📦 App
✔️ 23cf56a #7 2019-04-26 06:36:11 ~16 min android-e2e 📦 apk
✔️ 23cf56a #7 2019-04-26 06:36:50 ~16 min android 📦 apk
✔️ 23cf56a #7 2019-04-26 06:38:17 ~18 min windows 📦 exe
✔️ 23cf56a #7 2019-04-26 06:46:42 ~26 min ios 📦 ipa
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c7c84a8 #8 2019-04-26 09:12:28 ~15 min macos 📦 dmg
✔️ c7c84a8 #8 2019-04-26 09:13:01 ~15 min linux 📦 App
✔️ c7c84a8 #8 2019-04-26 09:13:22 ~16 min android-e2e 📦 apk
✔️ c7c84a8 #8 2019-04-26 09:13:22 ~16 min android 📦 apk
✔️ c7c84a8 #8 2019-04-26 09:14:32 ~17 min windows 📦 exe
✔️ c7c84a8 #8 2019-04-26 09:20:15 ~23 min ios 📦 ipa
✔️ 9b9eefb #9 2019-04-26 13:09:13 ~15 min macos 📦 dmg
✔️ 9b9eefb #9 2019-04-26 13:15:24 ~21 min linux 📦 App
✔️ 9b9eefb #9 2019-04-26 13:15:55 ~22 min android 📦 apk
✔️ 9b9eefb #9 2019-04-26 13:19:02 ~25 min windows 📦 exe
✔️ 9b9eefb #9 2019-04-26 13:20:21 ~26 min ios 📦 ipa

@rasom rasom force-pushed the load-more-messages branch 2 times, most recently from 057e803 to 834022d Compare April 25, 2019 08:26
@rasom rasom changed the title [WIP] Allow to fetch more history in chats (up to 30 days) [WIP] Allow to fetch more history in public chats (up to 30 days) Apr 25, 2019
@rasom rasom changed the title [WIP] Allow to fetch more history in public chats (up to 30 days) Allow to fetch more history in public chats (up to 30 days) Apr 25, 2019
@status-github-bot status-github-bot bot moved this from REVIEW to CONTRIBUTOR in Pipeline for QA Apr 25, 2019
hesterbruikman
hesterbruikman approved these changes Apr 25, 2019
View reviewed changes
Copy link
Contributor

@hesterbruikman hesterbruikman left a comment

Choose a reason for hiding this comment

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

  • I feel like I have my memory back being able to go into history! Great work!
  • I'll create that issue to polish and animate new messages being loaded.
  • I believe we decided to keep 'Fetch more messages' rather than load to maintain consistency. But I prefer load as it's more conventional. I'll check other places we need to update to Load for consistency.

@rasom
Copy link
Member Author

rasom commented Apr 25, 2019

@hesterbruikman it is still "Fetch more messages" for gaps which appear between history requests. I just wanted to emphasize that this one is rather about loading/fetching of older messages than about fetching of missed messages. It looks and works similarly but that's two different cases imo. Let me know if it should be changed.

@hesterbruikman
Copy link
Contributor

Thanks @rasom. Let's stick with "Fetch more messages" for both, for now. It is the most accurate capture of what happens. The more I think about it 'Load' might set the wrong expectations.

In time, a more suitable solution for older messages would be to fetch on scroll, which I realize comes with it's own challenges. It would however require less effort from the user to mentally process as it's a response to existing behavior rather than demand for something extra ('tap this button'). Cc @ERRORIST

@status-github-bot status-github-bot bot moved this from CONTRIBUTOR to TO TEST in Pipeline for QA Apr 25, 2019
@rasom
Copy link
Member Author

rasom commented Apr 25, 2019

a more suitable solution for older messages would be to fetch on scroll,

I would add fetching by scroll separately and we will still need to have current ui, for the case when fetching history is disabled by default on mobile connection. Because this fetching comes with some traffic cost

@yenda
Copy link
Contributor

yenda commented Apr 25, 2019

This features relies on mailservers being able to store 30 days of all whisper traffic doesn't it? How much is it right now? How does it grows with increased user base?

@cammellos
Copy link
Member

@yenda it's 30 days currently I believe, I don't know how much it grows with current user base as I don't know how large it is, but at the moment 30 days worth of data are around 2.2/2.5 GB

@jakubgs
Copy link
Member

jakubgs commented Apr 25, 2019

Correct, around 2.6 GB:

 $ ansible mail -o -a 'du -hs /docker/statusd-mail/data/wnode'
mail-03.do-ams3.eth.beta | CHANGED | rc=0 | (stdout) 2.6G	/docker/statusd-mail/data/wnode
mail-02.do-ams3.eth.beta | CHANGED | rc=0 | (stdout) 2.4G	/docker/statusd-mail/data/wnode
mail-01.do-ams3.eth.beta | CHANGED | rc=0 | (stdout) 2.6G	/docker/statusd-mail/data/wnode
mail-02.gc-us-central1-a.eth.beta | CHANGED | rc=0 | (stdout) 2.6G	/docker/statusd-mail/data/wnode
mail-03.ac-cn-hongkong-c.eth.beta | CHANGED | rc=0 | (stdout) 2.6G	/docker/statusd-mail/data/wnode
mail-02.ac-cn-hongkong-c.eth.beta | CHANGED | rc=0 | (stdout) 2.3G	/docker/statusd-mail/data/wnode
mail-01.gc-us-central1-a.eth.beta | CHANGED | rc=0 | (stdout) 2.6G	/docker/statusd-mail/data/wnode
mail-01.ac-cn-hongkong-c.eth.beta | CHANGED | rc=0 | (stdout) 2.4G	/docker/statusd-mail/data/wnode
mail-03.gc-us-central1-a.eth.beta | CHANGED | rc=0 | (stdout) 2.6G	/docker/statusd-mail/data/wnode

And it is indeed 30 days:
https://github.com/status-im/infra-eth-cluster/blob/c6a847fefa7267a731ac1e0eee638fdad2c6be00/ansible/roles/statusd-mailsrv/defaults/main.yml#L57

But considering the amount of space left we could easily double it. Though of course the more users we get the bigger it will grow over time.

@churik churik moved this from TO TEST to IN TESTING in Pipeline for QA Apr 26, 2019
@rasom rasom mentioned this pull request Apr 26, 2019
Merged
@churik churik self-assigned this Apr 26, 2019
@churik
Copy link
Member

churik commented Apr 26, 2019

@rasom thanks for additional "Fetch 48-60h", "Fetch 84-96h", you can remove it now.

  1. Just FYI: You can send message to the past if you set custom date #6946 and Timestamps for messages doesn't match real time if user send message from device with custom time #6419 are very visible in this PR - because if you have a message out of order after fetching the previous 24h you will be redirected to last message according to chat order.
    I would prefer to see then fixed if it is possible along with this PR or at least put high-priority after this PR.
    What do you think?
    Video (example with wrong timestamps): https://take.ms/zggUk

  2. On month history also more visible problems with scrolling, i.e. :

  • 2.1 blinking of the message that starts history when scrolling up
    ezgif com-crop

  • 2.2 that messages are not rendering quickly enough when scrolling fast
    ezgif com-video-to-gif (1)

  • 2.3 that there is no way to be scrolled fast to the last message from the middle of the chat.

I know, that it is not about this particular PR right now, just want to emphasize that these problems will be more visible if we decide to merge this PR now.

Tested on IOS and Android:

  • history can be fetched during the last month, day by day on ams3-do and mail-03-hongkong mailservers. Didn't notice any spikes.
  • history matches history on desktop (checked for several days)
  • "Fetch messages" (gaps in history) and feature from PR are working fine together
  • can't fetch messages when offline
  • messages fetching normally while offline->online during fetching
  • the feature is available only for public chats.
  • the desktop build is not affected (checked Mac OSx)

@rachelhamlin @annadanchenko let me know if you think we should include this PR in 0.12.0.
IMO we can (but taking into account issues 1 and 2)

@rasom
Copy link
Member Author

rasom commented Apr 26, 2019 •
edited

I know, that it is not about this particular PR right now, just want to emphasize that these problems will be more visible if we decide to merge this PR now.

both problems are out of the scope of this PR. That behavior on video doesn't seem to be a problem imo. Regarding slow scroll - it's a problem for anyone who used app regularly for more than month. I would say that ability to fetch history gives more advantages than slow scrolling disadvantages (assuming that's only a revealing of an existing issue).

@statustestbot
Copy link

98% of end-end tests have passed

Total executed tests: 48
Failed tests: 1
Passed tests: 47

Failed tests (1)

Click to expand
1. test_backup_recovery_phrase_warning_from_wallet

Device 1: Wait for PlusButton
Device 1: Wait for PlusButton

Donation was not received during 300 seconds!

Device sessions

Passed tests (47)

Click to expand
1. test_block_user_from_public_chat
Device sessions

2. test_filters_from_daap
Device sessions

3. test_copy_and_paste_messages
Device sessions

4. test_send_transaction_from_daap
Device sessions

5. test_request_and_receive_tokens_in_1_1_chat
Device sessions

6. test_deploy_contract_from_daap
Device sessions

7. test_public_chat_messaging
Device sessions

8. test_password_in_logcat_sign_in
Device sessions

9. test_text_message_1_1_chat
Device sessions

10. test_add_to_contacts
Device sessions

11. test_sign_typed_message (TestRail link is not found)
Device sessions

12. test_unread_messages_counter_1_1_chat
Device sessions

13. test_logcat_send_transaction_from_daap
Device sessions

14. test_send_message_in_group_chat
Device sessions

15. test_logcat_send_transaction_from_wallet
Device sessions

16. test_send_token_with_7_decimals
Device sessions

17. test_modify_transaction_fee_values
Device sessions

18. test_send_eth_from_wallet_to_address
Device sessions

19. test_manage_assets
Device sessions

20. test_logcat_send_transaction_in_1_1_chat
Device sessions

21. test_request_and_receive_eth_in_1_1_chat
Device sessions

22. test_swipe_to_delete_public_chat
Device sessions

23. test_send_emoji
Device sessions

24. test_search_chat_on_home
Device sessions

25. test_logcat_recovering_account
Device sessions

26. test_messaging_in_different_networks
Device sessions

27. test_send_tokens_in_1_1_chat
Device sessions

28. test_network_mismatch_for_send_request_commands
Device sessions

29. test_logcat_sign_message_from_daap
Device sessions

30. test_swipe_to_delete_1_1_chat
Device sessions

31. test_switch_users_and_add_new_account
Device sessions

32. test_send_stt_from_wallet
Device sessions

33. test_send_eth_in_1_1_chat
Device sessions

34. test_login_with_new_account
Device sessions

35. test_send_eth_from_wallet_to_contact
Device sessions

36. test_add_contact_from_public_chat
Device sessions

37. test_send_two_transactions_one_after_another_in_dapp
Device sessions

38. test_password_in_logcat_creating_account
Device sessions

39. test_backup_recovery_phrase
Device sessions

40. test_offline_status
Device sessions

41. test_open_google_com_via_open_dapp
Device sessions

42. test_unread_messages_counter_public_chat
Device sessions

43. test_sign_message_from_daap
Device sessions

44. test_user_can_remove_profile_picture
Device sessions

45. test_share_contact_code_and_wallet_address
Device sessions

46. test_request_eth_in_wallet
Device sessions

47. test_refresh_button_browsing_app_webview
Device sessions

@weekly-digest weekly-digest bot mentioned this pull request Apr 26, 2019
Closed
@mandrigin
Copy link
Contributor

I would say that ability to fetch history gives more advantages than slow scrolling disadvantages

I agree with that too

@churik churik moved this from IN TESTING to MERGE in Pipeline for QA Apr 26, 2019
@rachelhamlin
Copy link
Contributor

rachelhamlin commented Apr 26, 2019 •
edited

I don't think the time stamp issues (#6946, #6419) or slow scroll should block this PR—user has to manually change or have custom time on their device? Sounds far out. But whether to include this PR in the current release is test team's decision. Very nice to have, imo, not a need. Nice work @rasom.

@rasom rasom merged commit 9b9eefb into develop Apr 26, 2019
Pipeline for QA automation moved this from MERGE to DONE Apr 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the load-more-messages branch April 26, 2019 12:54
@status-github-bot
Copy link

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goranjovic goranjovic goranjovic approved these changes

@hesterbruikman hesterbruikman hesterbruikman approved these changes

@cammellos cammellos Awaiting requested review from cammellos

@flexsurfer flexsurfer Awaiting requested review from flexsurfer

@yenda yenda Awaiting requested review from yenda

@mandrigin mandrigin Awaiting requested review from mandrigin

Assignees

@churik churik

Labels
Tested - OK
Projects
Pipeline for QA
  
DONE
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet