Add recent downloads #892

natboehm · 2017-07-18T21:06:40Z

This addresses issue #702, sorting crates by downloads in the past 90 days. The implementation follows Option 2 detailed in the issue, adding a new option to all sorting menus for downloads in the last 90 days and keeping the option to sort by all-time downloads. The number of recent downloads has been added to display below all-time downloads next to each crate, labeled accordingly.

This PR is currently waiting on updates to Diesel, without which it will not function correctly.
( edit: updates to Diesel complete :) )

Fixes #702

vignesh-sankaran · 2017-07-19T03:29:37Z

@natboehm: Could you run a cargo fmt against the codebase and push the formatted code up please? :)

natboehm · 2017-07-19T14:38:27Z

@vignesh-sankaran forgot about that one, will do!

vignesh-sankaran · 2017-07-21T07:56:46Z

@natboehm Thanks for that :). Which version of Diesel were you waiting on in order for this PR to work?

natboehm · 2017-07-21T17:27:00Z

@vignesh-sankaran not sure about the version but I think we're waiting on this PR to be merged.

sgrif · 2017-07-21T21:19:20Z

I'm going to release tomorrow

vignesh-sankaran · 2017-07-22T10:02:09Z

@sgrif Were you also going to update the Diesel version that crates.io specifies?

sgrif · 2017-07-23T17:26:59Z

@natboehm If you rebase you should have everything you need now

carols10cents

Overall this is looking great!!!! This was a lot of work, good job!!!

I've got a few small comments, and there's also:

A clippy failure i think we should allow for now
A problem with nulls i'll talk to you about in a second - add NULLS LAST to the ordering by recent downloads sql and add a test
A few commit messages that can be cleaned up

carols10cents · 2017-07-25T16:18:20Z

src/tests/all.rs

@@ -1,4 +1,4 @@
-#![deny(warnings)]
+//#![deny(warnings)]


Could you put this back please ❤️ :)

carols10cents · 2017-07-25T16:23:52Z

app/routes/team.js

-        page: { refreshedModel: true },
-        sort: { refreshedModel: true },
+        page: { refreshModel: true },
+        sort: { refreshModel: true },


I have no idea how you figured out this was needed; I don't think I could have!!! I'm super impressed!!! 😻

carols10cents · 2017-07-25T16:46:07Z

tests/acceptance/crates-test.js

+
+    await visit('/crates');
+    const $recentDownloads = findWithAssert('div.recent-downloads:first span.num');
+    hasText(assert, $recentDownloads, 'Recent:');


Is this test's expected text meant to have a number in it like the assertion on line 61 does?

carols10cents · 2017-07-25T16:50:51Z

app/controllers/category/index.js

-        return (this.get('sort') === 'downloads') ? 'Downloads' : 'Alphabetical';
+        if (this.get('sort') === 'downloads') {
+            return 'All-Time Downloads';
+        } else if (this.get('sort') === 'recent-downloads') {


WDYT about removing the explicit check for 'recent-downloads' and letting the else case handle that here? Kind of like the default Alphabetical in app/controllers/crates.js?

carols10cents · 2017-07-25T16:52:17Z

app/styles/crate.scss

+    .recent-downloads {
+        @include display-flex;
+        @include align-items(center);
+


Nitpicky: could you remove this empty line please? :)

carols10cents · 2017-07-25T16:55:14Z

src/krate.rs

@@ -39,7 +40,17 @@ use util::{RequestUtils, CargoResult, internal, ChainError, human};
 use version::{EncodableVersion, NewVersion};
 use {Model, User, Keyword, Version, Category, Badge, Replica};

-#[derive(Debug, Clone, Queryable, Identifiable, AsChangeset)]
+#[derive(Insertable, Queryable, Identifiable, Associations, AsChangeset)]


Could you add Debug here please? I like having debug on all the things :) hmmmm I think sgrif told me there was a lint for this....

#![deny(missing_debug_implementations, missing_copy_implementations)] 😄

…oth totals are listed but has ability to show just one according to sort

…me downloads in results

… of crates displayed

…abbed

…relevance with search query

…page are default sorted by recent downloads

…st of crates returned is sorted by recent-downloads

…t to recent-downloads

… to recent-downloads

…descending

…=cargo-clippy to feature=clippy)

carols10cents added the S-waiting-on-author label Jul 22, 2017

natboehm force-pushed the add-recent-downloads branch from c6719b7 to 7648e13 Compare July 24, 2017 14:53

carols10cents closed this Jul 24, 2017

carols10cents reopened this Jul 24, 2017

natboehm force-pushed the add-recent-downloads branch from 7648e13 to 28c1bbd Compare July 25, 2017 15:26

carols10cents requested changes Jul 25, 2017

View reviewed changes

natboehm added 17 commits July 25, 2017 14:44

query for recent_downloads

bc182a5

correctly displays top recently downloaded crates

36fe81c

Added recent_downloads to encodable_crate

5017507

addition of dropdown selection for ability to sort by recent downloads

776ed77

all-time and in past 90 days downloads displayed in list, currently b…

642a095

…oth totals are listed but has ability to show just one according to sort

increased size of stats div

c105307

Remove unused code for showing only either recent downloads or all ti…

1f9c68a

…me downloads in results

Misplaced paren, fixes weird searching bug

13491f1

Add recent download sort to user page

3ba4e41

Added sort by recent downloads to team page

62a2110

Fixes bug on team page where dropdown selection was not changing sort…

908ad06

… of crates displayed

fix broken test for list of crates returned by search

420b97a

null error when trying to get count

2d87b06

Fixes error in tests where crates with zero downloads could not be gr…

6036807

…abbed

tests for adding crates when sort = recent-downloads

121fdbf

Change default sort from alpha to recent-downloads overall, alpha to …

7d66ac0

…relevance with search query

Add test for checking that the keyword index page and category index …

8946d2a

…page are default sorted by recent downloads

natboehm added 18 commits July 25, 2017 14:45

Add test to check that when searching exact match is first and the li…

549683f

…st of crates returned is sorted by recent-downloads

delete debug statement, clean up join function

15975d1

add comments for tests written

6609e04

Add recent download option to category index page, change default sor…

0fb2e18

…t to recent-downloads

Add recent download option to keyword index page, change default sort…

e11724a

… to recent-downloads

add recent_downloads to fixture data

34a30d5

fix double quote linting error

d3f1e16

frontend tests for displaying and sorting by recent downloads

b1a9aa5

change 'All Time' to 'All-Time' for consistency

625f04c

remove unused variable

9970d6d

cargo fmt

e9cdc76

fixes bug where null downloads would be listed first when sorting by …

b086a6d

…descending

put back deny warnings

d5455cc

Add Debug to CrateDownload struct

fb719cc

add recent_downloads to crate serializer, update test to match

b13c432

delete duplicate code & whitespace

1c3f3c6

allow too many arguments on krate::encodable

20f42d9

cargo fmt

1acfbef

natboehm force-pushed the add-recent-downloads branch from e094f52 to 1acfbef Compare July 25, 2017 18:45

natboehm added 2 commits July 25, 2017 15:04

really allow too many arguments on krate::encodable

dfb21b8

actually allow too many arguments on krate::encodable (change feature…

8c58451

…=cargo-clippy to feature=clippy)

carols10cents approved these changes Jul 25, 2017

View reviewed changes

carols10cents merged commit 550b3d6 into rust-lang:master Jul 26, 2017

natboehm deleted the add-recent-downloads branch September 15, 2017 14:40

Turbo87 removed the S-waiting-on-author label Dec 3, 2017

jonhoo mentioned this pull request Jan 13, 2018

Sort "my crates" by recent downloads #1231

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add recent downloads #892

Add recent downloads #892

natboehm commented Jul 18, 2017 •

edited

Loading

vignesh-sankaran commented Jul 19, 2017 •

edited

Loading

natboehm commented Jul 19, 2017

vignesh-sankaran commented Jul 21, 2017

natboehm commented Jul 21, 2017

sgrif commented Jul 21, 2017

vignesh-sankaran commented Jul 22, 2017

sgrif commented Jul 23, 2017

carols10cents left a comment •

edited

Loading

carols10cents Jul 25, 2017

carols10cents Jul 25, 2017

carols10cents Jul 25, 2017

carols10cents Jul 25, 2017

carols10cents Jul 25, 2017

carols10cents Jul 25, 2017

sgrif Jul 25, 2017 •

edited

Loading

		@@ -1,4 +1,4 @@
		#![deny(warnings)]
		//#![deny(warnings)]

Add recent downloads #892

Add recent downloads #892

Conversation

natboehm commented Jul 18, 2017 • edited Loading

vignesh-sankaran commented Jul 19, 2017 • edited Loading

natboehm commented Jul 19, 2017

vignesh-sankaran commented Jul 21, 2017

natboehm commented Jul 21, 2017

sgrif commented Jul 21, 2017

vignesh-sankaran commented Jul 22, 2017

sgrif commented Jul 23, 2017

carols10cents left a comment • edited Loading

Choose a reason for hiding this comment

carols10cents Jul 25, 2017

Choose a reason for hiding this comment

carols10cents Jul 25, 2017

Choose a reason for hiding this comment

carols10cents Jul 25, 2017

Choose a reason for hiding this comment

carols10cents Jul 25, 2017

Choose a reason for hiding this comment

carols10cents Jul 25, 2017

Choose a reason for hiding this comment

carols10cents Jul 25, 2017

Choose a reason for hiding this comment

sgrif Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

natboehm commented Jul 18, 2017 •

edited

Loading

vignesh-sankaran commented Jul 19, 2017 •

edited

Loading

carols10cents left a comment •

edited

Loading

sgrif Jul 25, 2017 •

edited

Loading