Merge remote-tracking branch 'upstream/master' into require-owner-approval

Author: carols10cents

app/index.html

@@ -20,7 +20,7 @@
     <noscript>
         <div id="main">
             <div class='noscript'>
-                This site requires that JavaScript to be enabled.
+                This site requires JavaScript to be enabled.
             </div>
         </div>
     </noscript>

app/templates/index.hbs

@@ -1,5 +1,5 @@
 <div id='title-header'>
-    <h1>The Rust community&rsquo;s crate host</h1>
+    <h1>The Rust community&rsquo;s crate registry</h1>
 
     <div class='links'>
         {{#link-to "install" class='yellow-button'}}

src/krate/mod.rs

@@ -462,50 +462,62 @@ impl Crate {
         req_user: &User,
         login: &str,
     ) -> CargoResult<String> {
-        let owner = match Owner::find_by_login(conn, login) {
-            Ok(owner @ Owner::User(_)) => owner,
-            Ok(Owner::Team(team)) => if team.contains_user(app, req_user)? {
-                Owner::Team(team)
-            } else {
-                return Err(human(&format_args!(
-                    "only members of {} can add it as \
-                     an owner",
-                    login
-                )));
-            },
-            Err(err) => if login.contains(':') {
-                Owner::Team(Team::create(app, conn, login, req_user)?)
-            } else {
-                return Err(err);
-            },
-        };
+        use diesel::insert;
 
-        let owner_invitation = NewCrateOwnerInvitation {
-            invited_user_id: owner.id(),
-            invited_by_user_id: req_user.id,
-            crate_id: self.id,
-        };
+        let owner = Owner::find_or_create_by_login(app, conn, req_user, login)?;
 
-        diesel::insert(&owner_invitation.on_conflict_do_nothing())
-            .into(crate_owner_invitations::table)
-            .execute(conn)?;
+        match owner {
+            // Users are invited and must accept before being added
+            owner @ Owner::User(_) => {
+                let owner_invitation = NewCrateOwnerInvitation {
+                    invited_user_id: owner.id(),
+                    invited_by_user_id: req_user.id,
+                    crate_id: self.id,
+                };
 
-        Ok(format!(
-            "user {} has been invited to be an owner of crate {}",
-            owner.login(),
-            self.name
-        ))
+                diesel::insert(&owner_invitation.on_conflict_do_nothing())
+                    .into(crate_owner_invitations::table)
+                    .execute(conn)?;
+
+                Ok(format!(
+                    "user {} has been invited to be an owner of crate {}",
+                    owner.login(),
+                    self.name
+                ))
+            },
+            // Teams are added as owners immediately
+            owner @ Owner::Team(_) => {
+                let crate_owner = CrateOwner {
+                    crate_id: self.id,
+                    owner_id: owner.id(),
+                    created_by: req_user.id,
+                    owner_kind: OwnerKind::Team as i32,
+                };
+
+                insert(&crate_owner.on_conflict(
+                    crate_owners::table.primary_key(),
+                    do_update().set(crate_owners::deleted.eq(false)),
+                )).into(crate_owners::table)
+                    .execute(conn)?;
+
+                Ok(format!(
+                    "team {} has been added as an owner of crate {}",
+                    owner.login(),
+                    self.name
+                ))
+            }
+        }
     }
 
     pub fn owner_remove(
         &self,
+        app: &App,
         conn: &PgConnection,
-        _req_user: &User,
+        req_user: &User,
         login: &str,
     ) -> CargoResult<()> {
-        let owner = Owner::find_by_login(conn, login).map_err(|_| {
-            human(&format_args!("could not find owner with login `{}`", login))
-        })?;
+        let owner = Owner::find_or_create_by_login(app, conn, req_user, login)?;
+
         let target = crate_owners::table.find((self.id(), owner.id(), owner.kind() as i32));
         diesel::update(target)
             .set(crate_owners::deleted.eq(true))
@@ -1363,6 +1375,7 @@ pub fn remove_owners(req: &mut Request) -> CargoResult<Response> {
 fn modify_owners(req: &mut Request, add: bool) -> CargoResult<Response> {
     let mut body = String::new();
     req.body().read_to_string(&mut body)?;
+
     let user = req.user()?;
     let conn = req.db_conn()?;
     let krate = Crate::by_name(&req.params()["crate_id"]).first::<Crate>(&*conn)?;
@@ -1408,7 +1421,7 @@ fn modify_owners(req: &mut Request, add: bool) -> CargoResult<Response> {
             if owners.len() == 1 {
                 return Err(human("cannot remove the sole owner of a crate"));
             }
-            krate.owner_remove(&conn, user, login)?;
+            krate.owner_remove(req.app(), &conn, user, login)?;
         }
     }
 

src/owner.rs

@@ -44,7 +44,8 @@ pub struct Team {
     /// We only query membership with github using the github_id, though.
     /// This is the only name we should ever talk to Cargo about.
     pub login: String,
-    /// We're assuming these are stable
+    /// The GitHub API works on team ID numbers. This can change, if a team
+    /// is deleted and then recreated with the same name!!!
     pub github_id: i32,
     /// Sugary goodness
     pub name: Option<String>,
@@ -107,7 +108,7 @@ impl<'a> NewTeam<'a> {
         use diesel::insert;
         use diesel::pg::upsert::*;
 
-        insert(&self.on_conflict(teams::github_id, do_update().set(self)))
+        insert(&self.on_conflict(teams::login, do_update().set(self)))
             .into(teams::table)
             .get_result(conn)
             .map_err(Into::into)
@@ -116,7 +117,7 @@ impl<'a> NewTeam<'a> {
 
 impl Team {
     /// Tries to create the Team in the DB (assumes a `:` has already been found).
-    pub fn create(
+    pub fn create_or_update(
         app: &App,
         conn: &PgConnection,
         login: &str,
@@ -135,7 +136,7 @@ impl Team {
                          format is github:org:team",
                     )
                 })?;
-                Team::create_github_team(app, conn, login, org, team, req_user)
+                Team::create_or_update_github_team(app, conn, login, org, team, req_user)
             }
             _ => Err(human(
                 "unknown organization handler, \
@@ -144,10 +145,10 @@ impl Team {
         }
     }
 
-    /// Tries to create a Github Team from scratch. Assumes `org` and `team` are
+    /// Tries to create or update a Github Team. Assumes `org` and `team` are
     /// correctly parsed out of the full `name`. `name` is passed as a
     /// convenience to avoid rebuilding it.
-    fn create_github_team(
+    fn create_or_update_github_team(
         app: &App,
         conn: &PgConnection,
         login: &str,
@@ -292,18 +293,22 @@ fn team_with_gh_id_contains_user(app: &App, github_id: i32, user: &User) -> Carg
 }
 
 impl Owner {
-    /// Finds the owner by name, failing out if it doesn't exist.
-    /// May be a user's GH login, or a full team name. This is case
+    /// Finds the owner by name. Always recreates teams to get the most
+    /// up-to-date GitHub ID. Fails out if the user isn't found in the
+    /// database, the team isn't found on GitHub, or if the user isn't a member
+    /// of the team on GitHub.
+    /// May be a user's GH login or a full team name. This is case
     /// sensitive.
-    pub fn find_by_login(conn: &PgConnection, name: &str) -> CargoResult<Owner> {
+    pub fn find_or_create_by_login(
+        app: &App,
+        conn: &PgConnection,
+        req_user: &User,
+        name: &str,
+    ) -> CargoResult<Owner> {
         if name.contains(':') {
-            teams::table
-                .filter(teams::login.eq(name))
-                .first(conn)
-                .map(Owner::Team)
-                .map_err(|_| {
-                    human(&format_args!("could not find team with name {}", name))
-                })
+            Ok(Owner::Team(
+                Team::create_or_update(app, conn, name, req_user)?,
+            ))
         } else {
             users::table
                 .filter(users::gh_login.eq(name))