Fix contract call for communities #17091
Merged
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
J-Son89
reviewed
Aug 23, 2023
| @@ -37,6 +37,7 @@ | |||
| :other (select-keys (:communities cc) other)})})) | |||
|
|
|||
| (rf/defn fetch-contract-communities | |||
| {:events [:fetch-contract-communities]} | |||
There was a problem hiding this comment.
curious, how was this even called before without an event name? 🤔
There was a problem hiding this comment.
It wasn't, but communities were retrieved every 10s I believe (seems to often to be honest), but I think it still best for UX to retrieve them on landing on the page, just in case
Jenkins BuildsClick to see older builds (59)
|
J-Son89
reviewed
Aug 23, 2023
| colors/white | ||
| colors/neutral-95))} | ||
| [discover-screen-content featured-communities]])) | ||
| (reagent/create-class |
There was a problem hiding this comment.
should we move away from the class components?
we could use reagent/with-let perhaps? (not 100% about this)
or a functional component with use-effect
https://reagent-project.github.io/docs/master/reagent.core.html#var-with-let
cc @ulisesmac
There was a problem hiding this comment.
Updated to use use-effect, how does it look now?
J-Son89
approved these changes
Aug 23, 2023
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
from
83c28e9 to
a747d72
Compare
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
from
a747d72 to
7b45b0a
Compare
J-Son89
reviewed
Aug 23, 2023
| colors/white | ||
| colors/neutral-95))} | ||
| [discover-screen-content featured-communities]])) | ||
| [:f> |
There was a problem hiding this comment.
I think we run into some performance issues doing this.
I think you have to do
(defn f-discover []
(fn []
(rn/use-effect ...
(defn discover []
[:f> f-discover]
37% of end-end tests have passedFailed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (16)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
2 times, most recently
from
ce23bc3 to
b8c9bc1
Compare
|
@cammellos thanx for the fix! Could you please clarify is the following behaviour is expected? Looks like a bug. ISSUE 1 All communities are displayed as token gatedAll communities (including communities from Discover communities page or newly created) are displayed as token gated. The UI flow of joining these communities is different from non-token gated. It causes all of community e2e to fail. Steps:
Actual result: telegram-cloud-document-2-5417890137671480613.mp4Expected result: what we have in develop telegram-cloud-document-2-5417890137671480615.mp4 |
|
@pavloburykh So from now on, any community, even if it's not token gated, will require the flow where a password is asked, so e2e tests will need to be changed as the behavior of the PR is correct (to always ask for the user password). The communties though that are not actually token gated, should not be showing a Does it answer your concern? |
Yes, thanx for clarification @cammellos! A few other points:
|
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
2 times, most recently
from
b3e8fb2 to
33312f9
Compare
ulisesmac
reviewed
Aug 23, 2023
Comment on lines
231
to
248
| (defn f-discover | ||
| [] | ||
| (fn [] | ||
| (rn/use-effect | ||
| (fn [] | ||
| (rf/dispatch [:fetch-contract-communities])) | ||
| []) | ||
| (let [featured-communities (rf/sub | ||
| [:communities/featured-contract-communities])] | ||
| [rn/view | ||
| {:style (style/discover-screen-container (colors/theme-colors | ||
| colors/white | ||
| colors/neutral-95))} | ||
| [discover-screen-content featured-communities]]))) | ||
|
|
||
| (defn discover | ||
| [] | ||
| (let [featured-communities (rf/sub [:communities/featured-contract-communities])] | ||
| [rn/view | ||
| {:style (style/discover-screen-container (colors/theme-colors | ||
| colors/white | ||
| colors/neutral-95))} | ||
| [discover-screen-content featured-communities]])) | ||
| [:f> f-discover]) |
There was a problem hiding this comment.
@cammellos @J-Son89
My opinion on this:
f-discoveris a form-2 component, but it's not required if we are already using use-effect.- the
use-effectcall will be only executed on mount and unmount, but we are not performing cleaning, so we only care about mount and I think it can be avoided. - So that it's not required to be a functional component.
So I'd suggest doing this:
(defn discover
[]
(let [_ (rf/dispatch [:fetch-contract-communities])] ; Executed only once when the component is mounted
(fn []
(let [featured-communities (rf/sub [:communities/featured-contract-communities])]
[rn/view
{:style (style/discover-screen-container (colors/theme-colors
colors/white
colors/neutral-95))}
[discover-screen-content featured-communities]]))))(it'd be almost the same as using reagent/with-let)
Also, that let is not required, the component could look as follows:
(defn discover
[]
(rf/dispatch [:fetch-contract-communities]) ; The same as before, but without let
(fn []
(let [featured-communities (rf/sub [:communities/featured-contract-communities])]
[rn/view
{:style (style/discover-screen-container (colors/theme-colors
colors/white
colors/neutral-95))}
[discover-screen-content featured-communities]])))But this might break the form-2 mental model for other devs 🤔, we could add an unused binding name (a var name starting with _), our linter is aware of this, so we won't get any warning:
;; Ignored let binding:
(let [_fetch-communities (rf/dispatch [:fetch-contract-communities])]
(fn [] ,,,))
Feel free to take any of those, I just think the current code looks a little more complex than needed 😅
There was a problem hiding this comment.
no strong opinion on my side, I'll go with the team convention, thanks!
flexsurfer
approved these changes
Aug 24, 2023
|
|
||
| (defn request-to-join-text | ||
| [open?] | ||
| (if open? | ||
| (i18n/label :t/join-open-community) | ||
| (i18n/label :t/request-to-join))) | ||
|
|
||
| (defn join-community-and-navigate-back! |
There was a problem hiding this comment.
just wondering why it has ! mark in the end
There was a problem hiding this comment.
habit, I use it for functions that have side effects, clojure recommends it for state changing functions (reset! etc), but not sure we do that throughout the codebase, so I'll change it
| requested-to-join-at)) | ||
| (rf/dispatch [:communities/request-to-join id]) | ||
| (rf/dispatch [:navigate-back])))) | ||
| (join-community-and-navigate-back! id)) |
| (defn f-discover | ||
| [] | ||
| (fn [] | ||
| (rn/use-effect |
There was a problem hiding this comment.
it's better to avoid using effects and f component, and just dispatch outside render function
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
from
33312f9 to
22ee4ed
Compare
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
from
abe04d3 to
a60e245
Compare
28% of end-end tests have passedFailed tests (31)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (12)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@cammellos works fine with 3 types of communities, created on mobile, and has an issue with Also found a difference in token-gated communities - in contrast to non-token gated they do not have join community rules to accept (so the screen below is missing) |
|
@churik I don't think so, is it ok if we track separately? |
|
@cammellos issue with lost message should I track separately? |
Yes, please, as it's not due to this commit |
40% of end-end tests have passedFailed tests (26)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (17)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
This was referenced Aug 28, 2023
Closed
|
Don't think it makes sense to report issues related to token-gated communities, as it should be aligned anyway to the next desktop version, as it was discussed - so waiting for e2e results and ready to be merged. Thank you for your collaboration @cammellos, it was a tough one |
|
@cammellos Logcat: |
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
from
a60e245 to
76f6dee
Compare
@churik this should be fixed in the next build could you please re-run the tests? Thank you! |
|
@cammellos |
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
from
76f6dee to
8127a94
Compare
44% of end-end tests have passedFailed tests (24)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (19)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
cammellos
force-pushed
the
bug/fix-contract-communities-call
branch
from
8127a94 to
ca7a519
Compare
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: #17069
Fixes: #17114
status-im/status-go#3938