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

[#21354] Exception when navigating back from route generation #21381

Merged
merged 1 commit into from
Oct 9, 2024
Merged

[#21354] Exception when navigating back from route generation #21381

merged 1 commit into from
Oct 9, 2024

Conversation

ulisesmac
Copy link
Contributor

@ulisesmac ulisesmac commented Oct 4, 2024 •
edited
Loading

fixes #21354

Summary

This PR solves the exception when fastly navigating back once the routes arrive.

The problem happened because the conversion-rate used to calculate the fiat->crypto value (calculated in the view ns ⚠️), was nil when navigating back.

Video of the issue solved:

Screencast.from.2024-10-03.19-04-56.mp4

Review notes

The component status-im.contexts.wallet.send.input-amount.view/view needs a refactor, there a lot of calculations made in the view and the let bindings to understand the logic of it is giant.

Platforms

  • Android
  • iOS

Steps to test

Please follow the video in the issue description and check is no longer reproducible.

status: ready

@ulisesmac ulisesmac requested review from briansztamfater, smohamedjavid and a team October 4, 2024 01:12
@ulisesmac ulisesmac self-assigned this Oct 4, 2024
@ulisesmac
Copy link
Contributor Author

@mariia-skrypnyk
The reproduction of the issue is a bit tricky, since you reported it, it'd be great if you test that it's been fixed

ulisesmac
ulisesmac commented Oct 4, 2024
View reviewed changes
src/status_im/contexts/wallet/send/input_amount/view.cljs
@@ -158,16 +158,14 @@
active-screen? (= view-id current-screen-id)
bottom (safe-area/get-bottom)
[crypto-currency? set-crypto-currency] (rn/use-state initial-crypto-currency?)
on-navigate-back on-navigate-back
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redundant binding

ulisesmac
ulisesmac commented Oct 4, 2024
View reviewed changes
src/status_im/contexts/wallet/send/input_amount/view.cljs
Comment on lines 165 to +168
{fiat-currency :currency} (rf/sub [:profile/profile])
{token-symbol :symbol
{token-symbol :symbol
token-networks :networks
:as
token} (rf/sub [:wallet/wallet-send-token])
:as token} (rf/sub [:wallet/wallet-send-token])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a code style fix

ulisesmac
ulisesmac commented Oct 4, 2024
View reviewed changes
src/status_im/contexts/wallet/send/send_amount/view.cljs
Comment on lines 12 to +13
:button-one-label (i18n/label :t/review-send)
:enabled-from-chain-ids (rf/sub
[:wallet/wallet-send-enabled-from-chain-ids])
:enabled-from-chain-ids (rf/sub [:wallet/wallet-send-enabled-from-chain-ids])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, just a code style fix

ulisesmac
ulisesmac commented Oct 4, 2024
View reviewed changes
src/utils/money.cljs
Comment on lines 241 to 248
[crypto fiat-price]
(when-let [^js bn (bignumber crypto)]
(-> (.times bn ^js (bignumber fiat-price))
(with-precision 2))))
(let [^js crypto-bn (bignumber crypto)
^js fiat-price-bn (bignumber fiat-price)]
(when (and crypto-bn fiat-price-bn)
(-> crypto-bn
(.times fiat-price-bn)
(with-precision 2)))))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the implementation checks that both numbers are bignumbers.

I considered to use fnil as:

(fnil bignumber 0)

But I was afraid that it broke existing usages where we relied on:

(bignumber nil) ;; => nil

@status-im-auto
Copy link
Member

status-im-auto commented Oct 4, 2024 •
edited
Loading

Jenkins Builds

❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 9b262fa #1 2024-10-04 01:17:49 ~5 min tests 📄log
✔️ 9b262fa #1 2024-10-04 01:21:40 ~9 min android-e2e 🤖apk 📲
✔️ 9b262fa #1 2024-10-04 01:21:51 ~9 min ios 📱ipa 📲
✔️ 9b262fa #1 2024-10-04 01:22:08 ~9 min android 🤖apk 📲
✔️ 6fa6209 #2 2024-10-09 00:56:32 ~4 min tests 📄log
✔️ 6fa6209 #2 2024-10-09 00:59:00 ~7 min android-e2e 🤖apk 📲
✔️ 6fa6209 #2 2024-10-09 00:59:33 ~7 min android 🤖apk 📲
✔️ 6fa6209 #2 2024-10-09 01:00:43 ~8 min ios 📱ipa 📲

briansztamfater
briansztamfater approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@briansztamfater briansztamfater left a comment

Choose a reason for hiding this comment

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

LGTM

@pavloburykh pavloburykh self-assigned this Oct 8, 2024
@status-im-auto
Copy link
Member

86% of end-end tests have passed

Total executed tests: 7
Failed tests: 1
Expected to fail tests: 0
Passed tests: 6

IDs of failed tests: 702843

Failed tests (1)

Click to expand
  • Rerun failed tests

    • Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_edit, id: 702843

    Device 1: Wait for text element `EmojisNumber` to be equal to `1`
    Device 1: Find `EmojisNumber` by `xpath`: `//*[starts-with(@text,'Message AFTER edit 2 (Edited)')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']/../..//*[@content-desc='emoji-reaction-2']/android.widget.TextView[2]` 

    critical/chats/test_public_chat_browsing.py:388: in test_community_message_edit
    self.errors.verify_no_errors()
base_test_case.py:192: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 Message is not edited
E    Message reaction is not shown for the sender



    Device sessions

    Passed tests (6)

    Click to expand

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    2. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
    Device sessions

    Class TestWalletOneDevice:

    1. test_wallet_add_remove_regular_account, id: 727231
    Device sessions

    Class TestWalletMultipleDevice:

    1. test_wallet_send_asset_from_drawer, id: 727230
    2. test_wallet_send_eth, id: 727229

    @pavloburykh
    Copy link
    Contributor

    @ulisesmac thank you for the PR. Ready for merge.

    @ulisesmac ulisesmac force-pushed the 21354-exception-after-quick-navigation branch from 9b262fa to 6fa6209 Compare October 9, 2024 00:51
    @ulisesmac ulisesmac merged commit a9e0b3d into develop Oct 9, 2024
    6 checks passed
    @ulisesmac ulisesmac deleted the 21354-exception-after-quick-navigation branch October 9, 2024 01:05
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @shivekkhurana shivekkhurana shivekkhurana approved these changes

    @vkjr vkjr vkjr approved these changes

    @briansztamfater briansztamfater briansztamfater approved these changes

    @smohamedjavid smohamedjavid Awaiting requested review from smohamedjavid

    Assignees

    @ulisesmac ulisesmac

    @pavloburykh pavloburykh

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

    Successfully merging this pull request may close these issues.

    'Undefined not a function' error is shown after quick back navigation
    6 participants
    @ulisesmac @status-im-auto @pavloburykh @shivekkhurana @vkjr @briansztamfater