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

[15550] Allow unpinning of deleted-for-me messages #15572

Merged
merged 5 commits into from
Apr 23, 2023
Merged

[15550] Allow unpinning of deleted-for-me messages #15572

merged 5 commits into from
Apr 23, 2023

Conversation

ibrkhalil
Copy link
Contributor

fixes #15550

Summary

This PR just adds a filter on deleted-for-me pinned messages and doesn't add them to the list of pinned messages

status: ready

@status-im-auto
Copy link
Member

status-im-auto commented Apr 3, 2023

Jenkins Builds

Click to see older builds (28)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 6efee61 #1 2023-04-03 22:37:51 ~6 min tests 📄log
✔️ 6efee61 #1 2023-04-03 22:38:54 ~7 min ios 📱ipa 📲
✔️ 6efee61 #1 2023-04-03 22:40:05 ~8 min android-e2e 🤖apk 📲
✔️ 6efee61 #1 2023-04-03 22:40:28 ~9 min android 🤖apk 📲
✔️ 7d3e446 #2 2023-04-08 20:21:14 ~7 min ios 📱ipa 📲
✔️ 7d3e446 #2 2023-04-08 20:22:58 ~9 min android-e2e 🤖apk 📲
✔️ 7d3e446 #2 2023-04-08 20:23:09 ~9 min android 🤖apk 📲
✔️ 7d3e446 #2 2023-04-08 20:23:25 ~9 min tests 📄log
✔️ bbe0f08 #3 2023-04-16 17:02:24 ~5 min android 🤖apk 📲
✔️ bbe0f08 #3 2023-04-16 17:02:39 ~5 min tests 📄log
✔️ bbe0f08 #3 2023-04-16 17:02:54 ~6 min android-e2e 🤖apk 📲
✔️ bbe0f08 #3 2023-04-16 17:03:39 ~7 min ios 📱ipa 📲
✔️ d0ea50f #4 2023-04-17 17:32:48 ~6 min android-e2e 🤖apk 📲
✔️ d0ea50f #4 2023-04-17 17:34:41 ~8 min android 🤖apk 📲
✔️ d0ea50f #4 2023-04-17 17:35:28 ~8 min tests 📄log
✔️ d0ea50f #4 2023-04-17 17:41:10 ~14 min ios 📱ipa 📲
✔️ 6d65821 #6 2023-04-17 18:52:26 ~5 min android 🤖apk 📲
✔️ 6d65821 #6 2023-04-17 18:53:51 ~6 min tests 📄log
✔️ 6d65821 #6 2023-04-17 18:54:03 ~6 min android-e2e 🤖apk 📲
✔️ 6d65821 #6 2023-04-17 18:54:24 ~7 min ios 📱ipa 📲
✔️ 2a6e2db #7 2023-04-19 11:49:43 ~5 min android-e2e 🤖apk 📲
✔️ 2a6e2db #7 2023-04-19 11:51:13 ~6 min tests 📄log
✔️ 2a6e2db #7 2023-04-19 11:51:14 ~6 min ios 📱ipa 📲
✔️ 2a6e2db #7 2023-04-19 11:51:44 ~7 min android 🤖apk 📲
✔️ 06559bd #8 2023-04-19 13:09:46 ~6 min android 🤖apk 📲
✔️ 06559bd #8 2023-04-19 13:09:47 ~6 min android-e2e 🤖apk 📲
✔️ 06559bd #8 2023-04-19 13:10:21 ~6 min tests 📄log
✔️ 06559bd #8 2023-04-19 13:10:50 ~7 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 2f66195 #9 2023-04-20 07:13:59 ~6 min android-e2e 🤖apk 📲
✔️ 2f66195 #9 2023-04-20 07:14:00 ~6 min android 🤖apk 📲
✔️ 2f66195 #9 2023-04-20 07:14:02 ~6 min tests 📄log
✔️ 2f66195 #9 2023-04-20 07:14:54 ~7 min ios 📱ipa 📲
✔️ b4ac4d4 #10 2023-04-23 17:07:32 ~6 min ios 📱ipa 📲
✔️ b4ac4d4 #10 2023-04-23 17:07:36 ~6 min android 🤖apk 📲
✔️ b4ac4d4 #10 2023-04-23 17:07:38 ~6 min android-e2e 🤖apk 📲
✔️ b4ac4d4 #10 2023-04-23 17:07:52 ~6 min tests 📄log

@yqrashawn
Copy link
Contributor

This may cause user not be able to pin more messages if there are 3 pinned and deleted-for-me messages, and we hide them in the pinned message list.
I think we need a better solution. Maybe show them in pinned message list and allow user to unpin them

@ibrkhalil
Copy link
Contributor Author

This may cause user not be able to pin more messages if there are 3 pinned and deleted-for-me messages, and we hide them in the pinned message list. I think we need a better solution. Maybe show them in pinned message list and allow user to unpin them

Going to test if that case might happen and how to avoid it, Thanks Rashawn

@briansztamfater
Copy link
Member

I think it shouldn't be filtered, just shown as "deleted for me" message.

@ilmotta
Copy link
Contributor

ilmotta commented Apr 5, 2023

In Telegram, if Alice deletes a pinned message only for her, then the pinned message also disappears, along with the actual message (for her only). I don't understand the point of displaying a deleted pinned message at the top and allowing Alice to unpin a deleted message. In essence, I much prefer Telegram's behavior here.

@ibrkhalil
Copy link
Contributor Author

In Telegram, if Alice deletes a pinned message only for her, then the pinned message also disappears, along with the actual message (for her only). I don't understand the point of displaying a deleted pinned message at the top and allowing Alice to unpin a deleted message. In essence, I much prefer Telegram's behavior here.

Love the mock names
I agree
CC: @pedro-et

@ibrkhalil ibrkhalil changed the title [15550] Filter out deleted-for-me messages from pinned messages popover [15550] Filter out deleted-for-me? messages from pinned messages popover Apr 8, 2023
@VolodLytvynenko
Copy link
Contributor

VolodLytvynenko commented Apr 10, 2023

In Telegram, if Alice deletes a pinned message only for her, then the pinned message also disappears, along with the actual message (for her only). I don't understand the point of displaying a deleted pinned message at the top and allowing Alice to unpin a deleted message. In essence, I much prefer Telegram's behavior here.

hi @ilmotta . agree with you about convenience. The "deleted message" showing in the pinned bar was implemented in #14488 (comment)

But, I think one of the reasons to avoid more than 3 pinned messages (we have limits of no more than 3 pinned messages. Telegram does not have the pinning messages limits) CC: @yqrashawn correct me if I am wrong about the purpose of this functionality which is done in #14488 (comment)

Example:

  1. User_A and User_B have 3 pinned messages.
  2. User_A removes one message by 'delete for me' (now User_A has 2 pinned messages, User_B has 3 pinned messages)
  3. User_A sends the 4-th message and pins it (now User_A has 3 pinned messages)

Expected result:

How many pinned messages the User_B should have after step 3?

Potential solution:

  • User_B has 4 pinned messages despite limits of 3 pinned and maybe we should consider rejecting the functionality of message pinning limiting
  • User_B has 3 pinned messages (the first pinned is removed)

CC: @pedro-et need your help here

@yqrashawn
Copy link
Contributor

@VladimrLitvinenko That's correct. #14488 allows user to unpin deleted messages (show both deleted/deleted for me messages in pinned message list), and it introduced another edge case, which is what to do with pinned deleted for me messages.

Users don't care about deleted-for-me messages, definitely, but I think users do care about if they can pin more messages.

@ibrkhalil ibrkhalil changed the title [15550] Filter out deleted-for-me? messages from pinned messages popover [15550] Allow unpinning of deleted-for-me messages Apr 17, 2023
@VolodLytvynenko VolodLytvynenko self-assigned this Apr 19, 2023
@status-im-auto
Copy link
Member

90% of end-end tests have passed

Total executed tests: 29
Failed tests: 3
Passed tests: 26
IDs of failed tests: 702850,702838,702742 

Failed tests (3)

Click to expand
  • Rerun failed tests

  • Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_send_check_timestamps_sender_username, id: 702838

    Device 2: Verifying that 'hello' is under today
    Device 2: Looking for a message by text: hello

    critical/test_public_chat_browsing.py:418: in test_community_message_send_check_timestamps_sender_username
        channel.verify_message_is_under_today_text(message, self.errors)
    ../views/chat_view.py:923: in verify_message_is_under_today_text
        message_element.wait_for_visibility_of_element()
    ../views/base_element.py:135: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 2: ChatElementByText by xpath:`//*[starts-with(@text,'hello')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']` is not found on the screen after wait_for_visibility_of_element 
    

    [[blocked by 14797]]

    Device sessions

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_decline, id: 702850

    Device 2: Find Button by accessibility id: tab-recent
    Device 2: Tap on found: Button

    medium/test_activity_center.py:142: in test_activity_center_contact_request_decline
        self.errors.verify_no_errors()
    base_test_case.py:184: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Username is not shown on 'Add contact' page after entering valid public key 
    

    [[Blocked by 15500]]

    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742

    Device 1: Find ChatMessageInput by accessibility id: chat-message-input
    Device 1: Clear text in ChatMessageInput

    critical/test_public_chat_browsing.py:352: in test_community_copy_and_paste_message_in_chat_input
        self.errors.verify_no_errors()
    base_test_case.py:184: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Message mmmeowesage_text text was not copied in community channel
    E    Message https://status.im text was not copied in community channel
    



    Device sessions

    Passed tests (26)

    Click to expand

    Class TestActivityMultipleDevicePR:

    1. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947
    Device sessions

    2. test_activity_center_admin_notification_accept_swipe, id: 702958
    Device sessions

    3. test_navigation_jump_to, id: 702936
    Device sessions

    4. test_activity_center_mentions, id: 702957
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_emoji_send_copy_paste_reply, id: 702840
    Device sessions

    2. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    3. test_community_mentions_push_notification, id: 702786
    Device sessions

    4. test_community_contact_block_unblock_offline, id: 702894
    Device sessions

    5. test_community_leave, id: 702845
    Device sessions

    6. test_community_unread_messages_badge, id: 702841
    Device sessions

    7. test_community_message_delete, id: 702839
    Device sessions

    8. test_community_message_edit, id: 702843
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    2. test_1_1_chat_edit_message, id: 702855
    Device sessions

    3. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    4. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    5. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
    Device sessions

    6. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783
    Device sessions

    7. test_1_1_chat_pin_messages, id: 702731
    Device sessions

    8. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    9. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_accept_swipe_mark_all_as_read, id: 702851
    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732
    Device sessions

    2. test_group_chat_join_send_text_messages_push, id: 702807
    Device sessions

    3. test_group_chat_offline_pn, id: 702808
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_navigate_to_channel_when_relaunch, id: 702846
    Device sessions

    @VolodLytvynenko
    Copy link
    Contributor

    Hi @ibrkhalil, thank you for your work. No issues from my side. PR is tested and ready to be merged

    @ibrkhalil ibrkhalil merged commit 9ddea48 into develop Apr 23, 2023
    @ibrkhalil ibrkhalil deleted the 15550 branch April 23, 2023 17:13
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    No open projects
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    The deleted pinned message can't be unpinned in 1-1 chat
    7 participants