Skip to content
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

Team page #790

Merged
merged 16 commits into from
Jun 21, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,12 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
.filter(crate_owners::owner_kind.eq(OwnerKind::User as i32)),
),
);
} else if let Some(team_id) = params.get("team_id").and_then(|s| s.parse::<i32>().ok()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to put this on index instead of adding a new route?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with how getting a user's crates works, mostly. If we're going to pull this out into its own route, then I think users should be pulled out too, which imo should be a separate PR. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that users should be pulled out as well, but I don't think that should block this PR being a separate route, if it's not significantly more work for this PR.

query = query.filter(crates::id.eq_any(
crate_owners::table.select(crate_owners::crate_id)
.filter(crate_owners::owner_id.eq(team_id))
.filter(crate_owners::owner_kind.eq(OwnerKind::Team as i32))
));
} else if params.get("following").is_some() {
query = query.filter(crates::id.eq_any(
follows::table.select(follows::crate_id).filter(
Expand Down
72 changes: 36 additions & 36 deletions src/owner.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use diesel;
use diesel::prelude::*;
use diesel::pg::PgConnection;
use pg::rows::Row;
Expand Down Expand Up @@ -36,7 +35,7 @@ pub enum Owner {

/// For now, just a Github Team. Can be upgraded to other teams
/// later if desirable.
#[derive(Queryable, Identifiable)]
#[derive(Queryable, Identifiable, RustcEncodable, RustcDecodable)]
pub struct Team {
/// Unique table id
pub id: i32,
Expand All @@ -61,7 +60,7 @@ pub struct EncodableTeam {
pub url: Option<String>,
}

#[derive(RustcEncodable)]
#[derive(RustcEncodable, RustcDecodable)]
pub struct EncodableOwner {
pub id: i32,
pub login: String,
Expand All @@ -81,6 +80,39 @@ pub enum Rights {
Full,
}

#[derive(Insertable, AsChangeset)]
#[table_name="teams"]
pub struct NewTeam<'a> {
pub login: &'a str,
pub github_id: i32,
pub name: Option<String>,
pub avatar: Option<String>,
}

impl<'a> NewTeam<'a> {
pub fn new(login: &'a str,
github_id: i32,
name: Option<String>,
avatar: Option<String>) -> Self {
NewTeam {
login: login,
github_id: github_id,
name: name,
avatar: avatar,
}
}

pub fn create_or_update(&self, conn:&PgConnection) -> CargoResult<Team> {
use diesel::insert;
use diesel::pg::upsert::*;

insert(&self.on_conflict(teams::github_id, do_update().set(self)))
.into(teams::table)
.get_result(conn)
.map_err(Into::into)
}
}

impl Team {
/// Tries to create the Team in the DB (assumes a `:` has already been found).
pub fn create(
Expand Down Expand Up @@ -181,39 +213,7 @@ impl Team {
let (handle, resp) = http::github(app, &url, &token)?;
let org: Org = http::parse_github_response(handle, &resp)?;

Team::insert(conn, login, team.id, team.name, org.avatar_url)
}

pub fn insert(
conn: &PgConnection,
login: &str,
github_id: i32,
name: Option<String>,
avatar: Option<String>,
) -> CargoResult<Self> {
use diesel::pg::upsert::*;

#[derive(Insertable, AsChangeset)]
#[table_name = "teams"]
struct NewTeam<'a> {
login: &'a str,
github_id: i32,
name: Option<String>,
avatar: Option<String>,
}
let new_team = NewTeam {
login: login,
github_id: github_id,
name: name,
avatar: avatar,
};

diesel::insert(&new_team.on_conflict(
teams::github_id,
do_update().set(&new_team),
)).into(teams::table)
.get_result(conn)
.map_err(Into::into)
NewTeam::new(login, team.id, team.name, org.avatar_url).create_or_update(&conn)
}

/// Phones home to Github to ask if this User is a member of the given team.
Expand Down
34 changes: 34 additions & 0 deletions src/tests/all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@ use cargo_registry::keyword::Keyword;
use cargo_registry::krate::NewCrate;
use cargo_registry::upload as u;
use cargo_registry::user::NewUser;
use cargo_registry::owner::{CrateOwner, NewTeam, Team};
use cargo_registry::version::NewVersion;
use cargo_registry::{User, Crate, Version, Dependency, Category, Model, Replica};
use conduit::{Request, Method};
use conduit_test::MockRequest;
use diesel::pg::PgConnection;
use diesel::prelude::*;
use diesel::pg::upsert::*;
use cargo_registry::schema::*;

macro_rules! t {
($e:expr) => (
Expand Down Expand Up @@ -212,6 +215,37 @@ fn user(login: &str) -> User {
}
}

fn new_team(login: &str) -> NewTeam {
NewTeam {
github_id: NEXT_ID.fetch_add(1, Ordering::SeqCst) as i32,
login: login,
name: None,
avatar: None,
}
}

fn add_team_to_crate(
t: &Team,
krate: &Crate,
u: &User,
conn: &PgConnection) -> CargoResult<()> {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're just talking about cutting the newline here, right? #ambiguousemoji

let crate_owner = CrateOwner {
crate_id: krate.id,
owner_id: t.id,
created_by: u.id,
owner_kind: 1, // Team owner kind is 1 according to owner.rs
};

diesel::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(())
}

use cargo_registry::util::CargoResult;

struct CrateBuilder<'a> {
Expand Down
97 changes: 97 additions & 0 deletions src/tests/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use cargo_registry::upload as u;
use cargo_registry::user::EncodableUser;
use cargo_registry::version::EncodableVersion;
use cargo_registry::category::Category;
use cargo_registry::owner::EncodableOwner;

#[derive(RustcDecodable)]
struct CrateList {
Expand Down Expand Up @@ -61,6 +62,10 @@ struct RevDeps {
struct Downloads {
version_downloads: Vec<EncodableVersionDownload>,
}
#[derive(RustcDecodable)]
struct TeamResponse { teams: Vec<EncodableOwner> }
#[derive(RustcDecodable)]
struct UserResponse { users: Vec<EncodableOwner> }

fn new_crate(name: &str) -> u::NewCrate {
u::NewCrate {
Expand Down Expand Up @@ -1740,3 +1745,95 @@ fn author_license_and_description_required() {
json.errors
);
}

/* Testing the crate ownership between two crates and one team.
Given two crates, one crate owned by both a team and a user,
one only owned by a user, check that the CrateList returned
for the user_id contains only the crates owned by that user,
and that the CrateList returned for the team_id contains
only crates owned by that team.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😻 love these comments!!

*/
#[test]
fn check_ownership_two_crates() {
let (_b, app, middle) = ::app();

let (krate_owned_by_team, team) = {
let conn = app.diesel_database.get().unwrap();
let u = ::new_user("user_foo")
.create_or_update(&conn)
.unwrap();
let t = ::new_team("team_foo")
.create_or_update(&conn)
.unwrap();
let krate = ::CrateBuilder::new("foo", u.id)
.expect_build(&conn);
::add_team_to_crate(&t, &krate, &u, &conn).unwrap();
(krate, t)
};

let (krate_not_owned_by_team, user) = {
let conn = app.diesel_database.get().unwrap();
let u = ::new_user("user_bar")
.create_or_update(&conn)
.unwrap();
(::CrateBuilder::new("bar", u.id)
.expect_build(&conn), u)
};

let mut req = ::req(app.clone(), Method::Get, "/api/v1/crates");

let query = format!("user_id={}", user.id);
let mut response = ok_resp!(middle.call(req.with_query(&query)));
let json: CrateList = ::json(&mut response);

assert_eq!(json.crates[0].name, krate_not_owned_by_team.name);
assert_eq!(json.crates.len(), 1);

let query = format!("team_id={}", team.id);
let mut response = ok_resp!(middle.call(req.with_query(&query)));

let json: CrateList = ::json(&mut response);
assert_eq!(json.crates.len(), 1);
assert_eq!(json.crates[0].name, krate_owned_by_team.name);
}

/* Given a crate owned by both a team and a user, check that the
JSON returned by the /owner_team route and /owner_user route
contains the correct kind of owner

Note that in this case function new_team must take a team name
of form github:org_name:team_name as that is the format
EncodableOwner::encodable is expecting
*/
#[test]
fn check_ownership_one_crate() {
let (_b, app, middle) = ::app();

let (team, user) = {
let conn = app.diesel_database.get().unwrap();
let u = ::new_user("user_cat")
.create_or_update(&conn)
.unwrap();
let t = ::new_team("github:test_org:team_sloth")
.create_or_update(&conn)
.unwrap();
let krate = ::CrateBuilder::new("best_crate", u.id)
.expect_build(&conn);
::add_team_to_crate(&t, &krate, &u, &conn).unwrap();
(t, u)
};

let mut req = ::req(app.clone(), Method::Get, "/api/v1/crates/best_crate/owner_team");
let mut response = ok_resp!(middle.call(&mut req));
let json: TeamResponse = ::json(&mut response);

assert_eq!(json.teams[0].kind, "team");
assert_eq!(json.teams[0].name, team.name);

let mut req = ::req(app.clone(), Method::Get, "/api/v1/crates/best_crate/owner_user");
let mut response = ok_resp!(middle.call(&mut req));
let json: UserResponse = ::json(&mut response);

assert_eq!(json.users[0].kind, "user");
assert_eq!(json.users[0].name, user.name);
}