Skip to content
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

Crate listings duplicated on category subpages #524

Closed
wesleywiser opened this Issue Jan 24, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@wesleywiser
Copy link
Member

wesleywiser commented Jan 24, 2017

Some crates are being listed multiple times on the top-level category pages. For example, "Development tools":

image

It looks like this happens when the crate declares the top level category as well as one or more sub categories in its Cargo.toml file.

@carols10cents

This comment has been minimized.

Copy link
Member

carols10cents commented Jan 26, 2017

Ah interesting. I guess it wasn't clear that a crate would automatically be in the supercategory if listed in the subcategory.

@carols10cents

This comment has been minimized.

Copy link
Member

carols10cents commented Feb 11, 2017

The fix to this will involve changing the query that gets crates in a category to do a SELECT DISTINCT, and we should probably add a test for this... looks like I didn't add any tests for the crate index API route that use the category parameter, oops.

The test should be kind of like this keyword parameter test but will involve creating a category and a subcategory, adding a crate to that category and subcategory, and querying for the category.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 17, 2017

So I tried to write a test, I expect this version to pass: https://is.gd/1E9xIQ (this gose in tests/krate.rs)
But it fails with bad response: (404, "Not Found").

When it passes the plan is to replace &["kw1".into()] with &["kw1".into(), "kw1::foo".into()] and have it fail. Then fix the bug and have it pass again.

What am I doing wrong? Am I on the right track?

@carols10cents

This comment has been minimized.

Copy link
Member

carols10cents commented Feb 17, 2017

@Eh2406 so the example test I cited was with keywords, while this bug is for categories. The test code you have is still calling the code to add a keyword to the crate: Keyword::update_crate(tx(&req), &krate, &["kw1".into()]).unwrap(); Instead, you'll need to add a category.

One difference between keywords and categories is that anyone can add a keyword by just starting to use it on their crate. Categories are set by crates.io, so you can only use a category that exists on crates.io already. So your test will need to create a category before putting a crate in the category. An example of a test that creates a category is this one. And in this particular case, you'll want to create two categories actually: the parent category and the child category.

Once the categories exist, you should be able to call Category::update_crate like this test does instead of the line in your code where Keyword::update_crate is.

Does that make sense?

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 18, 2017

Yes. How long did I stare at the code without noting the Keyword right on the line I was worried about? It always seems to be something small and obvious, lol.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 18, 2017

So, added a test to update_crate. This test usefully tells us that the api/v1 summary statistics are correct. Unfortunately, that is not where the bug happens. Then I read your original description, add a comparable test to the end of index_queries and finally have a test for the bug! Tomorrow I can start work on fixing it and a pr. :-)

carols10cents added a commit that referenced this issue Feb 21, 2017

Merge pull request #558 from Eh2406/fix524
Only show crate once, even if search is like several category #524
@carols10cents

This comment has been minimized.

Copy link
Member

carols10cents commented Feb 21, 2017

Merged in #558 so this is closed now!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.