-
Notifications
You must be signed in to change notification settings - Fork 131
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
feat(admin): allow deleting package versions #1011
Conversation
let count = db | ||
.count_package_dependents( | ||
crate::db::DependencyKind::Jsr, | ||
&format!("@{}/{}", scope, package), | ||
) | ||
.await?; | ||
|
||
if count > 0 { | ||
return Err(ApiError::DeleteVersionHasDependents); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is not correct, as it doesnt use the version as a requirement, and as such this check applies to all versions and is stricter than should be. This is good enough for now, however ideally this would build a graph and check if there are any other versions that fit the constraints used by dependents, and only if there is no such constraint would it error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add admin UI for deleting packages
api/src/db/database.rs
Outdated
#[instrument(name = "Database::yank_package_version", skip(self), err)] | ||
pub async fn delete_package_version( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/yank_package_version/delete_package_version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should log or instrument the package name here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats already done in the route handler function that calls this (same setup we have for all other db calls & endpoints)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM if you feel confident
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crowlKats We had discussed that users should not be able to publish a version that was deleted anew - you didn't implement that yet. Can you do that?
@@ -254,6 +260,19 @@ function Version({ | |||
</button> | |||
</form> | |||
)} | |||
{isPublished && iam.isStaff && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this only show up when sudo is on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, fixed in #1027
@lucacasonato this is already handled. opened #1027 for adding an additional test to showcase this. |
this PR adds an endpoint to be able to delete package versions, but only for admins. This is not exposed in the UI for admins either, as the fact that this is for admins only is just a temporary restriction, and the idea is to come up with sensible restrictions that apply to users so they can delete versions (see #899).