Add featured-community data #16015 #16232
Conversation
Jenkins BuildsClick to see older builds (45)
|
src/status_im/communities/core.cljs
Outdated
| (assoc-in db [:featured-communities id] (<-rpc community))) | ||
| db | ||
| (mapv #(get (:communities communities) (keyword %)) | ||
| (:contractFeaturedCommunities communities)))}) |
There was a problem hiding this comment.
Can this key be kebab case?
There was a problem hiding this comment.
Thanks for the review @J-Son89!
done
70% of end-end tests have passedFailed tests (10)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (23)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
|
|
hi @erikseppanen . Thank you for PR. Please, take a look at the following issue: ISSUE 1: Community in the featured section does not match the design until user logs in againSteps to reproduce:
Actual result:Community is not matched to design until user is re-logged com_design.mp4Expected result:Community is matched to design in the 'featured' section |
erikseppanen
force-pushed
the
issue-16015
branch
from
f82b337
to
eb02042
Compare
|
@VladimrLitvinenko To simulate zero featured communities, I commented out the insertion of fetched community data as requested here |
|
Thanks @VladimrLitvinenko! ISSUE 1: I'm not sure how to reproduce this behavior. It might have something to do with being configured with the appropriate pokt, infura, etc. tokens, which I add during development. |
|
I am not sure we should show created communities in the (apologies if that's something you arranged for testing) |
@cammellos @erikseppanen @VladimrLitvinenko The only communities that should be shown in the Community Directory are communities curated by the Community Directory Curation dApp. This applies to both all communities, and featured communities. This data should come from the curation dApp and nowhere else. |
|
@erikseppanen @cammellos @osmaczko thank you for all details provided in #16015 Unfortunately we still do not have clear understanding of how properly test this PR. Couple of questions:
Those are: (old) Status Community, Contributor's test community, From contract to portal. Are those coming from this contract https://goerli-optimism.etherscan.io/address/0x22EE86A14b49890957fE71990073C6C68E05F9C5 ?
Couple details regarding this issue (all of them can be observed in the video below):
telegram-cloud-document-2-5206442369339108360.mp4 |
Thanks @pavloburykh! Issue 2 is fixed: both top and bottom "Discover Community" sections are contract communities data (same as desktop app). Issue 1: Could you verify the issue still exists? |
Thanx @erikseppanen ! Issue 2 is fixed. Issue 1 is fixed partially: broken community does not appear. Community appears after data is fetched from waku. The remained issue is communities are not fetched until app re-opening (you can observe the behaviour on video below). Is it possible to refresh data from waku without need to re-login the app? telegram-cloud-document-2-5213424762096725022.mp4 |
|
@erikseppanen also could you please ping me up when review comments will be resolved? After that I will re-test to make sure no regression has been introduced. Thanx! |
erikseppanen
force-pushed
the
issue-16015
branch
2 times, most recently
from
a0108c5
to
13041b0
Compare
| [taoensso.timbre :as log] | ||
| [utils.re-frame :as rf])) | ||
|
|
||
| (defn camel->kebab |
There was a problem hiding this comment.
have you seen this (utils.collection/map-keys csk/->kebab-case-keyword data) in the code, so it seems it could be used ? cc @ilmotta
There was a problem hiding this comment.
or actually if its so simple function, probably we don't need to use external library, and better to use this one but move it to utils
There was a problem hiding this comment.
@flexsurfer I've seen that library, but haven't fully explored the pros/cons of using it for this. The current solution is pretty small and specific to communities (we have some special handling here, for predicates and public keys that the library isn't going to do for us).
There was a problem hiding this comment.
I don't see a reason to not use the library. It's a well known, tested, small library in the Clojure community, so implementing our own camel to kebab transformation function is just a waste of time IMO. It's already in our shadow-cljs.edn.
have you seen this (utils.collection/map-keys csk/->kebab-case-keyword data) in the code, so it seems it could be used
@flexsurfer, this implementation works very well for converting the map keys, but it's not recursive. Which is also good in some occasions if we're sure we don't need to recursively convert the data structures. For a recursive solution, we just need to use cske/transform-keys combined with csk/->kebab-case-keyword, both provided by the library.
Every major Clojure codebase I worked with that had to interface with JSON APIs needed a recursive transformation function for converting keys.
For those places where performance is critical we might want to benchmark and do a more direct conversion. Maybe using assoc + dissoc could lead to faster results than rename-keys or cske/transform-keys, but I have no clue. But again, this would only be necessary if we can meaningfully measure a slow down.
There was a problem hiding this comment.
I think we should use the library at least to transform from camel -> kebab since it's already implemented and tested, reimplement the recursive key transformation seems a lot of work.
btw, why do we have keywords starting with a number?
Converts keys that start with a digit (0-9) from keywords to strings (:0x0324 -> "0x0324")
AFAIK, this kind of keywords are supported because early Clojure wrongly allowed them to exist so they keep supporting them, but they are not recommended.
There was a problem hiding this comment.
@flexsurfer @ilmotta @ulisesmac Thanks for the feedback!
I added some tests and renamed the functions, to give a better idea of what's involved here. Also, here's what the input data looks like: wakuext_curatedCommunities
- JSON API: the data is not strictly JSON, although it does include camel->kebab conversion. For example, the keys are already keywords, and it performs additional operations that are not related to camel->kebab conversion (renaming predicates, stringifying number-prefixed keys, and recursion to transform nested maps). The camel->kebab conversion is just one line of code:
(string/lower-case (string/replace s #"([a-z])([A-Z])" "$1-$2")), which looks for a lowercase letter followed by an uppercase letter, puts a dash between them, then lowercases everything. This camel->kebab conversion is maybe 10% of the solution. - Motivation: I wrote these to replace the existing
communities.coreset/rename-keysfunctions. While working on this PR, I noticed that not only would would it require another<-function, but that there were already many camelcase words in the data that had been missed (not converted to kebab case). So I concluded that the current key-by-key approach leads to keys being overlooked. - Specific to contract communities: The current key-by-key renaming approach is used many times in the codebase, but I can't testify to those conversions following the same rules as contract communities here.
- General utility function: Creating a general utility function would entail additional analysis of the conversion rules of all the other data structures that use the key-by-key renaming approach, well suited for a separate PR that can be prioritized appropriately.
camel-snake-kebablibrary: I've used this library before, for transforming mongodb JSON documents. In that case, an approach was needed that would convert documents that were universally JSON, with little control over the the actual field names. It is important to note that the library does not presume the format it is converting FROM: it has to do the work of figuring out whether the input string is PascalCase, HTTP-Header-Case, or whatever before converting it to the desired case (non-trivial code). Our use-case here is to convert a single data structure once, at startup, whose fieldnames we can reliably predict because we define them. While I don't think we necessarily need a library for this, I went ahead and replaced the line of code I mentioned above with(csk/->kebab-case-string s).
| predicate? (keyword (str lower-cased "?")) | ||
| :else (keyword lower-cased)))) | ||
|
|
||
| (defn rename-keys |
There was a problem hiding this comment.
also would be cool to move this to some sort of utilities namespace
There was a problem hiding this comment.
@flexsurfer I think so too! But it's only been used for communities so far (for example, changing public keys from keywords to strings might not be universally desired).
Perhaps we could expand it as a utility to be used universally in another PR?
There was a problem hiding this comment.
I think the existing file src/utils/transforms.cljs is the proper place in the codebase for this sort of thing, since the function should be general enough to not require any specific domain knowledge.
erikseppanen
force-pushed
the
issue-16015
branch
from
13041b0
to
2af4611
Compare
erikseppanen
force-pushed
the
issue-16015
branch
2 times, most recently
from
d08cc22
to
b872e5d
Compare
@erikseppanen could you please elaborate on this comment? Current issue is high priority from perspective of newly created user and what content he will see after entering Discovery communities screen for the first time. And currently he will not see any communities as we do not update data until app re-login. Also, designers have added a design for skeleton of community cards https://www.figma.com/file/h9wo4GipgZURbqqr1vShFN/Communities-for-Mobile?type=design&node-id=12510-427273&t=TyfKww2h28KiCitk-0 So until communities data is fetched we should show such skeleton. But this feature can be logged and addressed as follow-up in order not to block current PR. 
|
79% of end-end tests have passedFailed tests (7)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (26)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
|
agreed with @erikseppanen to address it in followup issue. PR is ready for merge. |
erikseppanen
force-pushed
the
issue-16015
branch
from
b872e5d
to
cc6b515
Compare
erikseppanen
force-pushed
the
issue-16015
branch
from
cc6b515
to
b259cb3
Compare
|
@erikseppanen @pavloburykh Note that the app should start trying to fetch the featured community as soon as the app has launched, not when the user visits this screen. And this information should then be cached. And if the latest fetch of this data hasn't been completed, the previously cached data should be displayed instead. So with this scheme, other than literally the first time the user logs into the app, the user should never see the skeleton loading screen (because there will always be data stored locally to be displayed) |
@John-44 thank you for clarification. I will leave your comment in follow up issue description. |
fixes #16015
status: ready