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

Quo - Wallet/Required-Tokens Component #18164

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Quo - Wallet/Required-Tokens Component #18164

merged 1 commit into from
Dec 15, 2023

Conversation

ajayesivan
Copy link
Contributor

@ajayesivan ajayesivan commented Dec 13, 2023 •
edited
Loading

fix #18139

Quo Preview -> Wallet -> Required Tokens

iOS Screenshots

Dark Light
Simulator Screenshot - iPhone 13 - 2023-12-13 at 16 40 50 Simulator Screenshot - iPhone 13 - 2023-12-13 at 16 40 53
Simulator Screenshot - iPhone 13 - 2023-12-13 at 16 41 03 Simulator Screenshot - iPhone 13 - 2023-12-13 at 16 41 01

Android Screenshots

Dark Light
image image
image image

This component hasn't been integrated into the UI yet. Therefore, I suggest we defer manual testing for now and proceed with a design review.

status: ready

@ajayesivan ajayesivan self-assigned this Dec 13, 2023
@status-im-auto
Copy link
Member

status-im-auto commented Dec 13, 2023 •
edited
Loading

Jenkins Builds

Click to see older builds (28)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
❌ 57ef668 #1 2023-12-13 11:27:05 ~4 min tests 📄log
✔️ 57ef668 #1 2023-12-13 11:29:01 ~6 min ios 📱ipa 📲
✔️ 57ef668 #1 2023-12-13 11:30:58 ~8 min android-e2e 🤖apk 📲
✔️ 57ef668 #1 2023-12-13 11:31:44 ~9 min android 🤖apk 📲
❌ a67b190 #2 2023-12-13 11:37:08 ~2 min tests 📄log
✔️ a67b190 #2 2023-12-13 11:41:26 ~7 min android-e2e 🤖apk 📲
✔️ a67b190 #2 2023-12-13 11:42:01 ~7 min android 🤖apk 📲
✔️ a67b190 #2 2023-12-13 11:46:16 ~12 min ios 📱ipa 📲
❌ a7dd0f0 #3 2023-12-13 12:06:21 ~2 min tests 📄log
✔️ a7dd0f0 #3 2023-12-13 12:10:29 ~6 min ios 📱ipa 📲
✔️ a7dd0f0 #3 2023-12-13 12:11:51 ~7 min android-e2e 🤖apk 📲
✔️ a7dd0f0 #3 2023-12-13 12:12:08 ~7 min android 🤖apk 📲
❌ 5292460 #5 2023-12-14 07:30:08 ~2 min tests 📄log
✔️ 5292460 #5 2023-12-14 07:34:46 ~7 min android-e2e 🤖apk 📲
✔️ 5292460 #5 2023-12-14 07:35:12 ~7 min android 🤖apk 📲
✔️ 5292460 #5 2023-12-14 07:44:11 ~16 min ios 📱ipa 📲
❌ c77a61f #6 2023-12-14 10:36:25 ~2 min tests 📄log
✔️ c77a61f #6 2023-12-14 10:43:09 ~8 min android-e2e 🤖apk 📲
✔️ c77a61f #6 2023-12-14 10:43:16 ~9 min android 🤖apk 📲
✔️ c77a61f #6 2023-12-14 10:49:51 ~15 min ios 📱ipa 📲
✔️ 477c2fa #7 2023-12-14 16:27:00 ~4 min tests 📄log
❌ 477c2fa #7 2023-12-14 16:27:01 ~4 min ios 📄log
✔️ 477c2fa #7 2023-12-14 16:30:23 ~8 min android 🤖apk 📲
✔️ 477c2fa #7 2023-12-14 16:30:25 ~8 min android-e2e 🤖apk 📲
✔️ 0da1a89 #9 2023-12-14 17:12:24 ~4 min tests 📄log
✔️ 0da1a89 #9 2023-12-14 17:16:13 ~8 min android-e2e 🤖apk 📲
✔️ 0da1a89 #9 2023-12-14 17:16:17 ~8 min android 🤖apk 📲
✔️ 0da1a89 #9 2023-12-14 17:20:30 ~12 min ios 📱ipa 📲
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ affba3e #11 2023-12-15 03:59:24 ~4 min tests 📄log
✔️ affba3e #11 2023-12-15 04:02:45 ~8 min android 🤖apk 📲
✔️ affba3e #11 2023-12-15 04:02:46 ~8 min android-e2e 🤖apk 📲
✔️ affba3e #11 2023-12-15 04:09:23 ~14 min ios 📱ipa 📲
✔️ ba9c3ea #12 2023-12-15 05:23:31 ~3 min tests 📄log
✔️ ba9c3ea #12 2023-12-15 05:27:23 ~7 min android-e2e 🤖apk 📲
✔️ ba9c3ea #12 2023-12-15 05:27:30 ~7 min android 🤖apk 📲
✔️ ba9c3ea #12 2023-12-15 05:34:06 ~14 min ios 📱ipa 📲

@ajayesivan ajayesivan force-pushed the 18139-wallet-required-tokens-component branch from 57ef668 to a67b190 Compare December 13, 2023 11:33
J-Son89
J-Son89 approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@J-Son89 J-Son89 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work @ajayesivan ! :)

src/quo/components/wallet/required_tokens/view.cljs
[react-native.core :as rn]
[schema.core :as schema]))

(def ?schema
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

Copy link
Contributor

Choose a reason for hiding this comment

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

@clauxx, @yqrashawn - can you take a look at the schema :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my first attempt at crafting a schema, and I welcome any feedback or suggestions!

Copy link
Contributor

Choose a reason for hiding this comment

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

do we have any docs about usage of schema? and if we define schema do we still need to use docstring for components?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @mohsen-ghafouri - I think we should not use doc strings if using schemas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I believe we can use the schema alone, which reduces maintenance overhead. IMO schemas are generally easier to keep up-to-date compared to docstrings.

Copy link
Contributor

Choose a reason for hiding this comment

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

schema below LGTM

src/quo/core.cljs
@@ -403,6 +404,7 @@
(def network-bridge quo.components.wallet.network-bridge.view/view)
(def network-routing quo.components.wallet.network-routing.view/view)
(def progress-bar quo.components.wallet.progress-bar.view/view)
(def required-tokens quo.components.wallet.required-tokens.view/view)
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

mohsen-ghafouri
mohsen-ghafouri approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@mohsen-ghafouri mohsen-ghafouri left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM 🚀

@ajayesivan ajayesivan force-pushed the 18139-wallet-required-tokens-component branch from a67b190 to a7dd0f0 Compare December 13, 2023 12:04
smohamedjavid
smohamedjavid approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@smohamedjavid smohamedjavid left a comment

Choose a reason for hiding this comment

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

Nice work! 🚀

ulisesmac
ulisesmac approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@ulisesmac ulisesmac left a comment

Choose a reason for hiding this comment

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

LGTM!!

src/quo/components/wallet/required_tokens/view.cljs Outdated Show resolved Hide resolved
src/schema/common.cljs Outdated Show resolved Hide resolved
src/quo/components/wallet/required_tokens/view.cljs Outdated Show resolved Hide resolved
@ajayesivan ajayesivan force-pushed the 18139-wallet-required-tokens-component branch from a7dd0f0 to 6509e23 Compare December 14, 2023 07:25
@status-im-auto
Copy link
Member

83% of end-end tests have passed

Total executed tests: 48
Failed tests: 2
Expected to fail tests: 6
Passed tests: 40

IDs of failed tests: 702733,703133 

IDs of expected to fail tests: 702732,702948,702894,703503,702731,702808

Failed tests (2)

Click to expand
  • Rerun failed tests

    • Class TestCommunityOneDeviceMerged:

    1. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133

    Device 1: Find `Button` by `accessibility id`: `show-profiles`
    Device 1: Tap on found: Button 

    critical/chats/test_public_chat_browsing.py:274: in test_restore_multiaccount_with_waku_backup_remove_switch
    self.errors.verify_no_errors()
base_test_case.py:191: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 admin_closed was not restored from waku-backup!!



    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_text_message_delete_push_disappear, id: 702733

    Device 1: Getting PN by 'DELETE ME'
    Device 2: Could not reach home view by pressing system back button 

    critical/chats/test_1_1_public_chats.py:516: in test_1_1_chat_text_message_delete_push_disappear
    self.errors.verify_no_errors()
base_test_case.py:191: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 Message status was not changed to 'Delivered' after 60 s



    Device sessions

    Expected to fail tests (6)

    Click to expand

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_contact_block_unblock_offline, id: 702894

    Device 1: Find ProfileSendMessageButton by accessibility id: Chat-item-button
    Device 1: Sending message 'piy' 

    critical/chats/test_public_chat_browsing.py:741: in test_community_contact_block_unblock_offline
    self.errors.verify_no_errors()
base_test_case.py:191: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 Chat with unblocked user was not enabled after 1 minute

    [[Message can be missed after unblock: https://github.com//issues/16873]]

    Device sessions

    Class TestCommunityMultipleDeviceMergedTwo:

    1. test_community_hashtag_links_to_community_channels, id: 702948

    Device 2: Find Text by xpath: //android.view.ViewGroup[@content-desc='chat-item']//android.widget.TextView[contains(@text,'#cats')]
    Device 2: Looking for a message by text: it is just a message text 

    critical/chats/test_public_chat_browsing.py:1078: in test_community_hashtag_links_to_community_channels
    self.errors.verify_no_errors()
base_test_case.py:191: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 Receiver was not navigated to the cats channel
E    Sender was not navigated to the cats channel

    [[Can't navigate to a channel by hashtag link, https://github.com//issues/18095]]

    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732 

    Test is not run, e2e blocker

    [[reason: [NOTRUN] Pin feature is in development]]

    2. test_group_chat_offline_pn, id: 702808

    Device 3: Looking for a message by text: message from old member
    Device 3: Looking for a message by text: message from new member 

    critical/chats/test_group_chat.py:323: in test_group_chat_offline_pn
    self.errors.verify_no_errors()
base_test_case.py:191: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 Messages PN was not fetched from offline

    [[Data delivery issue]]

    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_discovery, id: 703503 

    Test is not run, e2e blocker

    [[reason: [NOTRUN] Curated communities not loading, https://github.com//issues/17852]]

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_pin_messages, id: 702731 

    Test is not run, e2e blocker

    [[reason: [NOTRUN] Pin feature is in development]]

    Passed tests (40)

    Click to expand

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_several_images_send_reply, id: 703194
    Device sessions

    2. test_community_one_image_send_reply, id: 702859
    Device sessions

    3. test_community_emoji_send_copy_paste_reply, id: 702840
    Device sessions

    4. test_community_mark_all_messages_as_read, id: 703086
    Device sessions

    5. test_community_edit_delete_message_when_offline, id: 704615
    Device sessions

    6. test_community_message_delete, id: 702839
    Device sessions

    7. test_community_message_send_check_timestamps_sender_username, id: 702838
    Device sessions

    8. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    9. test_community_message_edit, id: 702843
    Device sessions

    10. test_community_unread_messages_badge, id: 702841
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    2. test_community_undo_delete_message, id: 702869
    Device sessions

    3. test_community_navigate_to_channel_when_relaunch, id: 702846
    Device sessions

    4. test_community_mute_community_and_channel, id: 703382
    Device sessions

    Class TestActivityMultipleDevicePRTwo:

    1. test_activity_center_mentions, id: 702957
    Device sessions

    2. test_activity_center_admin_notification_accept_swipe, id: 702958
    Device sessions

    Class TestCommunityMultipleDeviceMergedTwo:

    1. test_community_markdown_support, id: 702809
    Device sessions

    2. test_community_mentions_push_notification, id: 702786
    Device sessions

    3. test_community_leave, id: 702845
    Device sessions

    4. test_community_join_when_node_owner_offline, id: 703629
    Device sessions

    Class TestDeepLinksOneDevice:

    1. test_links_open_universal_links_from_chat, id: 704613
    Device sessions

    2. test_links_deep_links, id: 702775
    Device sessions

    Class TestActivityMultipleDevicePR:

    1. test_navigation_jump_to, id: 702936
    Device sessions

    2. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947
    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_mute_chat, id: 703495
    Device sessions

    2. test_group_chat_send_image_save_and_share, id: 703297
    Device sessions

    3. test_group_chat_reactions, id: 703202
    Device sessions

    4. test_group_chat_join_send_text_messages_push, id: 702807
    Device sessions

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_add_contact_field_validation, id: 702777
    Device sessions

    2. test_activity_center_contact_request_accept_swipe_mark_all_as_read, id: 702851
    Device sessions

    3. test_activity_center_contact_request_decline, id: 702850
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:

    1. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    2. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783
    Device sessions

    3. test_1_1_chat_mute_chat, id: 703496
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
    Device sessions

    2. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    3. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    4. test_1_1_chat_edit_message, id: 702855
    Device sessions

    5. test_1_1_chat_send_image_save_and_share, id: 703391
    Device sessions

    6. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    @ajayesivan
    Copy link
    Contributor Author

    Hi @Francesca-G, Could you please do a design review of this component? We are not yet using it in the UI, you can verify it in the Quo Preview.

    Francesca-G
    Francesca-G approved these changes Dec 14, 2023
    View reviewed changes
    Copy link

    @Francesca-G Francesca-G left a comment

    Choose a reason for hiding this comment

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

    Looks good ✨

    Adding the follow up required label due to a minor issue in the token avatar and collectible size, see comment here

    @ajayesivan ajayesivan force-pushed the 18139-wallet-required-tokens-component branch 2 times, most recently from 64fe9ac to 0da1a89 Compare December 14, 2023 17:07
    @ajayesivan
    Copy link
    Contributor Author

    Looks good ✨

    Adding the follow up required label due to a minor issue in the token avatar and collectible size, see comment here

    @Francesca-G Thanks for the review! I have fixed the token & collectible size.

    ulisesmac
    ulisesmac approved these changes Dec 14, 2023
    View reviewed changes
    Copy link
    Contributor

    @ulisesmac ulisesmac left a comment

    Choose a reason for hiding this comment

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

    💯

    @ajayesivan ajayesivan force-pushed the 18139-wallet-required-tokens-component branch from 0da1a89 to 2823276 Compare December 15, 2023 03:53
    @ajayesivan ajayesivan force-pushed the 18139-wallet-required-tokens-component branch from 2823276 to affba3e Compare December 15, 2023 03:54
    @ajayesivan ajayesivan mentioned this pull request Dec 15, 2023
    Merged
    @ajayesivan ajayesivan force-pushed the 18139-wallet-required-tokens-component branch from affba3e to ba9c3ea Compare December 15, 2023 05:19
    @ajayesivan
    Copy link
    Contributor Author

    @status-im/mobile-qa, could you kindly verify if the failed e2e tests are related to this PR? Additionally, since this PR introduces a new Quo component that is currently only used in the preview, should we consider manual testing?

    @pavloburykh
    Copy link
    Contributor

    pavloburykh commented Dec 15, 2023 •
    edited
    Loading

    Hi @ajayesivan! Thank you for the PR.

    could you kindly verify if the failed e2e tests are related to this PR?

    Failed e2e are not PR related.

    Additionally, since this PR introduces a new Quo component that is currently only used in the preview, should we consider manual testing?

    In this case manual QA can be skipped.

    So we this PR is ready for merge!

    @ajayesivan ajayesivan merged commit 31dea1c into develop Dec 15, 2023
    6 checks passed
    @ajayesivan ajayesivan deleted the 18139-wallet-required-tokens-component branch December 15, 2023 07:54
    ulisesmac pushed a commit that referenced this pull request Dec 15, 2023
    @ajayesivan @ulisesmac
    Quo Wallet/Required-Tokens Component (#18164)
    c20a05b
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @mohsen-ghafouri mohsen-ghafouri mohsen-ghafouri approved these changes

    @J-Son89 J-Son89 J-Son89 approved these changes

    @Francesca-G Francesca-G Francesca-G approved these changes

    @ulisesmac ulisesmac ulisesmac approved these changes

    @smohamedjavid smohamedjavid smohamedjavid approved these changes

    @ilmotta ilmotta Awaiting requested review from ilmotta

    @FFFra FFFra Awaiting requested review from FFFra

    @yqrashawn yqrashawn Awaiting requested review from yqrashawn

    @codemaster115 codemaster115 Awaiting requested review from codemaster115

    Assignees

    @ajayesivan ajayesivan

    Labels
    None yet
    Projects
    Pipeline for QA
    Archived in project
    Legacy QA Automation Project
    Archived in project
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    Implement Wallet/Required tokens Quo Component
    9 participants
    @ajayesivan @status-im-auto @pavloburykh @yqrashawn @smohamedjavid @J-Son89 @mohsen-ghafouri @ulisesmac @Francesca-G