Skip to content

Add the ability to flag a user as deleted in GH #1586

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion app/components/user-avatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default Component.extend({
height: readOnly('width'),

alt: computed('user', function() {
return `${this.get('user.name')} (${this.get('user.login')})`;
return `${this.get('user.name')} (${this.get('user.display_name')})`;
Copy link
Contributor

Choose a reason for hiding this comment

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

The /crates/:crate_name/owner_user does not return the display name for owners, so this will display "undefied" for all owners (except for the currently logged in one, since the /me endpoint does return the display name).

We will probably need to update the owner_user endpoint as well (which uses the EncodableOwner struct for serialization).

}),

src: computed('size', 'user', function() {
Expand Down
2 changes: 1 addition & 1 deletion app/components/user-link.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export default Component.extend({
attributeBindings: ['title', 'href'],
tagName: 'a',

title: readOnly('user.login'),
title: readOnly('user.display_name'),

// TODO replace this with a link to a native crates.io profile
// page when they exist.
Expand Down
1 change: 1 addition & 0 deletions app/models/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export default DS.Model.extend({
email_verified: DS.attr('boolean'),
email_verification_sent: DS.attr('boolean'),
name: DS.attr('string'),
display_name: DS.attr('string'),
login: DS.attr('string'),
avatar: DS.attr('string'),
url: DS.attr('string'),
Expand Down
2 changes: 1 addition & 1 deletion app/templates/me/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<dt>Name</dt>
<dd>{{ model.user.name }}</dd>
<dt>GitHub Account</dt>
<dd>{{ model.user.login }}</dd>
<dd>{{ model.user.display_name }}</dd>
</dl>
</div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion app/templates/user.hbs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<div id='crates-heading' data-test-heading>
{{user-avatar user=model.user size='medium' data-test-avatar=true}}
<h1 data-test-username>
{{ model.user.login }}
{{ model.user.display_name }}
</h1>
{{#user-link user=model.user data-test-user-link=true}}
<img alt="GitHub profile" title="GitHub profile" src="/assets/GitHub-Mark.svg">
Expand Down
1 change: 1 addition & 0 deletions migrations/2018-12-27-132130_add_deleted_to_users/down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE users DROP COLUMN gh_deleted;
1 change: 1 addition & 0 deletions migrations/2018-12-27-132130_add_deleted_to_users/up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE users ADD COLUMN gh_deleted BOOLEAN NOT NULL DEFAULT FALSE;
3 changes: 3 additions & 0 deletions mirage/fixtures/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export default [
email: null,
id: 303,
login: 'blabaere',
display_name: 'blabaere',
name: 'Benoît Labaere',
url: 'https://github.com/blabaere',
},
Expand All @@ -13,6 +14,7 @@ export default [
email: '[email protected]',
id: 2,
login: 'thehydroimpulse',
display_name: 'thehydroimpulse',
name: 'Daniel Fagnan',
url: 'https://github.com/thehydroimpulse',
},
Expand All @@ -21,6 +23,7 @@ export default [
email: '[email protected]',
id: 10982,
login: 'iain8',
display_name: 'iain8',
name: 'Iain Buchanan',
url: 'https://github.com/iain8',
},
Expand Down
13 changes: 10 additions & 3 deletions src/controllers/user/other.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,21 @@ use crate::views::EncodablePublicUser;

/// Handles the `GET /users/:user_id` route.
pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
use self::users::dsl::{gh_login, id, users};
use self::users::dsl::{gh_deleted, gh_login, id, users};

let name = &req.params()["user_id"].to_lowercase();
let conn = req.db_conn()?;
let user = users
let mut query = users
.filter(crate::lower(gh_login).eq(name))
.order(id.desc())
.first::<User>(&*conn)?;
.into_boxed();
if name.starts_with("deleted_") {
if let Ok(deleted_id) = name["deleted_".len()..].parse::<i32>() {
query = query.or_filter(id.eq(deleted_id).and(gh_deleted));
}
};

let user = query.first::<User>(&conn)?;

#[derive(Serialize)]
struct R {
Expand Down
48 changes: 42 additions & 6 deletions src/models/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub struct User {
pub name: Option<String>,
pub gh_avatar: Option<String>,
pub gh_id: i32,
pub gh_deleted: bool,
}

#[derive(Insertable, Debug)]
Expand Down Expand Up @@ -78,6 +79,7 @@ impl<'a> NewUser<'a> {
name.eq(excluded(name)),
gh_avatar.eq(excluded(gh_avatar)),
gh_access_token.eq(excluded(gh_access_token)),
gh_deleted.eq(false),
))
.get_result::<User>(conn)?;

Expand Down Expand Up @@ -179,43 +181,77 @@ impl User {
email_verified: bool,
email_verification_sent: bool,
) -> EncodablePrivateUser {
let url = self.url().into_owned();
let display_name = self.display_name().into_owned();
let login = self.login().into_owned();
let User {
id,
email,
name,
gh_login,
gh_avatar,
..
} = self;
let url = format!("https://github.com/{}", gh_login);
EncodablePrivateUser {
id,
email,
email_verified,
email_verification_sent,
avatar: gh_avatar,
login: gh_login,
login,
name,
url: Some(url),
display_name: Some(display_name),
}
}

/// Converts this`User` model into an `EncodablePublicUser` for JSON serialization.
pub fn encodable_public(self) -> EncodablePublicUser {
let url = self.url().into_owned();
let display_name = self.display_name().into_owned();
let login = self.login().into_owned();
let User {
id,
name,
gh_login,
gh_avatar,
..
} = self;
let url = format!("https://github.com/{}", gh_login);
EncodablePublicUser {
id,
avatar: gh_avatar,
login: gh_login,
login,
name,
url: Some(url),
display_name: Some(display_name),
}
}

/// Returns a link to this user's GitHub profile, or @ghost if the user
/// has been deleted
fn url(&self) -> Cow<str> {
if self.gh_deleted {
Cow::Borrowed("https://github.com/ghost")
} else {
Cow::Owned(format!("https://github.com/{}", self.gh_login))
}
}

/// Returns the user's name for display purposes. Typically the same as
/// gh_login.
fn display_name(&self) -> Cow<str> {
if self.gh_deleted {
Cow::Owned(format!("{} (deleted)", self.gh_login))
} else {
Cow::Borrowed(&self.gh_login)
}
}

/// Returns the user's gh_login if they haven't been deleted, or a special
/// identifier if they have.
fn login(&self) -> Cow<str> {
if self.gh_deleted {
Cow::Owned(format!("deleted_{}", self.id))
} else {
Cow::Borrowed(&self.gh_login)
}
}
}
6 changes: 6 additions & 0 deletions src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,12 @@ table! {
///
/// (Automatically generated by Diesel.)
gh_id -> Int4,
/// The `gh_deleted` column of the `users` table.
///
/// Its SQL type is `Bool`.
///
/// (Automatically generated by Diesel.)
gh_deleted -> Bool,
}
}

Expand Down
35 changes: 35 additions & 0 deletions src/tests/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -628,3 +628,38 @@ fn test_existing_user_email() {
assert!(!r.user.email_verified);
assert!(!r.user.email_verification_sent);
}

#[test]
fn deleted_users_show_this_information() {
use cargo_registry::schema::users;

let (app, _anon, user) = TestApp::init().with_user();

app.db(|conn| {
let user = user.as_model();
diesel::update(user)
.set(users::gh_deleted.eq(true))
.execute(conn)
.unwrap();
});
let login = format!("deleted_{}", user.as_model().id);
let resp: UserShowPublicResponse = user.get(&format!("/api/v1/users/{}", login)).good();
let user = resp.user;

assert_eq!("foo (deleted)", user.display_name.unwrap());
assert_eq!("https://github.com/ghost", user.url.unwrap());
assert_eq!(login, user.login);
}

#[test]
fn users_with_names_starting_with_deleted_can_be_looked_up() {
let (app, _) = TestApp::init().empty();
let user = app.db_new_user("deleted_user");

let resp: UserShowPublicResponse = user.get("/api/v1/users/deleted_user").good();
let user = resp.user;

assert_eq!("deleted_user", user.display_name.unwrap());
assert_eq!("https://github.com/deleted_user", user.url.unwrap());
assert_eq!("deleted_user", user.login);
}
14 changes: 14 additions & 0 deletions src/views/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,25 @@ pub struct EncodableMe {
#[derive(Deserialize, Serialize, Debug)]
pub struct EncodablePrivateUser {
pub id: i32,

/// The user's login, used to identify the user in URLs.
/// This should not be used for display purposes
pub login: String,

/// The user's display name. Typically the same as login,
/// but changes if the user's GitHub account has been deleted.
/// Use this instead of login for display
pub display_name: Option<String>,

pub email: Option<String>,
pub email_verified: bool,
pub email_verification_sent: bool,

/// The user's full name as given to GitHub
pub name: Option<String>,
pub avatar: Option<String>,

/// The URL to the user's external profile
pub url: Option<String>,
}

Expand All @@ -176,6 +189,7 @@ pub struct EncodablePrivateUser {
pub struct EncodablePublicUser {
pub id: i32,
pub login: String,
pub display_name: Option<String>,
pub name: Option<String>,
pub avatar: Option<String>,
pub url: Option<String>,
Expand Down