Allow removing deleted organizations from crate owners

[nic-hartley]

Fixes #1818.

My intended solution here is:

• Add a parameter to decide how the team gets looked up (in the database: true, vs. in the GH API: false)
• Pass true when deleting owners and false when adding them

Unfortunately the Crate::owner_add and Crate::owner_remove methods are like four steps above the actual DB lookup, and they call into the exact same stack of methods to get an Owner. I could have also created a new stack of methods but this way seems like it'll require modifying less code, which is a big positive for someone who doesn't really understand how this project works.

That said, I'm not 100% confident in where I've put the branch. Right now it's right next to the error message itself but it might belong higher up, e.g. maybe in Team::create_or_update it should try to look up the team by name instead of passing it down to the lower-level code.

Todo:

• Get cargo test working
• Add the parameter to the call sites in owner_add and owner_remove, all the way down to Team::create_or_update_github_team, with a todo! in place of the actual implementation
• Reconsider whether it should be that deep down, or whether looking up Owners in the database should be higher up.
• Add/modify tests as necessary to ensure you can delete non-existent org owners
• Implement the in_db lookup branch, wherever that should terminate

[nic-hartley]

I'd appreciate some feedback. I've got a few ways I could do this and I'm not sure which is the best.

• Unconditionally look up the login in the database before trying to hit the remote API and return it from the DB if it exists.

  I like this from a cleanliness point of view, but we'd end up in an issue where you can add a dead org as an owner. It still wouldn't do anything, but it might be confusing if you try to create a new org under a dead one's name, successfully add an org with the same name, and still can't use it to auth. (But the only way to solve this would be something that purges dead orgs from the DB.)

• Pass a boolean down the call stack to decide whether to try looking it up in the DB (true for owner_remove, false for owner_add)

  This gets annoying to implement, and I'm not sure where in the call stack the boolean should be checked. I'm leaning towards Team::create_or_update but not for any concrete reasons I can explain.

• Create a new function, e.g. Owner::lookup (please bikeshed the name, not sure what the convention is) that only looks it up in the DB.

  This probably adds the most code but it separates things into two completely separate call stacks, which seems nice. I don't think we'd run into any bugs from this approach as any owner being removed (which is the only time you'd call lookup) would have already been added to the DB by create, but adding this sort of "stateful" assumption always worries me, especailly in codebases I'm not super familiar with.

[nic-hartley]

(paging @carols10cents for no other reason than you've helped me here before and I'd appreciate your advice, if you've got a moment)

[Turbo87]

I'm not that deeply familiar with the team code but using find_or_create_by_login() in the owner_remove() fn seems wrong to me. In that case we should probably be using something like find_by_login(), though unfortunately that does not exist yet.

I think @carols10cents's original comment explains it quite well too:

The problem is that the current code is asking GitHub for existence of the org/team before removing it from the crate's owners, and the code should not need to check anything with GitHub in this case; it should just change the record in crate_owners for that team+crate to deleted='t'.

[nic-hartley]

@Turbo87 Appreciate the feedback!

I think you're right; I'm gonna add something like Team::find_by_login and call that. (I'd thought it'd have to share with the other code path much more than it actually does -- I didn't realize login was a unique ID.) Name TBD once I poke through the codebase to see if there's an existing pattern.

[nic-hartley]

I spent an embarrassingly long time trying to figure out how I broke ratelimiting (publish_new_crate_rate_limited) before running the tests on main and discovering it fails there, too. I assume it's something with how I've configured things on my machine. Hopefully it passes on CI!

ETA: Yep, looks like it's just on my machine. I'll have to dig into why, but as long as it works on CI I'm content to just ignore it locally, lol.

[Turbo87]

yeah, unfortunately that test is currently flaky (see #6952)

just to manage expectations, due to RustConf review times might currently be a bit slower than usual. after 22 hours of travel I probably shouldn't be reviewing code anymore 😅

[nic-hartley]

Oh, good, it's not just on my machine! I'm glad it passed on CI.

Don't worry about it, it's not like this is urgent. (At least, I hope it's not, considering the issue's from 2019, hah.)

[nic-hartley]

Rebased onto the latest main

[Turbo87]

sorry that it took so long... 🙈

looks very good to me though. nice work! :)

[nic-hartley]

@Turbo87 No worries! Thanks for the review and merge!