Crate with all versions yanked display reduced info #5314
Conversation
| @@ -193,7 +205,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { | |||
| None | |||
| }; | |||
|
|
|||
| let badges = if include.badges { | |||
| let badges = if include.badges && !all_versions_yanked { | |||
There was a problem hiding this comment.
This is not required, but it avoids the database to do run unnecessary queries. It could be premature optimisation, let me know if you would like it removed.
| @@ -202,7 +214,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { | |||
| } else { | |||
| None | |||
| }; | |||
| let top_versions = if include.versions { | |||
| let top_versions = if include.versions && !all_versions_yanked { | |||
| None, | ||
| false, | ||
| None, | ||
| false, |
There was a problem hiding this comment.
Assuming there is no way to publish an yanked crate so setting up this to false.
|
did you check what effects this has on the frontend? |
|
@Turbo87 I did a basic test in my local machine, publishing crates locally. I just added screenshots in the PR description for the changes I can see. I can confirm the API is responding correctly too. I don't know much about the frontend code though (specifically about badges for example), if you can point me what other tests I should do locally, I will certainly do them. |
Turbo87
added
C-enhancement ✨
|
☔ The latest upstream changes (presumably 4d4aacc) made this pull request unmergeable. Please resolve the merge conflicts. |
|
FWIW the reason I haven't felt comfortable yet with merging this is that it increases the complexity of the code quite a bit, and in some cases even requires an extra database call. I'm wondering if it would be better to have a sort of "default version" reference from the crates table to the versions table and store the corresponding data there instead. 🤔 |
|
as mentioned in #1058 (comment), we decided to change course on how we want to deal with and display yanked crates/versions. sorry for letting this issue be in limbo for so long :-/ |
This PR reduces the information returned by the API when crates have all versions yanked. Specifically
badges,documentation,homepageandrepositoryare set toNoneandmax_versionis set to"0.0.0". These changes are applied on:GET /crates/:crate_id(crate page)GET /crates(search).In this PR the definition for a crate with all versions yanked is when
crate.versions()returns no results. The versions method loads all non-yanked versions.This definition simplifies the code a bit, specifically in
src/controllers/krate/search.rssince the versions for the queried crates are already loaded in memory, so there is no need to run yet one more query. Semantically though, this definition is not entirely correct.crate.versions()could also mean that all versions have been deleted.I don't think the difference matters much for this PR, but please do let me know if this is not a valid definition for all versions yanked crates and I will update the PR.
Fix #1058
Search page changes:
Crate with 2 yanked versions and 1 non-yanked.
Crate with all 3 versions yanked.Crate page changes:
Non-yanked crate.
Yanked crate.