Issue #680 : Crate owners can now remove themselves as owner #999
Conversation
|
@carols10cents is there something you need from me to facilitate the review? |
|
Oh @vignesh-sankaran mentioned he wanted to try reviewing this one :) |
| // Ensures that so long as at least one owner remains associated with the crate, | ||
| // a user can still remove their own login as an owner | ||
| #[test] | ||
| fn owners_can_remove_self() { |
There was a problem hiding this comment.
Thanks for adding a test here, it's appreciated :). It helps in that it increases the test coverage of the backend.
|
Ok so from a functionality standpoint, this works fine, I tried adding another user, deleting the mock crate owner and then attempting to delete the sole crate owner, and it worked fine. From a commit perspective, could you try squashing the commits down into 1 commit i.e. the first one? In your case, what you'd need to do is run Aside from that, I think we're good to merge this code in :). |
|
Hm there are still a lot of changes in the full diff, so the one squashed-down commit would still be pretty noisy. I think we can just take out the 2nd commit entirely and then squash the 3rd into the 1st. I'm guessing the old version of rustfmt you had reordered the I might just go ahead and take care of this actually... |
|
I'm on the road at the moment without access to a laptop. I can take a look
in a day or two
…On Aug 27, 2017 5:32 PM, "Carol (Nichols || Goulding)" < ***@***.***> wrote:
Hm there are still a lot of changes in the full diff, so the one
squashed-down commit would still be pretty noisy. I think we can just take
out the 2nd commit entirely and then squash the 3rd into the 1st.
I'm guessing the old version of rustfmt you had reordered the use
statements, and then the newer version didn't reorder the use statements,
so it left them reordered :)
I might just go ahead and take care of this actually...
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#999 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANHJocJronj6PbpvC4BWyMrbK8-i2iwnks5sceBvgaJpZM4O90xm>
.
|
|
Ah yeah then, no problem, i got it :) |
… if there is at least one other owner remaining.
src/tests/krate.rs
Outdated
|
|
||
| let mut response = ok_resp!(middle.call(req.with_method(Method::Get))); | ||
| let r: R = ::json(&mut response); | ||
| assert_eq!(r.users.len(), 1); |
There was a problem hiding this comment.
Hm, it looks like L1255-1257 are doing the exact same request and exact same assertion as L1251-1253, without any action in between? I'm going to remove the duplication while i'm in here, please let me know if there was a particular reason it was there that i'm missing :)
src/tests/krate.rs
Outdated
| let mut response = ok_resp!(middle.call(req.with_method(Method::Delete).with_body( | ||
| body.as_bytes(), | ||
| ))); | ||
| ::json::<::Bad>(&mut response); |
There was a problem hiding this comment.
It looks like this test is checking that after you remove yourself as an owner, you should not be allowed to remove other people as owners (because you no longer have permission). I think we're missing coverage for the case where the only owner attempts to remove themselves-- I'm going to repeat the request from L1266-1269 before the second owner is added, and assert that it's not allowed.
carols10cents
force-pushed
the
selfremoveowner
branch
from
3304c9b to
f25ec1c
Compare
|
bors: r+ |
Build succeeded |
If there is at least one other owner remaining, then an owner can remove themselves from a crate.