Mute community #15161
Merged
Mute community #15161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (412)
|
jo-mut
force-pushed
the
mute-community
branch
from
fe6f674
to
80344ed
Compare
jo-mut
force-pushed
the
mute-community
branch
from
80344ed
to
c170d8d
Compare
jo-mut
requested review from
J-Son89,
yqrashawn,
Parveshdhull,
smohamedjavid,
vkjr,
OmarBasem,
flexsurfer and
ilmotta
J-Son89
reviewed
Feb 23, 2023
src/status_im/communities/core.cljs
Outdated
| {:events [:community/mute-community-failed]} | ||
| [{:keys [db]} community-id muted? error] | ||
| (log/error "mute community failed" community-id error) | ||
| {:db (assoc-in db [:communities community-id :muted] (not muted?))}) |
There was a problem hiding this comment.
I would assume on error that we do not want to update the state because the request failed on the backend so nothing has actually happened.
I wonder is there any designs in for toasts etc like this where network requests have failed
There was a problem hiding this comment.
yeah you are right should just be muted instead of (not muted?)
There was a problem hiding this comment.
actually, I think you should just remove this code and move the other db change to on-success so the db is updated if the request is successful
There was a problem hiding this comment.
@J-Son89 Removing this code does not really go well with the flow of events.
When you mute a community, the local db is updated first then the request is sent. The effect of this is a real-time update of the ui that the community has been muted.
Updating the ui after mute-community has been processed successfully takes time and sometime does not seem to work untill the app is restarted. Thats why we are updating the community to the previous state when the request fails. I think this code is okay as it is
There was a problem hiding this comment.
This topic surfaces every now and then about what we should do if the RPC call fails. My (imperfect) understanding is more or less like this:
- Many times, the mobile app does optimistic updates on the app db state, i.e. we call the RPC endpoint and simultaneously update the app db state. The
:on-successof the RPC call is then used only if it returns new/unique data that wasn't available before the RPC call. The advantage of doing this is that it help make the app feel snappier. - If we use an optimistic update AND the RPC call fails, then (ideally) we should rollback the app db changes in the
:on-errorevent handler. We don't do this sometimes, we just log that something odd happened. This is bad in general. It is acceptable for now, but I suspect in the future we'll establish a stronger practice. - We can avoid the optimistic update entirely as well, and some events do it, i.e. the app db state is only updated after a successful RPC call. This can cause a slightly longer delay for the UI to update.
- There's pretty much zero chance of the RPC call failing due to network conditions, because it's all local. So that's one big reason we treat RPC calls almost like function calls in Clojure. In other words, a failure always means something happened, but never network related. This is very different from re-frame apps that do a lot of remote network calls (often HTTP ones), where those apps need to be much more strict about network failures.
There was a problem hiding this comment.
Updating the ui after mute-community has been processed successfully takes time and sometime does not seem to work untill the app is restarted.
This though is concerning, it looks like there might be issues, it should not happen, so worth looking into.
The code is ok, we use this pattern somewhere, optimistically updating the community, and this endpoint does not provide an updated community response, so that's ok
There was a problem hiding this comment.
wow nice thanks for this explanation @ilmotta
There was a problem hiding this comment.
@cammellos can be looked into in a separate issue?
J-Son89
reviewed
Feb 23, 2023
src/status_im/communities/core.cljs
Outdated
| (rf/defn mute-chat-toggled-successfully | ||
| {:events [:community/mute-community-successful]} | ||
| [_ community-id] | ||
| (log/debug "muted chat successfully" community-id)) |
There was a problem hiding this comment.
afaik, It would be better to update the db in here, as the network request has successfully gone through here 👍
although perhaps @cammellos or someone else can confirm/ shed some light on this.
J-Son89
reviewed
Feb 23, 2023
| @@ -134,7 +134,7 @@ | |||
| [:<> | |||
| [:f> scroll-page-header @scroll-height height name page-nav-right-section-buttons | |||
| cover-image sticky-header top-nav title-colum navigate-back?] | |||
| [rn/scroll-view | |||
| [rn/flat-list | |||
There was a problem hiding this comment.
Just because flat-list handle long list way better than scroll-view. Also i noticed the list item does not update the view with the muted-icon when you click to mute a community. Using flatlist this seems to work okay
There was a problem hiding this comment.
@J-Son89 usage of scroll-view should generally be avoided in RN. Always use FlatList or SectionList. Here is a quick comparison: https://stackoverflow.com/questions/55256221/flatlist-vs-scrollview
J-Son89
reviewed
Feb 23, 2023
| (when cover-image | ||
| [:f> display-picture @scroll-height cover-image]) | ||
| children])]]))) | ||
| (on-scroll @scroll-height))) |
There was a problem hiding this comment.
is this related to mute community ? or some other changes? maybe you can explain why you made this change.
There was a problem hiding this comment.
yeah. since I am using a flat-list instead of a scroll-view these views are all placed in the header component of the flat-list. The change is only necessary because am using flat-list here
J-Son89
reviewed
Feb 23, 2023
| @@ -41,12 +41,13 @@ | |||
| :on-press #(hide-sheet-and-dispatch [:chat.ui/mark-all-read-in-community-pressed id])}) | |||
|
|
|||
| (defn mute-community | |||
| [_ muted?] | |||
| [id muted?] | |||
| {:icon (if muted? :i/muted :i/activity-center) | |||
| :accessibility-label (if muted? :unmute-community :mute-community) | |||
| :label (i18n/label (if muted? :t/unmute-community :t/mute-community)) | |||
| :sub-label (when muted? (str "muted for 15 minutes")) | |||
There was a problem hiding this comment.
you should probably adjust this string. how long is the community muted for btw?
fro. the designs i believe it's not just muted or unmuted, it's muted for 15 mins, 1 hour etc.. or is this something we will handle a different time?
There was a problem hiding this comment.
at the moment the muting of a community is not time locked I guess its right to remove the string and handle that in a different PR
There was a problem hiding this comment.
ok cool, do you know if it's possible to time lock on the back end? or it's a feature that will be added later?
There was a problem hiding this comment.
I will have to confirm that
OmarBasem
approved these changes
Feb 24, 2023
jo-mut
force-pushed
the
mute-community
branch
from
c170d8d
to
04e5152
Compare
ilmotta
reviewed
Feb 24, 2023
src/status_im/communities/core.cljs
Outdated
| {:db (assoc-in db [:communities community-id :muted] muted?) | ||
| :json-rpc/call [{:method "wakuext_setCommunityMuted" | ||
| :params [community-id muted?] | ||
| :on-error #(log/error "mute community failed" %) |
There was a problem hiding this comment.
@jo-mut, @J-Son89, this is one example of an optimistic update that does not rollback the state if there was a failure. I think we should fix this because it generally means the end-user might take further actions on the app based on an incorrect view/state of the world.
Here the correct approach is to dispatch a separate event :community/set-muted-error and rollback the state.
| :shows-horizontal-scroll-indicator false | ||
| :separator [rn/view {:width 12}] | ||
| :data communities-ids | ||
| :render-fn render-fn}]) |
There was a problem hiding this comment.
The :render-fn keyword often confuses us. The component we pass to render-fn is really a normal component, like any other, so I think we shouldn't name it render-fn or anything containing the word render. The only difference is that this component receives a particular set of arguments, like the current element under iteration and the index.
The component could be named just community-list-item.
jo-mut
force-pushed
the
mute-community
branch
2 times, most recently
from
0f6be5c
to
7ab4605
Compare
I agree. A toast is very much relevant in this case. For instance when you mute the community from the overview page there is no telling to the user if the community has truly been muted or not |
jo-mut
force-pushed
the
mute-community
branch
2 times, most recently
from
f92c08d
to
3ccccc8
Compare
|
@pavloburykh pr is okay now |
@jo-mut thank you! Currently we are busy with release 1.24 testing. So we will proceed with PRs testing as soon as we finish with release. |
61% of end-end tests have passedFailed tests (14)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (22)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
hi @jo-mut! I am in the middle of testing the PR. I noticed that you have pushed more commits. Are these some new changes or just rebasing? In case of any new changes or resolving conflicts I will need to re-test PR from scratch. |
|
@pavloburykh yes I updated status-go version. Seems there was a problems with migration files that had not been applied. The issue with login on iOS and communities not being displayed have been resolved. Apart from that everythins is what you have previously tested My apologies on this pr. has taken so long |
Thanks. I was asking directly about this new commit |
|
@pavloburykh that is just to fix some lint issues |
cool, thank you! |
89% of end-end tests have passedFailed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (32)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@jo-mut thank you for the PR and fixes. Ready for merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #14817
Summary
This PR add implements mute community and refactors the scroll page component to use flat-list instead of scroll-view.
Screen.Recording.2023-02-24.at.01.12.41.mov