-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
User hover joined at #207
User hover joined at #207
Conversation
src/braid/core/server/db/common.clj
Outdated
@@ -22,14 +23,30 @@ | |||
:user/avatar | |||
{:group/_user [:group/id]}]) | |||
|
|||
(defn user-joined-at | |||
[user-id group-id] | |||
(d/q '[:find [?inst ...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could potentially do [:find ?inst .
, which would make it return a single value, but I suppose it's possible that the user leaves & re-joins the group multiple times...in that case though, we'd probably want to find the smallest value here, e.g. something like (->> (d/q ...) (reduce min))
.
Either way, this function should either return a single value (so down below, it can just be {g (user-joined-at ...)}
, without the first
) or change the name to indicate it's returning all joined times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer just returning one of the joined-times, let's say the first one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if we want first though, we'd need to order right? that was my idea with the reduce min
).
Functionally, this looks good; I'm just concerned about the number of additional queries this will generate (e.g. when getting all users in a group, it will now have to make a query for each group each user is in), but I'm not sure if there's a better way to do it (e.g. somehow putting joined time in the pull-pattern) |
src/braid/core/server/db/common.clj
Outdated
:group-ids (map :group/id (:group/_user e)) | ||
:joined-at (reduce into | ||
(map (fn [g] {g (first (user-joined-at (:user/id e) g))}) | ||
(map :group/id (:group/_user e))))}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The :joined-at
value can be rewritten to be clearer by using the ->>
macro:
(->> (:group/_user e)
(map :group/id)
(map (fn [group-id]
[group-id (user-joined-at (:user/id e) group-id)]))
(into {}))
Datomic is very new to me, but it doesn't seem possible to put a non-attribute in a pull pattern. |
Yeah, exactly. Your solution is perfectly fine for now, just something that we'll have to keep an eye on when this change gets deployed & perhaps revisit (we have had issues in the past where inefficient queries that run a lot effectively DoS the server). |
src/braid/core/server/db/common.clj
Outdated
(defn user-joined-at | ||
[user-id group-id] | ||
(->> | ||
(d/q '[:find ?inst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we put this back to [:find [?inst ...]
then we get a vector of dates, so we can just do the reduce without the first (since it currently isn't actually finding the earliest date).
src/braid/core/server/db/common.clj
Outdated
(reduce min) | ||
(first))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in the previous, if we change the select pattern, we can obviate the need for first
, but the (reduce min)
doesn't actually work here, if the user has joined the group multiple times (I know I suggested this initially, my mistake).
Instead of min
, we need something that can properly compare dates, something like
(defn earlier
[d1 d2]
(if (.before d1 d2)
d1
d2))
then (reduce earlier)
Hello,
here's a first try at #137, let me know what you think. I had to change the query described in the issue to match on assertions (i.e.
[?g :group/user ?t ?tx true]
rather thanfalse
) or else nothing would be returned. Not sure what the best value is.