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

Add ability to Share image from Status #12528

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Add ability to Share image from Status #12528

merged 1 commit into from
Sep 22, 2021

Conversation

briansztamfater
Copy link
Member

fixes #12472

Summary

This PR adds the ability to share an image to other apps from the image preview in Status.

Screenshots

Platforms

  • Android
  • iOS
Functional
  • 1-1 chats
  • public chats
  • group chats

Steps to test

  • Open Status
  • Send image from user A to user B
  • Open image preview
  • Verify share functionality and UI changes

status: ready

@briansztamfater briansztamfater self-assigned this Sep 1, 2021
@status-github-bot status-github-bot bot added this to REVIEW in Pipeline for QA Sep 1, 2021
@status-im-auto
Copy link
Member

status-im-auto commented Sep 1, 2021 •
edited

Jenkins Builds

Click to see older builds (21)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 425868f #1 2021-09-01 04:20:39 ~14 min ios 📦ipa 📲
✖️ 425868f #1 2021-09-01 04:27:02 ~21 min android-e2e 📦apk 📲
✖️ 425868f #1 2021-09-01 04:27:10 ~21 min android 📦apk 📲
✖️ 6135e72 #2 2021-09-01 12:02:51 ~15 min android 📦apk 📲
✖️ 6135e72 #2 2021-09-01 12:03:00 ~15 min ios 📦ipa 📲
✖️ 6135e72 #2 2021-09-01 12:03:23 ~16 min android-e2e 📦apk 📲
✖️ 56bb257 #3 2021-09-20 16:38:34 ~17 min android 📦apk 📲
✖️ 56bb257 #3 2021-09-20 16:38:43 ~17 min android-e2e 📦apk 📲
✖️ 56bb257 #3 2021-09-20 16:40:58 ~19 min ios 📦ipa 📲
✖️ 70904ea #4 2021-09-20 17:33:04 ~16 min ios 📦ipa 📲
✖️ 70904ea #4 2021-09-20 17:33:58 ~17 min android 📦apk 📲
✖️ 70904ea #4 2021-09-20 17:34:06 ~17 min android-e2e 📦apk 📲
✔️ c1e6b12 #5 2021-09-21 00:29:35 ~13 min ios 📦ipa 📲
✔️ c1e6b12 #5 2021-09-21 00:32:38 ~16 min android-e2e 📦apk 📲
✔️ c1e6b12 #5 2021-09-21 00:32:51 ~16 min android 📦apk 📲
✔️ 96268b1 #6 2021-09-21 02:59:44 ~14 min ios 📦ipa 📲
✔️ 96268b1 #6 2021-09-21 03:01:43 ~16 min android-e2e 📦apk 📲
✔️ 96268b1 #6 2021-09-21 03:01:48 ~16 min android 📦apk 📲
✔️ f3a6a00 #8 2021-09-21 15:34:50 ~17 min android 📦apk 📲
✔️ f3a6a00 #8 2021-09-21 15:34:54 ~17 min android-e2e 📦apk 📲
✔️ f3a6a00 #8 2021-09-21 15:38:34 ~21 min ios 📦ipa 📲
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 8e5d302 #7 2021-09-21 15:38:36 ~22 min ios 📦ipa 📲
✔️ fff813d #9 2021-09-21 15:41:42 ~21 min android 📦apk 📲
✔️ fff813d #9 2021-09-21 15:42:09 ~22 min ios 📦ipa 📲
✔️ fff813d #9 2021-09-21 15:42:49 ~22 min android-e2e 📦apk 📲

flexsurfer
flexsurfer reviewed Sep 1, 2021
View reviewed changes
src/status_im/utils/share.cljs Outdated
(:require ["react-native-share" :default react-native-share]))

(defn open [options on-success on-error]
(println react-native-share)
Copy link
Member

Choose a reason for hiding this comment

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

leftover

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

@flexsurfer
Copy link
Member

just wondering why two icons ?

@shivekkhurana
Copy link
Contributor

This is a timely PR. We need this feature for sharing NFTs too !!

@briansztamfater
Copy link
Member Author

just wondering why two icons ?

One is for download the image (that was already implemented, but the UI was changed) and the other is for sharing it.

@flexsurfer
Copy link
Member

just wondering why two icons ?

One is for download the image (that was already implemented, but the UI was changed) and the other is for sharing it.

but we now have save option in share ,so we don't need download icon anymore ?

@flexsurfer flexsurfer moved this from REVIEW to E2E Tests in Pipeline for QA Sep 1, 2021
@briansztamfater
Copy link
Member Author

just wondering why two icons ?

One is for download the image (that was already implemented, but the UI was changed) and the other is for sharing it.

but we now have save option in share ,so we don't need download icon anymore ?

Yeah it is valid concern, @hesterbruikman any thoughts on this?

@qoqobolo qoqobolo moved this from E2E Tests to IN TESTING in Pipeline for QA Sep 3, 2021
@qoqobolo qoqobolo self-assigned this Sep 3, 2021
@qoqobolo
Copy link
Contributor

qoqobolo commented Sep 6, 2021

@briansztamfater thanks for adding this! Works well except for these two issues.

ISSUE 1: Mismatch with design for vertical / zoomed images

Screenshot 2021-09-06 at 12 47 10

Figma:

Screenshot 2021-09-06 at 12 48 04

ISSUE 2: Can't share an image to Telegram / Mail on iOS

Get endless spinner when trying to share an image to Telegram and don't receive an email with shared image when sharing to Mail.
However, everything is fine with other apps, e.g. Facebook, Instagram, etc, so not sure, that this is a Status app issue.
Could you take a look, please?

Screenshot 2021-09-06 at 12 52 54

@briansztamfater
Copy link
Member Author

Thanks @qoqobolo, I'll take a look at those issues and come back to you

@qoqobolo qoqobolo moved this from IN TESTING to CONTRIBUTOR in Pipeline for QA Sep 9, 2021
@briansztamfater
Copy link
Member Author

@qoqobolo Both issues should be fixed now!

@qoqobolo qoqobolo moved this from CONTRIBUTOR to E2E Tests in Pipeline for QA Sep 21, 2021
@status-im-auto
Copy link
Member

79% of end-end tests have passed

Total executed tests: 68
Failed tests: 14
Passed tests: 54

IDs of failed tests: 6305,6244,5314,5412,5350,5758,6645,6272,5309,5323,6263,6249,6240,6237

Failed tests (14)

Click to expand
  • Rerun tests

    • 1. test_image_in_one_to_one_send_save_reply_timeline, id: 6305

    Device 2: *Find Button by xpath:* `//*[@text="Hey hey hey"]`
    Device 2: *Find Button by xpath:* `//*[@text="Hey hey hey"]`

    Device 2: Button by xpath:* `//*[@text="Hey hey hey"]` is not found on the screen

    Device sessions

    2. test_add_and_delete_watch_only_account_to_multiaccount_instance, id: 6244

    Device 1: *Swiping up*
    Device 1: *Waiting 400 seconds for ADI to display asset*

    Device 1: Balance ADI 0 is not changed during 400 seconds!

    Device sessions

    3. test_can_see_balance_and_all_transactions_history_on_cellular, id: 5314

    Device 1: *Swiping up*
    Device 1: *Waiting 400 seconds for STT to display asset*

    Device 1: Balance STT 0 is not changed during 400 seconds!

    Device sessions

    4. test_insufficient_funds_wallet_positive_balance, id: 5412

    Device 1: *Swiping up*
    Device 1: *Waiting 400 seconds for STT to display asset*

    Device 1: Balance STT 0 is not changed during 400 seconds!

    Device sessions

    5. test_send_token_with_7_decimals, id: 5350

    Device 1: *Swiping up*
    Device 1: *Waiting 400 seconds for ADI to display asset*

    Device 1: Balance ADI 0 is not changed during 400 seconds!

    Device sessions

    6. test_keycard_can_recover_keycard_account_card_pairing, id: 5758

    Device 1: *Getting LXS amount*
    Device 1: *Scrolling down to SilentButton*

    Device 1: SilentButton by xpath:* `//android.view.ViewGroup[@content-desc=':LXS-asset-value']//android.widget.TextView[1]` is not found on the screen

    Device sessions

    7. test_restore_account_migrate_multiaccount_to_keycard, id: 6645

    Device 1: *Getting ADI amount*
    Device 1: *Scrolling down to SilentButton*

    Device 1: SilentButton by xpath:* `//android.view.ViewGroup[@content-desc=':ADI-asset-value']//android.widget.TextView[1]` is not found on the screen

    Device sessions

    8. test_add_account_to_wallet_private_key_and_seed_phrase, id: 6272

    Device 1: *Swiping up*
    Device 1: *Waiting 400 seconds for ADI to display asset*

    Device 1: Balance ADI 0 is not changed during 400 seconds!

    Device sessions

    9. test_request_stt_from_daap, id: 5309

    Device 1: *Getting STT amount*
    Device 1: *Scrolling down to SilentButton*

    Device 1: SilentButton by xpath:* `//android.view.ViewGroup[@content-desc=':STT-asset-value']//android.widget.TextView[1]` is not found on the screen

    Device sessions

    10. test_share_copy_contact_code_and_wallet_address, id: 5323

    Device 1: *Tap on found Button*
    Device 1: **Sharing via messenger**

    Device 1: Button by xpath: `//*[contains(@text, "Direct share")]` is not found on the screen

    Device sessions

    11. test_request_and_receive_stt_in_1_1_chat_offline, id: 6263

    Device 2: *Getting STT amount*
    Device 2: *Scrolling down to SilentButton*

    Device 2: SilentButton by xpath:* `//android.view.ViewGroup[@content-desc=':STT-asset-value']//android.widget.TextView[1]` is not found on the screen

    Device sessions

    12. test_keycard_request_stt_from_daap, id: 6249

    Device 1: *Getting STT amount*
    Device 1: *Scrolling down to SilentButton*

    Device 1: SilentButton by xpath:* `//android.view.ViewGroup[@content-desc=':STT-asset-value']//android.widget.TextView[1]` is not found on the screen

    Device sessions

    13. test_restore_account_from_mnemonic_to_keycard, id: 6240

    Device 1: *Getting ADI amount*
    Device 1: *Scrolling down to SilentButton*

    Device 1: SilentButton by xpath:* `//android.view.ViewGroup[@content-desc=':ADI-asset-value']//android.widget.TextView[1]` is not found on the screen

    Device sessions

    14. test_fetching_balance_after_offline, id: 6237

    Device 1: *Swiping up*
    Device 1: *Waiting 400 seconds for STT to display asset*

    Device 1: Balance STT 0 is not changed during 400 seconds!

    Device sessions

    Passed tests (54)

    Click to expand

    1. test_add_account_to_multiaccount_instance_generate_new, id: 6224
    Device sessions

    2. test_can_add_existing_ens_on_mainnet, id: 5502
    Device sessions

    3. test_keycard_can_see_all_transactions_in_history, id: 6291
    Device sessions

    4. test_sign_message_and_2tx_in_batch_and_transactions_filters_from_daap, id: 5342
    Device sessions

    5. test_open_blocked_secure_not_secure_inlalid_offline_urls, id: 6210
    Device sessions

    6. test_open_public_chat_using_deep_link, id: 5396
    Device sessions

    7. test_offline_add_new_group_chat_member, id: 3998
    Device sessions

    8. test_send_non_english_message_to_newly_added_contact_on_different_networks, id: 5315
    Device sessions

    9. test_delete_close_all_tabs, id: 5390
    Device sessions

    10. test_create_new_group_chat_messaging_pn_delivered, id: 3994
    Device sessions

    11. test_delete_chats_via_delete_button_rejoin, id: 5387
    Device sessions

    12. test_send_transaction_set_recipient_options, id: 6328
    Device sessions

    13. test_keycard_sign_message_and_transactions_from_daap, id: 6251
    Device sessions

    14. test_recover_account_from_new_user_seedphrase, id: 6296
    Device sessions

    15. test_block_user_from_public_chat, id: 5786
    Device sessions

    16. test_send_audio_message_with_push_notification_check, id: 6316
    Device sessions

    17. test_redirect_to_public_chat_tapping_tag_message_fetch_more_history, id: 5675
    Device sessions

    18. test_edit_delete_message_in_one_to_one_and_public_chats, id: 695843
    Device sessions

    19. test_ens_mentions_pn_and_nickname_in_public_and_1_1_chats, id: 6226
    Device sessions

    20. test_keycard_send_eth_from_wallet_to_address, id: 6289
    Device sessions

    21. test_keycard_create_login_restore_unlock_same_seed, id: 5689
    Device sessions

    22. test_offline_is_shown_messaging_1_1_chat_sent_delivered, id: 5310
    Device sessions

    23. test_pair_devices_sync_one_to_one_contacts_nicknames_public_chat, id: 5762
    Device sessions

    24. test_long_press_to_delete_chat, id: 5319
    Device sessions

    25. test_copy_and_paste_messages, id: 5317
    Device sessions

    26. test_open_transaction_on_etherscan_copy_tx_hash, id: 5384
    Device sessions

    27. test_unread_messages_counter_public_chat, id: 5360
    Device sessions

    28. test_send_eth_from_wallet_to_address_incorrect_password, id: 5308
    Device sessions

    29. test_mobile_data_usage_complex_settings, id: 6228
    Device sessions

    30. test_start_chat_with_ens_mention_in_one_to_one, id: 5403
    Device sessions

    31. test_decline_transactions_in_1_1_chat_push_notification_changing_state, id: 6265
    Device sessions

    32. test_send_eth_in_1_1_chat_transaction_push, id: 6253
    Device sessions

    33. test_install_pack_and_send_sticker, id: 5782
    Device sessions

    34. test_switch_users_special_char_password_and_add_new_account_logcat, id: 5356
    Device sessions

    35. test_manage_assets, id: 5341
    Device sessions

    36. test_browser_managing_bookmarks, id: 6633
    Device sessions

    37. test_open_chat_by_pasting_chat_key_check_invalid_chat_key_cases, id: 5304
    Device sessions

    38. test_can_use_purchased_stickers_on_recovered_account, id: 5783
    Device sessions

    39. test_dapps_permissions, id: 5738
    Device sessions

    40. test_account_recovery_with_uppercase_whitespaces_seed_phrase_special_char_passw_logcat, id: 5394
    Device sessions

    41. test_send_eth_to_ens_in_chat, id: 6279
    Device sessions

    42. test_wallet_set_up, id: 5335
    Device sessions

    43. test_send_transaction_with_custom_token, id: 6208
    Device sessions

    44. test_home_view, id: 5379
    Device sessions

    45. test_logcat_backup_recovery_phrase, id: 5419
    Device sessions

    46. test_set_profile_picture, id: 6646
    Device sessions

    47. test_collectible_from_wallet, id: 5346
    Device sessions

    48. test_add_and_remove_mention_contact_with_nickname_from_public_chat, id: 5332
    Device sessions

    49. test_pass_phrase_validation, id: 5363
    Device sessions

    50. test_keycard_send_eth_in_1_1_chat, id: 6293
    Device sessions

    51. test_send_and_open_links_with_previews, id: 5373
    Device sessions

    52. test_reactions_to_message_in_chats, id: 6315
    Device sessions

    53. test_push_notification_1_1_chat_no_pn_activity_center, id: 6283
    Device sessions

    54. test_public_chat_messaging_emojis_timestamps, id: 5313
    Device sessions

    @qoqobolo qoqobolo moved this from E2E Tests to MERGE in Pipeline for QA Sep 21, 2021
    @qoqobolo qoqobolo moved this from MERGE to IN TESTING in Pipeline for QA Sep 21, 2021
    @churik
    Copy link
    Member

    churik commented Sep 21, 2021 •
    edited

    Can you please add accessibility-ids for new elements?
    Required for e2e.
    Thanks.
    @briansztamfater

    @briansztamfater briansztamfater force-pushed the feat/share-image-improvement branch 2 times, most recently from f3a6a00 to fff813d Compare September 21, 2021 15:19
    @briansztamfater
    Copy link
    Member Author

    Can you please add accessibility-ids for new elements?
    Required for e2e.
    Thanks.
    @briansztamfater

    @qoqobolo Added accessibility-ids to all close, save and share buttons. Let me know if that's ok and I can merge then :)
    Thanks!

    @qoqobolo
    Copy link
    Contributor

    @briansztamfater thanks a lot!
    Yep, PR is ready to be merged :)

    @qoqobolo qoqobolo moved this from IN TESTING to MERGE in Pipeline for QA Sep 22, 2021
    @briansztamfater
    Add ability to Share image from Status
    68ffddb 
    Signed-off-by: Brian Sztamfater <brian@status.im>
    @briansztamfater briansztamfater merged commit 68ffddb into develop Sep 22, 2021
    Pipeline for QA automation moved this from MERGE to DONE Sep 22, 2021
    @briansztamfater briansztamfater deleted the feat/share-image-improvement branch September 22, 2021 13:07
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @flexsurfer flexsurfer flexsurfer approved these changes

    @errorists errorists Awaiting requested review from errorists

    Assignees

    @briansztamfater briansztamfater

    @qoqobolo qoqobolo

    Labels
    feature feature requests request-manual-qa Tested - OK
    Projects
    Pipeline for QA
      
    DONE
    Pipeline for QA
    Archived in project
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    Add ability to Share image from Status
    6 participants
    @briansztamfater @status-im-auto @flexsurfer @shivekkhurana @qoqobolo @churik