-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add a method to delete user #215
Conversation
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.
Thank you so much for tackling this. I have left a few minor pieces of feedback. Great work!
@@ -326,6 +332,8 @@ pub enum ServerResponse { | |||
/// The id of the user created. | |||
id: u64, | |||
}, | |||
/// A user was deleted. | |||
UserDeleted, |
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.
Since there is no information being returned, let's remove this variant and have the networking client use Response::Ok
instead.
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.
Done
crates/bonsaidb-local/src/storage.rs
Outdated
// // Retrieve a user to delete | ||
// let doc = self | ||
// .admin() | ||
// .await | ||
// .collection::<User>() | ||
// .get(primary_key) | ||
// .await?; |
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.
Let's remove this disabled code
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.
Done.
#[cfg_attr(feature = "actionable-traits", actionable(protection = "simple"))] | ||
DeleteUser { | ||
/// The unique primary key of the user to be deleted. | ||
primary_key: NamedReference<'static, u64>, |
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.
To match the other networking requests, let's rename this to user
. A NamedReference
can be either a unique name (in this case, the username) or the primary key (in this case, u64). Since it can contain data that isn't the primary_key, that's why I used user
instead on the other request types.
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.
Done.
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 approved the changes before having a chance to run the full test suite -- cargo test --all-features
. Once I ran it locally, I saw the failure and was surprised I missed the server implementation on review.
crates/bonsaidb-local/src/storage.rs
Outdated
let doc = User::load(user, &admin) | ||
.await? | ||
.ok_or(bonsaidb_core::Error::UserNotFound)?; | ||
self.admin().await.collection::<User>().delete(&doc).await?; |
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.
self.admin().await.collection::<User>().delete(&doc).await?; | |
doc.delete(&admin).await?; |
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.
Done.
Also added more notes on what to run before pushing changes. I need to get CI hooked up to pull requests. Refs #215
Addresses #83.