Total downloads user #740
Total downloads user #740
Conversation
|
Will squash after finishing up. |
src/user/mod.rs
Outdated
| let data = crate_downloads::table.filter(crate_downloads::crate_id.eq_any( | ||
| crate_owners::table.select(crate_owners::crate_id) | ||
| .filter(crate_owners::owner_id.eq(name)) | ||
| )).select(sum(crate_downloads::downloads)).first::<i64>(&*conn)?; |
There was a problem hiding this comment.
Could this be crate_downloads.select(sum(downloads)).inner_join(crate_owners).filter(owner_id.eq(name)))?
There was a problem hiding this comment.
Also, should this be limited to only users and not teams?
There was a problem hiding this comment.
Hey @sgrif thanks for taking a look at this. Your query makes total sense. I'll change it :)
should this be limited to only users and not teams
Since we're sending user_id from the dashboard, we'll be basically querying crate_owners with owner_kind as OwnerKind::User only right? even if it's not explicitly laid out. Do you think to convey the intent properly we should add this as a filter as well?
There was a problem hiding this comment.
@sgrif I was trying to take the approach that you suggested. But crates.io does not seem to have model for crate_downloads and so inner_join gives me this -
the trait `diesel::JoinTo<_>` is not implemented for `schema::crate_downloads::table
Is there any other approach I can take to make inner_join work? Thanks! :)
There was a problem hiding this comment.
Since we're sending user_id from the dashboard, we'll be basically querying crate_owners with owner_kind as OwnerKind::User only right? even if it's not explicitly laid out. Do you think to convey the intent properly we should add this as a filter as well?
The id is never enough to specify that it's a user on its own. Users and teams can have the same ID, as they live on different tables.
Is there any other approach I can take to make inner_join work? Thanks! :)
I was going to link to the macro, but it's not rendering in our docs for some reason. You can use the joinable! macro for this: joinable!(child_table -> parent_table (foreign_key_name))
There was a problem hiding this comment.
@sgrif reg user_id - Ahhh! You're right. I did not realise that before :) pushed a commit now to restrict that.
regarding joinable! - raised an issue on diesel to get some doubts cleared - diesel-rs/diesel#938
There was a problem hiding this comment.
@sgrif discovered that the crates table itself had a downloads count :D :D
So ended up using inner_join between crates and crate_owners - https://github.com/rust-lang/crates.io/pull/740/files#diff-b1fb5e206b84f71c2896c4115eedf2efR389
Thanks ❤️
|
Hi @PritiKumr sorry this has taken me so long! I will get you some feedback in the next few days :) |
|
This looks good overall, but the new endpoint should have tests for it. |
PritiKumr
force-pushed
the
total-downloads-user
branch
from
d589e87 to
cf1e3b4
Compare
|
@sgrif Tests have been added. cc: @carols10cents |
|
Hi I'm really really really sorry this has taken me so long! I've resolved the conflicts and I think have the tests passing-- will be taking a look and testing this out and hopefully getting this merged in today or tomorrow! |
app/controllers/dashboard.js
Outdated
| @@ -26,6 +27,10 @@ export default Ember.Controller.extend({ | |||
| return this.get('myFollowing').slice(0, TO_SHOW); | |||
| }), | |||
|
|
|||
| visibleStats: computed('myStats', function() { | |||
There was a problem hiding this comment.
I was looking at the code, and this and the other computed that depend on arrays should probably be myArray.[].
I wouldn't block the PR on that, but just want to note so it gets addressed.
There was a problem hiding this comment.
I'm confused, I think myStats is an int? Am I misunderstanding something because that's very likely
There was a problem hiding this comment.
WHEW. Did i fix it right? 9719fed
|
Merged, will be deploying shortly!! Thank you so much for your patience on this @PritiKumr !!! |
| @@ -3,8 +3,16 @@ | |||
| <div id='crates-heading'> | |||
| {{svg-jar "dashboard"}} | |||
| <h1>My Dashboard</h1> | |||
| <div id="stats"> | |||
| <div class='downloads'> | |||
| <img class="download" src="/assets/download.svg" /> | |||
There was a problem hiding this comment.
FWIW this should probably be using {{svg-jar "download"}} instead of the explicit <img>, which will include the SVG as an inline element
There was a problem hiding this comment.
oops. could you send in a pull request for this please? Thanks!
|
Congrats @PritiKumr 🎼 👯 |
Addresses #151
Functionality is done. Working on adding some tests now. Do let me know if this looks good. :)
Looking forward to feedback ❤️