Autocompletion of usernames in comment boxes #9773
Conversation
|
|
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
Codecov Report
@@ Coverage Diff @@
## main #9773 +/- ##
=======================================
Coverage ? 78.23%
=======================================
Files ? 98
Lines ? 5940
Branches ? 0
=======================================
Hits ? 4647
Misses ? 1293
Partials ? 0 |
TildaDares
force-pushed
the
autocomplete
branch
2 times, most recently
from
9098aa6
to
6ca6da8
Compare
TildaDares
added this to the [Outreachy] Improve Education Related Usability and Functionality milestone
|
Code Climate has analyzed commit 1006127 and detected 0 issues on this pull request. View more on Code Climate. |
There was a problem hiding this comment.
@TildaDares This looks great!! 🎉 another consideration can be to play around a bit with the ordering of the clauses in our query and benchmark the results ✌️
Yes and you can also watch the rails log when running locally, and see if all the chained conditions are rolled up into a single query, and it should provide a timestamp of how long it took in development mode. But, if your local is a pretty small database, it may not really show performance issues on the scale we're looking at in the live site. Great ideas! Thanks! |
| .where("rusers.status = 1") | ||
| .group('rusers.id') | ||
| .order(order) | ||
| .limit(limit) |
There was a problem hiding this comment.
Here, i wonder -- is 5 enough? What if we changed the limit to 15? Or, alternatively, what if there were a way to narrow this list based on something specific to the current user? But, that might increase the complexity of the query and therefore slow it down a lot.
Finally, i wonder if we could add caching to this query. That could make this really speedy and it seems OK for there to be at least an hour long cache here, maybe? Look at this example -
This can be added in followup, so I'll go ahead and merge this now, but these are maybe some more refinements we can make -- and they're simple enough that maybe they could be an FTO too!
There was a problem hiding this comment.
What if we changed the limit to 15?
@jywarren Setting the limit to 15 sounds like a good idea.
Finally, i wonder if we could add caching to this query. That could make this really speedy and it seems OK for there to be at least an hour long cache here, maybe? Look at this example -
The query already has a cache or is there somewhere else I'm supposed to add it? The cache has an expiration time of 24 hours so I'll change it to an hour.
Line 509 in c76e485
|
Just noting that I think this is ready to go -- we can test it in stable, and we won't be publishing to the live site for at least a few days or maybe longer -- and the extra ideas I suggested above could be done in another PR! Thanks, everyone! @TildaDares this is great work! |
|
ah, ok, perfect! Thank you!
…On Fri, Jun 11, 2021 at 2:25 PM Tilda Udufo ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/models/user.rb
<#9773 (comment)>:
> @@ -505,6 +505,18 @@ def latest_location
recent_locations.last
end
+ def self.recently_active_users(limit = 5, order = 'last_updated DESC')
+ Rails.cache.fetch('users/active', expires_in: 24.hours) do
+ User.select('rusers.username, rusers.status, rusers.id, MAX(node_revisions.timestamp) AS last_updated')
+ .joins("INNER JOIN `node_revisions` ON `node_revisions`.`uid` = `rusers`.`id` ")
+ .where("node_revisions.status = 1")
+ .where("rusers.status = 1")
+ .group('rusers.id')
+ .order(order)
+ .limit(limit)
What if we changed the limit to 15?
@jywarren <https://github.com/jywarren> Setting the limit to 15 sounds
like a good idea.
Finally, i wonder if we could add caching to this query. That could make
this really speedy and it seems OK for there to be at least an hour long
cache here, maybe? Look at this example -
The query already has a cache or is there somewhere else I'm supposed to
add it? The cache has an expiration time of 24 hours so I'll change it to
an hour.
https://github.com/publiclab/plots2/blob/c76e4859dcb69090ccf31ed8fe6140e4eaa567d2/app/models/user.rb#L509
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9773 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J6VEE74FEI4U2JVLYDTSJIH7ANCNFSM46OZGQUA>
.
|
Fixes #9755
Part of larger planning issue in #9667
Test here https://unstable.publiclab.org/
Screen.Recording.2021-06-10.at.16.39.36.mov
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test@publiclab/reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!