Yanked crates shouldn't be displayed on user/team pages

[carols10cents]

Related to #145.

Since we currently only keep track of yanked-ness on version records rather than on crate records, implementing this involves:

• Changing the query for the crates a user owns and the query for the crates a team owns to have an additional condition that there must be at least one version associated with that crate that has yanked equal to false.
  • I expect figuring out the diesel incantation to express this condition to be the most difficult part of this issue; give it a try and post what you've tried and I can help figure it out if you get stuck!
• Make a new test similar to this one that if the user or team in the test owns a crate where all versions have been yanked, that crate does NOT show up in the JSON response.

[sodiumjoe]

I'd like to give this a shot.

[sgrif]

Is there ever a case where we want the /crates route to include yanked crates? If not something like this should do it:

diff --git a/src/krate/search.rs b/src/krate/search.rs
index 3a8030a..dfa59d0 100644
--- a/src/krate/search.rs
+++ b/src/krate/search.rs
@@ -50,6 +50,11 @@ pub fn search(req: &mut Request) -> CargoResult<Response> {
     let recent_downloads = sql::<Nullable<BigInt>>("SUM(crate_downloads.downloads)");
 
     let mut query = crates::table
+        .filter(exists(
+            versions::table
+                .filter(versions::crate_id.eq(crates::id))
+                .filter(versions::yanked.eq(false)),
+        ))
         .left_join(
             crate_downloads::table.on(
                 crates::id

[carols10cents]

Is there ever a case where we want the /crates route to include yanked crates?

Yes. When you're searching to see if a name is taken, we should show yanked crates because that name can't be used.

#1058
#145

[sgrif]

Gotcha. In that case, the patch I pasted above should still work, but placed in the conditionals where we want it, rather than the line I have it on.

[sgrif]

Alternatively, that filter can become exists(...).or(exact_match)

[sgrif]

I've opened diesel-rs/diesel#1389 which might make that a bit easier as well (this PR shouldn't be blocked on that though)

[bryanburgers]

Should we use something like /api/v1/crates?include_yanked=n?

I took a shot at this using

query = query.filter(exists(
    versions::table
        .filter(versions::crate_id.eq(crates::id))
        .filter(versions::yanked.eq(false))
));

from above in each of the user_id= and team_id= sections, and that seems to work just fine.

However, I'm wondering if that's the most clear. Are we getting ourselves into a situation where we perform this logic if this is true, and that logic if that is true, and it gets tangled and confusing?

What if each front-end route is allowed to specify whether it wants to show yanked versions or not, using /api/v1/crates?include_yanked=(y|n) (or something similar)?

I would suggest we include yanked crates by default because that is how all existing requests work. And then specifically update the /users/:id and /teams/:id routes to add ?include_yanked=n to their parameters.

The advantages are that whether or not to include or exclude yanked crates is somewhat orthogonal to the other requests, and having a specific parameter for this functionality would allow front-end code to choose whether to show yanked crates or not on a route-by-route basis. For example, I could see us wanting /api/v1/crates?letter=a&include_yanked=n or something in the future, and that would be an easy change.

However, I haven't considered the database query time impact. And I could be convinced that this extensibility isn't buying us anything except complexity.

Is this a path worth pursuing?

---

I tried it out and I imagine it looking something like this: bryanburgers@1ebfc08