chore: cleanup events and tests #21885
Conversation
Jenkins BuildsClick to see older builds (28)
|
| :json-rpc/call | ||
| :fx}) |
There was a problem hiding this comment.
This needed to be added while running tests locally, otherwise the rf/merge function wouldn't return a merged collection of effects.
| :on-error [:contacts/send-contact-request-error id] | ||
| :on-success [:transport/message-sent]}]]]})) | ||
|
|
||
| (rf/reg-event-fx :contact.ui/send-contact-request send-contact-request) | ||
|
|
||
| (defn send-contact-request-failure | ||
| (defn send-contact-request-error | ||
| [_ [id error]] | ||
| (log/error "Failed to send contact request" | ||
| {:error error | ||
| :event :contact.ui/send-contact-request | ||
| :id id})) | ||
| {:id id | ||
| :error error | ||
| :event :contact.ui/send-contact-request})) |
There was a problem hiding this comment.
Here's a pattern for the error handlers that has been introduced in other areas. Here we're creating an explicit error handler instead of inlining a function. This way it's more straightforward to unit test this code because it's all data.
| (defn remove-contact | ||
| "Remove a contact from current account's contact list" | ||
| {:events [:contact.ui/remove-contact-pressed]} | ||
| [{:keys [db]} {:keys [public-key]}] | ||
| {:db (-> db | ||
| (assoc-in [:contacts/contacts public-key :added?] false) | ||
| (assoc-in [:contacts/contacts public-key :active?] false) | ||
| (assoc-in [:contacts/contacts public-key :contact-request-state] | ||
| constants/contact-request-state-none)) | ||
| :json-rpc/call [{:method "wakuext_retractContactRequest" | ||
| :params [{:id public-key}] | ||
| :js-response true | ||
| :on-success #(rf/dispatch [:sanitize-messages-and-process-response %]) | ||
| :on-error #(log/error "failed to remove contact" public-key %)}]}) | ||
|
|
||
| (rf/defn update-nickname | ||
| {:events [:contacts/update-nickname]} | ||
| [_ public-key nickname] | ||
| {:json-rpc/call [{:method "wakuext_setContactLocalNickname" | ||
| :params [{:id public-key :nickname nickname}] | ||
| :js-response true | ||
| :on-success #(rf/dispatch [:sanitize-messages-and-process-response %]) | ||
| :on-error #(log/error "failed to set contact nickname " public-key nickname %)}]}) | ||
| [{:keys [db]} [{:keys [public-key]}]] | ||
| {:db (-> db | ||
| (assoc-in [:contacts/contacts public-key :added?] false) | ||
| (assoc-in [:contacts/contacts public-key :active?] false) | ||
| (assoc-in [:contacts/contacts public-key :contact-request-state] | ||
| constants/contact-request-state-none)) | ||
| :fx [[:json-rpc/call | ||
| [{:method "wakuext_retractContactRequest" | ||
| :params [{:id public-key}] | ||
| :js-response true | ||
| :on-success [:sanitize-messages-and-process-response] | ||
| :on-error [:contacts/remove-contact-error public-key]}]]]}) | ||
|
|
||
| (rf/reg-event-fx :contact.ui/remove-contact-pressed remove-contact) |
There was a problem hiding this comment.
Here we're also migrating towards using the :fx key for re-frame effects.
| (fn [cofx] | ||
| (delete-for-me/sync-all cofx)) | ||
| (fn [cofx] | ||
| (delete-message/send-all cofx)) |
There was a problem hiding this comment.
- Here we're wrapping these functions because
delete-for-me/sync-allanddelete-message/send-allare no longer defined withrf/defn.- It seems by default that functions defined with
rf/defnare composable withrf/merge(when we don't pass all the arguments I think), andrf/mergelikes to compose these functions by passing acofxobject. - So instead we just manually wrap the function, which seems to have the same behaviour as before.
- It seems by default that functions defined with
There was a problem hiding this comment.
This is correct @seanstrom. I had to do this a few times in the past.
| (i18n/label :t/contact-request-chat-pending) | ||
|
|
||
| contact-request-dismissed? | ||
| (i18n/label :t/contact-request-chat-add {:name primary-name}))}]])) | ||
|
|
There was a problem hiding this comment.
Adding a label because we were not handling the dismissed state and that was causing some schema errors
| (let [pending-sync-messages (reduce-kv filter-pending-sync-messages [] (:messages db)) | ||
| pending-effects (map (fn [message] | ||
| (fn [cofx] | ||
| (delete-and-sync cofx [message true]))) | ||
| pending-sync-messages)] | ||
| (apply rf/merge cofx pending-effects))) |
There was a problem hiding this comment.
Here we're needing to wrap the call to delete-and-sync because rf/merge is expecting a sequence of functions that receive a cofx object. Normally this behaviour is handled by rf/defn but delete-and-sync no longer uses that for defining its logic.
| (testing "foo" | ||
| (is (= 1 1))) |
| :json-rpc/call | ||
| :fx}) |
| (fn [cofx] | ||
| (delete-for-me/sync-all cofx)) | ||
| (fn [cofx] | ||
| (delete-message/send-all cofx)) |
There was a problem hiding this comment.
This is correct @seanstrom. I had to do this a few times in the past.
|
|
||
| (defn delete-and-send-error | ||
| [_ [message-id error]] | ||
| (log/error "failed to delete message " |
There was a problem hiding this comment.
For the purist, log calls are stateful and could be re-frame effects. When I see these wrapper functions for log calls I remember we could create a custom log effect and avoid the imperative calls to log. We used to do that in another re-frame project.
There was a problem hiding this comment.
I was thinking about this too, and I had commit for defining events and effects for call the log functions. Here's the commit: b4b808d
When you have chance, let me know what you think, I would definitely like to use log effects for this.
There was a problem hiding this comment.
That's pretty much what I had in mind too @seanstrom. I would consider using a shorter name. For the event, could be :log/error (reads a bit better to be in the singular, but it's personal preference). And then just register other events for the other commonly supported levels we use, like debug, warn, and info.
The event :logs/log-and-attach-error I would skip to keep things plain simple. Some other app contexts could then build their own effects abstracting logs, in case a strong pattern emerges in how certain errors are logged (for example for WalletConnect).
There was a problem hiding this comment.
Besides the PR reviewers, @clauxx @smohamedjavid what are your thoughts on this topic?
There was a problem hiding this comment.
As a developer, I don't need to add logs for every RPC call's on-error. In fact, the on-error or on-success should be defined ONLY if the client/UI will take any action (UI or app-db changes) based on it. Not to mention the user won't see our polite log message failed to do X/Y/Z.
I felt we have subconsciously created a pattern for RPC calls in our codebase to add logging on error and it's been followed everywhere (:on-error #(log/error "failed to do X/Y/Z")).
Now, with this PR, this might probably evolve to :on-error [:chat/delete-and-send-error] or :on-error [:log/error]
IMO, the logs should be added in the error handling part of the json-rpc call method (centralised place).
status-mobile/src/status_im/common/json_rpc/events.cljs
Lines 80 to 83 in 28a81f2
We could log just the method name and error response, optionally params, as we have sensitive data such as passwords or private keys.
The log structure can be something like:
[json-rpc] [wallet] {:name "wallet_fetchOrGetCachedWalletBalances" :error "<ERROR_RESPONSE_GOES_HERE>"}
[json-rpc] [chat] {:name "chat_editChat" :error "..."}
[json-rpc] [wakuext] {:name "wakuext_createGroupChatWithMembers" :error "..."}
The above would help devs to search for any RPC errors in the logs by their context (e.g. searching [wallet]).
The above are just my thoughts 😅
There was a problem hiding this comment.
Thanks @smohamedjavid. I see your point and I agree with the observation that logs shouldn't ideally be polluting so many of our event handlers. Usually in past projects I used to work with 2 layers:
- The generic request abstraction (in our case the :json/rpc-call effect)
- A domain/context specific abstraction for making requests. This layer we don't have in status-mobile. This is the layer where we would log and sometimes handle errors in a particular way.
We could log just the method name and error response, optionally params, as we have sensitive data such as passwords or private keys.
I think that's the problem of having only one general abstraction such as :json/rpc-call. It shouldn't know what to log because it lacks context by definition of being generic. And logging too much is risky.
One solution is to have a separate namespace(s) defining custom effects for all RPC endpoints. These effects can safely decide what to log and the generic :json/rpc-call we would gradually stop using. They wouldn't be tied to any UI concerns. This is a sensible approach to implement, low risk as well. Would give us a central place to apply malli schemas in and out as well.
The above would help devs to search for any RPC errors in the logs by their context (e.g. searching [wallet]).
Nice idea about log groups. In status-go there's work in progress to support log namespaces, similar idea.
Anyway, I also just shared a few thoughts. Seems like we could take this discussion outside the PR since there are different ideas to explore. Maybe the DX call?
seanstrom
force-pushed
the
seanstrom/cleanup-events-and-tests
branch
2 times, most recently
from
767ab80 to
dc46391
Compare
| @@ -38,3 +38,75 @@ | |||
| :logs/set-level | |||
| (fn [level] | |||
| (setup level))) | |||
|
|
|||
|
Thanks for the cleanup, @seanstrom! 🎉 |
seanstrom
force-pushed
the
seanstrom/cleanup-events-and-tests
branch
3 times, most recently
from
f1f8b29 to
7d404b7
Compare
seanstrom
force-pushed
the
seanstrom/cleanup-events-and-tests
branch
from
accbbe6 to
e4bc784
Compare
100% of end-end tests have passedPassed tests (8)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
95% of end-end tests have passedFailed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (53)Click to expandClass TestWalletMultipleDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Class TestFallbackMultipleDevice:
Class TestActivityMultipleDevicePRTwo:
|
|
@seanstrom thanks for the PR. No issues from my side. Ready for merge. |
…st dismissed state
seanstrom
force-pushed
the
seanstrom/cleanup-events-and-tests
branch
from
e4bc784 to
e7ec3c0
Compare
Summary
rf/defntowardsdefnwithrf/reg-event-fx.Platforms
Areas that maybe impacted
Functional
Steps to test
WIP
status: ready